| Message ID | 20220630085219.1305519-1-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement | expand |
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 >
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!
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; > }
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
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
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
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
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
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"?
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; }
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(-)