Marc-André Lureau <marcandre.lur...@gmail.com> writes: > Hi > > On Wed, Mar 2, 2016 at 8:35 PM, Markus Armbruster <arm...@redhat.com> 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