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

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

[SeokYeon Hwang]

pci_qdev_init() checks whether return value is 0 or not to figure out pci device is initialized successfully. Otherwise, device_realize() in qdev checks that return value is negative value to figure out the device is realized successfully.
When pci device returns positive number, pci_qdev_init() thinks that error is occured and makes the device unregistered. Nevertheless, qdev thinks that device is realized.
Finally, crash is occured by commands like 'qtree' that traverse qdev list.

So, pci_qdev_init() returns -1 when init function returns not 0.

Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
---
 hw/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 371699c..c149fdf 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1766,7 +1766,7 @@ static int pci_qdev_init(DeviceState *qdev)
         rc = pc->init(pci_dev);
         if (rc != 0) {
             do_pci_unregister_device(pci_dev);
-            return rc;
+            return -1;
         }
     }
 
-- 
2.1.0

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

[Paolo Bonzini]

On 05/11/2014 11:11, SeokYeon Hwang wrote:
> pci_qdev_init() checks whether return value is 0 or not to figure out pci device is initialized successfully. Otherwise, device_realize() in qdev checks that return value is negative value to figure out the device is realized successfully.
> When pci device returns positive number, pci_qdev_init() thinks that error is occured and makes the device unregistered. Nevertheless, qdev thinks that device is realized.
> Finally, crash is occured by commands like 'qtree' that traverse qdev list.
> 
> So, pci_qdev_init() returns -1 when init function returns not 0.
> 
> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 371699c..c149fdf 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1766,7 +1766,7 @@ static int pci_qdev_init(DeviceState *qdev)
>          rc = pc->init(pci_dev);
>          if (rc != 0) {
>              do_pci_unregister_device(pci_dev);
> -            return rc;
> +            return -1;
>          }
>      }
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Michael S. Tsirkin]

On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote:
> pci_qdev_init() checks whether return value is 0 or not to figure out pci device is initialized successfully. Otherwise, device_realize() in qdev checks that return value is negative value to figure out the device is realized successfully.
> When pci device returns positive number, pci_qdev_init() thinks that error is occured and makes the device unregistered. Nevertheless, qdev thinks that device is realized.
> Finally, crash is occured by commands like 'qtree' that traverse qdev list.
> 
> So, pci_qdev_init() returns -1 when init function returns not 0.
> 
> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>

Question: is there a simple way to trigger this error?

> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 371699c..c149fdf 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1766,7 +1766,7 @@ static int pci_qdev_init(DeviceState *qdev)
>          rc = pc->init(pci_dev);
>          if (rc != 0) {
>              do_pci_unregister_device(pci_dev);
> -            return rc;
> +            return -1;
>          }
>      }
>  
> -- 
> 2.1.0

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote:
>> pci_qdev_init() checks whether return value is 0 or not to figure
>> out pci device is initialized successfully. Otherwise,
>> device_realize() in qdev checks that return value is negative value
>> to figure out the device is realized successfully.
>> When pci device returns positive number, pci_qdev_init() thinks that
>> error is occured and makes the device unregistered. Nevertheless,
>> qdev thinks that device is realized.
>> Finally, crash is occured by commands like 'qtree' that traverse qdev list.
>>
>> So, pci_qdev_init() returns -1 when init function returns not 0.
>> 
>> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
>
> Question: is there a simple way to trigger this error?

Next question: what's the contract of PCIDeviceClass method init()?
Positive return value feels like bug to me...

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

[Paolo Bonzini]

On 05/11/2014 14:16, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
>> On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote:
>>> pci_qdev_init() checks whether return value is 0 or not to figure
>>> out pci device is initialized successfully. Otherwise,
>>> device_realize() in qdev checks that return value is negative value
>>> to figure out the device is realized successfully.
>>> When pci device returns positive number, pci_qdev_init() thinks that
>>> error is occured and makes the device unregistered. Nevertheless,
>>> qdev thinks that device is realized.
>>> Finally, crash is occured by commands like 'qtree' that traverse qdev list.
>>>
>>> So, pci_qdev_init() returns -1 when init function returns not 0.
>>>
>>> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
>>
>> Question: is there a simple way to trigger this error?
> 
> Next question: what's the contract of PCIDeviceClass method init()?
> Positive return value feels like bug to me...

I think bypassing the question by converting to realize makes the most
sense...

Paolo

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

[Michael S. Tsirkin]

On Wed, Nov 05, 2014 at 02:18:31PM +0100, Paolo Bonzini wrote:
> 
> 
> On 05/11/2014 14:16, Markus Armbruster wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > 
> >> On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote:
> >>> pci_qdev_init() checks whether return value is 0 or not to figure
> >>> out pci device is initialized successfully. Otherwise,
> >>> device_realize() in qdev checks that return value is negative value
> >>> to figure out the device is realized successfully.
> >>> When pci device returns positive number, pci_qdev_init() thinks that
> >>> error is occured and makes the device unregistered. Nevertheless,
> >>> qdev thinks that device is realized.
> >>> Finally, crash is occured by commands like 'qtree' that traverse qdev list.
> >>>
> >>> So, pci_qdev_init() returns -1 when init function returns not 0.
> >>>
> >>> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
> >>
> >> Question: is there a simple way to trigger this error?
> > 
> > Next question: what's the contract of PCIDeviceClass method init()?
> > Positive return value feels like bug to me...
> 
> I think bypassing the question by converting to realize makes the most
> sense...
> 
> Paolo

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.

-- 
MST

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

[Paolo Bonzini]

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

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

[SeokYeon Hwang]

> -----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.

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

[Markus Armbruster]

SeokYeon Hwang <syeon.hwang@samsung.com> writes:

>> -----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.

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.

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 <syeon.hwang@samsung.com> writes:
> 
> >> -----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.
> 
> 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

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

[SeokYeon Hwang]

> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Thursday, November 06, 2014 6:21 PM
> To: SeokYeon Hwang
> Cc: 'Paolo Bonzini'; 'Michael S. Tsirkin'; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling
> between pci_qdev_init() and qdev
> 
> SeokYeon Hwang <syeon.hwang@samsung.com> writes:
> 
> >> -----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.
> 
> 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 understand completely.
Thank you for your kind explanation.

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 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

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

[SeokYeon Hwang]

> -----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

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

[Markus Armbruster]

SeokYeon Hwang <syeon.hwang@samsung.com> 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.

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

[SeokYeon Hwang]

> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Friday, November 07, 2014 4:45 PM
> To: SeokYeon Hwang
> Cc: 'Michael S. Tsirkin'; 'Paolo Bonzini'; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling
> between pci_qdev_init() and qdev
> 
> SeokYeon Hwang <syeon.hwang@samsung.com> 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.

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

[Markus Armbruster]

SeokYeon Hwang <syeon.hwang@samsung.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster [mailto:armbru@redhat.com]
>> Sent: Friday, November 07, 2014 4:45 PM
>> To: SeokYeon Hwang
>> Cc: 'Michael S. Tsirkin'; 'Paolo Bonzini'; qemu-devel@nongnu.org
>> Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling
>> between pci_qdev_init() and qdev
>> 
>> SeokYeon Hwang <syeon.hwang@samsung.com> 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?

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

[SeokYeon Hwang]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Wednesday, November 05, 2014 9:46 PM
> To: SeokYeon Hwang
> Cc: qemu-devel@nongnu.org; armbru@redhat.com; pbonzini@redhat.com
> Subject: Re: [PATCH] pci: fixed mismatch of error-handling between
> pci_qdev_init() and qdev
> 
> On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote:
> > pci_qdev_init() checks whether return value is 0 or not to figure out
> pci device is initialized successfully. Otherwise, device_realize() in
> qdev checks that return value is negative value to figure out the device
> is realized successfully.
> > When pci device returns positive number, pci_qdev_init() thinks that
> error is occured and makes the device unregistered. Nevertheless, qdev
> thinks that device is realized.
> > Finally, crash is occured by commands like 'qtree' that traverse qdev
> list.
> >
> > So, pci_qdev_init() returns -1 when init function returns not 0.
> >
> > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
> 
> Question: is there a simple way to trigger this error?

You can reproduce this error by changing the return value of the unimportant
device's init() to 1.
Actually, I found this bug through the device that is not exist in upstream
qemu.
(It is Tizen emulator's device.)

> 
> > ---
> >  hw/pci/pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 371699c..c149fdf 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1766,7 +1766,7 @@ static int pci_qdev_init(DeviceState *qdev)
> >          rc = pc->init(pci_dev);
> >          if (rc != 0) {
> >              do_pci_unregister_device(pci_dev);
> > -            return rc;
> > +            return -1;
> >          }
> >      }
> >
> > --
> > 2.1.0