Re: [Qemu-devel] [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

2012-07-18 Thread Jan Kiszka 
On 2012-07-18 22:42, Ma, Stephen B. wrote:
> Sorry for taking so long to reply.  I am new to this.  Should this patch be 
> committed or just dropped

This bug was fixed by 266ca11a0433643a3cc3146a9837d9f2b0bfbe3b in the
meantime.

Jan

> 
> 
> -Original Message-
> From: Jan Kiszka [mailto:jan.kis...@web.de] 
> Sent: Sunday, June 17, 2012 11:25 PM
> To: Anthony Liguori
> Cc: Michael S. Tsirkin; 'qemu-devel@nongnu.org'; Ma, Stephen B.
> Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI 
> passthru device.
> 
> On 2012-06-17 16:28, Anthony Liguori wrote:
>> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>>> On Sun, Jun 17, 2012 at 06:26:33AM +, Ma, Stephen B. wrote:

 Michael,

 Thanks for the review.  I added the unparent to the qdev_free.


 ---
   hw/qdev.c |1 +
   1 files changed, 1 insertions(+), 0 deletions(-)

 diff --git a/hw/qdev.c b/hw/qdev.c
 index d2dc28b..ed1328d 100644
 --- a/hw/qdev.c
 +++ b/hw/qdev.c
 @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
   /* Unlink device from bus and free the structure.  */
   void qdev_free(DeviceState *dev)
   {
 +object_unparent(OBJECT(dev));
   object_delete(OBJECT(dev));
   }

 --
 1.7.1
>>>
>>> Anthony, any feedback?
>>
>> Yes, this is wrong.
>>
>> PCI passthrough isn't in qemu.git so it's not clear to me where this 
>> is happening.  Why would qdev_free be called when adding a PCI 
>> passthru device?
> 
> The bug is reproducible with any in-tree device (at least PCI) that happens 
> to return != 0 from its init handler.
> 
> Jan
> 
> 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

2012-07-18 Thread Ma, Stephen B. 
Sorry for taking so long to reply.  I am new to this.  Should this patch be 
committed or just dropped


-Original Message-
From: Jan Kiszka [mailto:jan.kis...@web.de] 
Sent: Sunday, June 17, 2012 11:25 PM
To: Anthony Liguori
Cc: Michael S. Tsirkin; 'qemu-devel@nongnu.org'; Ma, Stephen B.
Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI 
passthru device.

On 2012-06-17 16:28, Anthony Liguori wrote:
> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>> On Sun, Jun 17, 2012 at 06:26:33AM +, Ma, Stephen B. wrote:
>>>
>>> Michael,
>>>
>>> Thanks for the review.  I added the unparent to the qdev_free.
>>>
>>>
>>> ---
>>>   hw/qdev.c |1 +
>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index d2dc28b..ed1328d 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>>   /* Unlink device from bus and free the structure.  */
>>>   void qdev_free(DeviceState *dev)
>>>   {
>>> +object_unparent(OBJECT(dev));
>>>   object_delete(OBJECT(dev));
>>>   }
>>>
>>> --
>>> 1.7.1
>>
>> Anthony, any feedback?
> 
> Yes, this is wrong.
> 
> PCI passthrough isn't in qemu.git so it's not clear to me where this 
> is happening.  Why would qdev_free be called when adding a PCI 
> passthru device?

The bug is reproducible with any in-tree device (at least PCI) that happens to 
return != 0 from its init handler.

Jan

Re: [Qemu-devel] [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

2012-06-17 Thread Jan Kiszka 
On 2012-06-17 16:28, Anthony Liguori wrote:
> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>> On Sun, Jun 17, 2012 at 06:26:33AM +, Ma, Stephen B. wrote:
>>>
>>> Michael,
>>>
>>> Thanks for the review.  I added the unparent to the qdev_free.
>>>
>>>
>>> ---
>>>   hw/qdev.c |1 +
>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index d2dc28b..ed1328d 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>>   /* Unlink device from bus and free the structure.  */
>>>   void qdev_free(DeviceState *dev)
>>>   {
>>> +object_unparent(OBJECT(dev));
>>>   object_delete(OBJECT(dev));
>>>   }
>>>
>>> -- 
>>> 1.7.1
>>
>> Anthony, any feedback?
> 
> Yes, this is wrong.
> 
> PCI passthrough isn't in qemu.git so it's not clear to me where this is
> happening.  Why would qdev_free be called when adding a PCI passthru
> device?

The bug is reproducible with any in-tree device (at least PCI) that
happens to return != 0 from its init handler.

Jan




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

2012-06-17 Thread Anthony Liguori 

On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:

On Sun, Jun 17, 2012 at 06:26:33AM +, Ma, Stephen B. wrote:


Michael,

Thanks for the review.  I added the unparent to the qdev_free.


---
  hw/qdev.c |1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index d2dc28b..ed1328d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
  /* Unlink device from bus and free the structure.  */
  void qdev_free(DeviceState *dev)
  {
+object_unparent(OBJECT(dev));
  object_delete(OBJECT(dev));
  }

--
1.7.1


Anthony, any feedback?


Yes, this is wrong.

PCI passthrough isn't in qemu.git so it's not clear to me where this is 
happening.  Why would qdev_free be called when adding a PCI passthru device?


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

2012-06-17 Thread Michael S. Tsirkin 
On Sun, Jun 17, 2012 at 06:26:33AM +, Ma, Stephen B. wrote:
> 
> Michael,
> 
> Thanks for the review.  I added the unparent to the qdev_free.
> 
> 
> ---
>  hw/qdev.c |1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index d2dc28b..ed1328d 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>  /* Unlink device from bus and free the structure.  */
>  void qdev_free(DeviceState *dev)
>  {
> +object_unparent(OBJECT(dev));
>  object_delete(OBJECT(dev));
>  }
> 
> --
> 1.7.1

Anthony, any feedback?

-- 
MST

Re: [Qemu-devel] [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

2012-06-16 Thread Ma, Stephen B. 

Michael,

Thanks for the review.  I added the unparent to the qdev_free.


---
 hw/qdev.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index d2dc28b..ed1328d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
+object_unparent(OBJECT(dev));
 object_delete(OBJECT(dev));
 }

--
1.7.1

-Original Message-
From: Michael S. Tsirkin [mailto:m...@redhat.com] 
Sent: Tuesday, June 12, 2012 1:27 AM
To: Ma, Stephen B.
Cc: 'qemu-devel@nongnu.org'
Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI 
passthru device.

On Tue, Jun 12, 2012 at 04:31:20AM +, Ma, Stephen B. wrote:
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6a8f6bd..d2dc28b 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -139,7 +139,7 @@ DeviceState *qdev_try_create(BusState *bus, const char 
> *type)
>  /* Initialize a device.  Device properties should be set before calling
> this function.  IRQs and MMIO regions should be connected/mapped after
> calling this function.
> -   On failure, destroy the device and return negative value.
> +   On failure, return a negative value.
> Return 0 on success.  */
>  int qdev_init(DeviceState *dev)
>  {


Yes, I agree. qdev_init did now allocate the device so
it should not free it.


> @@ -150,7 +150,6 @@ int qdev_init(DeviceState *dev)
>  
>  rc = dc->init(dev);
>  if (rc < 0) {
> -qdev_free(dev);
>  return rc;
>  }
>

Another thing we need to fix is unparent the device in
qdev_free.

-- 
MST

Re: [Qemu-devel] [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

2012-06-12 Thread Michael S. Tsirkin 
On Tue, Jun 12, 2012 at 04:31:20AM +, Ma, Stephen B. wrote:
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6a8f6bd..d2dc28b 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -139,7 +139,7 @@ DeviceState *qdev_try_create(BusState *bus, const char 
> *type)
>  /* Initialize a device.  Device properties should be set before calling
> this function.  IRQs and MMIO regions should be connected/mapped after
> calling this function.
> -   On failure, destroy the device and return negative value.
> +   On failure, return a negative value.
> Return 0 on success.  */
>  int qdev_init(DeviceState *dev)
>  {


Yes, I agree. qdev_init did now allocate the device so
it should not free it.


> @@ -150,7 +150,6 @@ int qdev_init(DeviceState *dev)
>  
>  rc = dc->init(dev);
>  if (rc < 0) {
> -qdev_free(dev);
>  return rc;
>  }
>

Another thing we need to fix is unparent the device in
qdev_free.

-- 
MST