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

[Simon Glass]

Hi,

On Mon, Feb 25, 2013 at 7:04 AM, Tom Rini <trini at ti.com> wrote:
> 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 at 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 at 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!

Should be - see lib/crc32.c. There is already at least one other copy
(ubifs I think) so we should try to avoid adding more.

Maybe the polynomial is different here? But even so it should go with
the existing code I think.

Regards,
Simon


>
> --
> Tom