[U-Boot] [PATCH V2 3/3] FAT: implement fat_set_blk_dev(), convert cmd_fat.c

[Stephen Warren]

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.