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

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> This kills off the funny state described in the previous commit.
>>
>> Simplify ivshmem_io_read() accordingly, and update documentation.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/specs/ivshmem-spec.txt |  20 ++++----
>>  hw/misc/ivshmem.c           | 121 +++++++++++++++++++++++++++-----------------
>>  qemu-doc.texi               |   9 +---
>>  3 files changed, 87 insertions(+), 63 deletions(-)
>>
>> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
>> index 4fc6f37..3eb8c97 100644
>> --- a/docs/specs/ivshmem-spec.txt
>> +++ b/docs/specs/ivshmem-spec.txt
>> @@ -62,11 +62,11 @@ There are two ways to use this device:
>>    likely want to write a kernel driver to handle interrupts.  Requires
>>    the device to be configured for interrupts, obviously.
>>
>> -If the device is configured for interrupts, BAR2 is initially invalid.
>> -It becomes safely accessible only after the ivshmem server provided
>> -the shared memory.  Guest software should wait for the IVPosition
>> -register (described below) to become non-negative before accessing
>> -BAR2.
>> +Before QEMU 2.6.0, BAR2 can initially be invalid if the device is
>> +configured for interrupts.  It becomes safely accessible only after
>> +the ivshmem server provided the shared memory.  Guest software should
>> +wait for the IVPosition register (described below) to become
>> +non-negative before accessing BAR2.
>>
>>  The device is not capable to tell guest software whether it is
>>  configured for interrupts.
>> @@ -82,7 +82,7 @@ BAR 0 contains the following registers:
>>          4     4   read/write        0   Interrupt Status
>>                                          bit 0: peer interrupt
>>                                          bit 1..31: reserved
>> -        8     4   read-only   0 or -1   IVPosition
>> +        8     4   read-only   0 or ID   IVPosition
>>         12     4   write-only      N/A   Doorbell
>>                                          bit 0..15: vector
>>                                          bit 16..31: peer ID
>> @@ -100,12 +100,14 @@ when an interrupt request from a peer is received.  Reading the
>>  register clears it.
>>
>>  IVPosition Register: if the device is not configured for interrupts,
>> -this is zero.  Else, it's -1 for a short while after reset, then
>> -changes to the device's ID (between 0 and 65535).
>> +this is zero.  Else, it is the device's ID (between 0 and 65535).
>> +
>> +Before QEMU 2.6.0, the register may read -1 for a short while after
>> +reset.
>>
>>  There is no good way for software to find out whether the device is
>>  configured for interrupts.  A positive IVPosition means interrupts,
>> -but zero could be either.  The initial -1 cannot be reliably observed.
>> +but zero could be either.
>>
>>  Doorbell Register: writing this register requests to interrupt a peer.
>>  The written value's high 16 bits are the ID of the peer to interrupt,
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 352937f..831da53 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -234,12 +234,7 @@ static uint64_t ivshmem_io_read(void *opaque, hwaddr addr,
>>              break;
>>
>>          case IVPOSITION:
>> -            /* return my VM ID if the memory is mapped */
>> -            if (memory_region_is_mapped(&s->ivshmem)) {
>> -                ret = s->vm_id;
>> -            } else {
>> -                ret = -1;
>> -            }
>> +            ret = s->vm_id;
>>              break;
>>
>>          default:
>> @@ -511,7 +506,8 @@ static bool fifo_update_and_get_i64(IVShmemState *s,
>>      return false;
>>  }
>>
>> -static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector)
>> +static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector,
>> +                                     Error **errp)
>
> I prefer to return -1 in case of error, even if Error** is also returned.

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.

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

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.

Thanks!

[...]