Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Paolo Bonzini]

On 18/02/2016 10:56, Markus Armbruster wrote:
> Alistair Francis <address@hidden> writes:
> 
>> If the device being added when running qdev_device_add() has
>> a reset function, register it so that it can be called.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>>
>>  qdev-monitor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 81e3ff3..0a99d01 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> **errp)
>>  
>>      if (bus) {
>>          qdev_set_parent_bus(dev, bus);
>> +    } else if (dc->reset) {
>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>      }
>>  
>>      id = qemu_opts_id(opts);
> 
> This looks wrong to me.
> 
> You stuff all the device reset methods into the global reset_handlers
> list, where they get called in some semi-random order.  This breaks when
> there are reset order dependencies between devices, e.g. between a
> device and the bus it plugs into.

There is no bus here, see the "if" above the one that's being added.

However, what devices have done so far is to register/unregister the
reset in the realize/unrealize methods, and I suggest doing the same.

If you really want to add the magic qemu_register_reset, you should at
least do one of the following:

* add a matching unregister (no idea where)

* assert that the device is not hot-unpluggable, otherwise hot-unplug
would leave a dangling pointer.

Paolo

> Propagating the reset signal to all the devices is a qdev problem.
> Copying Andreas for further insight.