Re: [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus

[Philippe Mathieu-Daudé]

On 7/31/19 11:29 AM, Damien Hedde wrote:
> On 7/31/19 8:05 AM, David Gibson wrote:
>> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
>>> Deprecate old reset apis and make them use the new one while they
>>> are still used somewhere.
>>>
>>> Signed-off-by: Damien Hedde <address@hidden>
>>> ---
>>>  hw/core/qdev.c         | 22 +++-------------------
>>>  include/hw/qdev-core.h | 28 ++++++++++++++++++++++------
>>>  2 files changed, 25 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 559ced070d..e9e5f2d5f9 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, 
>>> void (*func)(Object *))
>>>      }
>>>  }
>>>  
>>> -static int qdev_reset_one(DeviceState *dev, void *opaque)
>>> -{
>>> -    device_legacy_reset(dev);
>>> -
>>> -    return 0;
>>> -}
>>> -
>>> -static int qbus_reset_one(BusState *bus, void *opaque)
>>> -{
>>> -    BusClass *bc = BUS_GET_CLASS(bus);
>>> -    if (bc->reset) {
>>> -        bc->reset(bus);
>>> -    }
>>> -    return 0;
>>> -}
>>> -
>>>  void qdev_reset_all(DeviceState *dev)
>>>  {
>>> -    qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, 
>>> NULL);
>>> +    device_reset(dev, false);
>>>  }
>>>  
>>>  void qdev_reset_all_fn(void *opaque)
>>> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
>>>  
>>>  void qbus_reset_all(BusState *bus)
>>>  {
>>> -    qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, 
>>> NULL);
>>> +    bus_reset(bus, false);
>>>  }
>>>  
>>>  void qbus_reset_all_fn(void *opaque)
>>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool 
>>> value, Error **errp)
>>>              }
>>>          }
>>>          if (dev->hotplugged) {
>>> -            device_legacy_reset(dev);
>>> +            device_reset(dev, true);
>>
>> So.. is this change in the device_reset() signature really necessary?
>> Even if there are compelling reasons to handle warm reset in the new
>> API, that doesn't been you need to change device_reset() itself from
>> its established meaning of a cold (i.e. as per power cycle) reset.
>> Warm resets are generally called in rather more specific circumstances
>> (often under guest software direction) so it seems likely that users
>> would want to engage with the new reset API directly.  Or we could
>> just create a device_warm_reset() wrapper.  That would also avoid the
>> bare boolean parameter, which is not great for readability (you have
>> to look up the signature to have any idea what it means).

If the boolean is not meaningful, we can use an enum...

> I've added device_reset_cold/warm wrapper functions to avoid having to
> pass the boolean parameter. it seems I forgot to use them in qdev.c
> I suppose, like you said, we could live with
> + no function with the boolean parameter
> + device_reset doing cold reset
> + device_reset_warm (or device_warm_reset) for the warm version
> 
> Damien
>