Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling between pci_qdev_init() and qdev

[Markus Armbruster]

SeokYeon Hwang <address@hidden> writes:

>> -----Original Message-----
>> From: Markus Armbruster [mailto:address@hidden
>> Sent: Friday, November 07, 2014 4:45 PM
>> To: SeokYeon Hwang
>> Cc: 'Michael S. Tsirkin'; 'Paolo Bonzini'; address@hidden
>> Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling
>> between pci_qdev_init() and qdev
>> 
>> SeokYeon Hwang <address@hidden> writes:
>> 
>> [...]
>> > But if 2.2 comes with all "realized" devices, if there is no "init"
>> > devices, then this patch isn't needed.
>> 
>> No PCI devices will be converted to realize in 2.2.  We'll start the
>> conversion early in the 2.3 development cycle.  Whether we can finish it
>> in the same cycle is uncertain.
>
> Then...
> I think 'pci_qdev_init()' should be fixed because it contains obviously
> wrong logic.
> I didn't understand why this problem should be ignored now for reasons of it
> will be removed in the far future.
> (or because all devices that use this function didn't return positive value
> as error now.)
>
> It will be corrected by
> 1. return '-1' instead of 'rc'.
> 2. check 'rc < 0' instead of 'rc != 0'. (if we can guarantee no pci device
> returns positive value as error.)
>
> What do you think about it ??
>
> If I didn't understand the context of this thread, please let me know.
>
> Thank you very much.

I think (2) best matches how we use init() methods elsewhere,
e.g. in device_realize(), hda_codec_dev_init(), sysbus_device_init(),
i2c_slave_qdev_init(), smbus_device_init(), ide_qdev_init(), ...

Changes the meaning of positive return values from PCIDeviceClass init()
from failure to success.  Could theoretically break some devices and fix
others.  To make sure it doesn't break any, you'd have to audit them
all.  I feel that time would be better spent on conversions to
realize().

Worries about such breakage could be a reason to do (1) instead.  Needs
a suitable comment.

Michael, got a preference?