Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del

[Michael Tokarev]

On 08.08.2012 16:39, Paolo Bonzini wrote:
> Il 08/08/2012 14:22, Michael Tokarev ha scritto:
>> @@ -152,6 +152,16 @@ int qdev_init(DeviceState *dev)
>> +
>> +    if (!OBJECT(dev)->parent) {
>> +        static int unattached_count = 0;
>> +        gchar *name = g_strdup_printf("device[%d]", unattached_count++);
>> +
>> +        object_property_add_child(container_get("/unattached"), name,
>> +                                  OBJECT(dev), NULL);
>> +        g_free(name);
>> +    }
>>
>> ie, there's now extra call to object_property_add_child(),
>> which does object_ref().  So no doubt there's no a new
>> assertion failure: (obj->ref == 0).
>>
>> Once again, patch description does not reflect what is
>> actually done by the patch.
> 
> Huh? The add_child ensures that there is a canonical path.
> 
>> Can we please stop this
>> practice of accepting patches with desrciption not
>> reflecting reality?

I must clarify this.

I'm not trying to blame Paolo for the wrong patch which
"broke" things (it exposed an old bug in other codepath).
I'm just saying that the patch description appears to be
"too innocent" -- yes, now each device has a path, or,
in other words, a NAME, but this patch ALSO changes
refcounting, and THIS is what I'm referring to above --
it isn't only gives a name, but also links "unowned"
objects to a bus.  To me it is a wrong (too innocent)
description.  I browsed comits trying to find which
change might have caused it, and provided ther was
a mention of "references" or "connecting" in this patch
description somewhere, I'd found this change much faster,
without a painful (*) bisection.

Maybe it is just me who does not know this code area
well enough, so things aren't as obvious for me as for
others, I dunno, but to me the description -- the only
thing I'm trying to "blame" Paolo for here -- might be
quite a bit better.

(*) painful because this bisection come in the process
of another bisection dealing with another usb issue,
which come to yet another bug in usb handling somewhere
(giving another assertion).

>> Back to the point: should there be a call to object_unref()
>> somewhere?
> 
> There should be a call to object_unparent() somewhere actually.
> We've been peppering the code with them for a few months now while
> waiting for "the" solution, but I fail to see what the solution
> could be other than the patch below (something similar has probably
> been proposed already).  BTW there are probably a lot of other similar
> bugs somewhere.

This sounds ecouraging -- "alot of other similar bugs".. :(

Something similar should be applied to 1.1-stable.  FWIW, some
changes are not needed there, like this:
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -159,7 +159,6 @@ int qdev_init(DeviceState *dev)
>  
>      rc = dc->init(dev);
>      if (rc < 0) {
> -        object_unparent(OBJECT(dev));
>          qdev_free(dev);
>          return rc;
>      }

and this:

> --- a/hw/shpc.c
> +++ b/hw/shpc.c
> @@ -253,7 +253,6 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, 
> int slot)
>           ++devfn) {
>          PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
>          if (affected_dev) {
> -            object_unparent(OBJECT(affected_dev));
>              qdev_free(&affected_dev->qdev);
>          }
>      }

the lines being removed does not exist in 1.1-stable.

I can guess this is due to previous attempts to fix
similar issues in other places.

Thank you!

/mjt