[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 24/38] ivshmem: Propagate errors through ivshmem
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 24/38] ivshmem: Propagate errors through ivshmem_recv_setup() |
Date: |
Thu, 03 Mar 2016 08:16:57 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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