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

[Daniel Kiper]

On Wed, Nov 08, 2023 at 08:10:35PM +0530, Mukesh Kumar Chaurasiya wrote:
> The returned data for "vscsi-report-luns" is actually an array ofdisk
> buffer-addr and buffer-len pairs. The expression "(args.table + 4 + 8 * i)"
> is incorrect, in addition to being confusing. In addition, the list of

These sentences do not parse well due to at least "addition" repetition.
Please fix this.

> luns is not NULL-terminated, as implemented by the "while (*ptr)":
> since the first reported LUN is likely to be "0" this loop ends up
> processing no LUNs at all in most cases. The list of LUNs is terminated
> according to length, not by a value.

In general this commit message does not parse well mostly due to listing
mistakes in row followed by explanation how (all of) them are fixed.
Please think how to improve that.

Additionally, current commit message suggest (a bit) that this patch
should be split into two. This does not seem true but it is disturbing.
Again, it has to be fixed.

> Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
> ---
>  grub-core/disk/ieee1275/ofdisk.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c 
> b/grub-core/disk/ieee1275/ofdisk.c
> index c58a1fce8..eca447dc0 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -225,8 +225,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>       grub_ieee1275_cell_t table;
>        }
>        args;
> +      struct lun_len {

s/lun_len/lun_buf/?

> +     grub_uint64_t *buf_adr;

s/buf_adr/buf_addr/

> +     grub_uint64_t buf_len;
> +      } *tbl;
>        char *buf, *bufptr;
> -      unsigned i;
> +      unsigned i, j;

s/unsigned/unsigned int/

>        if (grub_ieee1275_open (alias->path, &ihandle))
>       return;
> @@ -251,12 +255,13 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>       return;
>        bufptr = grub_stpcpy (buf, alias->path);
>
> +      tbl = (struct lun_len *)args.table;

Please add space before args.table.

>        for (i = 0; i < args.nentries; i++)
>       {
>         grub_uint64_t *ptr;
>
> -       ptr = *(grub_uint64_t **) (args.table + 4 + 8 * i);
> -       while (*ptr)
> +       ptr = (grub_uint64_t *)(grub_addr_t)tbl[i].buf_adr;

Please add space before tbl[i].buf_adr.

> +       for (j = 0; j < tbl[i].buf_len; j++)
>           {
>             grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, *ptr++);
>             dev_iterate_real (buf, buf);

Daniel