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

[Michael Tokarev]

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.

/mjt