public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [PATCH v2 0/2] 'part name' subcommand and some robustification
@ 2025-10-20 12:10 Rasmus Villemoes
  2025-10-20 12:10 ` [PATCH v2 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Rasmus Villemoes @ 2025-10-20 12:10 UTC (permalink / raw)
  To: u-boot; +Cc: Quentin Schulz, Tom Rini, Rasmus Villemoes

Implement a "part name" subcommand, mirroring the existing "part
number" subcommand.

In the discussion for v1 of that, it came up that there's a bit of
inconsistency in how much and what one can assume to be initialized in
'struct disk_partition' after a successful call of one of the
get_info* family of functions. The new patch 1/2 tries to consolidate
that by making sure all ->get_info invocations go through a common
helper that at least always initializes the string members.

Rasmus Villemoes (2):
  disk/part.c: ensure strings in struct disk_partition are valid after
    successful get_info
  cmd/part.c: implement "part name" subcommand

 cmd/gpt.c              |  4 +--
 cmd/part.c             | 16 ++++++++++-
 disk/part.c            | 62 ++++++++++++++++++++++++------------------
 doc/usage/cmd/part.rst | 13 +++++++++
 include/part.h         | 16 +++++++++++
 5 files changed, 81 insertions(+), 30 deletions(-)

-- 
2.51.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info
  2025-10-20 12:10 [PATCH v2 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
@ 2025-10-20 12:10 ` Rasmus Villemoes
  2025-11-03 14:07   ` Quentin Schulz
  2025-10-20 12:11 ` [PATCH v2 2/2] cmd/part.c: implement "part name" subcommand Rasmus Villemoes
  2025-11-07 20:19 ` [PATCH v2 0/2] 'part name' subcommand and some robustification Tom Rini
  2 siblings, 1 reply; 11+ messages in thread
From: Rasmus Villemoes @ 2025-10-20 12:10 UTC (permalink / raw)
  To: u-boot; +Cc: Quentin Schulz, Tom Rini, Rasmus Villemoes

Not all ->get_info implementations necessarily populate all the string
members of struct disk_partition.

Currently, only part_get_info_by_type() (and thereby part_get_info)
ensure that the uuid strings are initialized; part_get_info_by_type()
and part_get_info_by_uuid() do not. In fact, the latter could lead to
a false positive match - if the ->get_info backend does not populate
info->uuid, stale contents in info could cause the strncasecmp() to
succeed.

None of the functions currently ensure that the ->name and ->type
strings are initialized.

Instead of forcing all callers of any of these functions to
pre-initialize info, or all implementations of the ->get_info method
to ensure there are valid C strings in all four fields, create a small
helper function and factor all invocations of ->get_info through that.

This also consolidates the -ENOSYS check and standardizes on using
log_debug() for reporting absence, instead of the current mix of
PRINTF and log_debug(). It does mean we have to special-case -ENOSYS
in the error cases inside the loops in the _by_uuid() and _by_name()
functions, but it's still a net win in #LOC.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
v2: new patch in v2.

 cmd/gpt.c      |  4 ++--
 disk/part.c    | 62 ++++++++++++++++++++++++++++----------------------
 include/part.h | 16 +++++++++++++
 3 files changed, 53 insertions(+), 29 deletions(-)

diff --git a/cmd/gpt.c b/cmd/gpt.c
index e18e5036a06..84221881c39 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -724,7 +724,7 @@ static int gpt_enumerate(struct blk_desc *desc)
 			continue;
 
 		for (i = 1; i < part_drv->max_entries; i++) {
-			ret = part_drv->get_info(desc, i, &pinfo);
+			ret = part_driver_get_info(part_drv, desc, i, &pinfo);
 			if (ret)
 				continue;
 
@@ -820,7 +820,7 @@ static int gpt_setenv(struct blk_desc *desc, const char *name)
 		int i;
 
 		for (i = 1; i < part_drv->max_entries; i++) {
-			ret = part_drv->get_info(desc, i, &pinfo);
+			ret = part_driver_get_info(part_drv, desc, i, &pinfo);
 			if (ret)
 				continue;
 
diff --git a/disk/part.c b/disk/part.c
index be2b45d5a29..194eccadb7b 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -72,6 +72,28 @@ struct part_driver *part_driver_lookup_type(struct blk_desc *desc)
 	return NULL;
 }
 
+static void disk_partition_clr(struct disk_partition *info)
+{
+	/* The common case is no UUID support */
+	disk_partition_clr_uuid(info);
+	disk_partition_clr_type_guid(info);
+	info->name[0] = '\0';
+	info->type[0] = '\0';
+}
+
+int part_driver_get_info(struct part_driver *drv, struct blk_desc *desc, int part,
+			 struct disk_partition *info)
+{
+	if (!drv->get_info) {
+		log_debug("## Driver %s does not have the get_info() method\n",
+			  drv->name);
+		return -ENOSYS;
+	}
+
+	disk_partition_clr(info);
+	return drv->get_info(desc, part, info);
+}
+
 int part_get_type_by_name(const char *name)
 {
 	struct part_driver *drv =
@@ -322,12 +344,9 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
 			  struct disk_partition *info)
 {
 	struct part_driver *drv;
+	int ret = -ENOENT;
 
 	if (blk_enabled()) {
-		/* The common case is no UUID support */
-		disk_partition_clr_uuid(info);
-		disk_partition_clr_type_guid(info);
-
 		if (part_type == PART_TYPE_UNKNOWN) {
 			drv = part_driver_lookup_type(desc);
 		} else {
@@ -339,18 +358,13 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
 			      desc->part_type);
 			return -EPROTONOSUPPORT;
 		}
-		if (!drv->get_info) {
-			PRINTF("## Driver %s does not have the get_info() method\n",
-			       drv->name);
-			return -ENOSYS;
-		}
-		if (drv->get_info(desc, part, info) == 0) {
+
+		ret = part_driver_get_info(drv, desc, part, info);
+		if (!ret)
 			PRINTF("## Valid %s partition found ##\n", drv->name);
-			return 0;
-		}
 	}
 
-	return -ENOENT;
+	return ret;
 }
 
 int part_get_info(struct blk_desc *desc, int part,
@@ -657,15 +671,12 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
 	if (!part_drv)
 		return -1;
 
-	if (!part_drv->get_info) {
-		log_debug("## Driver %s does not have the get_info() method\n",
-			  part_drv->name);
-		return -ENOSYS;
-	}
-
 	for (i = 1; i < part_drv->max_entries; i++) {
-		ret = part_drv->get_info(desc, i, info);
+		ret = part_driver_get_info(part_drv, desc, i, info);
 		if (ret != 0) {
+			/* -ENOSYS means no ->get_info method. */
+			if (ret == -ENOSYS)
+				return ret;
 			/*
 			 * Partition with this index can't be obtained, but
 			 * further partitions might be, so keep checking.
@@ -695,15 +706,12 @@ int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
 	if (!part_drv)
 		return -1;
 
-	if (!part_drv->get_info) {
-		log_debug("## Driver %s does not have the get_info() method\n",
-			  part_drv->name);
-		return -ENOSYS;
-	}
-
 	for (i = 1; i < part_drv->max_entries; i++) {
-		ret = part_drv->get_info(desc, i, info);
+		ret = part_driver_get_info(part_drv, desc, i, info);
 		if (ret != 0) {
+			/* -ENOSYS means no ->get_info method. */
+			if (ret == -ENOSYS)
+				return ret;
 			/*
 			 * Partition with this index can't be obtained, but
 			 * further partitions might be, so keep checking.
diff --git a/include/part.h b/include/part.h
index 6caaa6526aa..d940f8b5d0e 100644
--- a/include/part.h
+++ b/include/part.h
@@ -509,6 +509,22 @@ struct part_driver {
 	int (*test)(struct blk_desc *desc);
 };
 
+/**
+ * part_driver_get_info() - Call the part_driver's get_info method
+ *
+ * On success, string members of info are guaranteed to be properly
+ * initialized, though they may be empty.
+ *
+ * @drv:	Partition driver
+ * @desc:	Block device descriptor
+ * @part:	Partition number to read
+ * @info:	Returned partition information
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int part_driver_get_info(struct part_driver *drv, struct blk_desc *desc, int part,
+			 struct disk_partition *info);
+
 /* Declare a new U-Boot partition 'driver' */
 #define U_BOOT_PART_TYPE(__name)					\
 	ll_entry_declare(struct part_driver, __name, part_driver)
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] cmd/part.c: implement "part name" subcommand
  2025-10-20 12:10 [PATCH v2 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
  2025-10-20 12:10 ` [PATCH v2 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
@ 2025-10-20 12:11 ` Rasmus Villemoes
  2025-11-03 14:14   ` Quentin Schulz
  2025-11-07 20:19 ` [PATCH v2 0/2] 'part name' subcommand and some robustification Tom Rini
  2 siblings, 1 reply; 11+ messages in thread
From: Rasmus Villemoes @ 2025-10-20 12:11 UTC (permalink / raw)
  To: u-boot; +Cc: Quentin Schulz, Tom Rini, Rasmus Villemoes

This is a natural buddy to the existing "part number", allowing one to
get the partition name for a given partition number.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
v2: include documentation update

 cmd/part.c             | 16 +++++++++++++++-
 doc/usage/cmd/part.rst | 13 +++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/cmd/part.c b/cmd/part.c
index db7bc5819c0..975a0a08a99 100644
--- a/cmd/part.c
+++ b/cmd/part.c
@@ -25,7 +25,8 @@
 enum cmd_part_info {
 	CMD_PART_INFO_START = 0,
 	CMD_PART_INFO_SIZE,
-	CMD_PART_INFO_NUMBER
+	CMD_PART_INFO_NUMBER,
+	CMD_PART_INFO_NAME,
 };
 
 static int do_part_uuid(int argc, char *const argv[])
@@ -154,6 +155,9 @@ static int do_part_info(int argc, char *const argv[], enum cmd_part_info param)
 	case CMD_PART_INFO_NUMBER:
 		snprintf(buf, sizeof(buf), "0x%x", part);
 		break;
+	case CMD_PART_INFO_NAME:
+		snprintf(buf, sizeof(buf), "%s", info.name);
+		break;
 	default:
 		printf("** Unknown cmd_part_info value: %d\n", param);
 		return 1;
@@ -182,6 +186,11 @@ static int do_part_number(int argc, char *const argv[])
 	return do_part_info(argc, argv, CMD_PART_INFO_NUMBER);
 }
 
+static int do_part_name(int argc, char *const argv[])
+{
+	return do_part_info(argc, argv, CMD_PART_INFO_NAME);
+}
+
 static int do_part_set(int argc, char *const argv[])
 {
 	const char *devname, *partstr, *typestr;
@@ -273,6 +282,8 @@ static int do_part(struct cmd_tbl *cmdtp, int flag, int argc,
 		return do_part_size(argc - 2, argv + 2);
 	else if (!strcmp(argv[1], "number"))
 		return do_part_number(argc - 2, argv + 2);
+	else if (!strcmp(argv[1], "name"))
+		return do_part_name(argc - 2, argv + 2);
 	else if (!strcmp(argv[1], "types"))
 		return do_part_types(argc - 2, argv + 2);
 	else if (!strcmp(argv[1], "set"))
@@ -305,6 +316,9 @@ U_BOOT_CMD(
 	"part number <interface> <dev> <part> <varname>\n"
 	"    - set environment variable to the partition number using the partition name\n"
 	"      part must be specified as partition name\n"
+	"part name <interface> <dev> <part> <varname>\n"
+	"    - set environment variable to the partition name using the partition number\n"
+	"      part must be specified as partition number\n"
 #ifdef CONFIG_PARTITION_TYPE_GUID
 	"part type <interface> <dev>:<part>\n"
 	"    - print partition type\n"
diff --git a/doc/usage/cmd/part.rst b/doc/usage/cmd/part.rst
index e7f6e54ecea..df510685154 100644
--- a/doc/usage/cmd/part.rst
+++ b/doc/usage/cmd/part.rst
@@ -16,6 +16,7 @@ Synopsis
     part start <interface> <dev> <part> <varname>
     part size <interface> <dev> <part> <varname>
     part number <interface> <dev> <part> <varname>
+    part name <interface> <dev> <part> <varname>
     part set <interface> <dev> <part> <type>
     part type <interface> <dev>:<part> [varname]
     part types
@@ -86,6 +87,18 @@ part must be specified as partition name.
     varname
         a variable to store the current partition number value into
 
+The 'part name' command sets an environment variable to the partition name using the partition number,
+part must be specified as partition number.
+
+    interface
+        interface for accessing the block device (mmc, sata, scsi, usb, ....)
+    dev
+        device number
+    part
+        partition number
+    varname
+        a variable to store the current partition name into
+
 The 'part set' command sets the type of a partition. This is useful when
 autodetection fails or does not do the correct thing:
 
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info
  2025-10-20 12:10 ` [PATCH v2 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
@ 2025-11-03 14:07   ` Quentin Schulz
  0 siblings, 0 replies; 11+ messages in thread
From: Quentin Schulz @ 2025-11-03 14:07 UTC (permalink / raw)
  To: Rasmus Villemoes, u-boot; +Cc: Tom Rini

Hi Rasmus,

On 10/20/25 2:10 PM, Rasmus Villemoes wrote:
> Not all ->get_info implementations necessarily populate all the string
> members of struct disk_partition.
> 
> Currently, only part_get_info_by_type() (and thereby part_get_info)
> ensure that the uuid strings are initialized; part_get_info_by_type()
> and part_get_info_by_uuid() do not. In fact, the latter could lead to
> a false positive match - if the ->get_info backend does not populate
> info->uuid, stale contents in info could cause the strncasecmp() to
> succeed.
> 
> None of the functions currently ensure that the ->name and ->type
> strings are initialized.
> 
> Instead of forcing all callers of any of these functions to
> pre-initialize info, or all implementations of the ->get_info method
> to ensure there are valid C strings in all four fields, create a small
> helper function and factor all invocations of ->get_info through that.
> 

Considering we only have code calling get_info() function of the part 
driver in the two files changed in this commit, but there's nothing 
forbidding users to call it from their driver or board files, but I 
guess this is good enough and we can worry about non-existing users later?

> This also consolidates the -ENOSYS check and standardizes on using
> log_debug() for reporting absence, instead of the current mix of
> PRINTF and log_debug(). It does mean we have to special-case -ENOSYS
> in the error cases inside the loops in the _by_uuid() and _by_name()
> functions, but it's still a net win in #LOC.
> 

Acked-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] cmd/part.c: implement "part name" subcommand
  2025-10-20 12:11 ` [PATCH v2 2/2] cmd/part.c: implement "part name" subcommand Rasmus Villemoes
@ 2025-11-03 14:14   ` Quentin Schulz
  2025-11-03 20:38     ` Rasmus Villemoes
  0 siblings, 1 reply; 11+ messages in thread
From: Quentin Schulz @ 2025-11-03 14:14 UTC (permalink / raw)
  To: Rasmus Villemoes, u-boot; +Cc: Tom Rini

Hi Rasmus,

On 10/20/25 2:11 PM, Rasmus Villemoes wrote:
> This is a natural buddy to the existing "part number", allowing one to
> get the partition name for a given partition number.
> 

Acked-by: Quentin Schuloz <quentin.schulz@cherry.de>

Reading the code, it seems the part command tries to auto-detect the 
base of the part number as passed by the user, which isn't that usual I 
believe in U-Boot (usually either forced hex or dec?), so maybe it's 
worth mentioning in the docs.

Thanks!
Quentin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] cmd/part.c: implement "part name" subcommand
  2025-11-03 14:14   ` Quentin Schulz
@ 2025-11-03 20:38     ` Rasmus Villemoes
  2025-11-04  9:36       ` Quentin Schulz
  0 siblings, 1 reply; 11+ messages in thread
From: Rasmus Villemoes @ 2025-11-03 20:38 UTC (permalink / raw)
  To: Quentin Schulz; +Cc: u-boot, Tom Rini

On Mon, Nov 03 2025, Quentin Schulz <quentin.schulz@cherry.de> wrote:

> Hi Rasmus,
>
> On 10/20/25 2:11 PM, Rasmus Villemoes wrote:
>> This is a natural buddy to the existing "part number", allowing one to
>> get the partition name for a given partition number.
>> 
>
> Acked-by: Quentin Schuloz <quentin.schulz@cherry.de>
>

Thanks.

> 
> Reading the code, it seems the part command tries to auto-detect the
> base of the part number as passed by the user, which isn't that usual
> I believe in U-Boot (usually either forced hex or dec?), so maybe it's
> worth mentioning in the docs.

Fun story: I started doing this because I have a legacy stand-alone app
which is involved in the bootflow, and it communicates the partition to
load the kernel from by setting an env variable via

  sprintf(buf, "%d", kernel_part);
  env_set("kernel_part", buf);

and on the U-Boot side, that $kernel_part is/was used with various "read
mmc ..." or "mmc read ..." commands. Guess how well that works when
kernel_part ends up being >= 10 and the U-Boot commands unconditionally
interprets the partition argument as hex...

So what I wanted to do on the U-Boot side is to translate that
$kernel_part into a name ASAP and then use that name exclusively, and
for that the sane semantics of the "part" command came in handy. Except
it lacked the ability to do the translation.

As for the "[not] that usual", `git grep strto -- cmd/` says that the
base argument can be 0, 10 or 16 depending on the phase of the moon -
it's a constant source of pain to figure out how numeric arguments
to commands will be interpreted.

Rasmus

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] cmd/part.c: implement "part name" subcommand
  2025-11-03 20:38     ` Rasmus Villemoes
@ 2025-11-04  9:36       ` Quentin Schulz
  0 siblings, 0 replies; 11+ messages in thread
From: Quentin Schulz @ 2025-11-04  9:36 UTC (permalink / raw)
  To: Rasmus Villemoes; +Cc: u-boot, Tom Rini

Hi Rasmus,

On 11/3/25 9:38 PM, Rasmus Villemoes wrote:
> On Mon, Nov 03 2025, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> 
>> Hi Rasmus,
>>
>> On 10/20/25 2:11 PM, Rasmus Villemoes wrote:
>>> This is a natural buddy to the existing "part number", allowing one to
>>> get the partition name for a given partition number.
>>>
>>
>> Acked-by: Quentin Schuloz <quentin.schulz@cherry.de>
>>
Can't even type my own (short) name without typo :(

>>
>> Reading the code, it seems the part command tries to auto-detect the
>> base of the part number as passed by the user, which isn't that usual
>> I believe in U-Boot (usually either forced hex or dec?), so maybe it's
>> worth mentioning in the docs.
> 
> Fun story: I started doing this because I have a legacy stand-alone app
> which is involved in the bootflow, and it communicates the partition to
> load the kernel from by setting an env variable via
> 
>    sprintf(buf, "%d", kernel_part);
>    env_set("kernel_part", buf);
> 
> and on the U-Boot side, that $kernel_part is/was used with various "read
> mmc ..." or "mmc read ..." commands. Guess how well that works when
> kernel_part ends up being >= 10 and the U-Boot commands unconditionally
> interprets the partition argument as hex...
> 
> So what I wanted to do on the U-Boot side is to translate that
> $kernel_part into a name ASAP and then use that name exclusively, and
> for that the sane semantics of the "part" command came in handy. Except
> it lacked the ability to do the translation.
> 
> As for the "[not] that usual", `git grep strto -- cmd/` says that the
> base argument can be 0, 10 or 16 depending on the phase of the moon -
> it's a constant source of pain to figure out how numeric arguments
> to commands will be interpreted.
> 

Yeah, and I vaguely remember being upset by the same lack of consistency 
for printing numbers.

I share the pain. But that's API so cannot change it without breaking 
stuff :/

Quentin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/2] 'part name' subcommand and some robustification
  2025-10-20 12:10 [PATCH v2 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
  2025-10-20 12:10 ` [PATCH v2 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
  2025-10-20 12:11 ` [PATCH v2 2/2] cmd/part.c: implement "part name" subcommand Rasmus Villemoes
@ 2025-11-07 20:19 ` Tom Rini
  2025-11-08  0:02   ` Rasmus Villemoes
  2 siblings, 1 reply; 11+ messages in thread
From: Tom Rini @ 2025-11-07 20:19 UTC (permalink / raw)
  To: Rasmus Villemoes; +Cc: u-boot, Quentin Schulz

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

On Mon, Oct 20, 2025 at 02:10:58PM +0200, Rasmus Villemoes wrote:

> Implement a "part name" subcommand, mirroring the existing "part
> number" subcommand.
> 
> In the discussion for v1 of that, it came up that there's a bit of
> inconsistency in how much and what one can assume to be initialized in
> 'struct disk_partition' after a successful call of one of the
> get_info* family of functions. The new patch 1/2 tries to consolidate
> that by making sure all ->get_info invocations go through a common
> helper that at least always initializes the string members.
> 
> Rasmus Villemoes (2):
>   disk/part.c: ensure strings in struct disk_partition are valid after
>     successful get_info
>   cmd/part.c: implement "part name" subcommand
> 
>  cmd/gpt.c              |  4 +--
>  cmd/part.c             | 16 ++++++++++-
>  disk/part.c            | 62 ++++++++++++++++++++++++------------------
>  doc/usage/cmd/part.rst | 13 +++++++++
>  include/part.h         | 16 +++++++++++
>  5 files changed, 81 insertions(+), 30 deletions(-)

This leads to some of the bootstd tests failing in CI, unfortunately.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/2] 'part name' subcommand and some robustification
  2025-11-07 20:19 ` [PATCH v2 0/2] 'part name' subcommand and some robustification Tom Rini
@ 2025-11-08  0:02   ` Rasmus Villemoes
  2025-11-08 15:01     ` Tom Rini
  0 siblings, 1 reply; 11+ messages in thread
From: Rasmus Villemoes @ 2025-11-08  0:02 UTC (permalink / raw)
  To: Tom Rini; +Cc: u-boot, Quentin Schulz

On Fri, Nov 07 2025, Tom Rini <trini@konsulko.com> wrote:

> On Mon, Oct 20, 2025 at 02:10:58PM +0200, Rasmus Villemoes wrote:
>
>> Implement a "part name" subcommand, mirroring the existing "part
>> number" subcommand.
>> 
>> In the discussion for v1 of that, it came up that there's a bit of
>> inconsistency in how much and what one can assume to be initialized in
>> 'struct disk_partition' after a successful call of one of the
>> get_info* family of functions. The new patch 1/2 tries to consolidate
>> that by making sure all ->get_info invocations go through a common
>> helper that at least always initializes the string members.
>> 
>> Rasmus Villemoes (2):
>>   disk/part.c: ensure strings in struct disk_partition are valid after
>>     successful get_info
>>   cmd/part.c: implement "part name" subcommand
>> 
>>  cmd/gpt.c              |  4 +--
>>  cmd/part.c             | 16 ++++++++++-
>>  disk/part.c            | 62 ++++++++++++++++++++++++------------------
>>  doc/usage/cmd/part.rst | 13 +++++++++
>>  include/part.h         | 16 +++++++++++
>>  5 files changed, 81 insertions(+), 30 deletions(-)
>
> This leads to some of the bootstd tests failing in CI, unfortunately.

Do you have a link?

Also, how exactly should one run those bootstd tests? When I just build
sandbox_defconfig and do 'ut bootstd', I get 130 failures, so I assume I
need to do something extra.

Rasmus

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/2] 'part name' subcommand and some robustification
  2025-11-08  0:02   ` Rasmus Villemoes
@ 2025-11-08 15:01     ` Tom Rini
  2025-11-10  0:33       ` Rasmus Villemoes
  0 siblings, 1 reply; 11+ messages in thread
From: Tom Rini @ 2025-11-08 15:01 UTC (permalink / raw)
  To: Rasmus Villemoes; +Cc: u-boot, Quentin Schulz

[-- Attachment #1: Type: text/plain, Size: 1994 bytes --]

On Sat, Nov 08, 2025 at 01:02:14AM +0100, Rasmus Villemoes wrote:
> On Fri, Nov 07 2025, Tom Rini <trini@konsulko.com> wrote:
> 
> > On Mon, Oct 20, 2025 at 02:10:58PM +0200, Rasmus Villemoes wrote:
> >
> >> Implement a "part name" subcommand, mirroring the existing "part
> >> number" subcommand.
> >> 
> >> In the discussion for v1 of that, it came up that there's a bit of
> >> inconsistency in how much and what one can assume to be initialized in
> >> 'struct disk_partition' after a successful call of one of the
> >> get_info* family of functions. The new patch 1/2 tries to consolidate
> >> that by making sure all ->get_info invocations go through a common
> >> helper that at least always initializes the string members.
> >> 
> >> Rasmus Villemoes (2):
> >>   disk/part.c: ensure strings in struct disk_partition are valid after
> >>     successful get_info
> >>   cmd/part.c: implement "part name" subcommand
> >> 
> >>  cmd/gpt.c              |  4 +--
> >>  cmd/part.c             | 16 ++++++++++-
> >>  disk/part.c            | 62 ++++++++++++++++++++++++------------------
> >>  doc/usage/cmd/part.rst | 13 +++++++++
> >>  include/part.h         | 16 +++++++++++
> >>  5 files changed, 81 insertions(+), 30 deletions(-)
> >
> > This leads to some of the bootstd tests failing in CI, unfortunately.
> 
> Do you have a link?

Right, sorry:
https://source.denx.de/u-boot/u-boot/-/jobs/1292597

> Also, how exactly should one run those bootstd tests? When I just build
> sandbox_defconfig and do 'ut bootstd', I get 130 failures, so I assume I
> need to do something extra.

So, you want to run it through pytest rather than directly, I find:
https://docs.u-boot.org/en/latest/develop/pytest/usage.html
And then
https://lore.kernel.org/all/20251029143346.1320868-1-kory.maincent@bootlin.com
has the full rather than partial list of host requirements, if you don't
want to grab the docker container CI uses and run inside that.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/2] 'part name' subcommand and some robustification
  2025-11-08 15:01     ` Tom Rini
@ 2025-11-10  0:33       ` Rasmus Villemoes
  0 siblings, 0 replies; 11+ messages in thread
From: Rasmus Villemoes @ 2025-11-10  0:33 UTC (permalink / raw)
  To: Tom Rini; +Cc: u-boot, Quentin Schulz

On Sat, Nov 08 2025, Tom Rini <trini@konsulko.com> wrote:

> On Sat, Nov 08, 2025 at 01:02:14AM +0100, Rasmus Villemoes wrote:
>> On Fri, Nov 07 2025, Tom Rini <trini@konsulko.com> wrote:
>> 
>> > On Mon, Oct 20, 2025 at 02:10:58PM +0200, Rasmus Villemoes wrote:
>> >
>> >> Implement a "part name" subcommand, mirroring the existing "part
>> >> number" subcommand.
>> >> 
>> >> In the discussion for v1 of that, it came up that there's a bit of
>> >> inconsistency in how much and what one can assume to be initialized in
>> >> 'struct disk_partition' after a successful call of one of the
>> >> get_info* family of functions. The new patch 1/2 tries to consolidate
>> >> that by making sure all ->get_info invocations go through a common
>> >> helper that at least always initializes the string members.
>> >> 
>> >> Rasmus Villemoes (2):
>> >>   disk/part.c: ensure strings in struct disk_partition are valid after
>> >>     successful get_info
>> >>   cmd/part.c: implement "part name" subcommand
>> >> 
>> >>  cmd/gpt.c              |  4 +--
>> >>  cmd/part.c             | 16 ++++++++++-
>> >>  disk/part.c            | 62 ++++++++++++++++++++++++------------------
>> >>  doc/usage/cmd/part.rst | 13 +++++++++
>> >>  include/part.h         | 16 +++++++++++
>> >>  5 files changed, 81 insertions(+), 30 deletions(-)
>> >
>> > This leads to some of the bootstd tests failing in CI, unfortunately.
>> 
>> Do you have a link?
>
> Right, sorry:
> https://source.denx.de/u-boot/u-boot/-/jobs/1292597
>
>> Also, how exactly should one run those bootstd tests? When I just build
>> sandbox_defconfig and do 'ut bootstd', I get 130 failures, so I assume I
>> need to do something extra.
>
> So, you want to run it through pytest rather than directly, I find:
> https://docs.u-boot.org/en/latest/develop/pytest/usage.html
> And then
> https://lore.kernel.org/all/20251029143346.1320868-1-kory.maincent@bootlin.com
> has the full rather than partial list of host requirements, if you don't
> want to grab the docker container CI uses and run inside that.

Thanks, I think I figured out the problem. I'll resend properly
tomorrow, but this incremental diff should fix it:

diff --git a/disk/part.c b/disk/part.c
index 194eccadb7b..49a0fca6b89 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -360,8 +360,11 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
                }
 
                ret = part_driver_get_info(drv, desc, part, info);
-               if (!ret)
+               if (ret && ret != -ENOSYS) {
+                       ret = -ENOENT;
+               } else {
                        PRINTF("## Valid %s partition found ##\n", drv->name);
+               }
        }
 
        return ret;

In essence, the problem is/was that part_get_info_by_type() used to
translate any error from ->get_info into -ENOENT, so we have to preserve
that, while still doing the -ENOSYS thing.

Rasmus

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-11-10  0:33 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-20 12:10 [PATCH v2 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
2025-10-20 12:10 ` [PATCH v2 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
2025-11-03 14:07   ` Quentin Schulz
2025-10-20 12:11 ` [PATCH v2 2/2] cmd/part.c: implement "part name" subcommand Rasmus Villemoes
2025-11-03 14:14   ` Quentin Schulz
2025-11-03 20:38     ` Rasmus Villemoes
2025-11-04  9:36       ` Quentin Schulz
2025-11-07 20:19 ` [PATCH v2 0/2] 'part name' subcommand and some robustification Tom Rini
2025-11-08  0:02   ` Rasmus Villemoes
2025-11-08 15:01     ` Tom Rini
2025-11-10  0:33       ` Rasmus Villemoes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox