Re: [PATCH v2 1/6] libqos/virtio.c: init all elems in qvring_indirect_desc_setup()

[Thomas Huth]

On 01/03/2024 16.57, Michael Tokarev wrote:
> 17.02.2024 22:26, Daniel Henrique Barboza:
>
> > diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> > index 410513225f..4f39124eba 100644
> > --- a/tests/qtest/libqos/virtio.c
> > +++ b/tests/qtest/libqos/virtio.c
> > @@ -280,14 +280,27 @@ QVRingIndirectDesc 
> > *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
> >       indirect->elem = elem;
> >       indirect->desc = guest_alloc(alloc, sizeof(struct vring_desc) * elem);
> > -    for (i = 0; i < elem - 1; ++i) {
> > +    for (i = 0; i < elem; ++i) {
> >           /* indirect->desc[i].addr */
> >           qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0);
> > -        /* indirect->desc[i].flags */
> > -        qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
> > -                       VRING_DESC_F_NEXT);
> > -        /* indirect->desc[i].next */
> > -        qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
> > +
> > +        /*
> > +         * If it's not the last element of the ring, set
> > +         * the chain (VRING_DESC_F_NEXT) flag and
> > +         * desc->next. Clear the last element - there's
> > +         * no guarantee that guest_alloc() will do it.
> > +         */
> > +        if (i != elem - 1) {
> > +            /* indirect->desc[i].flags */
> > +            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
> > +                           VRING_DESC_F_NEXT);
> > +
> > +            /* indirect->desc[i].next */
> > +            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
> > +        } else {
> > +            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12, 0);
> > +            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, 0);
> > +        }
> >       }
>
> In my view it would be cleaner to add 2 extra function calls after this
> loop for the i==elem-1 case:
>
>      for (i = 0; i < elem - 1; ++i) {
>          /* indirect->desc[i].addr */
>          qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0);
>          /* indirect->desc[i].flags */
>          qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
>                         VRING_DESC_F_NEXT);
>          /* indirect->desc[i].next */
>          qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
>
>      }
>
>       /* clear last element */
>       qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0);
>       qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12, 0);
>       qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, 0);
>
>
> FWIW, -- it's too late to change it now I think.

Sorry, too late, since the first two patches looked like some good (and 
riscv-independent) libqos fixes, I went ahead and stuck them into my pull 
request today. Feel free to send a follow up patch if it bugs you too much.

 Thomas