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

[Adnan Ali <adnan.ali@codethink.co.uk>]

On 12/03/13 22:37, Simon Glass wrote:
> Hi Adnan,
>
> On Tue, Mar 12, 2013 at 7:53 AM, Adnan Ali <adnan.ali@codethink.co.uk> wrote:
>> Hi
>>
>>
>> On 12/03/13 14:15, Simon Glass wrote:
>>> Hi Adnan,
>>>
>>> On Mon, Mar 11, 2013 at 6:18 AM, Adnan Ali <adnan.ali@codethink.co.uk>
>>> 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.
>>>>
>>>> v5:     merged with master.
>>>> v4:     btrls command added.
>>>> v3:     patch re-formated.
>>>> v2:     patch error removed.
>>>>
>>>> Signed-off-by: Adnan Ali <adnan.ali@codethink.co.uk>
>>>> ---
>>>>    Makefile                   |    1 +
>>>>    common/Makefile            |    1 +
>>>>    common/cmd_btr.c           |   65 +++
>>>>    fs/btrfs/Makefile          |   51 ++
>>>>    fs/btrfs/btrfs.c           | 1348
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>    fs/btrfs/crc32_c.c         |   54 ++
>>>>    fs/fs.c                    |   10 +
>>>>    include/btrfs.h            |  416 ++++++++++++++
>>>>    include/config_fallbacks.h |    4 +
>>>>    include/fs.h               |    1 +
>>>>    10 files changed, 1951 insertions(+)
>>>>    create mode 100644 common/cmd_btr.c
>>>>    create mode 100644 fs/btrfs/Makefile
>>>>    create mode 100644 fs/btrfs/btrfs.c
>>>>    create mode 100644 fs/btrfs/crc32_c.c
>>>>    create mode 100644 include/btrfs.h
>>>>
>>> I have ignored the code before this point in btrfs.c since it is
>>> imported and you want to keep the code style as is, but if you don't
>>> mind I will make a few comments after that point>
>>      Here are my comments
>>
>>>> +struct btrfs_info fs;
>>>> +
>>>> +/*
>>>> + *  mount btrfs file-system
>>>> + */
>>>> +int btrfs_probe(block_dev_desc_t *rbdd, disk_partition_t *info)
>>>> +{
>>>> +        btrfs_block_dev_desc = rbdd;
>>>> +        part_info = info;
>>>> +        btr_part_offset = info->start;
>>>> +       if (btrfs_fs_init(&fs) < 0 ) {
>>>> +          debug("btrfs probe failed\n");
>>>> +          return -1;
>>>> +       }
>>> You should use tabs for intend (some of this bit uses spaces, some uses
>>> tabs).
>>     Some how i missed will check it again. Ack
>>
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + *  Read file data
>>>> + */
>>>> +int btrfs_read_file(const char *filename, void *buf, int offset, int
>>>> len)
>>>> +{
>>>> +       int file_len=0;
>>>> +        int len_read;
>>>> +        struct com32_filedata filedata;
>>>> +        int handle;
>>>> +        if (offset != 0) {
>>>> +                debug("** Cannot support non-zero offset **\n");
>>>> +                return -1;
>>>> +        }
>>>> +
>>>> +        handle=btrfs_open_file(filename, &filedata);
>>>> +        if (handle < 0) {
>>>> +                debug("** File not found %s Invalid handle**\n",
>>>> filename);
>>>> +                return -1;
>>>> +        }
>>>> +
>>>> +        /*file handle is valid get the size of the file*/
>>>> +        len = filedata.size;
>>>> +        if (len == 0)
>>>> +                len = file_len;
>>>> +
>>>> +        len_read = getfssec(&filedata, (char *)buf);
>>>> +        if (len_read != len) {
>>>> +                debug("** Unable to read file %s **\n", filename);
>>>> +                return -1;
>>>> +        }
>>>> +
>>>> +        return len_read;
>>>> +
>>>> +}
>>>> +
>>>> +/*
>>>> + * Show directory entries
>>>> + */
>>>> +char btrfs_ls(char *dirname)
>>>> +{
>>>> +  struct btrfs_dirent *de;
>>>> +  struct _DIR_ *dir;
>>>> +
>>>> +  if(*dirname == '/' && *(dirname+1) == 0)
>>>> +     *dirname = '.';
>>>> +
>>>> +  dir = opendir(dirname);
>>>> +  if (dir == NULL)
>>>> +        return -1;
>>>> +
>>>> +  while ((de = readdir(dir)) != NULL)
>>>> +        ;
>>> This doesn't seem to print anything.
>>       readdir->btrfs_read, prints contents on media.
>>
>>>> +
>>>> +  return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + *  umount btrfs file-system
>>>> + */
>>>> +void btrfs_close(void)
>>>> +{
>>>> +
>>> Remove blank line
>>     Ack
>>
>>>> +}
>>>> diff --git a/fs/btrfs/crc32_c.c b/fs/btrfs/crc32_c.c
>>>> new file mode 100644
>>>> index 0000000..78d0447
>>>> --- /dev/null
>>>> +++ b/fs/btrfs/crc32_c.c
>>>> @@ -0,0 +1,54 @@
>>>> +/*
>>>> + * 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
>>>> + */
>>> Old comment?
>>     this crc was part of port
>>
>>>> +
>>>> +/*
>>>> + * Steps through buffer one byte at at time, calculates reflected
>>>> + * crc using table.
>>>> + */
>>>> +#include <linux/stat.h>
>>>> +#include <command.h>
>>>> +#include <asm/byteorder.h>
>>>> +#include <linux/compiler.h>
>>>> +#include <common.h>
>>>> +#include <config.h>
>>>> +
>>>> +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;
>>>> +}
>>>> +
>>>> +inline void crc32c_init(u32 *crc32c_table, u32 pol)
>>> The 'inline' isn't needed.
>>      Ack
>>
>>>> +{
>>>> +       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++) {
>>> Can remove this inner {} since you only have one line of code.
>>       Ack
>>
>>>> +                       v = (v >> 1) ^ ((v & 1) ? poly : 0);
>>>> +               }
>>>> +               crc32c_table[i] = v;
>>>> +       }
>>>> +}
>>> I suggest you move your crc_32.c file to lib/ since it might be more
>>> generally useful. You should probably include the function prototypes
>>> in include/crc.h.
>>    The crc32_c.c was part of port as header file and you previously said
>> to move to btrfs folder and make it c file.
>>
>>> Also you seem to recalculate the crc table on every operation. Is is
>>> possible to only do this ones on startup?
>>    Yes every time prob is called its calculated. It can be calculated at
>> boot time. Can you point me to file and function that it can be called.
>> before that can we make decision on crc32_c.c.
> Perhaps you could call it once in your btrfs_probe() function, and set
> a flag once you have done it once?
>
>>
>> Plus once i make all these changes will it be accepted than ;).
> I hope so. I think it fits in nicely. Did you have any comments as to
> why the btrload command doesn't have [bytes [pos] like fatload, for
> example?
   I will send patch v6 to cover all things you pointed out that isn't
   related to port.
   I think [bytes [pos]] can be done in btrfs. I prefered adding 
subvolume which
  was unique to btrfs. I can add that later send patch for this, once 
this goes
  in main stream.  If that is acceptable to you.
>
> Regards,
> Simon
>
>>>
>>> Regards,
>>> Simon
>> Regards
>> Adnan
>>
Thanks
Adnan