Re: [Qemu-devel] [PATCH 24/38] ivshmem: Propagate errors through ivshmem_recv_setup()

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Wed, Mar 2, 2016 at 8:35 PM, Markus Armbruster <address@hidden> wrote:
>> You know, I'd prefer that, too, and I've argued for it unsuccessfully.
>> As it is, we fairly consistently return void when the function returns
>> errors through Error ** and has no non-error value.
>
> Good to know we are on same side.
>
>>>>  {
>>>>      PCIDevice *pdev = PCI_DEVICE(s);
>>>>      MSIMessage msg = msix_get_message(pdev, vector);
>>>> @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s, 
>>>> int vector)
>>>>
>>>>      ret = kvm_irqchip_add_msi_route(kvm_state, msg, pdev);
>>>>      if (ret < 0) {
>>>> -        error_report("ivshmem: kvm_irqchip_add_msi_route failed");
>>>> -        return -1;
>>>> +        error_setg(errp, "kvm_irqchip_add_msi_route failed");
>>>> +        return;
>>>>      }
>>>>
>>>>      s->msi_vectors[vector].virq = ret;
>>>>      s->msi_vectors[vector].pdev = pdev;
>>>> -
>>>> -    return 0;
>>>>  }
>>>>
>>>> -static void setup_interrupt(IVShmemState *s, int vector)
>>>> +static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
>>>>  {
>>>>      EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
>>>>      bool with_irqfd = kvm_msi_via_irqfd_enabled() &&
>>>>          ivshmem_has_feature(s, IVSHMEM_MSI);
>>>>      PCIDevice *pdev = PCI_DEVICE(s);
>>>> +    Error *err = NULL;
>>>>
>>>>      IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector);
>>>>
>>>> @@ -546,13 +541,16 @@ static void setup_interrupt(IVShmemState *s, int 
>>>> vector)
>>>>          watch_vector_notifier(s, n, vector);
>>>>      } else if (msix_enabled(pdev)) {
>>>>          IVSHMEM_DPRINTF("with irqfd\n");
>>>> -        if (ivshmem_add_kvm_msi_virq(s, vector) < 0) {
>>>> +        ivshmem_add_kvm_msi_virq(s, vector, &err);
>>>> +        if (err) {
>>>> +            error_propagate(errp, err);
>>>>              return;
>>>
>>> That would make this simpler, avoiding local err variables, and
>>> propagate. But you seem to prefer that form. I don't know if there is
>>> any conventions (I am used to glib conventions, and usually a bool
>>> success is returned, even if the function takes a GError)
>>
>> Does GLib spell out this convention somewhere?
>
> For glib, there is a paragraph about return bool/GError conventions
> (which is usually adapted to other return type):
> https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html

While I can't see a hard-and-fast rule there, the text clearly shows a
strong preference for making the function value a reliable error
indicator whenever possible.

Thanks!

>>
>> I can perhaps try to cook up a patch to demonstrate the advantages of
>> returning a success/failure value even with Error **, and try to get our
>> convention changed.
>>
>> Until then, we better stick to the existing convention, whether we like
>> it or not.
>
> ok