lkml.org 
[lkml]   [2011]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 02/10] Add yaffs2 file system: attrib and xattrib handling
On 02/09/2011 04:26 PM, Charles Manning wrote:
> Signed-off-by: Charles Manning <cdhmanning@gmail.com>

Hi Charles,

Some comments below,

~Ryan

> ---
> fs/yaffs2/yaffs_attribs.c | 124 ++++++++++++++++++++++++++++
> fs/yaffs2/yaffs_attribs.h | 28 ++++++
> fs/yaffs2/yaffs_nameval.c | 201 +++++++++++++++++++++++++++++++++++++++++++++
> fs/yaffs2/yaffs_nameval.h | 28 ++++++
> 4 files changed, 381 insertions(+), 0 deletions(-)
> create mode 100644 fs/yaffs2/yaffs_attribs.c
> create mode 100644 fs/yaffs2/yaffs_attribs.h
> create mode 100644 fs/yaffs2/yaffs_nameval.c
> create mode 100644 fs/yaffs2/yaffs_nameval.h
>
> diff --git a/fs/yaffs2/yaffs_attribs.c b/fs/yaffs2/yaffs_attribs.c
> new file mode 100644
> index 0000000..fe914e5
> --- /dev/null
> +++ b/fs/yaffs2/yaffs_attribs.c
> @@ -0,0 +1,124 @@
> +/*
> + * YAFFS: Yet Another Flash File System. A NAND-flash specific file system.
> + *
> + * Copyright (C) 2002-2011 Aleph One Ltd.
> + * for Toby Churchill Ltd and Brightstar Engineering
> + *
> + * Created by Charles Manning <charles@aleph1.co.uk>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "yaffs_guts.h"
> +#include "yaffs_attribs.h"
> +
> +void yaffs_load_attribs(struct yaffs_obj *obj, struct yaffs_obj_hdr *oh)
> +{
> + obj->yst_uid = oh->yst_uid;
> + obj->yst_gid = oh->yst_gid;
> + obj->yst_atime = oh->yst_atime;
> + obj->yst_mtime = oh->yst_mtime;
> + obj->yst_ctime = oh->yst_ctime;
> + obj->yst_rdev = oh->yst_rdev;
> +}
> +
> +void yaffs_load_attribs_oh(struct yaffs_obj_hdr *oh, struct yaffs_obj *obj)
> +{
> + oh->yst_uid = obj->yst_uid;
> + oh->yst_gid = obj->yst_gid;
> + oh->yst_atime = obj->yst_atime;
> + oh->yst_mtime = obj->yst_mtime;
> + oh->yst_ctime = obj->yst_ctime;
> + oh->yst_rdev = obj->yst_rdev;
> +
> +}
> +
> +void yaffs_load_current_time(struct yaffs_obj *obj, int do_a, int do_c)
> +{
> + obj->yst_mtime = Y_CURRENT_TIME;
> + if (do_a)
> + obj->yst_atime = obj->yst_mtime;
> + if (do_c)
> + obj->yst_ctime = obj->yst_mtime;
> +}
> +
> +void yaffs_attribs_init(struct yaffs_obj *obj, u32 gid, u32 uid, u32 rdev)
> +{
> + yaffs_load_current_time(obj, 1, 1);
> + obj->yst_rdev = rdev;
> + obj->yst_uid = uid;
> + obj->yst_gid = gid;
> +}
> +
> +loff_t yaffs_get_file_size(struct yaffs_obj *obj)
> +{
> + YCHAR *alias = NULL;
> + obj = yaffs_get_equivalent_obj(obj);
> +
> + switch (obj->variant_type) {
> + case YAFFS_OBJECT_TYPE_FILE:
> + return obj->variant.file_variant.file_size;
> + case YAFFS_OBJECT_TYPE_SYMLINK:
> + alias = obj->variant.symlink_variant.alias;
> + if (!alias)
> + return 0;
> + return strnlen(alias, YAFFS_MAX_ALIAS_LENGTH);
> + default:
> + return 0;
> + }
> +}
> +
> +int yaffs_set_attribs(struct yaffs_obj *obj, struct iattr *attr)
> +{
> + unsigned int valid = attr->ia_valid;
> +
> + if (valid & ATTR_MODE)
> + obj->yst_mode = attr->ia_mode;
> + if (valid & ATTR_UID)
> + obj->yst_uid = attr->ia_uid;
> + if (valid & ATTR_GID)
> + obj->yst_gid = attr->ia_gid;
> +
> + if (valid & ATTR_ATIME)
> + obj->yst_atime = Y_TIME_CONVERT(attr->ia_atime);
> + if (valid & ATTR_CTIME)
> + obj->yst_ctime = Y_TIME_CONVERT(attr->ia_ctime);
> + if (valid & ATTR_MTIME)
> + obj->yst_mtime = Y_TIME_CONVERT(attr->ia_mtime);
> +
> + if (valid & ATTR_SIZE)
> + yaffs_resize_file(obj, attr->ia_size);
> +
> + yaffs_update_oh(obj, NULL, 1, 0, 0, NULL);
> +
> + return YAFFS_OK;
> +
> +}
> +
> +int yaffs_get_attribs(struct yaffs_obj *obj, struct iattr *attr)
> +{
> + unsigned int valid = 0;
> +
> + attr->ia_mode = obj->yst_mode;
> + valid |= ATTR_MODE;
> + attr->ia_uid = obj->yst_uid;
> + valid |= ATTR_UID;
> + attr->ia_gid = obj->yst_gid;
> + valid |= ATTR_GID;
> +
> + Y_TIME_CONVERT(attr->ia_atime) = obj->yst_atime;
> + valid |= ATTR_ATIME;
> + Y_TIME_CONVERT(attr->ia_ctime) = obj->yst_ctime;
> + valid |= ATTR_CTIME;
> + Y_TIME_CONVERT(attr->ia_mtime) = obj->yst_mtime;
> + valid |= ATTR_MTIME;
> +
> + attr->ia_size = yaffs_get_file_size(obj);
> + valid |= ATTR_SIZE;
> +
> + attr->ia_valid = valid;
> +
> + return YAFFS_OK;
> +}

Is there are reason this cannot be written as:

int yaffs_get_attribs(struct yaffs_obj *obj, struct iattr *attr)
{
attr->ia_mode = obj->yst_mode;
attr->ia_uid = obj->yst_uid;
attr->ia_gid = obj->yst_gid;
Y_TIME_CONVERT(attr->ia_atime) = obj->yst_atime;
Y_TIME_CONVERT(attr->ia_ctime) = obj->yst_ctime;
Y_TIME_CONVERT(attr->ia_mtime) = obj->yst_mtime;

attr->ia_size = yaffs_get_file_size(obj);
attr->ia_valid = (ATTR_MODE | ATTR_UID | ATTR_GID |
ATTR_ATIME | ATTR_CTIME | ATTR_MTIME |
ATTR_SIZE);
return YAFFS_OK;
}
Which is a bit more readable?

> diff --git a/fs/yaffs2/yaffs_attribs.h b/fs/yaffs2/yaffs_attribs.h
> new file mode 100644
> index 0000000..5b21b08
> --- /dev/null
> +++ b/fs/yaffs2/yaffs_attribs.h
> @@ -0,0 +1,28 @@
> +/*
> + * YAFFS: Yet another Flash File System . A NAND-flash specific file system.
> + *
> + * Copyright (C) 2002-2011 Aleph One Ltd.
> + * for Toby Churchill Ltd and Brightstar Engineering
> + *
> + * Created by Charles Manning <charles@aleph1.co.uk>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License version 2.1 as
> + * published by the Free Software Foundation.
> + *
> + * Note: Only YAFFS headers are LGPL, YAFFS C code is covered by GPL.
> + */
> +
> +#ifndef __YAFFS_ATTRIBS_H__
> +#define __YAFFS_ATTRIBS_H__
> +
> +#include "yaffs_guts.h"
> +
> +void yaffs_load_attribs(struct yaffs_obj *obj, struct yaffs_obj_hdr *oh);
> +void yaffs_load_attribs_oh(struct yaffs_obj_hdr *oh, struct yaffs_obj *obj);
> +void yaffs_attribs_init(struct yaffs_obj *obj, u32 gid, u32 uid, u32 rdev);
> +void yaffs_load_current_time(struct yaffs_obj *obj, int do_a, int do_c);
> +int yaffs_set_attribs(struct yaffs_obj *obj, struct iattr *attr);
> +int yaffs_get_attribs(struct yaffs_obj *obj, struct iattr *attr);
> +
> +#endif
> diff --git a/fs/yaffs2/yaffs_nameval.c b/fs/yaffs2/yaffs_nameval.c
> new file mode 100644
> index 0000000..e75411b
> --- /dev/null
> +++ b/fs/yaffs2/yaffs_nameval.c
> @@ -0,0 +1,201 @@
> +/*
> + * YAFFS: Yet Another Flash File System. A NAND-flash specific file system.
> + *
> + * Copyright (C) 2002-2011 Aleph One Ltd.
> + * for Toby Churchill Ltd and Brightstar Engineering
> + *
> + * Created by Charles Manning <charles@aleph1.co.uk>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * This simple implementation of a name-value store assumes a small number of
> +* values and fits into a small finite buffer.
> + *
> + * Each attribute is stored as a record:
> + * sizeof(int) bytes record size.
> + * strnlen+1 bytes name null terminated.
> + * nbytes value.
> + * ----------
> + * total size stored in record size

This code feels like it should have helper functions to minimise the
amount of pointer arithmetic and memcpy magic.

> + *
> + * This code has not been tested with unicode yet.
> + */
> +
> +#include "yaffs_nameval.h"
> +
> +#include "yportenv.h"
> +
> +static int nval_find(const char *xb, int xb_size, const YCHAR *name,
> + int *exist_size)
> +{
> + int pos = 0;
> + int size;
> +
> + memcpy(&size, xb, sizeof(int));
> + while (size > 0 && (size < xb_size) && (pos + size < xb_size)) {

You don't need thr parens around the expressions here.

> + if (!strncmp((YCHAR *) (xb + pos + sizeof(int)), name, size)) {

Make a helper function:

static YCHAR *nval_name(const char *xb, int pos)
{
return (YCHAR *)(xb + pos + sizeof(int));
}
Then:
if (!strncmp(nval_name(xb, pos), name, size)) {
Much easier to read.

> + if (exist_size)
> + *exist_size = size;
> + return pos;
> + }
> + pos += size;
> + if (pos < xb_size - sizeof(int))
> + memcpy(&size, xb + pos, sizeof(int));
> + else
> + size = 0;

The else here is effectively an error break with the test for size > 0
in the while loop? Why not remove the size > 0 test and just do a break
here? Probably reverse the test too:
if (pos >= xb_size - sizeof(int))
break;
memcpy(&size, xb + pos, sizeof(int));

> + }
> + if (exist_size)
> + *exist_size = 0;
> + return -ENODATA;
> +}
> +
> +static int nval_used(const char *xb, int xb_size)
> +{
> + int pos = 0;
> + int size;
> +
> + memcpy(&size, xb + pos, sizeof(int));
> + while (size > 0 && (size < xb_size) && (pos + size < xb_size)) {
> + pos += size;
> + if (pos < xb_size - sizeof(int))
> + memcpy(&size, xb + pos, sizeof(int));
> + else
> + size = 0;

Same here, remove the size > 0 test and just break from the loop.
> + }
> + return pos;
> +}
> +
> +int nval_del(char *xb, int xb_size, const YCHAR *name)
> +{
> + int pos = nval_find(xb, xb_size, name, NULL);
> + int size;
> +
> + if (pos < 0 || pos >= xb_size)
> + return -ENODATA;
> +
> + /* Find size, shift rest over this record,
> + * then zero out the rest of buffer */
> + memcpy(&size, xb + pos, sizeof(int));
> + memcpy(xb + pos, xb + pos + size, xb_size - (pos + size));
> + memset(xb + (xb_size - size), 0, size);
> + return 0;
> +}
> +
> +int nval_set(char *xb, int xb_size, const YCHAR *name, const char *buf,
> + int bsize, int flags)
> +{
> + int pos;
> + int namelen = strnlen(name, xb_size);
> + int reclen;
> + int size_exist = 0;
> + int space;
> + int start;

These int definitions could all go on one line.

> +
> + pos = nval_find(xb, xb_size, name, &size_exist);
> +
> + if (flags & XATTR_CREATE && pos >= 0)
> + return -EEXIST;
> + if (flags & XATTR_REPLACE && pos < 0)
> + return -ENODATA;
> +
> + start = nval_used(xb, xb_size);
> + space = xb_size - start + size_exist;
> +
> + reclen = (sizeof(int) + namelen + 1 + bsize);
> +
> + if (reclen > space)
> + return -ENOSPC;
> +
> + if (pos >= 0) {
> + nval_del(xb, xb_size, name);
> + start = nval_used(xb, xb_size);
> + }
> +
> + pos = start;
> +
> + memcpy(xb + pos, &reclen, sizeof(int));
> + pos += sizeof(int);
> + strncpy((YCHAR *) (xb + pos), name, reclen);
> + pos += (namelen + 1);
> + memcpy(xb + pos, buf, bsize);

static const char *nval_value(const char *xb, int pos, int namelen)
{
return xb + sizeof(int) + namelen + 1;
}
With the above helper replace the above with:

memcpy(xb + pos, &reclen, sizeof(int));
strncpy(nval_name(xb, pos), name, namelen);
strncpy(nval_value(xb, pos, namelen), buf, bsize);
pos += reclen;

Note the change from reclen to namelen in the first strncpy. I think
reclen is incorrect here?

> + return 0;
> +}
> +
> +int nval_get(const char *xb, int xb_size, const YCHAR * name, char *buf,
> + int bsize)
> +{
> + int pos = nval_find(xb, xb_size, name, NULL);
> + int size;
> +
> + if (pos >= 0 && pos < xb_size) {

if (pos >= xb_size)
return -ERANGE;
if (pos < 0)
return -ENODATA;
Then drop the indentation for the code below.

> +
> + memcpy(&size, xb + pos, sizeof(int));
> + pos += sizeof(int); /* advance past record length */
> + size -= sizeof(int);
> +
> + /* Advance over name string */
> + while (xb[pos] && size > 0 && pos < xb_size) {
> + pos++;
> + size--;
> + }
> + /*Advance over NUL */
> + pos++;
> + size--;
> +
> + if (size <= bsize) {
> + memcpy(buf, xb + pos, size);
> + return size;
> + }
> + }
> + if (pos >= 0)
> + return -ERANGE;
> +
> + return -ENODATA;
> +}
> +
> +int nval_list(const char *xb, int xb_size, char *buf, int bsize)
> +{
> + int pos = 0;
> + int size;
> + int name_len;
> + int ncopied = 0;
> + int filled = 0;

These can all go on one line.

> +
> + memcpy(&size, xb + pos, sizeof(int));
> + while (size > sizeof(int) &&
> + size <= xb_size &&
> + (pos + size) < xb_size &&
> + !filled) {
> + pos += sizeof(int);
> + size -= sizeof(int);
> + name_len = strnlen((YCHAR *) (xb + pos), size);

name_len = strnlen(nval_name(xb, pos), size);

> + if (ncopied + name_len + 1 < bsize) {
> + memcpy(buf, xb + pos, name_len * sizeof(YCHAR));
> + buf += name_len;
> + *buf = '\0';
> + buf++;
> + if (sizeof(YCHAR) > 1) {
> + *buf = '\0';
> + buf++;
> + }

This could be simplified as:

memset(buf, 0, sizeof(YCHAR));
buf += sizeof(YCHAR);

> + ncopied += (name_len + 1);
> + } else {
> + filled = 1;
> + }
> + pos += size;
> + if (pos < xb_size - sizeof(int))
> + memcpy(&size, xb + pos, sizeof(int));
> + else
> + size = 0;
> + }
> + return ncopied;
> +}
> +
> +int nval_hasvalues(const char *xb, int xb_size)
> +{
> + return nval_used(xb, xb_size) > 0;
> +}
> diff --git a/fs/yaffs2/yaffs_nameval.h b/fs/yaffs2/yaffs_nameval.h
> new file mode 100644
> index 0000000..951e64f
> --- /dev/null
> +++ b/fs/yaffs2/yaffs_nameval.h
> @@ -0,0 +1,28 @@
> +/*
> + * YAFFS: Yet another Flash File System . A NAND-flash specific file system.
> + *
> + * Copyright (C) 2002-2011 Aleph One Ltd.
> + * for Toby Churchill Ltd and Brightstar Engineering
> + *
> + * Created by Charles Manning <charles@aleph1.co.uk>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License version 2.1 as
> + * published by the Free Software Foundation.
> + *
> + * Note: Only YAFFS headers are LGPL, YAFFS C code is covered by GPL.
> + */
> +
> +#ifndef __NAMEVAL_H__
> +#define __NAMEVAL_H__
> +
> +#include "yportenv.h"
> +
> +int nval_del(char *xb, int xb_size, const YCHAR * name);
> +int nval_set(char *xb, int xb_size, const YCHAR * name, const char *buf,
> + int bsize, int flags);
> +int nval_get(const char *xb, int xb_size, const YCHAR * name, char *buf,
> + int bsize);
> +int nval_list(const char *xb, int xb_size, char *buf, int bsize);
> +int nval_hasvalues(const char *xb, int xb_size);
> +#endif


--
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon 5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934


\
 
 \ /
  Last update: 2011-02-09 23:35    [from the cache]
©2003-2011 Jasper Spaans