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 <ler...@redhat.com>
>> ---
>> 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 <ebl...@redhat.com>
Thanks!
Laszlo
