[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH] nvme: Make nvme_init error handlin
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable |
Date: |
Fri, 25 May 2018 09:27:34 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Fam Zheng <address@hidden> writes:
> On Fri, 05/25 07:47, Markus Armbruster wrote:
>> Fam Zheng <address@hidden> 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 <address@hidden>
>> >> > ---
>> >> > 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.