[PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Markus Armbruster]

We allocate VuVirtqElement with g_malloc() in
virtqueue_alloc_element(), but free it with free() in
vhost-user-blk.c.  Harmless, but use g_free() anyway.

One of the calls is guarded by a "not null" condition.  Useless,
because it cannot be null (it's dereferenced right before), and even
it it could be, free() and g_free() do the right thing.  Drop the
conditional.

Fixes: Coverity CID 1490290
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
Not even compile-tested, because I can't figure out how this thing is
supposed to be built.  Its initial commit message says "make
vhost-user-blk", but that doesn't work anymore.

 contrib/vhost-user-blk/vhost-user-blk.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
index 9cb78ca1d0..d6932a2645 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -106,10 +106,7 @@ static void vub_req_complete(VubReq *req)
                   req->size + 1);
     vu_queue_notify(vu_dev, req->vq);
 
-    if (req->elem) {
-        free(req->elem);
-    }
-
+    g_free(req->elem);
     g_free(req);
 }
 
@@ -243,7 +240,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
     /* refer to hw/block/virtio_blk.c */
     if (elem->out_num < 1 || elem->in_num < 1) {
         fprintf(stderr, "virtio-blk request missing headers\n");
-        free(elem);
+        g_free(elem);
         return -1;
     }
 
@@ -325,7 +322,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
     return 0;
 
 err:
-    free(elem);
+    g_free(elem);
     g_free(req);
     return -1;
 }
-- 
2.35.3

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Raphael Norwitz]

On Thu, Jun 30, 2022 at 10:52:19AM +0200, Markus Armbruster wrote:
> We allocate VuVirtqElement with g_malloc() in
> virtqueue_alloc_element(), but free it with free() in
> vhost-user-blk.c.  Harmless, but use g_free() anyway.
> 
> One of the calls is guarded by a "not null" condition.  Useless,
> because it cannot be null (it's dereferenced right before), and even

NIT: if it

> it it could be, free() and g_free() do the right thing.  Drop the
> conditional.
> 

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> Fixes: Coverity CID 1490290
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> Not even compile-tested, because I can't figure out how this thing is
> supposed to be built.  Its initial commit message says "make
> vhost-user-blk", but that doesn't work anymore.
> 

make contrib/vhost-user-blk/vhost-user-blk works for me.

>  contrib/vhost-user-blk/vhost-user-blk.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
> index 9cb78ca1d0..d6932a2645 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -106,10 +106,7 @@ static void vub_req_complete(VubReq *req)
>                    req->size + 1);
>      vu_queue_notify(vu_dev, req->vq);
>  
> -    if (req->elem) {
> -        free(req->elem);
> -    }
> -
> +    g_free(req->elem);
>      g_free(req);
>  }
>  
> @@ -243,7 +240,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
>      /* refer to hw/block/virtio_blk.c */
>      if (elem->out_num < 1 || elem->in_num < 1) {
>          fprintf(stderr, "virtio-blk request missing headers\n");
> -        free(elem);
> +        g_free(elem);
>          return -1;
>      }
>  
> @@ -325,7 +322,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
>      return 0;
>  
>  err:
> -    free(elem);
> +    g_free(elem);
>      g_free(req);
>      return -1;
>  }
> -- 
> 2.35.3
> 

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Markus Armbruster]

Raphael Norwitz <raphael.norwitz@nutanix.com> writes:

> On Thu, Jun 30, 2022 at 10:52:19AM +0200, Markus Armbruster wrote:
>> We allocate VuVirtqElement with g_malloc() in
>> virtqueue_alloc_element(), but free it with free() in
>> vhost-user-blk.c.  Harmless, but use g_free() anyway.
>> 
>> One of the calls is guarded by a "not null" condition.  Useless,
>> because it cannot be null (it's dereferenced right before), and even
>
> NIT: if it

Yes.

>> it it could be, free() and g_free() do the right thing.  Drop the
>> conditional.
>> 
>
> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
>
>> Fixes: Coverity CID 1490290
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> Not even compile-tested, because I can't figure out how this thing is
>> supposed to be built.  Its initial commit message says "make
>> vhost-user-blk", but that doesn't work anymore.
>> 
>
> make contrib/vhost-user-blk/vhost-user-blk works for me.

Could we use a contrib/README with an explanation what "contrib" means,
and how to build and use the stuff there?

Thanks!

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Markus Armbruster]

Could this go via qemu-trivial now?

Markus Armbruster <armbru@redhat.com> writes:

