Re: [Qemu-devel] [PATCH 3/4] qdev: allow both pre- and post-order vists in qdev walking functions

[Bandan Das]

Paolo Bonzini <address@hidden> writes:

> Resetting should be done in post-order, not pre-order.  However,
> qdev_walk_children and qbus_walk_children do not allow this.  Fix
> it by adding two extra arguments to the functions.
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/core/qdev.c         |   45 +++++++++++++++++++++++++++++++++------------
>  include/hw/qdev-core.h |   13 +++++++++----
>  2 files changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 758de9f..1c114b7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -240,12 +240,12 @@ static int qbus_reset_one(BusState *bus, void *opaque)
>  
>  void qdev_reset_all(DeviceState *dev)
>  {
> -    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
> +    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, 
> NULL);
>  }
>  
>  void qbus_reset_all(BusState *bus)
>  {
> -    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
> +    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, 
> NULL);
>  }
>  
>  void qbus_reset_all_fn(void *opaque)
> @@ -343,49 +343,70 @@ BusState *qdev_get_child_bus(DeviceState *dev, const 
> char *name)
>      return NULL;
>  }
>  
> -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
> -                       qbus_walkerfn *busfn, void *opaque)
> +int qbus_walk_children(BusState *bus,
> +                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
> +                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
> +                       void *opaque)
>  {

It's either pre or post right ? I also thought that the traversal 
applies to the whole hierarchy not just buses or devices individually..
Why not just have a single parameter that says pre or post, not much 
difference but atleast one parameter less.

Bandan

>      BusChild *kid;
>      int err;
>  
> -    if (busfn) {
> -        err = busfn(bus, opaque);
> +    if (pre_busfn) {
> +        err = pre_busfn(bus, opaque);
>          if (err) {
>              return err;
>          }
>      }
>  
>      QTAILQ_FOREACH(kid, &bus->children, sibling) {
> -        err = qdev_walk_children(kid->child, devfn, busfn, opaque);
> +        err = qdev_walk_children(kid->child,
> +                                 pre_devfn, pre_busfn,
> +                                 post_devfn, post_busfn, opaque);
>          if (err < 0) {
>              return err;
>          }
>      }
>  
> +    if (post_busfn) {
> +        err = post_busfn(bus, opaque);
> +        if (err) {
> +            return err;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
> -                       qbus_walkerfn *busfn, void *opaque)
> +int qdev_walk_children(DeviceState *dev,
> +                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
> +                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
> +                       void *opaque)
>  {
>      BusState *bus;
>      int err;
>  
> -    if (devfn) {
> -        err = devfn(dev, opaque);
> +    if (pre_devfn) {
> +        err = pre_devfn(dev, opaque);
>          if (err) {
>              return err;
>          }
>      }
>  
>      QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> -        err = qbus_walk_children(bus, devfn, busfn, opaque);
> +        err = qbus_walk_children(bus, pre_devfn, pre_busfn,
> +                                 post_devfn, post_busfn, opaque);
>          if (err < 0) {
>              return err;
>          }
>      }
>  
> +    if (post_devfn) {
> +        err = post_devfn(dev, opaque);
> +        if (err) {
> +            return err;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index d840f06..21ea2c6 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -274,10 +274,15 @@ BusState *qbus_create(const char *typename, DeviceState 
> *parent, const char *nam
>  /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
>   *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
>   *           0 otherwise. */
> -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
> -                       qbus_walkerfn *busfn, void *opaque);
> -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
> -                       qbus_walkerfn *busfn, void *opaque);
> +int qbus_walk_children(BusState *bus,
> +                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
> +                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
> +                       void *opaque);
> +int qdev_walk_children(DeviceState *dev,
> +                       qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn,
> +                       qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
> +                       void *opaque);
> +
>  void qdev_reset_all(DeviceState *dev);
>  
>  /**