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

Heinrich Schuchardt
2023-08-28T20:00:20Z

On 8/28/23 21:29, Joshua Watt wrote:
> On Fri, Aug 25, 2023 at 7:57 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> 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 [].
>
> It's not optional; there is no standardized string for the list of GPT
> partitions like there is for MBR

You can use the command without partition string. It will tell you if
the device is partitioned or not.

>
>>
>>> + 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.
>
> I don't believe the argument is optional, but either way it can be
> fixed in a subsequent patch series. I'd rather not hold up the
> documentation to fix bugs
>
>>
>>> + 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.
>
> They are sorted to match the gpt command help text. Reordering of both
> can be done later if desired.

There is no requirement to follow the gpt help =sequence here.

We should not reproduce what is not helpful.

Best regards

Heinrich

>
>>
>>> +
>>> +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.
>
> Can be fixed later.
>
>>
>> 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
>>