Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable

[Markus Armbruster <armbru@redhat.com>]

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.