* [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable @ 2018-05-21 6:35 Fam Zheng 2018-05-21 8:35 ` Peter Maydell 2018-05-24 17:16 ` Paolo Bonzini 0 siblings, 2 replies; 9+ messages in thread From: Fam Zheng @ 2018-05-21 6:35 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, Fam Zheng, Kevin Wolf, Max Reitz, qemu-block Coverity doesn't like the tests under fail label (report CID 1385847). Reset the fields so the clean up order is more apparent. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/nvme.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/block/nvme.c b/block/nvme.c index 6f71122bf5..8239b920c8 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, qemu_co_queue_init(&s->dma_flush_queue); s->nsid = namespace; s->aio_context = bdrv_get_aio_context(bs); + + /* Fields we've not touched should be zero-initialized by block layer + * already, but reset them anyway to make the error handling code easier to + * reason. */ + s->regs = NULL; + s->vfio = NULL; + ret = event_notifier_init(&s->irq_notifier, 0); if (ret) { error_setg(errp, "Failed to init event notifier"); -- 2.14.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable 2018-05-21 6:35 [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable Fam Zheng @ 2018-05-21 8:35 ` Peter Maydell 2018-05-21 8:56 ` Fam Zheng 2018-05-24 17:16 ` Paolo Bonzini 1 sibling, 1 reply; 9+ messages in thread From: Peter Maydell @ 2018-05-21 8:35 UTC (permalink / raw) To: Fam Zheng Cc: QEMU Developers, Kevin Wolf, Paolo Bonzini, Qemu-block, Max Reitz On 21 May 2018 at 07:35, Fam Zheng <famz@redhat.com> wrote: > Coverity doesn't like the tests under fail label (report CID 1385847). > Reset the fields so the clean up order is more apparent. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/nvme.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/block/nvme.c b/block/nvme.c > index 6f71122bf5..8239b920c8 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, > qemu_co_queue_init(&s->dma_flush_queue); > s->nsid = namespace; > s->aio_context = bdrv_get_aio_context(bs); > + > + /* Fields we've not touched should be zero-initialized by block layer > + * already, but reset them anyway to make the error handling code easier to > + * reason. */ > + s->regs = NULL; > + s->vfio = NULL; > + > ret = event_notifier_init(&s->irq_notifier, 0); > if (ret) { > error_setg(errp, "Failed to init event notifier"); I suspect that either coverity or some compilers will complain that the assignment to s->vfio here is redundant, because we'll either return early without looking at it, or we'll get to the assignment s->vfio = qemu_vfio_open_pci(...) which overrides it. thanks -- PMM ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable 2018-05-21 8:35 ` Peter Maydell @ 2018-05-21 8:56 ` Fam Zheng 0 siblings, 0 replies; 9+ messages in thread From: Fam Zheng @ 2018-05-21 8:56 UTC (permalink / raw) To: Peter Maydell Cc: QEMU Developers, Kevin Wolf, Paolo Bonzini, Qemu-block, Max Reitz On Mon, 05/21 09:35, Peter Maydell wrote: > On 21 May 2018 at 07:35, Fam Zheng <famz@redhat.com> wrote: > > Coverity doesn't like the tests under fail label (report CID 1385847). > > Reset the fields so the clean up order is more apparent. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block/nvme.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/block/nvme.c b/block/nvme.c > > index 6f71122bf5..8239b920c8 100644 > > --- a/block/nvme.c > > +++ b/block/nvme.c > > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, > > qemu_co_queue_init(&s->dma_flush_queue); > > s->nsid = namespace; > > s->aio_context = bdrv_get_aio_context(bs); > > + > > + /* Fields we've not touched should be zero-initialized by block layer > > + * already, but reset them anyway to make the error handling code easier to > > + * reason. */ > > + s->regs = NULL; > > + s->vfio = NULL; > > + > > ret = event_notifier_init(&s->irq_notifier, 0); > > if (ret) { > > error_setg(errp, "Failed to init event notifier"); > > I suspect that either coverity or some compilers will complain that > the assignment to s->vfio here is redundant, because we'll either > return early without looking at it, or we'll get to the assignment > s->vfio = qemu_vfio_open_pci(...) > which overrides it. Hmm, okay, the compiler wants you to be at least as smart as it is, but no smarter! I will revise the patch. Fam ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable 2018-05-21 6:35 [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable Fam Zheng 2018-05-21 8:35 ` Peter Maydell @ 2018-05-24 17:16 ` Paolo Bonzini 2018-05-25 2:16 ` Fam Zheng 1 sibling, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2018-05-24 17:16 UTC (permalink / raw) To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Max Reitz, qemu-block On 21/05/2018 08:35, Fam Zheng wrote: > Coverity doesn't like the tests under fail label (report CID 1385847). > Reset the fields so the clean up order is more apparent. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/nvme.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/block/nvme.c b/block/nvme.c > index 6f71122bf5..8239b920c8 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, > qemu_co_queue_init(&s->dma_flush_queue); > s->nsid = namespace; > s->aio_context = bdrv_get_aio_context(bs); > + > + /* Fields we've not touched should be zero-initialized by block layer > + * already, but reset them anyway to make the error handling code easier to > + * reason. */ > + s->regs = NULL; > + s->vfio = NULL; > + > ret = event_notifier_init(&s->irq_notifier, 0); > if (ret) { > error_setg(errp, "Failed to init event notifier"); > I think we should just mark it as a false positive or do something like fail_regs: qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); fail_vfio: qemu_vfio_close(s->vfio); fail: g_free(s->queues); event_notifier_cleanup(&s->irq_notifier); return ret; even though it's a larger patch. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable 2018-05-24 17:16 ` Paolo Bonzini @ 2018-05-25 2:16 ` Fam Zheng 2018-05-25 5:47 ` Markus Armbruster 0 siblings, 1 reply; 9+ messages in thread From: Fam Zheng @ 2018-05-25 2:16 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, Kevin Wolf, Max Reitz, qemu-block On Thu, 05/24 19:16, Paolo Bonzini wrote: > On 21/05/2018 08:35, Fam Zheng wrote: > > Coverity doesn't like the tests under fail label (report CID 1385847). > > Reset the fields so the clean up order is more apparent. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block/nvme.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/block/nvme.c b/block/nvme.c > > index 6f71122bf5..8239b920c8 100644 > > --- a/block/nvme.c > > +++ b/block/nvme.c > > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, > > qemu_co_queue_init(&s->dma_flush_queue); > > s->nsid = namespace; > > s->aio_context = bdrv_get_aio_context(bs); > > + > > + /* Fields we've not touched should be zero-initialized by block layer > > + * already, but reset them anyway to make the error handling code easier to > > + * reason. */ > > + s->regs = NULL; > > + s->vfio = NULL; > > + > > ret = event_notifier_init(&s->irq_notifier, 0); > > if (ret) { > > error_setg(errp, "Failed to init event notifier"); > > > > I think we should just mark it as a false positive or do something like > > fail_regs: > qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); > fail_vfio: > qemu_vfio_close(s->vfio); > fail: > g_free(s->queues); > event_notifier_cleanup(&s->irq_notifier); > return ret; > > even though it's a larger patch. And that makes five labels in total, I'm not sure I like it: fail_handler: aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier, false, NULL, NULL); fail_queue: nvme_free_queue_pair(bs, s->queues[0]); fail_regs: qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); fail_vfio: qemu_vfio_close(s->vfio); fail: g_free(s->queues); event_notifier_cleanup(&s->irq_notifier); return ret; Maybe we just mark it as false positive then? Fam ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable 2018-05-25 2:16 ` Fam Zheng @ 2018-05-25 5:47 ` Markus Armbruster 2018-05-25 6:25 ` Fam Zheng 0 siblings, 1 reply; 9+ messages in thread From: Markus Armbruster @ 2018-05-25 5:47 UTC (permalink / raw) To: Fam Zheng; +Cc: Paolo Bonzini, Kevin Wolf, qemu-devel, qemu-block, Max Reitz Fam Zheng <famz@redhat.com> writes: > On Thu, 05/24 19:16, Paolo Bonzini wrote: >> On 21/05/2018 08:35, Fam Zheng wrote: >> > Coverity doesn't like the tests under fail label (report CID 1385847). >> > Reset the fields so the clean up order is more apparent. >> > >> > Signed-off-by: Fam Zheng <famz@redhat.com> >> > --- >> > block/nvme.c | 7 +++++++ >> > 1 file changed, 7 insertions(+) >> > >> > diff --git a/block/nvme.c b/block/nvme.c >> > index 6f71122bf5..8239b920c8 100644 >> > --- a/block/nvme.c >> > +++ b/block/nvme.c >> > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, >> > qemu_co_queue_init(&s->dma_flush_queue); >> > s->nsid = namespace; >> > s->aio_context = bdrv_get_aio_context(bs); >> > + >> > + /* Fields we've not touched should be zero-initialized by block layer >> > + * already, but reset them anyway to make the error handling code easier to >> > + * reason. */ >> > + s->regs = NULL; >> > + s->vfio = NULL; >> > + >> > ret = event_notifier_init(&s->irq_notifier, 0); >> > if (ret) { >> > error_setg(errp, "Failed to init event notifier"); >> > >> >> I think we should just mark it as a false positive or do something like >> >> fail_regs: >> qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); >> fail_vfio: >> qemu_vfio_close(s->vfio); >> fail: >> g_free(s->queues); >> event_notifier_cleanup(&s->irq_notifier); >> return ret; >> >> even though it's a larger patch. > > And that makes five labels in total, I'm not sure I like it: > > fail_handler: > aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier, > false, NULL, NULL); > fail_queue: > nvme_free_queue_pair(bs, s->queues[0]); > fail_regs: > qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); > fail_vfio: > qemu_vfio_close(s->vfio); > fail: > g_free(s->queues); > event_notifier_cleanup(&s->irq_notifier); > return ret; Doesn't look materially worse to me :) With nice cleanup functions that detect "hasn't been set up" and do nothing then, like free(NULL), you can use just one label. Sadly, cleanup functions are often not nice that way. > Maybe we just mark it as false positive then? > > Fam ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable 2018-05-25 5:47 ` Markus Armbruster @ 2018-05-25 6:25 ` Fam Zheng 2018-05-25 7:27 ` Markus Armbruster 2018-05-25 13:07 ` Eric Blake 0 siblings, 2 replies; 9+ messages in thread From: Fam Zheng @ 2018-05-25 6:25 UTC (permalink / raw) To: Markus Armbruster Cc: Paolo Bonzini, Kevin Wolf, qemu-devel, qemu-block, Max Reitz On Fri, 05/25 07:47, Markus Armbruster wrote: > Fam Zheng <famz@redhat.com> writes: > > > On Thu, 05/24 19:16, Paolo Bonzini wrote: > >> On 21/05/2018 08:35, Fam Zheng wrote: > >> > Coverity doesn't like the tests under fail label (report CID 1385847). > >> > Reset the fields so the clean up order is more apparent. > >> > > >> > Signed-off-by: Fam Zheng <famz@redhat.com> > >> > --- > >> > block/nvme.c | 7 +++++++ > >> > 1 file changed, 7 insertions(+) > >> > > >> > diff --git a/block/nvme.c b/block/nvme.c > >> > index 6f71122bf5..8239b920c8 100644 > >> > --- a/block/nvme.c > >> > +++ b/block/nvme.c > >> > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, > >> > qemu_co_queue_init(&s->dma_flush_queue); > >> > s->nsid = namespace; > >> > s->aio_context = bdrv_get_aio_context(bs); > >> > + > >> > + /* Fields we've not touched should be zero-initialized by block layer > >> > + * already, but reset them anyway to make the error handling code easier to > >> > + * reason. */ > >> > + s->regs = NULL; > >> > + s->vfio = NULL; > >> > + > >> > ret = event_notifier_init(&s->irq_notifier, 0); > >> > if (ret) { > >> > error_setg(errp, "Failed to init event notifier"); > >> > > >> > >> I think we should just mark it as a false positive or do something like > >> > >> fail_regs: > >> qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); > >> fail_vfio: > >> qemu_vfio_close(s->vfio); > >> fail: > >> g_free(s->queues); > >> event_notifier_cleanup(&s->irq_notifier); > >> return ret; > >> > >> even though it's a larger patch. > > > > And that makes five labels in total, I'm not sure I like it: > > > > fail_handler: > > aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier, > > false, NULL, NULL); > > fail_queue: > > nvme_free_queue_pair(bs, s->queues[0]); > > fail_regs: > > qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); > > fail_vfio: > > qemu_vfio_close(s->vfio); > > fail: > > g_free(s->queues); > > event_notifier_cleanup(&s->irq_notifier); > > return ret; > > Doesn't look materially worse to me :) The labels themselves are not ugly or bad, but the goto statements above will be harder to manage. > > With nice cleanup functions that detect "hasn't been set up" and do > nothing then, like free(NULL), you can use just one label. Sadly, > cleanup functions are often not nice that way. nvme_free_queue_pair and qemu_vfio_close are cleanup functions and we can improve them, but to make qemu_vfio_pci_unmap_bar behave similarly is just odd: it's not a clean up function, at least not for s->vfio. Fam ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable 2018-05-25 6:25 ` Fam Zheng @ 2018-05-25 7:27 ` Markus Armbruster 2018-05-25 13:07 ` Eric Blake 1 sibling, 0 replies; 9+ messages in thread From: Markus Armbruster @ 2018-05-25 7:27 UTC (permalink / raw) To: Fam Zheng; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, qemu-block, Max Reitz Fam Zheng <famz@redhat.com> writes: > On Fri, 05/25 07:47, Markus Armbruster wrote: >> Fam Zheng <famz@redhat.com> writes: >> >> > On Thu, 05/24 19:16, Paolo Bonzini wrote: >> >> On 21/05/2018 08:35, Fam Zheng wrote: >> >> > Coverity doesn't like the tests under fail label (report CID 1385847). >> >> > Reset the fields so the clean up order is more apparent. >> >> > >> >> > Signed-off-by: Fam Zheng <famz@redhat.com> >> >> > --- >> >> > block/nvme.c | 7 +++++++ >> >> > 1 file changed, 7 insertions(+) >> >> > >> >> > diff --git a/block/nvme.c b/block/nvme.c >> >> > index 6f71122bf5..8239b920c8 100644 >> >> > --- a/block/nvme.c >> >> > +++ b/block/nvme.c >> >> > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, >> >> > qemu_co_queue_init(&s->dma_flush_queue); >> >> > s->nsid = namespace; >> >> > s->aio_context = bdrv_get_aio_context(bs); >> >> > + >> >> > + /* Fields we've not touched should be zero-initialized by block layer >> >> > + * already, but reset them anyway to make the error handling code easier to >> >> > + * reason. */ >> >> > + s->regs = NULL; >> >> > + s->vfio = NULL; >> >> > + >> >> > ret = event_notifier_init(&s->irq_notifier, 0); >> >> > if (ret) { >> >> > error_setg(errp, "Failed to init event notifier"); >> >> > >> >> >> >> I think we should just mark it as a false positive or do something like >> >> >> >> fail_regs: >> >> qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); >> >> fail_vfio: >> >> qemu_vfio_close(s->vfio); >> >> fail: >> >> g_free(s->queues); >> >> event_notifier_cleanup(&s->irq_notifier); >> >> return ret; >> >> >> >> even though it's a larger patch. >> > >> > And that makes five labels in total, I'm not sure I like it: >> > >> > fail_handler: >> > aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier, >> > false, NULL, NULL); >> > fail_queue: >> > nvme_free_queue_pair(bs, s->queues[0]); >> > fail_regs: >> > qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); >> > fail_vfio: >> > qemu_vfio_close(s->vfio); >> > fail: >> > g_free(s->queues); >> > event_notifier_cleanup(&s->irq_notifier); >> > return ret; >> >> Doesn't look materially worse to me :) > > The labels themselves are not ugly or bad, but the goto statements above will be > harder to manage. Slightly. The difference between three and five feels smaller than say the one between one and three. Admittedly subjective. >> With nice cleanup functions that detect "hasn't been set up" and do >> nothing then, like free(NULL), you can use just one label. Sadly, >> cleanup functions are often not nice that way. > > nvme_free_queue_pair and qemu_vfio_close are cleanup functions and we can > improve them, but to make qemu_vfio_pci_unmap_bar behave similarly is just odd: > it's not a clean up function, at least not for s->vfio. The technique isn't "all or nothing". Reducing the number of labels is nice even when you can't reduce them to one. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable 2018-05-25 6:25 ` Fam Zheng 2018-05-25 7:27 ` Markus Armbruster @ 2018-05-25 13:07 ` Eric Blake 1 sibling, 0 replies; 9+ messages in thread From: Eric Blake @ 2018-05-25 13:07 UTC (permalink / raw) To: Fam Zheng, Markus Armbruster Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, qemu-block, Max Reitz On 05/25/2018 01:25 AM, Fam Zheng wrote: >>> And that makes five labels in total, I'm not sure I like it: >>> >>> fail_handler: >>> aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier, >>> false, NULL, NULL); >>> fail_queue: >>> nvme_free_queue_pair(bs, s->queues[0]); >>> fail_regs: >>> qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); >>> fail_vfio: >>> qemu_vfio_close(s->vfio); >>> fail: >>> g_free(s->queues); >>> event_notifier_cleanup(&s->irq_notifier); >>> return ret; >> >> Doesn't look materially worse to me :) > > The labels themselves are not ugly or bad, but the goto statements above will be > harder to manage. > >> >> With nice cleanup functions that detect "hasn't been set up" and do >> nothing then, like free(NULL), you can use just one label. Sadly, >> cleanup functions are often not nice that way. > > nvme_free_queue_pair and qemu_vfio_close are cleanup functions and we can > improve them, but to make qemu_vfio_pci_unmap_bar behave similarly is just odd: > it's not a clean up function, at least not for s->vfio. But even then, you can do: fail: if (s->vfio) { qemu_vfio_close(s->vfio); } That is, there are ways to make a single cleanup path more applicable, regardless of where you decided you needed an early cleanup. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-05-25 13:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-21 6:35 [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable Fam Zheng 2018-05-21 8:35 ` Peter Maydell 2018-05-21 8:56 ` Fam Zheng 2018-05-24 17:16 ` Paolo Bonzini 2018-05-25 2:16 ` Fam Zheng 2018-05-25 5:47 ` Markus Armbruster 2018-05-25 6:25 ` Fam Zheng 2018-05-25 7:27 ` Markus Armbruster 2018-05-25 13:07 ` Eric Blake
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).