[U-Boot] cmd: part: unify syntax of uuid according to start/size subcommands

[Eugeniu Rosca]

Hi Roman,

Adding the maintainers:
$ scripts/get_maintainer.pl --no-l --no-rolestats this.patch
  Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
  Kever Yang <kever.yang at rock-chips.com>

Adding the top contributors:
$ git log --follow u-boot/master -- cmd/part.c | grep -o "\-by:.*" | \
        sed 's/\-by: //' | sort | uniq -c | sort -rn | head
     11 Stephen Warren <swarren at nvidia.com>
      7 Simon Glass <sjg at chromium.org>
      5 Bin Meng <bmeng.cn at gmail.com>
      3 Tom Rini <trini at konsulko.com>
      3 Sjoerd Simons <sjoerd.simons at collabora.co.uk>
      2 Stefan Roese <sr at denx.de>
      2 Sam Protsenko <semen.protsenko at linaro.org>
      2 Przemyslaw Marczak <p.marczak at samsung.com>
      2 Paul Kocialkowski <contact at paulk.fr>
      2 Lukasz Majewski <lukma at denx.de>
      2 Heiko Schocher <hs at denx.de>
      1 Patrick Delaunay <patrick.delaunay at st.com>

On Thu, Apr 25, 2019 at 02:18:22PM +0300, roman.stratiienko at globallogic.com wrote:
> From: Roman Stratiienko <roman.stratiienko at globallogic.com>
> 
> This allows retrieving uuid of the partition using it's name.

IMHO not seeing any real-life motivation (backed up by use-cases and,
ideally, some commands/console output) in the patch description is the
right recipe for getting no feedback from community. Fortunately, I am
willing to put some time to fill this gap.

The story which I see behind the patch is that you are unhappy about
not being able to pass the partition name in order to get the partition
uuid. Currently, 'part' does require the partition index as input:

 => part uuid mmc 1:1
 d117f98e-6f2c-d04b-a5b2-331a19f91cb2
 => part uuid mmc 1:misc
 ** Bad partition specification mmc 1:misc **

It looks to me that pretty much the same driving force guided:
 - http://git.denx.de/?p=u-boot.git;a=commitdiff;h=36df616a2
  ("cmd: part: Allow passing partition name to start and size")
 - https://patchwork.ozlabs.org/patch/1044151/
  ("[U-Boot,v3,1/7] cmd: part: Add 'number' sub-command")

So, the motivation is clear to me (and I share it!).

The problem which I see with this patch is that it changes the usage
pattern of the 'part uuid' sub-command, which breaks current (mainline
and potential out-of-tree) users of 'part uuid'. Below occurrences in
u-boot/master will require an update if this patch is merged as-is:

$ git grep 'part uuid ' -- include/
include/configs/embestmx6boards.h:	"finduuid=part uuid mmc 0:1 uuid\0" \
include/configs/imx6_logic.h:	"finduuid=part uuid mmc ${mmcdev}:2 uuid\0" \
include/configs/mx6cuboxi.h:	"finduuid=part uuid mmc 0:1 uuid\0" \
include/configs/mx6sabre_common.h:	"finduuid=part uuid mmc ${mmcdev}:2 uuid\0" \
include/configs/mx6slevk.h:	"finduuid=part uuid mmc 1:2 uuid\0" \
include/configs/mx6sxsabresd.h:	"finduuid=part uuid mmc 2:2 uuid\0" \
include/configs/pico-imx6ul.h:	"finduuid=part uuid mmc 0:1 uuid\0" \
include/configs/pico-imx7d.h:	"finduuid=part uuid mmc 0:1 uuid\0" \
include/configs/theadorable-x86-common.h:	"setroot=part uuid scsi 0:${partnr} uuid;"		\
include/configs/wandboard.h:	"finduuid=part uuid mmc 0:1 uuid\0" \
include/configs/warp.h:	"finduuid=part uuid mmc 0:2 uuid\0" \
include/configs/warp7.h:	"finduuid=part uuid mmc 0:${rootpart} uuid\0" \
include/environment/ti/mmc.h:	"finduuid=part uuid mmc ${bootpart} uuid\0" \

