On 10/09/2012 05:06 PM, Benoît Thébaudeau wrote:
> Hi Stephen,
>
> See my comments inlined below.
>
> On Tuesday, October 9, 2012 10:41:41 PM, Stephen Warren wrote:
>> This makes the FAT filesystem API more consistent with other
>> block-based
>> filesystems. If in the future standard multi-filesystem commands such as
>> "ls" or "load" are implemented, having FAT work the same way as other
>> filesystems will be necessary.
>>
>> Convert cmd_fat.c to the new API, so the code looks more like other files
>> implementing the same commands for other filesystems.
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> +int fat_register_device(block_dev_desc_t *dev_desc, int part_no)
>> +{
>> + disk_partition_t info;
>> +
>> + /* First close any currently found FAT filesystem */
>> + cur_dev = NULL;
>> +
>> + /* Read the partition table, if present */
>> + if (get_partition_info(dev_desc, part_no, &info)) {
>> + if (part_no != 0) {
>> + printf("** Partition %d not valid on device %d **\n",
>> + part_no, dev_desc->dev);
>> + return -1;
>> + }
>> +
>> + info.part = 0;
>> + info.start = 0;
>> + info.size = dev_desc->lba;
>> + info.blksz = dev_desc->blksz;
>> + memset(info.name, 0, sizeof(info.name));
>> + memset(info.type, 0, sizeof(info.type));
>> +#ifdef CONFIG_PARTITION_UUIDS
>> + memset(info.uuid, 0, sizeof(info.uuid));
>> +#endif
>> + info.bootable = 0;
>
> Sorry for pointing this out only now, but now that cmd_fat.c has been switched
> to fat_set_blk_dev(), the code above is handled for this file by
> get_device_and_partition():
> ---
> info->start = 0;
> info->size = (*dev_desc)->lba;
> info->blksz = (*dev_desc)->blksz;
> info->bootable = 0;
> #ifdef CONFIG_PARTITION_UUIDS
> info->uuid[0] = 0;
> #endif
> ---
>
> This code does not initialize the fields part, name and type, and it clears
> only
> the 1st byte of the uuid field. Is this on purpose for some reason, or is this
> an issue?
get_device_and_partition() should set indeed set info->part in the
whole-disk case; I'll fix that.
I think get_device_and_partition() never cleared name or type in this
case; I guess that's a pre-existing inconsistency between the two pieces
of code? I'll have a look and add a cleanup patch to make the code
consistent.
The uuid field is a string, so clearing just the first byte is fine; I
ended up using memset in the new code in this patch since the existing
fat_register_device() implementation used memset() on name and type;
something I imagine is not strictly necessary. Since the function is
being re-written anyway, I'll take a look to make sure that clearing
only the first byte is fine, and remove all the memsets and just zero
out the first byte.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
