[U-Boot] [RFC] fs: make it possible to read the filesystem UUID

[Stephen Warren <swarren@nvidia.com>]

On 10/29/2014 06:15 AM, Christian Gmeiner wrote:
> Hi all.
> 
> Adding Stephen Warren, Simon Glass and Pavel Machek to CC list
> (./scripts/get_maintainer.pl -f fs/fs.c).
> 
> 2014-10-22 15:29 GMT+02:00 Christian Gmeiner <christian.gmeiner@gmail.com>:
>> Some filesystems have a UUID stored in its superblock. To
>> allow using root=UUID=... for the kernel command line we
>> need a way to read-out the filesystem UUID. This is what this
>> patch tries to do.

Well, you can always to root=PARTUUID= instead of root=UUID=. The
advantage is that the kernel handles that so you don't even need an initrd.

Still, having this feature seems useful for systems that expect to parse
root=UUID= in an initrd.

>>  common/Makefile       |  1 +
>>  common/cmd_fs_uuid.c  | 23 +++++++++++++++++++++++
>>  fs/ext4/ext4_common.c |  9 +++++++++
>>  fs/fs.c               | 28 ++++++++++++++++++++++++++++
>>  include/ext4fs.h      |  1 +
>>  include/fs.h          |  2 ++

You should document the new CONFIG define in the README.

> diff --git a/common/cmd_fs_uuid.c b/common/cmd_fs_uuid.c

> +U_BOOT_CMD(
> +       fs_uuid,        3,      0,      do_fs_uuid_wrapper,
> +       "filesystem UUDI related commands",

Typo ................ ^^^^

And not a great command description. How about:

Look up a filesystem UUID

> +       "<interface> <dev>:<part>\n"
> +       "    - print filesystem UUID\n"
> +);

Other commands don't have _ in the name. How about just fsuuid instead?

>> diff --git a/fs/fs.c b/fs/fs.c

>> @@ -85,6 +85,7 @@ struct fstype_info {
>>         int (*size)(const char *filename);
>>         int (*read)(const char *filename, void *buf, int offset, int len);
>>         int (*write)(const char *filename, void *buf, int offset, int len);
>> +       void (*uuid)(char *uuid_str);

This new function should return an error code. The ext4 implementation
in this patch doesn't always look up the UUID...

>> +void fs_uuid(char *uuid_str)
>> +{
>> +       struct fstype_info *info = fs_get_info(fs_type);
>> +
>> +       if (info->uuid)
>> +               info->uuid(uuid_str);
>> +}

This function should propagate the error code back to the caller, and
return an error itself if !info->uuid.

>> +int do_fs_uuid(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>> +               int fstype)
>> +{
>> +       char uuid[37];
>> +       memset(uuid, 0, sizeof(uuid));
>> +
>> +       if (argc != 3)
>> +               return CMD_RET_USAGE;
>> +
>> +       if (fs_set_blk_dev(argv[1], argv[2], fstype))
>> +               return 1;
>> +
>> +       fs_uuid(uuid);
>> +       setenv("fs_uuid", uuid);

I'd prefer if the function didn't unconditionally set a hard-coded
environment variable name. Rather, take a look at how the "part uuid"
command is implemented:

part uuid mmc 0:1         - print out the UUID
part uuid mmc 0:1 uuidvar - set uuidvar environment variable to the UUID

->
fsuuid mmc 0:1
fsuuid mmc 0:1 uuidvar

This allows interactive users to easily print out the value, whereas
scripts can control which environment variable the data is stored in, in
case they need to look up multiple different UUIDs or just want to use a
meaningful non-hard-coded variable name.