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

[Marc-André Lureau]

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

>
> 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




-- 
Marc-André Lureau