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

[SeokYeon Hwang <syeon.hwang@samsung.com>]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Thursday, November 06, 2014 6:24 PM
> To: SeokYeon Hwang
> Cc: 'Paolo Bonzini'; 'Markus Armbruster'; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling
> between pci_qdev_init() and qdev
> 
> On Thu, Nov 06, 2014 at 11:26:01AM +0900, SeokYeon Hwang wrote:
> >
> >
> > > -----Original Message-----
> > > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of
> > > Paolo Bonzini
> > > Sent: Wednesday, November 05, 2014 11:55 PM
> > > To: Michael S. Tsirkin
> > > Cc: Markus Armbruster; SeokYeon Hwang; qemu-devel@nongnu.org
> > > 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.
> > But, I think this patch is still worth before all 'init()' convert to
> > 'realize()'.
> > Moreover, It has no side effect at all.
> >
> > Thanks.
> >
> 
> The root cause is API misuse: functions that return int should return a
> negative code on failure, either 0 or >= 0 on success.
> In rare cases, we use int as bool, so 0 on failure, 1 on success.
> 
> Your device returned 1 on failure, this broke things.
> So don't do this then :)
> 
> The question would be: are there existing devices that return a positive
> return code on init. If there are, it's a bug, but the best fix might be
> your patch - easier that fixing many devices.
> 
> If there aren't, the patch isn't needed.
> 
> --
> MST

You are right.
Yes, it is API misuse. So we had fixed the device that return positive value
on failure.

But is that all right ?? I don't think so, because this logic is obviously
wrong.
'pci_qdev_init()' should return -1 (not rc) on failure or should check 'rc <
0' not 'rc != 0'. That is what I wanted to fix.

But if 2.2 comes with all "realized" devices, if there is no "init" devices,
then this patch isn't needed.

Thanks