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:

> Il 07/12/2013 00:27, Bandan Das ha scritto:
>> 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.
>
> This is a generic walk function.
> For reset you want post-order, but in other cases pre-order may make
> more sense, for example realize.

Sorry, not sure if I get it. What I meant was will this work ?

 int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque)
+                       qbus_walkerfn *busfn, bool ispostorder, void *opaque)
 {
     BusChild *kid;
     int err;
 
-    if (busfn) {
+    if (!ispostorder && busfn) {
         err = busfn(bus, opaque);
         if (err) {
             return err;
@@ -351,17 +351,24 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn 
*devfn,
     }
 
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        err = qdev_walk_children(kid->child, devfn, busfn, opaque);
+        err = qdev_walk_children(kid->child, devfn, busfn, ispostorder, 
opaque);
         if (err < 0) {
             return err;
         }
     }
 
+    if (ispostorder && busfn) {
+        err = busfn(bus, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
     return 0;
 }

Or there's a case where we would like to traverse pre for parent and post
for children's buses (or something similar)..

> Paolo
>
>> 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);
>>>  
>>>  /**