Hi, On 5/19/25 11:23, Jerome Forissier wrote:
Hi Patrick,On 5/19/25 10:04, Patrick Delaunay wrote:Add support of shortname for type parameter and restore "system" as short name for EFI System Partition (ESP) for filed "type" of the "gpt write" command. Fixes: d54e1004b8b1 ("lib/uuid.c: use unique name for PARTITION_SYSTEM_GUID") Signed-off-by: Patrick Delaunay <patrick.delau...@foss.st.com> --- lib/uuid.c | 148 +++++++++++++++++++++++++++-------------------------- 1 file changed, 76 insertions(+), 72 deletions(-) diff --git a/lib/uuid.c b/lib/uuid.c index 6abbcf27b1f3..ee02fa4d600d 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -62,184 +62,185 @@ int uuid_str_valid(const char *uuid) return 1; }+/* List of known GUID for GPT partition type */static const struct { - const char *string; + const char *string; /* name for type parameter of gpt command */const char *type?
yes it is possible...I don't change existing name just to avoid to change too many part of the existing code.
+ const char *description;/* description used for %pUs */ efi_guid_t guid; } list_guid[] = { #ifndef USE_HOSTCC -#if defined(CONFIG_PARTITION_TYPE_GUID) || defined(CONFIG_CMD_EFIDEBUG) || \ - defined(CONFIG_EFI) - {"EFI System Partition", PARTITION_SYSTEM_GUID}, -#endif -#ifdef CONFIG_PARTITION_TYPE_GUID - {"mbr", LEGACY_MBR_PARTITION_GUID}, - {"msft", PARTITION_MSFT_RESERVED_GUID}, - {"data", PARTITION_BASIC_DATA_GUID}, - {"linux", PARTITION_LINUX_FILE_SYSTEM_DATA_GUID}, - {"raid", PARTITION_LINUX_RAID_GUID}, - {"swap", PARTITION_LINUX_SWAP_GUID}, - {"lvm", PARTITION_LINUX_LVM_GUID}, - {"u-boot-env", PARTITION_U_BOOT_ENVIRONMENT}, - {"cros-kern", PARTITION_CROS_KERNEL}, - {"cros-root", PARTITION_CROS_ROOT}, - {"cros-fw", PARTITION_CROS_FIRMWARE}, - {"cros-rsrv", PARTITION_CROS_RESERVED}, -#endif +#if CONFIG_IS_ENABLED(EFI_PARTITION) + {"mbr", NULL, LEGACY_MBR_PARTITION_GUID}, + {"msft", NULL, PARTITION_MSFT_RESERVED_GUID}, + {"data", NULL, PARTITION_BASIC_DATA_GUID}, + {"linux", NULL, PARTITION_LINUX_FILE_SYSTEM_DATA_GUID}, + {"raid", NULL, PARTITION_LINUX_RAID_GUID}, + {"swap", NULL, PARTITION_LINUX_SWAP_GUID}, + {"lvm", NULL, PARTITION_LINUX_LVM_GUID}, + {"u-boot-env", NULL, PARTITION_U_BOOT_ENVIRONMENT}, + {"cros-kern", NULL, PARTITION_CROS_KERNEL}, + {"cros-root", NULL, PARTITION_CROS_ROOT}, + {"cros-fw", NULL, PARTITION_CROS_FIRMWARE}, + {"cros-rsrv", NULL, PARTITION_CROS_RESERVED}, #if defined(CONFIG_CMD_EFIDEBUG) || defined(CONFIG_EFI) { - "Device Path", + "system", "EFI System Partition", + PARTITION_SYSTEM_GUID, + },The patch adds quite many NULLs to the list_guid[] only for this very special case. We would be better off hardcoding the "system" case into uuid_guid_get_bin() I think.
I don't like the hardcoded solution in lib or the other possible workaround:#if defined(CONFIG_PARTITION_TYPE_GUID) || defined(CONFIG_CMD_EFIDEBUG) || \
defined(CONFIG_EFI)
{"EFI System Partition", PARTITION_SYSTEM_GUID},
+ {"system", PARTITION_SYSTEM_GUID},
#endif
with this simple modification
- first occurrence is used to display information (long name =
uuid_guid_get_str)
- second occurrence s used for gpt command parameter 'type=system'
with ( uuid_str_to_bin => uuid_guid_get_bin)
And it is not ONLY for "system".... but also for "mbr", "u-boot-env"... my proposal avoids confusion for- short name (with limited size and without SPACE) with can be used for short in 'type=' parameter
also used as description when description is absent- long description for partition (only used for information), without limitation (size, space).....
and prepare addition for new short name for other knwon type UIDPS: only the last added EFI partitions don't respected the first restriction...
but someone can be added short name for EFI partitons, without change long description
for example
+ "dtb", "device tree",
+ "dtbo", "Device-Tree Fixup",
We must have limited size in short name because in cmd/gpt.c:533 we have
#ifdef CONFIG_PARTITION_TYPE_GUID
/* guid */
val = extract_val(tok, "type");
if (val) {
/* 'type' is optional */
if (extract_env(val, &p))
p = val;
if (strnlen(p, max_str_part) >= sizeof(parts[i].type_guid)) {
printf("Wrong type guid format for partition %d\n",
i);
errno = -4;
goto err;
}
strncpy((char *)parts[i].type_guid, p, max_str_part);
free(val);
}
#endif
So for STRING parameter, used as short cut) in 'type=' is also copied in
parts[i].type_guid
=> shor name size is limited at 36
+ { + NULL, "Device Path", EFI_DEVICE_PATH_PROTOCOL_GUID, }, { - "Device Path To Text", + NULL, "Device Path To Text", EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID, }, { - "Device Path Utilities", + NULL, "Device Path Utilities", EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID, }, { - "Unicode Collation 2", + NULL, "Unicode Collation 2", EFI_UNICODE_COLLATION_PROTOCOL2_GUID, }, { - "Driver Binding", + NULL, "Driver Binding", EFI_DRIVER_BINDING_PROTOCOL_GUID, }, { - "Simple Text Input", + NULL, "Simple Text Input", EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID, }, { - "Simple Text Input Ex", + NULL, "Simple Text Input Ex", EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID, }, { - "Simple Text Output", + NULL, "Simple Text Output", EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID, }, { - "Block IO", + NULL, "Block IO", EFI_BLOCK_IO_PROTOCOL_GUID, }, { - "Disk IO", + NULL, "Disk IO", EFI_DISK_IO_PROTOCOL_GUID, }, { - "Simple File System", + NULL, "Simple File System", EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID, }, { - "Loaded Image", + NULL, "Loaded Image", EFI_LOADED_IMAGE_PROTOCOL_GUID, }, { - "Loaded Image Device Path", + NULL, "Loaded Image Device Path", EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID, }, { - "Graphics Output", + NULL, "Graphics Output", EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID, }, { - "HII String", + NULL, "HII String", EFI_HII_STRING_PROTOCOL_GUID, }, { - "HII Database", + NULL, "HII Database", EFI_HII_DATABASE_PROTOCOL_GUID, }, { - "HII Config Access", + NULL, "HII Config Access", EFI_HII_CONFIG_ACCESS_PROTOCOL_GUID, }, { - "HII Config Routing", + NULL, "HII Config Routing", EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID, }, { - "Load File", + NULL, "Load File", EFI_LOAD_FILE_PROTOCOL_GUID, }, { - "Load File2", + NULL, "Load File2", EFI_LOAD_FILE2_PROTOCOL_GUID, }, { - "Random Number Generator", + NULL, "Random Number Generator", EFI_RNG_PROTOCOL_GUID, }, { - "Simple Network", + NULL, "Simple Network", EFI_SIMPLE_NETWORK_PROTOCOL_GUID, }, { - "PXE Base Code", + NULL, "PXE Base Code", EFI_PXE_BASE_CODE_PROTOCOL_GUID, }, { - "Device-Tree Fixup", + NULL, "Device-Tree Fixup", EFI_DT_FIXUP_PROTOCOL_GUID, }, { - "TCG2", + NULL, "TCG2", EFI_TCG2_PROTOCOL_GUID, }, { - "Firmware Management", + NULL, "Firmware Management", EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID }, #if IS_ENABLED(CONFIG_EFI_HTTP_PROTOCOL) { - "HTTP", + NULL, "HTTP", EFI_HTTP_PROTOCOL_GUID, }, { - "HTTP Service Binding", + NULL, "HTTP Service Binding", EFI_HTTP_SERVICE_BINDING_PROTOCOL_GUID, }, { - "IPv4 Config2", + NULL, "IPv4 Config2", EFI_IP4_CONFIG2_PROTOCOL_GUID, }, #endif /* Configuration table GUIDs */ { - "ACPI table", + NULL, "ACPI table", EFI_ACPI_TABLE_GUID, }, { - "EFI System Resource Table", + NULL, "EFI System Resource Table", EFI_SYSTEM_RESOURCE_TABLE_GUID, }, { - "device tree", + NULL, "device tree", EFI_FDT_GUID, }, { - "SMBIOS table", + NULL, "SMBIOS table", SMBIOS_TABLE_GUID, }, { - "SMBIOS3 table", + NULL, "SMBIOS3 table", SMBIOS3_TABLE_GUID, }, { - "Runtime properties", + NULL, "Runtime properties", EFI_RT_PROPERTIES_TABLE_GUID, }, { - "TCG2 Final Events Table", + NULL, "TCG2 Final Events Table", EFI_TCG2_FINAL_EVENTS_TABLE_GUID, }, { - "EFI Conformance Profiles Table", + NULL, "EFI Conformance Profiles Table", EFI_CONFORMANCE_PROFILES_TABLE_GUID, }, #ifdef CONFIG_EFI_RISCV_BOOT_PROTOCOL { - "RISC-V Boot", + NULL, "RISC-V Boot", RISCV_EFI_BOOT_PROTOCOL_GUID, }, #endif @@ -247,35 +248,36 @@ static const struct { #ifdef CONFIG_CMD_NVEDIT_EFI /* signature database */ { - "EFI_GLOBAL_VARIABLE_GUID", + NULL, "EFI_GLOBAL_VARIABLE_GUID", EFI_GLOBAL_VARIABLE_GUID, }, { - "EFI_IMAGE_SECURITY_DATABASE_GUID", + NULL, "EFI_IMAGE_SECURITY_DATABASE_GUID", EFI_IMAGE_SECURITY_DATABASE_GUID, }, /* certificate types */ { - "EFI_CERT_SHA256_GUID", + NULL, "EFI_CERT_SHA256_GUID", EFI_CERT_SHA256_GUID, }, { - "EFI_CERT_X509_GUID", + NULL, "EFI_CERT_X509_GUID", EFI_CERT_X509_GUID, }, { - "EFI_CERT_TYPE_PKCS7_GUID", + NULL, "EFI_CERT_TYPE_PKCS7_GUID", EFI_CERT_TYPE_PKCS7_GUID, }, #endif #if defined(CONFIG_CMD_EFIDEBUG) || defined(CONFIG_EFI) - { "EFI_LZMA_COMPRESSED", EFI_LZMA_COMPRESSED }, - { "EFI_DXE_SERVICES", EFI_DXE_SERVICES }, - { "EFI_HOB_LIST", EFI_HOB_LIST }, - { "EFI_MEMORY_TYPE", EFI_MEMORY_TYPE }, - { "EFI_MEM_STATUS_CODE_REC", EFI_MEM_STATUS_CODE_REC }, - { "EFI_GUID_EFI_ACPI1", EFI_GUID_EFI_ACPI1 }, + { NULL, "EFI_LZMA_COMPRESSED", EFI_LZMA_COMPRESSED }, + { NULL, "EFI_DXE_SERVICES", EFI_DXE_SERVICES }, + { NULL, "EFI_HOB_LIST", EFI_HOB_LIST }, + { NULL, "EFI_MEMORY_TYPE", EFI_MEMORY_TYPE }, + { NULL, "EFI_MEM_STATUS_CODE_REC", EFI_MEM_STATUS_CODE_REC }, + { NULL, "EFI_GUID_EFI_ACPI1", EFI_GUID_EFI_ACPI1 },
here we can use existing as short name/description as string have no space and size < 36....
+ { "EFI_LZMA_COMPRESSED", NULL, EFI_LZMA_COMPRESSED },
+ { "EFI_DXE_SERVICES", NULL, EFI_DXE_SERVICES },
+ { "EFI_HOB_LIST", NULL, EFI_HOB_LIST },
+ { "EFI_MEMORY_TYPE", NULL, EFI_MEMORY_TYPE },
+ { "EFI_MEM_STATUS_CODE_REC", NULL, EFI_MEM_STATUS_CODE_REC },
+ { "EFI_GUID_EFI_ACPI1", NULL, EFI_GUID_EFI_ACPI1 },
#endif +#endif /* EFI_PARTITION */ #endif /* !USE_HOSTCC */ };@@ -284,7 +286,8 @@ int uuid_guid_get_bin(const char *guid_str, unsigned char *guid_bin)int i;for (i = 0; i < ARRAY_SIZE(list_guid); i++) {- if (!strcmp(list_guid[i].string, guid_str)) { + if (list_guid[i].string && + !strcmp(list_guid[i].string, guid_str)) { memcpy(guid_bin, &list_guid[i].guid, 16); return 0; } @@ -298,6 +301,8 @@ const char *uuid_guid_get_str(const unsigned char *guid_bin)Should this be called uuid_guid_get_type() instead? 'str' or 'string' are confusing. We should have 'type' and 'decription' IMO.for (i = 0; i < ARRAY_SIZE(list_guid); i++) {if (!memcmp(list_guid[i].guid.b, guid_bin, 16)) { + if (list_guid[i].description) + return list_guid[i].description; return list_guid[i].string;.description is never (or should never be) NULL. No need to return .string as a fallback.
it is the case for ALL the known type with only short description ....
not need to have long description if short string is enough....
+ {"mbr", NULL, LEGACY_MBR_PARTITION_GUID},
+ {"msft", NULL, PARTITION_MSFT_RESERVED_GUID},
+ {"data", NULL, PARTITION_BASIC_DATA_GUID},
+ {"linux", NULL, PARTITION_LINUX_FILE_SYSTEM_DATA_GUID},
+ {"raid", NULL, PARTITION_LINUX_RAID_GUID},
+ {"swap", NULL, PARTITION_LINUX_SWAP_GUID},
+ {"lvm", NULL, PARTITION_LINUX_LVM_GUID},
+ {"u-boot-env", NULL, PARTITION_U_BOOT_ENVIRONMENT},
+ {"cros-kern", NULL, PARTITION_CROS_KERNEL},
+ {"cros-root", NULL, PARTITION_CROS_ROOT},
+ {"cros-fw", NULL, PARTITION_CROS_FIRMWARE},
+ {"cros-rsrv", NULL, PARTITION_CROS_RESERVED},
} } @@ -312,10 +317,9 @@ int uuid_str_to_bin(const char *uuid_str, unsigned char *uuid_bin, uint64_t tmp64;if (!uuid_str_valid(uuid_str)) {-#ifdef CONFIG_PARTITION_TYPE_GUID - if (!uuid_guid_get_bin(uuid_str, uuid_bin)) + if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) && + !uuid_guid_get_bin(uuid_str, uuid_bin)) return 0; -#endif return -EINVAL; }Thanks,
Regards
