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

[Eric Blake <eblake@redhat.com>]

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