My personal opinion is that it is a bit late to change the usage of
'part uuid' (I would be happy if that's wrong!).

To maintain backward compatibility, I see below solutions:
 - change the blk_get_device_part_str() API in disk/part.c so that
it accepts not only partition index, but also partition name as input.
This affects a lot of blk_get_device_part_str() users/callers, hence
requires some global approval. I doubt this is chosen in the end.
 - tune the do_part_uuid() in cmd/part.c similar to the aforementioned
commit http://git.denx.de/?p=u-boot.git;a=commitdiff;h=36df616a2
("cmd: part: Allow passing partition name to start and size").

Any likes and dislikes, yes and no on the above?

> 
> Signed-off-by: Roman Stratiienko <roman.stratiienko at globallogic.com>
> ---
>  cmd/part.c | 40 ++++++++++------------------------------
>  1 file changed, 10 insertions(+), 30 deletions(-)
> 
> diff --git a/cmd/part.c b/cmd/part.c
> index bee204f..57657ec 100644
> --- a/cmd/part.c
> +++ b/cmd/part.c
> @@ -24,31 +24,9 @@
>  enum cmd_part_info {
>  	CMD_PART_INFO_START = 0,
>  	CMD_PART_INFO_SIZE,
> +	CMD_PART_INFO_UUID,
>  };
>  
> -static int do_part_uuid(int argc, char * const argv[])
> -{
> -	int part;
> -	struct blk_desc *dev_desc;
> -	disk_partition_t info;
> -
> -	if (argc < 2)
> -		return CMD_RET_USAGE;
> -	if (argc > 3)
> -		return CMD_RET_USAGE;
> -
> -	part = blk_get_device_part_str(argv[0], argv[1], &dev_desc, &info, 0);
> -	if (part < 0)
> -		return 1;
> -
> -	if (argc > 2)
> -		env_set(argv[2], info.uuid);
> -	else
> -		printf("%s\n", info.uuid);
> -
> -	return 0;
> -}
> -
>  static int do_part_list(int argc, char * const argv[])
>  {
>  	int ret;
> @@ -149,6 +127,9 @@ static int do_part_info(int argc, char * const argv[], enum cmd_part_info param)
>  	case CMD_PART_INFO_SIZE:
>  		snprintf(buf, sizeof(buf), LBAF, info.size);
>  		break;
> +	case CMD_PART_INFO_UUID:
> +		snprintf(buf, sizeof(buf), "%s", info.uuid);
> +		break;
>  	default:
>  		printf("** Unknown cmd_part_info value: %d\n", param);
>  		return 1;
> @@ -177,14 +158,14 @@ static int do_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	if (argc < 2)
>  		return CMD_RET_USAGE;
>  
> -	if (!strcmp(argv[1], "uuid"))
> -		return do_part_uuid(argc - 2, argv + 2);
> -	else if (!strcmp(argv[1], "list"))
> +	if (!strcmp(argv[1], "list"))
>  		return do_part_list(argc - 2, argv + 2);
>  	else if (!strcmp(argv[1], "start"))
>  		return do_part_start(argc - 2, argv + 2);
>  	else if (!strcmp(argv[1], "size"))
>  		return do_part_size(argc - 2, argv + 2);
> +	else if (!strcmp(argv[1], "uuid"))
> +		return do_part_info(argc - 2, argv + 2, CMD_PART_INFO_UUID);
>  
>  	return CMD_RET_USAGE;
>  }
> @@ -192,10 +173,6 @@ static int do_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  U_BOOT_CMD(
>  	part,	CONFIG_SYS_MAXARGS,	1,	do_part,
>  	"disk partition related commands",
> -	"uuid <interface> <dev>:<part>\n"
> -	"    - print partition UUID\n"
> -	"part uuid <interface> <dev>:<part> <varname>\n"
> -	"    - set environment variable to partition UUID\n"
>  	"part list <interface> <dev>\n"
>  	"    - print a device's partition table\n"
>  	"part list <interface> <dev> [flags] <varname>\n"
> @@ -206,5 +183,8 @@ U_BOOT_CMD(
>  	"      part can be either partition number or partition name\n"
>  	"part size <interface> <dev> <part> <varname>\n"
>  	"    - set environment variable to the size of the partition (in blocks)\n"
> +	"      part can be either partition number or partition name\n"
> +	"part uuid <interface> <dev> <part> <varname>\n"
> +	"    - set environment variable to the uuid of the partition\n"
>  	"      part can be either partition number or partition name"
>  );

-- 
Best Regards,
Eugeniu.