[U-Boot] [PATCH] cmd: part: unify syntax of uuid according to start/size subcommands
From: Roman Stratiienko roman.stratiienko@globallogic.com
This allows retrieving uuid of the partition using it's name.
Signed-off-by: Roman Stratiienko roman.stratiienko@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" );
Hi Roman,
Adding the maintainers: $ scripts/get_maintainer.pl --no-l --no-rolestats this.patch Philipp Tomsich philipp.tomsich@theobroma-systems.com Kever Yang kever.yang@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@nvidia.com 7 Simon Glass sjg@chromium.org 5 Bin Meng bmeng.cn@gmail.com 3 Tom Rini trini@konsulko.com 3 Sjoerd Simons sjoerd.simons@collabora.co.uk 2 Stefan Roese sr@denx.de 2 Sam Protsenko semen.protsenko@linaro.org 2 Przemyslaw Marczak p.marczak@samsung.com 2 Paul Kocialkowski contact@paulk.fr 2 Lukasz Majewski lukma@denx.de 2 Heiko Schocher hs@denx.de 1 Patrick Delaunay patrick.delaunay@st.com
On Thu, Apr 25, 2019 at 02:18:22PM +0300, roman.stratiienko@globallogic.com wrote:
From: Roman Stratiienko roman.stratiienko@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@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;@@ -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"
);
On 4/25/19 1:36 PM, Eugeniu Rosca wrote:
Hi Roman,
...
On Thu, Apr 25, 2019 at 02:18:22PM +0300, roman.stratiienko@globallogic.com wrote:
From: Roman Stratiienko roman.stratiienko@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:
("cmd: part: Allow passing partition name to start and size")
("[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:
Yes, I don't think you want to change the cmdline format for this command, or all existing use-cases will break. That's not just the scripts you mentioned, but also people's own scripts stored on their own boot media or U-Boot environments, or just their memory of how to run the commands.
Right now IIRC the following works:
part uuid mmc 1:1
Perhaps we can make the following work, where the partitionk ID parameter isn't a simple integer, or where that ID doesn't exist:
part uuid mmc 1:"partname"
That should be backwards-compatible, and a sane enough syntax.
BTW, it looks like the patch also re-orders a bunch of code while editing it. I guess this is to keep the code that handles all the sub-commands in alphabetical order, which is fine. However, such cleanup should be a separate patch, so that the patch which introduces new/changed behaviour /only/ does that, so it's clear what's going on.
participants (3)
-
Eugeniu Rosca -
roman.stratiienkoļ¼ globallogic.com -
Stephen Warren