[PATCH v3 8/8] cmd: gpt: Add command to swap partition order

[Heinrich Schuchardt]

On 8/25/23 21:38, Joshua Watt wrote:
> Adds a command called "gpt swap-postition" which will swap the order two
> partitions are listed in the GPT partition table (but leaves them
> pointing to the same locations on disk).

Why is this functionality needed in a boot firmware?

> 
> Signed-off-by: Joshua Watt <JPEWhacker at gmail.com>
> Reviewed-by: Simon Glass <sjg at chromium.org > ---
>   cmd/gpt.c                 | 52 ++++++++++++++++++++++++++++++++++++---
>   doc/usage/cmd/gpt.rst     | 23 +++++++++++++++++
>   test/py/tests/test_gpt.py | 18 ++++++++++++++
>   3 files changed, 90 insertions(+), 3 deletions(-)
> 
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index 0479b3a6a4..3c39446258 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -858,8 +858,9 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
>   	u8 part_count = 0;
>   	int partlistlen, ret, numparts = 0, partnum, i = 1, ctr1 = 0, ctr2 = 0;
>   
> -	if ((subcomm == NULL) || (name1 == NULL) || (name2 == NULL) ||
> -	    (strcmp(subcomm, "swap") && (strcmp(subcomm, "rename"))))
> +	if (!subcomm || !name1 || !name2 ||
> +	    (strcmp(subcomm, "swap") && strcmp(subcomm, "rename") &&
> +	     strcmp(subcomm, "swap-position")))
>   		return -EINVAL;
>   
>   	ret = get_disk_guid(dev_desc, disk_guid);
> @@ -920,6 +921,47 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
>   			ret = -EINVAL;
>   			goto out;
>   		}
> +	} else if (!strcmp(subcomm, "swap-position")) {
> +		int idx1, idx2;
> +		struct disk_partition first, second;
> +
> +		idx1 = simple_strtoul(name1, NULL, 10);
> +		if (idx1 <= 0 || idx1 > numparts) {

The number of partitions is irrelevant here.

Take a partition table with two partitions 37 and 63. numparts is 2. But 
of course you can swap the two. You have to check that idx1 relates to 
an existing partition here.

I would have preferred to see the numerous bugs in the gpt command to be 
fixed before adding new functionality.

> +			printf("Illegal partition number %s\n", name1);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		idx2 = simple_strtoul(name2, NULL, 10);
> +		if (idx2 <= 0 || idx2 > numparts) {
> +			printf("Illegal partition number %s\n", name2);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		if (idx1 == idx2) {
> +			printf("Cannot swap partition with itself\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		i = 1;
> +		list_for_each(pos, &disk_partitions) {
> +			curr = list_entry(pos, struct disk_part, list);
> +			if (i == idx1)
> +				first = curr->gpt_part_info;
> +			else if (i == idx2)
> +				second = curr->gpt_part_info;
> +			i++;
> +		}
> +
> +		i = 1;
> +		list_for_each(pos, &disk_partitions) {
> +			curr = list_entry(pos, struct disk_part, list);
> +			if (i == idx1)
> +				curr->gpt_part_info = second;
> +			else if (i == idx2)
> +				curr->gpt_part_info = first;
> +			i++;
> +		}
>   	} else { /* rename */
>   		if (strlen(name2) > PART_NAME_LEN) {
>   			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
> @@ -1123,7 +1165,8 @@ static int do_gpt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   	} else if (strcmp(argv[1], "read") == 0) {
>   		ret = do_get_gpt_info(blk_dev_desc, (argc == 5) ? argv[4] : NULL);
>   	} else if ((strcmp(argv[1], "swap") == 0) ||
> -		   (strcmp(argv[1], "rename") == 0)) {
> +		   (strcmp(argv[1], "rename") == 0) ||
> +		   (strcmp(argv[1], "swap-position") == 0)) {
>   		ret = do_rename_gpt_parts(blk_dev_desc, argv[1], argv[4], argv[5]);
>   	} else if ((strcmp(argv[1], "set-bootable") == 0)) {
>   		ret = gpt_set_bootable(blk_dev_desc, argv[4]);
> @@ -1176,6 +1219,8 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
>   	" gpt swap <interface> <dev> <name1> <name2>\n"
>   	"    - change all partitions named name1 to name2\n"
>   	"      and vice-versa\n"
> +	" gpt swap-position <interface> <dev> <part1> <part2>\n"
> +	"    - Swap the order of name1 with name2 in the partition table\n"
>   	" gpt rename <interface> <dev> <part> <name>\n"
>   	"    - rename the specified partition\n"
>   	" gpt set-bootable <interface> <dev> <list>\n"
> @@ -1184,5 +1229,6 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
>   	" gpt swap mmc 0 foo bar\n"
>   	" gpt rename mmc 0 3 foo\n"
>   	" gpt set-bootable mmc 0 boot_a,boot_b\n"
> +	" gpt swap-position mmc 0 1 2\n"
>   #endif
>   );
> diff --git a/doc/usage/cmd/gpt.rst b/doc/usage/cmd/gpt.rst
> index 353dfb05cf..bc14bda6e5 100644
> --- a/doc/usage/cmd/gpt.rst
> +++ b/doc/usage/cmd/gpt.rst
> @@ -17,6 +17,7 @@ Synopsis
>       gpt read <interface> <device no> [<varname>]
>       gpt swap <interface> <dev> <name1> <name2>
>       gpt rename <interface> <dev> <part> <name>
> +    gpt swap-position <interface> <dev> <part1> <part2>
>       gpt set-bootable <interface> <dev> <partition list>
>   
>   Description
> @@ -138,6 +139,28 @@ CONFIG_CMD_GPT_RENAME=y is required.
>   The 'gpt rename' command renames all partitions named 'part' to be 'name1'.
>   CONFIG_CMD_GPT_RENAME=y is required.
>   
> +The 'gpt swap-position' command swaps the order of two partition index in the

Two requires a plural form.

You don't swap indices, you swap the partition table entries indexed by 
part1 and part2.

> +partition table, but otherwise leaves the actual partition data untouched. For
> +example, to swap the order of the 'boot' and 'rootfs' partitions in the disk

You don't swap the order in the disks. You swap the entries in the 
partition table.

Best regards

Heinrich

> +described above, you would do:
> +
> +::
> +    => gpt setenv mmc 0 rootfs
> +    => echo ${gpt_partition_entry}
> +    2
> +    => gpt setenv mmc 0 boot
> +    => echo ${gpt_partition_entry}
> +    1
> +
> +    => gpt swap-position mmc 0 1 2
> +
> +    => gpt setenv mmc 0 rootfs
> +    => echo ${gpt_partition_entry}
> +    1
> +    => gpt setenv mmc 0 boot
> +    => echo ${gpt_partition_entry}
> +    2
> +
>   The 'gpt set-bootable' command sets the bootable flag for all partitions in the
>   table. If the partition name is in 'partition list' (separated by ','), the
>   bootable flag is set, otherwise it is cleared. CONFIG_CMD_GPT_RENAME=y is
> diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py
> index 666d387ac8..3d23b98cea 100644
> --- a/test/py/tests/test_gpt.py
> +++ b/test/py/tests/test_gpt.py
> @@ -330,3 +330,21 @@ def test_gpt_write(state_disk_image, u_boot_console):
>       output = u_boot_console.run_command('gpt guid host 0')
>       assert '375a56f7-d6c9-4e81-b5f0-09d41ca89efe' in output
>   
> + at pytest.mark.buildconfigspec('cmd_gpt')
> + at pytest.mark.buildconfigspec('cmd_gpt_rename')
> + at pytest.mark.buildconfigspec('cmd_part')
> + at pytest.mark.requiredtool('sgdisk')
> +def test_gpt_swap_position(state_disk_image, u_boot_console):
> +    """Test the gpt swap-position command."""
> +
> +    u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
> +    output = u_boot_console.run_command('part list host 0')
> +    assert '1\t0x00000800\t0x00000fff\t"part1"' in output
> +    assert '2\t0x00001000\t0x00001bff\t"part2"' in output
> +
> +    output = u_boot_console.run_command('gpt swap-position host 0 1 2')
> +    assert 'success!' in output
> +
> +    output = u_boot_console.run_command('part list host 0')
> +    assert '2\t0x00000800\t0x00000fff\t"part1"' in output
> +    assert '1\t0x00001000\t0x00001bff\t"part2"' in output