[U-Boot] [PATCH v3] Introduced btrfs file-system with btrload command

[Tom Rini <trini@ti.com>]

On Mon, Feb 25, 2013 at 12:24:37PM +0000, Adnan Ali wrote:

> Introduces btrfs file-system to read file
> from volume/sub-volumes with btrload command. This
> implementation has read-only support.
> This btrfs implementation is based on syslinux btrfs
> code, commit 269ebc845ebc8b46ef4b0be7fa0005c7fdb95b8d.
> 
> Signed-off-by: Adnan Ali <adnan.ali@codethink.co.uk>

A few things:
- In general in fs/btrfs/btrfs.c I see some coding style problems (lack
  of spacing, non-printf's longer than 80-wide).  Do these come from
  syslinux and thus will make any re-syncs easier?
- It looks like you added support for CONFIG_CMD_FS_GENERIC, if so did
  you test that?
- Can you please enable this support code on at least one platform,
  preferably the one you've tested and developed with?

[snip]
> diff --git a/fs/fs.c b/fs/fs.c
[snip]
> +        //file handle is valid get the size of the file

/* Style comments only */

> +        len=filedata.size;

And spacing between variable and assignment.

> @@ -178,7 +248,6 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype)
>  	for (i = 0; i < ARRAY_SIZE(fstypes); i++) {
>  		if ((fstype != FS_TYPE_ANY) && (fstype != fstypes[i].fstype))
>  			continue;
> -
>  		if (!fstypes[i].probe()) {
>  			fs_type = fstypes[i].fstype;
>  			return 0;
[snip]
> @@ -208,7 +280,6 @@ static void fs_close(void)
>  int fs_ls(const char *dirname)
>  {
>  	int ret;
> -
>  	switch (fs_type) {
>  	case FS_TYPE_FAT:
>  		ret = fs_ls_fat(dirname);

Unrelated, please drop.

> @@ -237,11 +311,13 @@ int fs_read(const char *filename, ulong addr, int offset, int len)
>  	case FS_TYPE_EXT:
>  		ret = fs_read_ext(filename, addr, offset, len);
>  		break;
> +        case FS_TYPE_BTR:
> +		ret = fs_read_btr(filename, addr, offset, len);
> +		break; 
>  	default:
>  		ret = fs_read_unsupported(filename, addr, offset, len);
>  		break;
>  	}
> -
>  	fs_close();

And unrelated whitespace changes here as well.

> diff --git a/include/btrfs.h b/include/btrfs.h
[snip]
> +/*
> + * Extent structure: contains the mapping of some chunk of a file
> + * that is contiguous on disk.
> + */
> +struct extent {
> +    //sector_t    pstart;         /* Physical start sector */
> +    __le64   pstart;

Fix please.

> +/*
> + * Our definition of "not whitespace"
> + */
> +static inline char not_whitespace(char c)
> +{
> +  return (unsigned char)c > ' ';
> +}

Can't you just use isspace from <linux/ctypes.h> ?

> diff --git a/include/crc32c.h b/include/crc32c.h
> new file mode 100644
> index 0000000..d04916e
> --- /dev/null
> +++ b/include/crc32c.h
> @@ -0,0 +1,48 @@
> +/*
> + * Copied from Linux kernel crypto/crc32c.c
> + * Copyright (c) 2004 Cisco Systems, Inc.
> + * Copyright (c) 2008 Herbert Xu <herbert@gondor.apana.org.au>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + *
> + */
> +
> +/*
> + * This is the CRC-32C table
> + * Generated with:
> + * width = 32 bits
> + * poly = 0x1EDC6F41
> + * reflect input bytes = true
> + * reflect output bytes = true
> + */
> +
> +/*
> + * Steps through buffer one byte at at time, calculates reflected
> + * crc using table.
> + */
> +
> +static inline u32 crc32c_cal(u32 crc, const char *data, size_t length, u32 *crc32c_table)
> +{
> +	while (length--)
> +		crc = crc32c_table[(u8)(crc ^ *data++)] ^ (crc >> 8);
> +
> +	return crc;
> +}
> +
> +static inline void crc32c_init(u32 *crc32c_table, u32 pol)
> +{
> +	int i, j;
> +	u32 v;
> +	const u32 poly = pol; /* Bit-reflected CRC32C polynomial */
> +
> +	for (i = 0; i < 256; i++) {
> +		v = i;
> +		for (j = 0; j < 8; j++) {
> +			v = (v >> 1) ^ ((v & 1) ? poly : 0);
> +		}
> +		crc32c_table[i] = v;
> +	}
> +}

Simon, since you've just been over all the crc32 recently, do we have
these functions somewhere already that can be easily tapped in to?
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130225/e1521cf0/attachment.pgp>