Re: [Qemu-devel] [RFC PATCH V4 1/6] qdev : add a maximum device allowed field for the bus.

[Peter Maydell]

On 30 November 2012 17:12,  <address@hidden> wrote:
> From: KONRAD Frederic <address@hidden>
>
> Only one device can be connected to virtio-bus.
> This patch add a field max_dev which is :
>     * the maximum amount of devices connected on the bus ( when
>     * max_dev!=0 ).
>     * have no effect ( when max_dev=0 ).
>
> The function qbus_find_recursive is modified :
>     * to return a non full bus when the "bus=" option is not present.
>     * to give an error when the "bus=" option is pointing a full bus.

You don't need to describe the exact details of code changes in
a commit message -- anybody who wants that can use 'git show'.
Commit messages should describe the overall change and rationale,
not the mechanics. So for instance you should talk about the
effects as visible to the QEMU user, not the changes in semantics
of a particular function.

(Also I don't like commit messages that say "this patch", because
they're not patches once they've been applied; but that's just a
personal quirk.)

> Signed-off-by: KONRAD Frederic <address@hidden>
> ---
>  hw/qdev-core.h    |    2 ++
>  hw/qdev-monitor.c |   11 +++++++++++
>  qerror.h          |    3 +++
>  3 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index fff7f0f..f5019b1 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -113,6 +113,8 @@ struct BusState {
>      const char *name;
>      int allow_hotplug;
>      int max_index;
> +    /* maximum devices allowed on the bus. */

", 0 if 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..4471b3a 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;
>      }
> +    /* Check if max_dev is reached */

This comment is veering perilously close to "Add one to x" territory.

> +    if ((bus->max_dev != 0) && (bus->max_dev <= bus->max_index)) {
> +        if (name != NULL) {
> +            /* bus was explicitly specified : return an error. */
> +            qerror_report(QERR_BUS_IS_FULL, bus->name);

So I could be wrong here, but I think we aren't defining new
QERR constants any more, but just having format-string style
errors inline. In this case that would be something like:

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

-- PMM