> We allocate VuVirtqElement with g_malloc() in
> virtqueue_alloc_element(), but free it with free() in
> vhost-user-blk.c.  Harmless, but use g_free() anyway.
>
> One of the calls is guarded by a "not null" condition.  Useless,
> because it cannot be null (it's dereferenced right before), and even
> it it could be, free() and g_free() do the right thing.  Drop the
> conditional.
>
> Fixes: Coverity CID 1490290
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> Not even compile-tested, because I can't figure out how this thing is
> supposed to be built.  Its initial commit message says "make
> vhost-user-blk", but that doesn't work anymore.
>
>  contrib/vhost-user-blk/vhost-user-blk.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
> index 9cb78ca1d0..d6932a2645 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -106,10 +106,7 @@ static void vub_req_complete(VubReq *req)
>                    req->size + 1);
>      vu_queue_notify(vu_dev, req->vq);
>  
> -    if (req->elem) {
> -        free(req->elem);
> -    }
> -
> +    g_free(req->elem);
>      g_free(req);
>  }
>  
> @@ -243,7 +240,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
>      /* refer to hw/block/virtio_blk.c */
>      if (elem->out_num < 1 || elem->in_num < 1) {
>          fprintf(stderr, "virtio-blk request missing headers\n");
> -        free(elem);
> +        g_free(elem);
>          return -1;
>      }
>  
> @@ -325,7 +322,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
>      return 0;
>  
>  err:
> -    free(elem);
> +    g_free(elem);
>      g_free(req);
>      return -1;
>  }

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Michael S. Tsirkin]

On Thu, Jun 30, 2022 at 10:52:19AM +0200, Markus Armbruster wrote:
> We allocate VuVirtqElement with g_malloc() in
> virtqueue_alloc_element(), but free it with free() in
> vhost-user-blk.c.  Harmless, but use g_free() anyway.
> 
> One of the calls is guarded by a "not null" condition.  Useless,
> because it cannot be null (it's dereferenced right before), and even
> it it could be, free() and g_free() do the right thing.  Drop the
> conditional.
> 
> Fixes: Coverity CID 1490290
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Thanks!

Acked-by: Michael S. Tsirkin <mst@redhat.com>

trivial tree pls.


> ---
> Not even compile-tested, because I can't figure out how this thing is
> supposed to be built.  Its initial commit message says "make
> vhost-user-blk", but that doesn't work anymore.
> 
>  contrib/vhost-user-blk/vhost-user-blk.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
> index 9cb78ca1d0..d6932a2645 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -106,10 +106,7 @@ static void vub_req_complete(VubReq *req)
>                    req->size + 1);
>      vu_queue_notify(vu_dev, req->vq);
>  
> -    if (req->elem) {
> -        free(req->elem);
> -    }
> -
> +    g_free(req->elem);
>      g_free(req);
>  }
>  
> @@ -243,7 +240,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
>      /* refer to hw/block/virtio_blk.c */
>      if (elem->out_num < 1 || elem->in_num < 1) {
>          fprintf(stderr, "virtio-blk request missing headers\n");
> -        free(elem);
> +        g_free(elem);
>          return -1;
>      }
>  
> @@ -325,7 +322,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
>      return 0;
>  
>  err:
> -    free(elem);
> +    g_free(elem);
>      g_free(req);
>      return -1;
>  }
> -- 
> 2.35.3

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Peter Maydell]

On Fri, 1 Jul 2022 at 06:41, Markus Armbruster <armbru@redhat.com> wrote:
> Could we use a contrib/README with an explanation what "contrib" means,
> and how to build and use the stuff there?

I would rather we got rid of contrib/ entirely. Our git repo
should contain things we care about enough to really support
and believe in, in which case they should be in top level
directories matching what they are (eg tools/). If we don't
believe in these things enough to really support them, then
we should drop them, and let those who do care maintain them
as out-of-tree tools if they like.

subprojects/ is similarly vague.

thanks
-- PMM

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Raphael Norwitz]

On Tue, Jul 26, 2022 at 03:57:42PM +0100, Peter Maydell wrote:
> On Fri, 1 Jul 2022 at 06:41, Markus Armbruster <armbru@redhat.com> wrote:
> > Could we use a contrib/README with an explanation what "contrib" means,
> > and how to build and use the stuff there?
> 
> I would rather we got rid of contrib/ entirely. Our git repo
> should contain things we care about enough to really support
> and believe in, in which case they should be in top level
> directories matching what they are (eg tools/). If we don't
> believe in these things enough to really support them, then
> we should drop them, and let those who do care maintain them
> as out-of-tree tools if they like.
>

I can't speak for a lot of stuff in contrib/ but I find the vhost-user
backends like vhost-user-blk and vhost-user-scsi helpful for testing and
development. I would like to keep maintaining those two at least.

> subprojects/ is similarly vague.
> 

Again, I can't say much for other stuff in subprojects/ but
libvhost-user is clearly important. Maybe we move libvhost-user to
another directroy and the libvhost-user based backends there too?

> thanks
> -- PMM

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Peter Maydell]

