Re: [RESEND V3] ieee1275/ofdisk: vscsi lun handling on lun len

[avnish]

On 2024-09-09 13:53, Mukesh Kumar Chaurasiya wrote:
> The information about "vscsi-report-luns" data is a list of disk 
> details
> with pairs of memory addresses and lengths.
>
>                   8 bytes     8 bytes
> lun-addr  --->   ------------------------              8 bytes
>         ^        |  buf-addr | lun-count| --------> -------------
>         |        ------------------------           |   lun     |
>         |        |  buf-addr | lun-count| ----|     -------------
>      "len"       ------------------------     |     |  ...      |
>         |        |    ...               |     |     -------------
>         |        ------------------------     |     |   lun     |
>         |        |  buf-addr | lun-count|     |     -------------
>         V        ------------------------     |
>                                               |---> -------------
>                                                     |   lun     |
>                                                     -------------
>                                                     |  ...      |
>                                                     -------------
>                                                     |   lun     |
>                                                     -------------
> The way the expression (args.table + 4 + 8 * i) is used is incorrect 
> and
> can be confusing. The list of LUNs doesn't end with NULL, indicated by
> while (*ptr). Usually, this loop doesn't process any LUNs because it 
> ends
> before checking any as first reported LUN is likely to be 0. The list 
> of
> LUNs ends based on its length, not by a NULL value.
>
> Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
> ---
>  grub-core/disk/ieee1275/ofdisk.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c 
> b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..69655eb37 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -43,6 +43,11 @@ struct ofdisk_hash_ent
>    struct ofdisk_hash_ent *next;
>  };
>
> +struct lun_buf {
> +  grub_uint64_t buf_addr;
> +  grub_uint64_t lun_count;
> +};
> +

Hi Mukesh,

Coding style looks off in structure definition. '{' needs to be moved to 
next line.
Thank you!

Regards,
Avnish Chouhan

>  static grub_err_t
>  grub_ofdisk_get_block_size (const char *device, grub_uint32_t 
> *block_size,
>                             struct ofdisk_hash_ent *op);
> @@ -222,8 +227,9 @@ dev_iterate (const struct grub_ieee1275_devalias 
> *alias)
>         grub_ieee1275_cell_t table;
>        }
>        args;
> +      struct lun_buf *tbl;
>        char *buf, *bufptr;
> -      unsigned i;
> +      unsigned int i, j;
>
>        if (grub_ieee1275_open (alias->path, &ihandle))
>         return;
> @@ -248,17 +254,18 @@ dev_iterate (const struct grub_ieee1275_devalias 
> *alias)
>         return;
>        bufptr = grub_stpcpy (buf, alias->path);
>
> +      tbl = (struct lun_len *) args.table;
>        for (i = 0; i < args.nentries; i++)
> -       {
> -         grub_uint64_t *ptr;
> -
> -         ptr = *(grub_uint64_t **) (args.table + 4 + 8 * i);
> -         while (*ptr)
> -           {
> -	      grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, 
> *ptr++);
> -             dev_iterate_real (buf, buf);
> -           }
> -       }
> +        {
> +          grub_uint64_t *ptr;
> +
> +          ptr = (grub_uint64_t *)(grub_addr_t) tbl[i].buf_addr;
> +          for (j = 0; j < tbl[i].lun_count; j++)
> +           {
> +             grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, 
> *ptr++);
> +             dev_iterate_real (buf, buf);
> +           }
> +        }
>        grub_ieee1275_close (ihandle);
>        grub_free (buf);
>        return;