lkml.org 
[lkml]   [2011]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    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    [W:0.055 / U:3.056 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site