Re: [Qemu-devel] [PATCH 1/3] virtio-ccw: remove qdev_unparent in unplug routing

[Cornelia Huck]

On Mon, 11 Mar 2013 13:22:45 +0100
Alexander Graf <address@hidden> wrote:

> On 03/11/2013 01:16 PM, Paolo Bonzini wrote:
> > Il 11/03/2013 13:04, Cornelia Huck ha scritto:
> >> On Fri, 8 Mar 2013 21:11:13 +0100
> >> Alexander Graf<address@hidden>  wrote:
> >>
> >>> On 25.02.2013, at 12:10, Christian Borntraeger wrote:
> >>>
> >>>> On 25/02/13 11:44, Paolo Bonzini wrote:
> >>>>> Il 25/02/2013 09:09, Christian Borntraeger ha scritto:
> >>>>>> Hmm, the old sequence was
> >>>>>>
> >>>>>>      object_unparent(OBJECT(dev));
> >>>>>>      qdev_free(dev) ---+
> >>>>>>                        |
> >>>>>>                        V
> >>>>>> ...
> >>>>>>             object_unparent(OBJECT(dev));  now the last reference is 
> >>>>>> gone, object is freed
> >>>>>>             object_unref(OBJECT(dev));     now the reference of a 
> >>>>>> deleted object becomes -1
> >>>>>> ...
> >>>>>>
> >>>>>> Isnt that a problem in itself that we modify a reference counter in an 
> >>>>>> deleted object?
> >>>>> The second object_unparent should do nothing.  So before you had:
> >>>>>
> >>>>>       object_unparent(OBJECT(dev));         leaves refcount=1
> >>>>>       qdev_free(dev) ---+
> >>>>>                         |
> >>>>>                         V
> >>>>>              object_unparent(OBJECT(dev));  do nothing
> >>>>>              object_unref(OBJECT(dev));     refcount=0, object freed
> >>>>>
> >>>>> After the object_unref was removed you had:
> >>>>>
> >>>>>       object_unparent(OBJECT(dev));         refcount=0, object freed
> >>>>>       qdev_free(dev) ---+
> >>>>>                         |
> >>>>>                         V
> >>>>>              object_unparent(OBJECT(dev));  dangling pointer!
> >>>>>
> >>>>
> >>>> Got it. Thanks
> >>> So is the patch valid?
> >> To my understanding, yes.
> > Yes, except that the "fixed a crash" part in the commit message is
> > probably no longer accurate.  No big deal. :)
> 
> Ok, Connie could you please include it in your next pull then please?

Sure, will do.

> 
> 
> Alex
>