Hi Rasmus,
On Tue Nov 11, 2025 at 2:24 AM IST, 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.
>
> 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 <[email protected]>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> cmd/gpt.c | 4 ++--
> disk/part.c | 63 +++++++++++++++++++++++++++++---------------------
> include/part.h | 16 +++++++++++++
> 3 files changed, 55 insertions(+), 28 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..49a0fca6b89 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,16 @@ 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 && ret != -ENOSYS) {
> + ret = -ENOENT;
Why are we overwriting the err code from part_driver_get_info here?
Regards,
Anshul
> + } else {
> 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 +674,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 +709,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)
