Re: [PATCH v3 2/8] doc: Add gpt command documentation

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 8/25/23 21:38, Joshua Watt wrote:
> Adds initial documentation for the gpt command

Thanks a lot for filling the gap.

>
> Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
> ---
>   doc/usage/cmd/gpt.rst | 141 ++++++++++++++++++++++++++++++++++++++++++
>   doc/usage/index.rst   |   1 +
>   2 files changed, 142 insertions(+)
>   create mode 100644 doc/usage/cmd/gpt.rst
>
> diff --git a/doc/usage/cmd/gpt.rst b/doc/usage/cmd/gpt.rst
> new file mode 100644
> index 0000000000..f6e082fb94
> --- /dev/null
> +++ b/doc/usage/cmd/gpt.rst
> @@ -0,0 +1,141 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +gpt command
> +===========
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    gpt repair <interface> <device no>
> +    gpt write <interface> <device no> <partition string>
> +    gpt verify <interface> <device no> <partition string>

<partition string> is not required. Please, add brackets [].

> +    gpt setenv <interface> <device no> <partition name>

gpt setenv can be called without partition name which leads to a crash.

If that parameter is meant to be optional, this should be indicated
here. Otherwise an argc check is missing.

> +    gpt enumerate <interface> <device no>
> +    gpt guid <interface> <device no> [<varname>]
> +    gpt read <interface> <device no> [<varname>]
> +    gpt swap <interface> <dev> <name1> <name2>
> +    gpt rename <interface> <dev> <part> <name>

Thanks a lot for providing a man-page for this command.

If <device no> and <dev> both relate to the same object type, we should
use the same text. <dev> would be fine.

The sequence looks random. Please, sort the sub-commands either
logically or alphabetically.

> +
> +Description
> +-----------
> +
> +The gpt command lets users read, create, modify, or verify the GPT (GUID
> +Partition Table) partition layout.
> +
> +The syntax of the text description of the partition list is similar to
> +the one used by the 'mbr' command. The string contains one or more partition

Please, link the mbr page:

by the :doc:`mbr command <mbr>`.

> +descriptors, each separated by a ";". Each descriptor contains one or more
> +fields, with each field separated by a ",". Fields are either of the form
> +"key=value" to set a specific value, or simple "flag" to set a boolean flag

At this point it remains unclear to the reader what this ;-separated
string format relates to.
Is it an output format?
Is it a format used in variables?
Is it used for the parameter 'partition string'?

Maybe describe it after the parameters and relate it to the 'partition
string' parameter.

Please, describe all parameters (in this indented format):

interface
     interface for accessing the block device (mmc, sata, scsi, usb, ....)

device no
     device number

...

> +
> +The first descriptor can optionally be used to describe parameters for the
> +whole disk with the following fields:
> +
> +* uuid_disk=UUID - Set the UUID for the disk
> +
> +Partition descriptors can have the following fields:
> +* name=NAME - The partition name, required

Maybe better

name=<NAME>

This is not rendered as an unordered list but everything is in one line.
Please, add the missing blank lines.

Please, generate the HTML documentation as described in
https://u-boot.readthedocs.io/en/latest/build/documentation.html
and check the output before resubmitting.

> +* start=BYTES - The partition start offset in bytes, required
> +* size=BYTES - The partition size, in bytes or "-" to expand it to the whole free area
> +* bootable - Set the legacy bootable flag
> +* uuid=UUID - Set the partition UUID, optional if CONFIG_RANDOM_UUID=y is enabled
> +* type=UUID - Set the partition type GUID, requires CONFIG_PARTITION_TYPE_GUID=y
> +

The following should be in a separate 'Examples' section to match the
other man-pages.

> +Here is an example how to create a 6 partitions, some of the predefined sizes:
> +
> +::
> +
> +    => setenv gpt_parts 'uuid_disk=bec9fc2a-86c1-483d-8a0e-0109732277d7;
> +        name=boot,start=4M,size=128M,bootable,type=ebd0a0a2-b9e5-4433-87c0-68b6b72699c7,
> +        name=rootfs,size=3072M,type=0fc63daf-8483-4772-8e79-3d69d8477de4;
> +        name=system-data,size=512M,type=0fc63daf-8483-4772-8e79-3d69d8477de4;
> +        name=[ext],size=-,type=0fc63daf-8483-4772-8e79-3d69d8477de4;
> +        name=user,size=-,type=0fc63daf-8483-4772-8e79-3d69d8477de4;
> +        name=modules,size=100M,type=0fc63daf-8483-4772-8e79-3d69d8477de4;
> +        name=ramdisk,size=8M,type=0fc63daf-8483-4772-8e79-3d69d8477de4
> +    => gpt write mmc 0 $gpt_parts
> +
> +
> +If 'uuid' is not specified, but CONFIG_RANDOM_UUID is enabled, a random UUID
> +will be generated for the partition
> +
> +The 'gpt verify' command returns 0 if the layout matches the one on the storage
> +device or 1 if not. To check if the layout on the MMC #0 storage device
> +matches the provided text description one has to issue following command:

The command can be used with and without partition parameter. This
should be described here.

I would prefer:

sets the return value $? to 0 (true) if the layout
or 1 (false) if not

> +
> +::
> +
> +    => gpt verify mmc 0 $gpt_parts
> +
> +The verify sub-command is especially useful in the system update scripts:
> +
> +::
> +
> +    => if gpt verify mmc 0 $gpt_parts; then
> +         echo GPT layout needs to be updated
> +         ...
> +       fi
> +
> +The 'gpt write' command returns 0 on success write or 1 on failure.
> +
> +The 'gpt setenv' command will set a series of environment variables with
> +information about a particular partition. The variables are:
> +
> +* gpt_partition_addr (the starting offset of the partition, in hexadecimal blocks)
> +* gpt_partition_size (the size of the partition, in hexadecimal blocks)

The blocks that are not hexadecimal, the numbers are.

gpt_partition_addr (first block of the partition as hexadecimal number)
gpt_partition_size (number of blocks as hexadecimal number)

> +* gpt_partition_name (the name of the partition)
> +* gpt_partition_entry (the partition number in the table, e.g. 1, 2, 3, etc.)

This value is currently decimal but this seems to be a bug. We need a
hexadecimal number to use this variable in a command like

     load host 0:$gpt_partition_entry $loadaddr filename

Another bug is that this command assumes that partitions are
continuously numbered.

> +
> +To get the information about the partition named 'rootfs', issue the following
> +command:
> +
> +::
> +    => gpt setenv mmc 0 rootfs
> +    => echo ${gpt_partition_addr}
> +    2000
> +    => echo ${gpt_partition_size}
> +    14a000
> +    => echo ${gpt_partition_name}
> +    rootfs
> +    => echo ${gpt_partition_entry}
> +    2
> +
> +The 'gpt enumerate' command will set the variable 'gpt_partition_list' with the
> +list of partition names on the device. For example:
> +
> +::
> +    => gpt enumerate
> +    => echo gpt_partition_list
> +    boot rootfs system-data [ext] user modules ramdisk

As we cannot see from the output if there is a partition 'user' and a
partition 'modules' or only a single partition 'user modules' this
sub-command seems to need some rework.

Best regards

Heinrich

> +
> +The 'gpt guid' command will report the GUID of a disk. If 'varname' is
> +specified, the command will set the variable to the GUID, otherwise it will be
> +printed out. For example:
> +
> +::
> +    => gpt guid mmc 0
> +    bec9fc2a-86c1-483d-8a0e-0109732277d7
> +    => gpt guid mmc gpt_disk_uuid
> +    => echo ${gpt_disk_uuid}
> +    bec9fc2a-86c1-483d-8a0e-0109732277d7
> +
> +The 'gpt read' command will print out the current state of the GPT partition
> +table. If 'varname' is specified, the variable will be filled with a partition
> +string as described above that is suitable for passing to other 'gpt' commands.
> +If omitted, a human readable description is printed out.
> +CONFIG_CMD_GPT_RENAME=y is required.
> +
> +The 'gpt swap' command changes the names of all partitions that are named
> +'name1' to be 'name2', and all partitions named 'name2' to be 'name1'.
> +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.
> +
> +Configuration
> +-------------
> +
> +To use the 'gpt' command you must specify CONFIG_CMD_GPT=y. To enable 'gpt
> +read', 'gpt swap' and 'gpt rename', you must specify CONFIG_CMD_GPT_RENAME=y.
> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index 3326ec82ff..4bfaabbd16 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -65,6 +65,7 @@ Shell commands
>      cmd/for
>      cmd/fwu_mdata
>      cmd/gpio
> +   cmd/gpt
>      cmd/host
>      cmd/imxtract
>      cmd/load