Hi Adnan,
On Tue, Mar 12, 2013 at 7:53 AM, Adnan Ali <adnan....@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....@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....@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 <herb...@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?
Regards,
Simon
>>
>>
>> Regards,
>> Simon
>
> Regards
> Adnan
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
