Re: [PATCH 6/8] Add yaffs2 file system: xattrib code

[Jesper Juhl]

A few minor comments can be found below.


On Wed, 1 Dec 2010, Charles Manning wrote:

> Signed-off-by: Charles Manning <cdhmanning@xxxxxxxxx>
> ---
>  fs/yaffs2/yaffs_nameval.c |  201 +++++++++++++++++++++++++++++++++++++++++++++
>  fs/yaffs2/yaffs_nameval.h |   28 ++++++
>  2 files changed, 229 insertions(+), 0 deletions(-)
>  create mode 100644 fs/yaffs2/yaffs_nameval.c
>  create mode 100644 fs/yaffs2/yaffs_nameval.h
> 
> diff --git a/fs/yaffs2/yaffs_nameval.c b/fs/yaffs2/yaffs_nameval.c
> new file mode 100644
> index 0000000..d8c548a
> --- /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-2010 Aleph One Ltd.
> + *   for Toby Churchill Ltd and Brightstar Engineering
> + *
> + * Created by Charles Manning <charles@xxxxxxxxxxxx>
> + *
> + * 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 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)) {
> +		if (yaffs_strncmp
> +		    ((YCHAR *) (xb + pos + sizeof(int)), name, size) == 0) {
> +			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;
> +	}
> +	if (exist_size)
> +		*exist_size = 0;
> +	return -1;
> +}
> +
> +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;
> +	}
> +	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) {
> +		/* 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;
> +	} else {
> +		return -ENODATA;
> +        }
> +}

Why not get rid of the 'else' branch and simply write this as 

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) {
             /* 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;
     }
     return -ENODATA;
}


> +
> +int nval_set(char *xb, int xb_size, const YCHAR * name, const char *buf,
                                                 ^^^^^

Why this insistance on making pointer variables look like 
multiplication...?
  int* foo; // prefered by many C++ people
  int *foo; // prefered by many C people
  int * foo; // just plain ugly
(this is not the only location, but I'm not going to point out any more)


> +	     int bsize, int flags)
> +{
> +	int pos;
> +	int namelen = yaffs_strnlen(name, xb_size);
> +	int reclen;
> +	int size_exist = 0;
> +	int space;
> +	int start;
> +
> +	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);
> +	yaffs_strncpy((YCHAR *) (xb + pos), name, reclen);
> +	pos += (namelen + 1);
> +	memcpy(xb + pos, buf, bsize);
> +	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) {
> +
> +		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;
> +	else
> +		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;
> +
> +	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 = yaffs_strnlen((YCHAR *) (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++;
> +			}
> +			ncopied += (name_len + 1);
> +		} else {
> +			filled = 1;
> +                }
Trivial little nit. The line above this comment is indented with spaces, 
not tabs as it should be.


> +		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..2bb02b6
> --- /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-2010 Aleph One Ltd.
> + *   for Toby Churchill Ltd and Brightstar Engineering
> + *
> + * Created by Charles Manning <charles@xxxxxxxxxxxx>
> + *
> + * 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
> 

-- 
Jesper Juhl <jj@xxxxxxxxxxxxx>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/