Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement
Peter Maydell writes: > On Wed, 27 Jul 2022 at 18:28, Raphael Norwitz > wrote: >> >> On Tue, Jul 26, 2022 at 03:57:42PM +0100, Peter Maydell wrote: >> > On Fri, 1 Jul 2022 at 06:41, Markus Armbruster 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
Re: [PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement
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 --- 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
On Wed, 27 Jul 2022 at 18:28, Raphael Norwitz wrote: > > On Tue, Jul 26, 2022 at 03:57:42PM +0100, Peter Maydell wrote: > > On Fri, 1 Jul 2022 at 06:41, Markus Armbruster 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
On Tue, Jul 26, 2022 at 03:57:42PM +0100, Peter Maydell wrote: > On Fri, 1 Jul 2022 at 06:41, Markus Armbruster 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
On Fri, 1 Jul 2022 at 06:41, Markus Armbruster 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
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 Thanks! Acked-by: Michael S. Tsirkin 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
Could this go via qemu-trivial now? Markus Armbruster 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 > --- > 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
Raphael Norwitz 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 > >> Fixes: Coverity CID 1490290 >> Signed-off-by: Markus Armbruster >> --- >> 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
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 > Fixes: Coverity CID 1490290 > Signed-off-by: Markus Armbruster > --- > 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 >
[PATCH] contrib/vhost-user-blk: Clean up deallocation of VuVirtqElement
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 --- 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