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

[David Gibson]

On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote:
> 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...

That's certainly better, but I'm not seeing a compelling reason not to
have separate function names.  It's just as clear and means less churn.

> 
> > 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
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature