[PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue

Stefan Hajnoczi posted 5 patches 9 months, 3 weeks ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Markus Armbruster <armbru@redhat.com>, Michael Roth <michael.roth@amd.com>
There is a newer version of this series
[PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue
Posted by Stefan Hajnoczi 9 months, 3 weeks ago
It is not possible to instantiate a virtio-blk device with 0 virtqueues.
The following check is located in ->realize():

  if (!conf->num_queues) {
      error_setg(errp, "num-queues property must be larger than 0");
      return;
  }

Later on we access s->vq_aio_context[0] under the assumption that there
is as least one virtqueue. Hanna Czenczek <hreitz@redhat.com> noted that
it would help to show that the array index is already valid.

Add an assertion to document that s->vq_aio_context[0] is always
safe...and catch future code changes that break this assumption.

Suggested-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e8b37fd5f4..a0735a9bca 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1825,6 +1825,7 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
      * Try to change the AioContext so that block jobs and other operations can
      * co-locate their activity in the same AioContext. If it fails, nevermind.
      */
+    assert(nvqs > 0); /* enforced during ->realize() */
     r = blk_set_aio_context(s->conf.conf.blk, s->vq_aio_context[0],
                             &local_err);
     if (r < 0) {
-- 
2.43.0
Re: [PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue
Posted by Hanna Czenczek 9 months, 3 weeks ago
On 05.02.24 18:26, Stefan Hajnoczi wrote:
> It is not possible to instantiate a virtio-blk device with 0 virtqueues.
> The following check is located in ->realize():
>
>    if (!conf->num_queues) {
>        error_setg(errp, "num-queues property must be larger than 0");
>        return;
>    }
>
> Later on we access s->vq_aio_context[0] under the assumption that there
> is as least one virtqueue. Hanna Czenczek<hreitz@redhat.com>  noted that
> it would help to show that the array index is already valid.
>
> Add an assertion to document that s->vq_aio_context[0] is always
> safe...and catch future code changes that break this assumption.
>
> Suggested-by: Hanna Czenczek<hreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi<stefanha@redhat.com>
> ---
>   hw/block/virtio-blk.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Re: [PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue
Posted by Manos Pitsidianakis 9 months, 3 weeks ago
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>It is not possible to instantiate a virtio-blk device with 0 virtqueues.
>The following check is located in ->realize():
>
>  if (!conf->num_queues) {
>      error_setg(errp, "num-queues property must be larger than 0");
>      return;
>  }
>
>Later on we access s->vq_aio_context[0] under the assumption that there
>is as least one virtqueue. Hanna Czenczek <hreitz@redhat.com> noted that
>it would help to show that the array index is already valid.
>
>Add an assertion to document that s->vq_aio_context[0] is always
>safe...and catch future code changes that break this assumption.
>
>Suggested-by: Hanna Czenczek <hreitz@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> hw/block/virtio-blk.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>index e8b37fd5f4..a0735a9bca 100644
>--- a/hw/block/virtio-blk.c
>+++ b/hw/block/virtio-blk.c
>@@ -1825,6 +1825,7 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
>      * Try to change the AioContext so that block jobs and other operations can
>      * co-locate their activity in the same AioContext. If it fails, nevermind.
>      */
>+    assert(nvqs > 0); /* enforced during ->realize() */
>     r = blk_set_aio_context(s->conf.conf.blk, s->vq_aio_context[0],
>                             &local_err);
>     if (r < 0) {
>-- 
>2.43.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>