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

[Michael S. Tsirkin]

On Thu, Nov 06, 2014 at 10:20:32AM +0100, Markus Armbruster wrote:
> 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.

I agree with Markus here. A positive return value should not indicate an
error.

-- 
MST