Re: [PATCH 08/20] init: refactor devt_from_partuuid

[Jan Kara <jack@xxxxxxx>]

On Wed 18-11-20 09:47:48, Christoph Hellwig wrote:
> The code in devt_from_partuuid is very convoluted.  Refactor a bit by
> sanitizing the goto and variable name usage.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

                                                                Honza

> ---
>  init/do_mounts.c | 68 ++++++++++++++++++++++--------------------------
>  1 file changed, 31 insertions(+), 37 deletions(-)
> 
> diff --git a/init/do_mounts.c b/init/do_mounts.c
> index aef2f24461c7f1..afa26a4028d25e 100644
> --- a/init/do_mounts.c
> +++ b/init/do_mounts.c
> @@ -105,13 +105,10 @@ static int match_dev_by_uuid(struct device *dev, const 
> void *data)
>   */
>  static dev_t devt_from_partuuid(const char *uuid_str)
>  {
> -     dev_t res = 0;
>       struct uuidcmp cmp;
>       struct device *dev = NULL;
> -     struct gendisk *disk;
> -     struct hd_struct *part;
> +     dev_t devt = 0;
>       int offset = 0;
> -     bool clear_root_wait = false;
>       char *slash;
>  
>       cmp.uuid = uuid_str;
> @@ -120,52 +117,49 @@ static dev_t devt_from_partuuid(const char *uuid_str)
>       /* Check for optional partition number offset attributes. */
>       if (slash) {
>               char c = 0;
> +
>               /* Explicitly fail on poor PARTUUID syntax. */
> -             if (sscanf(slash + 1,
> -                        "PARTNROFF=%d%c", &offset, &c) != 1) {
> -                     clear_root_wait = true;
> -                     goto done;
> -             }
> +             if (sscanf(slash + 1, "PARTNROFF=%d%c", &offset, &c) != 1)
> +                     goto clear_root_wait;
>               cmp.len = slash - uuid_str;
>       } else {
>               cmp.len = strlen(uuid_str);
>       }
>  
> -     if (!cmp.len) {
> -             clear_root_wait = true;
> -             goto done;
> -     }
> +     if (!cmp.len)
> +             goto clear_root_wait;
>  
> -     dev = class_find_device(&block_class, NULL, &cmp,
> -                             &match_dev_by_uuid);
> +     dev = class_find_device(&block_class, NULL, &cmp, &match_dev_by_uuid);
>       if (!dev)
> -             goto done;
> -
> -     res = dev->devt;
> +             return 0;
>  
> -     /* Attempt to find the partition by offset. */
> -     if (!offset)
> -             goto no_offset;
> +     if (offset) {
> +             /*
> +              * Attempt to find the requested partition by adding an offset
> +              * to the partition number found by UUID.
> +              */
> +             struct hd_struct *part;
>  
> -     res = 0;
> -     disk = part_to_disk(dev_to_part(dev));
> -     part = disk_get_part(disk, dev_to_part(dev)->partno + offset);
> -     if (part) {
> -             res = part_devt(part);
> -             put_device(part_to_dev(part));
> +             part = disk_get_part(dev_to_disk(dev),
> +                                  dev_to_part(dev)->partno + offset);
> +             if (part) {
> +                     devt = part_devt(part);
> +                     put_device(part_to_dev(part));
> +             }
> +     } else {
> +             devt = dev->devt;
>       }
>  
> -no_offset:
>       put_device(dev);
> -done:
> -     if (clear_root_wait) {
> -             pr_err("VFS: PARTUUID= is invalid.\n"
> -                    "Expected PARTUUID=<valid-uuid-id>[/PARTNROFF=%%d]\n");
> -             if (root_wait)
> -                     pr_err("Disabling rootwait; root= is invalid.\n");
> -             root_wait = 0;
> -     }
> -     return res;
> +     return devt;
> +
> +clear_root_wait:
> +     pr_err("VFS: PARTUUID= is invalid.\n"
> +            "Expected PARTUUID=<valid-uuid-id>[/PARTNROFF=%%d]\n");
> +     if (root_wait)
> +             pr_err("Disabling rootwait; root= is invalid.\n");
> +     root_wait = 0;
> +     return 0;
>  }
>  
>  /**
> -- 
> 2.29.2
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR