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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 07/12/2013 00:27, Bandan Das ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com>
>> ---
>>  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.

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