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

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.