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

[Fam Zheng <famz@redhat.com>]

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