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