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: Paolo Bonzini [mailto:address@hidden On Behalf Of Paolo
>> Bonzini
>> Sent: Wednesday, November 05, 2014 11:55 PM
>> To: Michael S. Tsirkin
>> Cc: Markus Armbruster; SeokYeon Hwang; address@hidden
>> Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling
>> between pci_qdev_init() and qdev
>> 
>> 
>> 
>> On 05/11/2014 14:28, Michael S. Tsirkin wrote:
>> > > I think bypassing the question by converting to realize makes the
>> > > most sense...
>> >
>> > I'm fine with doing that but Markus's patches wouldn't yet have solved
>> > the problem by themselves since init is still around, right?
>> >
>> > This probably means fixing this bug can't justify merging the realize
>> > patchset after freeze.
>> 
>> Yes, I agree.  I meant that the API is not very well defined.  I would
>> handle everything else on a case-by-case basis, by reviewing each init
>> function that is converted to realize.
>> 
>> Since the patch was for an out-of-tree device, it can wait for 2.3 anyway.
>> 
>> Paolo
>
> I cannot fully understand your conversation.

You appear to have a PCIDeviceClass method init() returning a positive
value.  Doesn't work.  Only values <= 0 do.

Your proposed fix is to make its caller treat a positive value like a
negative one.

Paolo points out that init()'s contract is unclear.  His preferred way
of clarifying it is to convert PCI from init() to realize(), which has a
sufficiently clear contract.

Doesn't help you now.  My "pci: Partial conversion to realize" series,
will help you once it lands, but only if you convert your device.

You obviously want a solution earlier.  The one you proposed implicitly
clarifies the PCIDeviceClass init() contract to "zero means success,
anything else failure".  I don't think that's a good idea, because it
makes PCIDeviceClass's init() differ from DeviceClass's.  There,
non-negative value means success, negative means failure (see
device_realize()).

Fix your device not to return positive values instead.

You could additionally fix pci_qdev_init() to treat positive numbers as
success, for consistency with device_realize(), but that requires
auditing all existing PCIDeviceClass init() methods.  Waste of your
time, because they all go away when we convert to realize().

> But, I think this patch is still worth before all 'init()' convert to
> 'realize()'.
> Moreover, It has no side effect at all.

I don't like it, because it makes PCIDeviceClass's init() inconsistent
with DeviceClass's.