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

[Eric Blake]

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