On Wed, 27 Jul 2022 at 18:28, Raphael Norwitz
<raphael.norwitz@nutanix.com> wrote:
>
> On Tue, Jul 26, 2022 at 03:57:42PM +0100, Peter Maydell wrote:
> > On Fri, 1 Jul 2022 at 06:41, Markus Armbruster <armbru@redhat.com> wrote:
> > > Could we use a contrib/README with an explanation what "contrib" means,
> > > and how to build and use the stuff there?
> >
> > I would rather we got rid of contrib/ entirely. Our git repo
> > should contain things we care about enough to really support
> > and believe in, in which case they should be in top level
> > directories matching what they are (eg tools/). If we don't
> > believe in these things enough to really support them, then
> > we should drop them, and let those who do care maintain them
> > as out-of-tree tools if they like.
> >
>
> I can't speak for a lot of stuff in contrib/ but I find the vhost-user
> backends like vhost-user-blk and vhost-user-scsi helpful for testing and
> development. I would like to keep maintaining those two at least.

Right, I don't mean we should just delete contrib/, but for the
things currently in it that we do care about, we should define
what their relationship to QEMU is and put them in a part of
the source tree that says what they actually are. contrib/
just means "nobody thought about it".

-- PMM

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Laurent Vivier]

Le 30/06/2022 à 10:52, Markus Armbruster a écrit :
> We allocate VuVirtqElement with g_malloc() in
> virtqueue_alloc_element(), but free it with free() in
> vhost-user-blk.c.  Harmless, but use g_free() anyway.
> 
> One of the calls is guarded by a "not null" condition.  Useless,
> because it cannot be null (it's dereferenced right before), and even
> it it could be, free() and g_free() do the right thing.  Drop the
> conditional.
> 
> Fixes: Coverity CID 1490290
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> Not even compile-tested, because I can't figure out how this thing is
> supposed to be built.  Its initial commit message says "make
> vhost-user-blk", but that doesn't work anymore.
> 
>   contrib/vhost-user-blk/vhost-user-blk.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
> index 9cb78ca1d0..d6932a2645 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -106,10 +106,7 @@ static void vub_req_complete(VubReq *req)
>                     req->size + 1);
>       vu_queue_notify(vu_dev, req->vq);
>   
> -    if (req->elem) {
> -        free(req->elem);
> -    }
> -
> +    g_free(req->elem);
>       g_free(req);
>   }
>   
> @@ -243,7 +240,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
>       /* refer to hw/block/virtio_blk.c */
>       if (elem->out_num < 1 || elem->in_num < 1) {
>           fprintf(stderr, "virtio-blk request missing headers\n");
> -        free(elem);
> +        g_free(elem);
>           return -1;
>       }
>   
> @@ -325,7 +322,7 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
>       return 0;
>   
>   err:
> -    free(elem);
> +    g_free(elem);
>       g_free(req);
>       return -1;
>   }

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 27 Jul 2022 at 18:28, Raphael Norwitz
> <raphael.norwitz@nutanix.com> wrote:
>>
>> On Tue, Jul 26, 2022 at 03:57:42PM +0100, Peter Maydell wrote:
>> > On Fri, 1 Jul 2022 at 06:41, Markus Armbruster <armbru@redhat.com> wrote:
>> > > Could we use a contrib/README with an explanation what "contrib" means,
>> > > and how to build and use the stuff there?
>> >
>> > I would rather we got rid of contrib/ entirely. Our git repo
>> > should contain things we care about enough to really support
>> > and believe in, in which case they should be in top level
>> > directories matching what they are (eg tools/). If we don't
>> > believe in these things enough to really support them, then
>> > we should drop them, and let those who do care maintain them
>> > as out-of-tree tools if they like.
>> >
>>
>> I can't speak for a lot of stuff in contrib/ but I find the vhost-user
>> backends like vhost-user-blk and vhost-user-scsi helpful for testing and
>> development. I would like to keep maintaining those two at least.
>
> Right, I don't mean we should just delete contrib/, but for the
> things currently in it that we do care about, we should define
> what their relationship to QEMU is and put them in a part of
> the source tree that says what they actually are. contrib/
> just means "nobody thought about it".

I split plugins a while ago between:

  tests/plugin
  contrib/plugins

where the former are really basic plugins that show usage, exercise the
API and are included in the check-tcg tests. The contrib plugins are
slightly more random mix of useful (e.g. cache, execlog), downright
experimental (lockstep) and stuff I can't actually test (e.g. drcov).

I'll quite happily continue to process patches that update and enhance
contrib/plugins but at a push a few could be promoted to less of a
dumping ground (tools/tcg-plugins?).

I guess it would only really matter if we were installing plugins as
part of "make install"?

-- 
Alex Bennée