[RFC,V8,01/15] qdev : add a maximum device allowed field for the bus.

From: KONRAD Frederic <fred.konrad@greensocs.com>

Add a max_dev field to BusState to specify the maximum amount of devices allowed
on the bus ( have no effect if max_dev=0 )

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/qdev-core.h    |  2 ++
 hw/qdev-monitor.c | 11 +++++++++++
 2 files changed, 13 insertions(+)

fred.konrad@greensocs.com writes:

> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Add a max_dev field to BusState to specify the maximum amount of devices allowed
> on the bus ( have no effect if max_dev=0 )
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/qdev-core.h    |  2 ++
>  hw/qdev-monitor.c | 11 +++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index d672cca..af909b9 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -104,6 +104,8 @@ struct BusState {
>      const char *name;
>      int allow_hotplug;
>      int max_index;
> +    /* maximum devices allowed on the bus, 0 : no limit. */
> +    int max_dev;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
>  };
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index a1b4d6a..7a9d275 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -292,6 +292,17 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
>      if (bus_typename && !object_dynamic_cast(OBJECT(bus), bus_typename)) {
>          match = 0;
>      }
> +    if ((bus->max_dev != 0) && (bus->max_dev <= bus->max_index)) {
> +        if (name != NULL) {
> +            /* bus was explicitly specified : return an error. */
> +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
> +                          bus->name);
> +            return NULL;
> +        } else {
> +            /* bus was not specified : try to find another one. */
> +            match = 0;
> +        }
> +    }
>      if (match) {
>          return bus;
>      }

Nice change, but I wonder if this should be a class property instead of
an object property?  Would different objects of the same class ever set
this differently?

Regards,

Anthony Liguori

> -- 
> 1.7.11.7

Am 02.01.2013 15:08, schrieb Anthony Liguori:
> fred.konrad@greensocs.com writes:
> 
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> Add a max_dev field to BusState to specify the maximum amount of devices allowed
>> on the bus ( have no effect if max_dev=0 )
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>  hw/qdev-core.h    |  2 ++
>>  hw/qdev-monitor.c | 11 +++++++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
>> index d672cca..af909b9 100644
>> --- a/hw/qdev-core.h
>> +++ b/hw/qdev-core.h
>> @@ -104,6 +104,8 @@ struct BusState {
>>      const char *name;
>>      int allow_hotplug;
>>      int max_index;
>> +    /* maximum devices allowed on the bus, 0 : no limit. */
>> +    int max_dev;

Can't for the virtio-bus case (which this is for AFAIU) the same effect
be achieved by setting max_index? If not, this could use some more
documentation - btw using gtk-doc style comments (above struct) would be
a bonus.

Regards,
Andreas

P.S. Please remember to use English punctuation rules, i.e. no spaces
before colon or inside parenthesis. ;)

On 02/01/2013 15:16, Andreas Färber wrote:
> Am 02.01.2013 15:08, schrieb Anthony Liguori:
>> fred.konrad@greensocs.com writes:
>>
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>
>>> Add a max_dev field to BusState to specify the maximum amount of devices allowed
>>> on the bus ( have no effect if max_dev=0 )
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> ---
>>>   hw/qdev-core.h    |  2 ++
>>>   hw/qdev-monitor.c | 11 +++++++++++
>>>   2 files changed, 13 insertions(+)
>>>
>>> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
>>> index d672cca..af909b9 100644
>>> --- a/hw/qdev-core.h
>>> +++ b/hw/qdev-core.h
>>> @@ -104,6 +104,8 @@ struct BusState {
>>>       const char *name;
>>>       int allow_hotplug;
>>>       int max_index;
>>> +    /* maximum devices allowed on the bus, 0 : no limit. */
>>> +    int max_dev;
> Can't for the virtio-bus case (which this is for AFAIU) the same effect
> be achieved by setting max_index? If not, this could use some more
> documentation - btw using gtk-doc style comments (above struct) would be
> a bonus.
no, max_index is just a variable which count the number of bus children 
I think.
max_index is incremented each time bus_add_child is called.

maybe the name max_index is not a good choice ?

>
> Regards,
> Andreas
>
> P.S. Please remember to use English punctuation rules, i.e. no spaces
> before colon or inside parenthesis. ;)
:s sorry for that.

>

On 02/01/2013 15:08, Anthony Liguori wrote:
> fred.konrad@greensocs.com writes:
>
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> Add a max_dev field to BusState to specify the maximum amount of devices allowed
>> on the bus ( have no effect if max_dev=0 )
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   hw/qdev-core.h    |  2 ++
>>   hw/qdev-monitor.c | 11 +++++++++++
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
>> index d672cca..af909b9 100644
>> --- a/hw/qdev-core.h
>> +++ b/hw/qdev-core.h
>> @@ -104,6 +104,8 @@ struct BusState {
>>       const char *name;
>>       int allow_hotplug;
>>       int max_index;
>> +    /* maximum devices allowed on the bus, 0 : no limit. */
>> +    int max_dev;
>>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>>       QLIST_ENTRY(BusState) sibling;
>>   };
>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> index a1b4d6a..7a9d275 100644
>> --- a/hw/qdev-monitor.c
>> +++ b/hw/qdev-monitor.c
>> @@ -292,6 +292,17 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
>>       if (bus_typename && !object_dynamic_cast(OBJECT(bus), bus_typename)) {
>>           match = 0;
>>       }
>> +    if ((bus->max_dev != 0) && (bus->max_dev <= bus->max_index)) {
>> +        if (name != NULL) {
>> +            /* bus was explicitly specified : return an error. */
>> +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
>> +                          bus->name);
>> +            return NULL;
>> +        } else {
>> +            /* bus was not specified : try to find another one. */
>> +            match = 0;
>> +        }
>> +    }
>>       if (match) {
>>           return bus;
>>       }
> Nice change, but I wonder if this should be a class property instead of
> an object property?  Would different objects of the same class ever set
> this differently?
I don't know. What do you think is the best ?

Fred

>
> Regards,
>
> Anthony Liguori
>
>> -- 
>> 1.7.11.7