Re: [Qemu-devel] [PATCH 15/16] pci-assign: propagate errors from assign_intx()

[Laszlo Ersek]

On 04/30/14 00:41, Eric Blake wrote:
> On 04/10/2014 02:24 AM, Laszlo Ersek wrote:
>> Among the callers, only assigned_initfn() should set the  monitor's stored
> 
> unintentional double space

Yep, my mistake in paragraph refilling.

> 
>> error. Other callers may run in contexts where the monitor's stored error
>> makes no sense. For example:
>>
>> assigned_dev_pci_write_config()
>>   assigned_dev_update_msix()
>>     assign_intx()
>>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  hw/i386/kvm/pci-assign.c | 39 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>
> 
>> @@ -954,12 +954,15 @@ static void assigned_dev_update_irq_routing(PCIDevice 
>> *dev)
>>  {
>>      AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, dev);
>>      Error *err = NULL;
>>      int r;
>>  
>> -    r = assign_intx(assigned_dev);
>> +    r = assign_intx(assigned_dev, &err);
>>      if (r < 0) {
>> +        error_report("%s", error_get_pretty(err));
>> +        error_free(err);
>> +        err = NULL;
> 
> Rather than assigning err = NULL,
> 
>>          qdev_unplug(&dev->qdev, &err);
>>          assert(!err);
> 
> why not also replace these two lines with the one-liner
>  qdev_unplug(&dev->qdev, &error_abort);

I did learn about "error_abort" while working on the series (hadn't
known about it before), but I didn't "internalize" it enough to do what
you propose :)

> 
> I'm also a bit surprised this used 'err' instead of 'local_err'; but it
> was pre-existing; I'm also not sure if the recent error cleanup patches
> from Markus will impact this code.

We'll see :)

> 
> Reviewed-by: Eric Blake <address@hidden>

Thanks!
Laszlo