[v3] qdev/core: fix qbus_is_full()

Commit Message

Anthony Krowiak Dec. 17, 2018, 3:57 p.m. UTC

The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
value of the BusState structure with the max_dev value of the BusClass structure
to determine whether the maximum number of children has been reached for the
bus. The problem is, the max_index field of the BusState structure does not
necessarily reflect the number of devices that have been plugged into
the bus.

Whenever a child device is plugged into the bus, the bus's max_index value is
assigned to the child device and then incremented. If the child is subsequently
unplugged, the value of the max_index does not change and no longer reflects the
number of children.

When the bus's max_index value reaches the maximum number of devices
allowed for the bus (i.e., the max_dev field in the BusClass structure),
attempts to plug another device will be rejected claiming that the bus is
full -- even if the bus is actually empty.

To resolve the problem, a new 'num_children' field is being added to the
BusState structure to keep track of the number of children plugged into the
bus. It will be incremented when a child is plugged, and decremented when a
child is unplugged.

Signed-off-by: Tony Krowiak <[e-mail]>
Reviewed-by: Pierre Morel<[e-mail]>
Reviewed-by: Halil Pasic <[e-mail]>
---
 hw/core/qdev.c         | 3 +++
 include/hw/qdev-core.h | 1 +
 qdev-monitor.c         | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

Comments

Igor Mammedov Dec. 18, 2018, 2:18 p.m. UTC | #1

On Mon, 17 Dec 2018 10:57:30 -0500
Tony Krowiak <[e-mail]> wrote:

> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> value of the BusState structure with the max_dev value of the BusClass structure
> to determine whether the maximum number of children has been reached for the
> bus. The problem is, the max_index field of the BusState structure does not
> necessarily reflect the number of devices that have been plugged into
> the bus.
> 
> Whenever a child device is plugged into the bus, the bus's max_index value is
> assigned to the child device and then incremented. If the child is subsequently
> unplugged, the value of the max_index does not change and no longer reflects the
> number of children.
> 
> When the bus's max_index value reaches the maximum number of devices
> allowed for the bus (i.e., the max_dev field in the BusClass structure),
> attempts to plug another device will be rejected claiming that the bus is
> full -- even if the bus is actually empty.
> 
> To resolve the problem, a new 'num_children' field is being added to the
> BusState structure to keep track of the number of children plugged into the
> bus. It will be incremented when a child is plugged, and decremented when a
> child is unplugged.
> 
> Signed-off-by: Tony Krowiak <[e-mail]>
> Reviewed-by: Pierre Morel<[e-mail]>
> Reviewed-by: Halil Pasic <[e-mail]>

Reviewed-by: Igor Mammedov <[e-mail]>

> ---
>  hw/core/qdev.c         | 3 +++
>  include/hw/qdev-core.h | 1 +
>  qdev-monitor.c         | 2 +-
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55b27c2..956923f33520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>              snprintf(name, sizeof(name), "child[%d]", kid->index);
>              QTAILQ_REMOVE(&bus->children, kid, sibling);
>  
> +            bus->num_children--;
> +
>              /* This gives back ownership of kid->child back to us.  */
>              object_property_del(OBJECT(bus), name, NULL);
>              object_unref(OBJECT(kid->child));
> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>      char name[32];
>      BusChild *kid = g_malloc0(sizeof(*kid));
>  
> +    bus->num_children++;
>      kid->index = bus->max_index++;
>      kid->child = child;
>      object_ref(OBJECT(kid->child));
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566e3..521f0a947ead 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -206,6 +206,7 @@ struct BusState {
>      HotplugHandler *hotplug_handler;
>      int max_index;
>      bool realized;
> +    int num_children;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
>  };
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 07147c63bf8b..45a8ba49644c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>  static inline bool qbus_is_full(BusState *bus)
>  {
>      BusClass *bus_class = BUS_GET_CLASS(bus);
> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>  }
>  
>  /*

Anthony Krowiak Jan. 8, 2019, 4:08 p.m. UTC | #2

On 12/17/18 10:57 AM, Tony Krowiak wrote:
> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> value of the BusState structure with the max_dev value of the BusClass structure
> to determine whether the maximum number of children has been reached for the
> bus. The problem is, the max_index field of the BusState structure does not
> necessarily reflect the number of devices that have been plugged into
> the bus.
> 
> Whenever a child device is plugged into the bus, the bus's max_index value is
> assigned to the child device and then incremented. If the child is subsequently
> unplugged, the value of the max_index does not change and no longer reflects the
> number of children.
> 
> When the bus's max_index value reaches the maximum number of devices
> allowed for the bus (i.e., the max_dev field in the BusClass structure),
> attempts to plug another device will be rejected claiming that the bus is
> full -- even if the bus is actually empty.
> 
> To resolve the problem, a new 'num_children' field is being added to the
> BusState structure to keep track of the number of children plugged into the
> bus. It will be incremented when a child is plugged, and decremented when a
> child is unplugged.
> 
> Signed-off-by: Tony Krowiak <[e-mail]>
> Reviewed-by: Pierre Morel<[e-mail]>
> Reviewed-by: Halil Pasic <[e-mail]>
> ---
>   hw/core/qdev.c         | 3 +++
>   include/hw/qdev-core.h | 1 +
>   qdev-monitor.c         | 2 +-
>   3 files changed, 5 insertions(+), 1 deletion(-)

Ping ...
I could not determine who the maintainer is for the three files
listed above. I checked the MAINTAINERS file and the prologue of each
individual file. Can someone please tell me who is responsible
for merging these changes? Any additional review comments?

> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55b27c2..956923f33520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>               snprintf(name, sizeof(name), "child[%d]", kid->index);
>               QTAILQ_REMOVE(&bus->children, kid, sibling);
>   
> +            bus->num_children--;
> +
>               /* This gives back ownership of kid->child back to us.  */
>               object_property_del(OBJECT(bus), name, NULL);
>               object_unref(OBJECT(kid->child));
> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>       char name[32];
>       BusChild *kid = g_malloc0(sizeof(*kid));
>   
> +    bus->num_children++;
>       kid->index = bus->max_index++;
>       kid->child = child;
>       object_ref(OBJECT(kid->child));
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566e3..521f0a947ead 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -206,6 +206,7 @@ struct BusState {
>       HotplugHandler *hotplug_handler;
>       int max_index;
>       bool realized;
> +    int num_children;
>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>       QLIST_ENTRY(BusState) sibling;
>   };
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 07147c63bf8b..45a8ba49644c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>   static inline bool qbus_is_full(BusState *bus)
>   {
>       BusClass *bus_class = BUS_GET_CLASS(bus);
> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>   }
>   
>   /*
>

Cornelia Huck Jan. 8, 2019, 4:31 p.m. UTC | #3

On Tue, 8 Jan 2019 11:08:56 -0500
Tony Krowiak <[e-mail]> wrote:

> On 12/17/18 10:57 AM, Tony Krowiak wrote:
> > The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> > value of the BusState structure with the max_dev value of the BusClass structure
> > to determine whether the maximum number of children has been reached for the
> > bus. The problem is, the max_index field of the BusState structure does not
> > necessarily reflect the number of devices that have been plugged into
> > the bus.
> > 
> > Whenever a child device is plugged into the bus, the bus's max_index value is
> > assigned to the child device and then incremented. If the child is subsequently
> > unplugged, the value of the max_index does not change and no longer reflects the
> > number of children.
> > 
> > When the bus's max_index value reaches the maximum number of devices
> > allowed for the bus (i.e., the max_dev field in the BusClass structure),
> > attempts to plug another device will be rejected claiming that the bus is
> > full -- even if the bus is actually empty.
> > 
> > To resolve the problem, a new 'num_children' field is being added to the
> > BusState structure to keep track of the number of children plugged into the
> > bus. It will be incremented when a child is plugged, and decremented when a
> > child is unplugged.
> > 
> > Signed-off-by: Tony Krowiak <[e-mail]>
> > Reviewed-by: Pierre Morel<[e-mail]>
> > Reviewed-by: Halil Pasic <[e-mail]>
> > ---
> >   hw/core/qdev.c         | 3 +++
> >   include/hw/qdev-core.h | 1 +
> >   qdev-monitor.c         | 2 +-
> >   3 files changed, 5 insertions(+), 1 deletion(-)  
> 
> Ping ...
> I could not determine who the maintainer is for the three files
> listed above. I checked the MAINTAINERS file and the prologue of each
> individual file. Can someone please tell me who is responsible
> for merging these changes? Any additional review comments?
> 
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 6b3cc55b27c2..956923f33520 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
> >               snprintf(name, sizeof(name), "child[%d]", kid->index);
> >               QTAILQ_REMOVE(&bus->children, kid, sibling);
> >   
> > +            bus->num_children--;
> > +
> >               /* This gives back ownership of kid->child back to us.  */
> >               object_property_del(OBJECT(bus), name, NULL);
> >               object_unref(OBJECT(kid->child));
> > @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> >       char name[32];
> >       BusChild *kid = g_malloc0(sizeof(*kid));
> >   
> > +    bus->num_children++;
> >       kid->index = bus->max_index++;

Hm... I'm wondering what happens for insane numbers of hotplugging
operations here?

(Preexisting problem for busses without limit, but busses with a limit
could now run into that as well.)

> >       kid->child = child;
> >       object_ref(OBJECT(kid->child));
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index a24d0dd566e3..521f0a947ead 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -206,6 +206,7 @@ struct BusState {
> >       HotplugHandler *hotplug_handler;
> >       int max_index;
> >       bool realized;
> > +    int num_children;
> >       QTAILQ_HEAD(ChildrenHead, BusChild) children;
> >       QLIST_ENTRY(BusState) sibling;
> >   };
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 07147c63bf8b..45a8ba49644c 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
> >   static inline bool qbus_is_full(BusState *bus)
> >   {
> >       BusClass *bus_class = BUS_GET_CLASS(bus);
> > -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> > +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
> >   }
> >   
> >   /*
> >   

The approach the patch takes looks sane to me.

Halil Pasic Jan. 8, 2019, 4:50 p.m. UTC | #4

On Tue, 8 Jan 2019 17:31:13 +0100
Cornelia Huck <[e-mail]> wrote:

> On Tue, 8 Jan 2019 11:08:56 -0500
> Tony Krowiak <[e-mail]> wrote:
> 
> > On 12/17/18 10:57 AM, Tony Krowiak wrote:
> > > The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> > > value of the BusState structure with the max_dev value of the BusClass structure
> > > to determine whether the maximum number of children has been reached for the
> > > bus. The problem is, the max_index field of the BusState structure does not
> > > necessarily reflect the number of devices that have been plugged into
> > > the bus.
> > > 
> > > Whenever a child device is plugged into the bus, the bus's max_index value is
> > > assigned to the child device and then incremented. If the child is subsequently
> > > unplugged, the value of the max_index does not change and no longer reflects the
> > > number of children.
> > > 
> > > When the bus's max_index value reaches the maximum number of devices
> > > allowed for the bus (i.e., the max_dev field in the BusClass structure),
> > > attempts to plug another device will be rejected claiming that the bus is
> > > full -- even if the bus is actually empty.
> > > 
> > > To resolve the problem, a new 'num_children' field is being added to the
> > > BusState structure to keep track of the number of children plugged into the
> > > bus. It will be incremented when a child is plugged, and decremented when a
> > > child is unplugged.
> > > 
> > > Signed-off-by: Tony Krowiak <[e-mail]>
> > > Reviewed-by: Pierre Morel<[e-mail]>
> > > Reviewed-by: Halil Pasic <[e-mail]>
> > > ---
> > >   hw/core/qdev.c         | 3 +++
> > >   include/hw/qdev-core.h | 1 +
> > >   qdev-monitor.c         | 2 +-
> > >   3 files changed, 5 insertions(+), 1 deletion(-)  
> > 
> > Ping ...
> > I could not determine who the maintainer is for the three files
> > listed above. I checked the MAINTAINERS file and the prologue of each
> > individual file. Can someone please tell me who is responsible
> > for merging these changes? Any additional review comments?
> > 
> > > 
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index 6b3cc55b27c2..956923f33520 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
> > >               snprintf(name, sizeof(name), "child[%d]", kid->index);
> > >               QTAILQ_REMOVE(&bus->children, kid, sibling);
> > >   
> > > +            bus->num_children--;
> > > +
> > >               /* This gives back ownership of kid->child back to us.  */
> > >               object_property_del(OBJECT(bus), name, NULL);
> > >               object_unref(OBJECT(kid->child));
> > > @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> > >       char name[32];
> > >       BusChild *kid = g_malloc0(sizeof(*kid));
> > >   
> > > +    bus->num_children++;
> > >       kid->index = bus->max_index++;
> 
> Hm... I'm wondering what happens for insane numbers of hotplugging
> operations here?
> 
> (Preexisting problem for busses without limit, but busses with a limit
> could now run into that as well.)
> 

How does this patch change things? I mean bus->num_children gets
decremented on unplug.

Regards,
Halil

> > >       kid->child = child;
> > >       object_ref(OBJECT(kid->child));
> > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > index a24d0dd566e3..521f0a947ead 100644
> > > --- a/include/hw/qdev-core.h
> > > +++ b/include/hw/qdev-core.h
> > > @@ -206,6 +206,7 @@ struct BusState {
> > >       HotplugHandler *hotplug_handler;
> > >       int max_index;
> > >       bool realized;
> > > +    int num_children;
> > >       QTAILQ_HEAD(ChildrenHead, BusChild) children;
> > >       QLIST_ENTRY(BusState) sibling;
> > >   };
> > > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > > index 07147c63bf8b..45a8ba49644c 100644
> > > --- a/qdev-monitor.c
> > > +++ b/qdev-monitor.c
> > > @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
> > >   static inline bool qbus_is_full(BusState *bus)
> > >   {
> > >       BusClass *bus_class = BUS_GET_CLASS(bus);
> > > -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> > > +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
> > >   }
> > >   
> > >   /*
> > >   
> 
> The approach the patch takes looks sane to me.
>

Cornelia Huck Jan. 8, 2019, 5:06 p.m. UTC | #5

On Tue, 8 Jan 2019 17:50:21 +0100
Halil Pasic <[e-mail]> wrote:

> On Tue, 8 Jan 2019 17:31:13 +0100
> Cornelia Huck <[e-mail]> wrote:
> 
> > On Tue, 8 Jan 2019 11:08:56 -0500
> > Tony Krowiak <[e-mail]> wrote:
> >   
> > > On 12/17/18 10:57 AM, Tony Krowiak wrote:  
> > > > The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> > > > value of the BusState structure with the max_dev value of the BusClass structure
> > > > to determine whether the maximum number of children has been reached for the
> > > > bus. The problem is, the max_index field of the BusState structure does not
> > > > necessarily reflect the number of devices that have been plugged into
> > > > the bus.
> > > > 
> > > > Whenever a child device is plugged into the bus, the bus's max_index value is
> > > > assigned to the child device and then incremented. If the child is subsequently
> > > > unplugged, the value of the max_index does not change and no longer reflects the
> > > > number of children.
> > > > 
> > > > When the bus's max_index value reaches the maximum number of devices
> > > > allowed for the bus (i.e., the max_dev field in the BusClass structure),
> > > > attempts to plug another device will be rejected claiming that the bus is
> > > > full -- even if the bus is actually empty.
> > > > 
> > > > To resolve the problem, a new 'num_children' field is being added to the
> > > > BusState structure to keep track of the number of children plugged into the
> > > > bus. It will be incremented when a child is plugged, and decremented when a
> > > > child is unplugged.
> > > > 
> > > > Signed-off-by: Tony Krowiak <[e-mail]>
> > > > Reviewed-by: Pierre Morel<[e-mail]>
> > > > Reviewed-by: Halil Pasic <[e-mail]>
> > > > ---
> > > >   hw/core/qdev.c         | 3 +++
> > > >   include/hw/qdev-core.h | 1 +
> > > >   qdev-monitor.c         | 2 +-
> > > >   3 files changed, 5 insertions(+), 1 deletion(-)    
> > > 
> > > Ping ...
> > > I could not determine who the maintainer is for the three files
> > > listed above. I checked the MAINTAINERS file and the prologue of each
> > > individual file. Can someone please tell me who is responsible
> > > for merging these changes? Any additional review comments?
> > >   
> > > > 
> > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > index 6b3cc55b27c2..956923f33520 100644
> > > > --- a/hw/core/qdev.c
> > > > +++ b/hw/core/qdev.c
> > > > @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
> > > >               snprintf(name, sizeof(name), "child[%d]", kid->index);
> > > >               QTAILQ_REMOVE(&bus->children, kid, sibling);
> > > >   
> > > > +            bus->num_children--;
> > > > +
> > > >               /* This gives back ownership of kid->child back to us.  */
> > > >               object_property_del(OBJECT(bus), name, NULL);
> > > >               object_unref(OBJECT(kid->child));
> > > > @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> > > >       char name[32];
> > > >       BusChild *kid = g_malloc0(sizeof(*kid));
> > > >   
> > > > +    bus->num_children++;
> > > >       kid->index = bus->max_index++;  
> > 
> > Hm... I'm wondering what happens for insane numbers of hotplugging
> > operations here?
> > 
> > (Preexisting problem for busses without limit, but busses with a limit
> > could now run into that as well.)
> >   
> 
> How does this patch change things? I mean bus->num_children gets
> decremented on unplug.

We don't stop anymore if max_index >= max_dev, which means that we can
now trigger that even if max_dev != 0.

> 
> Regards,
> Halil
> 
> > > >       kid->child = child;
> > > >       object_ref(OBJECT(kid->child));
> > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > > index a24d0dd566e3..521f0a947ead 100644
> > > > --- a/include/hw/qdev-core.h
> > > > +++ b/include/hw/qdev-core.h
> > > > @@ -206,6 +206,7 @@ struct BusState {
> > > >       HotplugHandler *hotplug_handler;
> > > >       int max_index;
> > > >       bool realized;
> > > > +    int num_children;
> > > >       QTAILQ_HEAD(ChildrenHead, BusChild) children;
> > > >       QLIST_ENTRY(BusState) sibling;
> > > >   };
> > > > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > > > index 07147c63bf8b..45a8ba49644c 100644
> > > > --- a/qdev-monitor.c
> > > > +++ b/qdev-monitor.c
> > > > @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
> > > >   static inline bool qbus_is_full(BusState *bus)
> > > >   {
> > > >       BusClass *bus_class = BUS_GET_CLASS(bus);
> > > > -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> > > > +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
> > > >   }
> > > >   
> > > >   /*
> > > >     
> > 
> > The approach the patch takes looks sane to me.
> >   
>

Anthony Krowiak Jan. 8, 2019, 8:34 p.m. UTC | #6

On 1/8/19 12:06 PM, Cornelia Huck wrote:
> On Tue, 8 Jan 2019 17:50:21 +0100
> Halil Pasic <[e-mail]> wrote:
> 
>> On Tue, 8 Jan 2019 17:31:13 +0100
>> Cornelia Huck <[e-mail]> wrote:
>>
>>> On Tue, 8 Jan 2019 11:08:56 -0500
>>> Tony Krowiak <[e-mail]> wrote:
>>>    
>>>> On 12/17/18 10:57 AM, Tony Krowiak wrote:
>>>>> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
>>>>> value of the BusState structure with the max_dev value of the BusClass structure
>>>>> to determine whether the maximum number of children has been reached for the
>>>>> bus. The problem is, the max_index field of the BusState structure does not
>>>>> necessarily reflect the number of devices that have been plugged into
>>>>> the bus.
>>>>>
>>>>> Whenever a child device is plugged into the bus, the bus's max_index value is
>>>>> assigned to the child device and then incremented. If the child is subsequently
>>>>> unplugged, the value of the max_index does not change and no longer reflects the
>>>>> number of children.
>>>>>
>>>>> When the bus's max_index value reaches the maximum number of devices
>>>>> allowed for the bus (i.e., the max_dev field in the BusClass structure),
>>>>> attempts to plug another device will be rejected claiming that the bus is
>>>>> full -- even if the bus is actually empty.
>>>>>
>>>>> To resolve the problem, a new 'num_children' field is being added to the
>>>>> BusState structure to keep track of the number of children plugged into the
>>>>> bus. It will be incremented when a child is plugged, and decremented when a
>>>>> child is unplugged.
>>>>>
>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>>> Reviewed-by: Pierre Morel<[e-mail]>
>>>>> Reviewed-by: Halil Pasic <[e-mail]>
>>>>> ---
>>>>>    hw/core/qdev.c         | 3 +++
>>>>>    include/hw/qdev-core.h | 1 +
>>>>>    qdev-monitor.c         | 2 +-
>>>>>    3 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> Ping ...
>>>> I could not determine who the maintainer is for the three files
>>>> listed above. I checked the MAINTAINERS file and the prologue of each
>>>> individual file. Can someone please tell me who is responsible
>>>> for merging these changes? Any additional review comments?
>>>>    
>>>>>
>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>> index 6b3cc55b27c2..956923f33520 100644
>>>>> --- a/hw/core/qdev.c
>>>>> +++ b/hw/core/qdev.c
>>>>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>>>>>                snprintf(name, sizeof(name), "child[%d]", kid->index);
>>>>>                QTAILQ_REMOVE(&bus->children, kid, sibling);
>>>>>    
>>>>> +            bus->num_children--;
>>>>> +
>>>>>                /* This gives back ownership of kid->child back to us.  */
>>>>>                object_property_del(OBJECT(bus), name, NULL);
>>>>>                object_unref(OBJECT(kid->child));
>>>>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>>>>>        char name[32];
>>>>>        BusChild *kid = g_malloc0(sizeof(*kid));
>>>>>    
>>>>> +    bus->num_children++;
>>>>>        kid->index = bus->max_index++;
>>>
>>> Hm... I'm wondering what happens for insane numbers of hotplugging
>>> operations here?
>>>
>>> (Preexisting problem for busses without limit, but busses with a limit
>>> could now run into that as well.)
>>>    
>>
>> How does this patch change things? I mean bus->num_children gets
>> decremented on unplug.
> 
> We don't stop anymore if max_index >= max_dev, which means that we can
> now trigger that even if max_dev != 0.

I guess I am missing your point. If max_dev == 0, then there is nothing
stopping an insane number of hot plug operations; either before this
patch, or with this patch. With the patch, once the number of children
hot plugged reaches max_dev, the qbus_is_full function will return false
and no more children can be plugged. If a child device is unplugged,
the num_children - which counts the number of children attached to the
bus - will be decremented, so it always reflects the number of children
added to the bus. Besides, checking max_index against max_dev
is erroneous, because max_index is incremented every time a child device
is plugged and is never decremented. It really operates as more of a
uinique identifier than a counter and is also used to create a unique
property name when the child device is linked to the bus as a property
(see bus_add_child function in hw/core/qdev.c).

> 
>>
>> Regards,
>> Halil
>>
>>>>>        kid->child = child;
>>>>>        object_ref(OBJECT(kid->child));
>>>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>>>> index a24d0dd566e3..521f0a947ead 100644
>>>>> --- a/include/hw/qdev-core.h
>>>>> +++ b/include/hw/qdev-core.h
>>>>> @@ -206,6 +206,7 @@ struct BusState {
>>>>>        HotplugHandler *hotplug_handler;
>>>>>        int max_index;
>>>>>        bool realized;
>>>>> +    int num_children;
>>>>>        QTAILQ_HEAD(ChildrenHead, BusChild) children;
>>>>>        QLIST_ENTRY(BusState) sibling;
>>>>>    };
>>>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>>>> index 07147c63bf8b..45a8ba49644c 100644
>>>>> --- a/qdev-monitor.c
>>>>> +++ b/qdev-monitor.c
>>>>> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>>>>>    static inline bool qbus_is_full(BusState *bus)
>>>>>    {
>>>>>        BusClass *bus_class = BUS_GET_CLASS(bus);
>>>>> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
>>>>> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>>>>>    }
>>>>>    
>>>>>    /*
>>>>>      
>>>
>>> The approach the patch takes looks sane to me.
>>>    
>>
>

Cornelia Huck Jan. 9, 2019, 10:14 a.m. UTC | #7

On Tue, 8 Jan 2019 15:34:37 -0500
Tony Krowiak <[e-mail]> wrote:

> On 1/8/19 12:06 PM, Cornelia Huck wrote:
> > On Tue, 8 Jan 2019 17:50:21 +0100
> > Halil Pasic <[e-mail]> wrote:
> >   
> >> On Tue, 8 Jan 2019 17:31:13 +0100
> >> Cornelia Huck <[e-mail]> wrote:
> >>  
> >>> On Tue, 8 Jan 2019 11:08:56 -0500
> >>> Tony Krowiak <[e-mail]> wrote:
> >>>      
> >>>> On 12/17/18 10:57 AM, Tony Krowiak wrote:  
> >>>>> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> >>>>> value of the BusState structure with the max_dev value of the BusClass structure
> >>>>> to determine whether the maximum number of children has been reached for the
> >>>>> bus. The problem is, the max_index field of the BusState structure does not
> >>>>> necessarily reflect the number of devices that have been plugged into
> >>>>> the bus.
> >>>>>
> >>>>> Whenever a child device is plugged into the bus, the bus's max_index value is
> >>>>> assigned to the child device and then incremented. If the child is subsequently
> >>>>> unplugged, the value of the max_index does not change and no longer reflects the
> >>>>> number of children.
> >>>>>
> >>>>> When the bus's max_index value reaches the maximum number of devices
> >>>>> allowed for the bus (i.e., the max_dev field in the BusClass structure),
> >>>>> attempts to plug another device will be rejected claiming that the bus is
> >>>>> full -- even if the bus is actually empty.
> >>>>>
> >>>>> To resolve the problem, a new 'num_children' field is being added to the
> >>>>> BusState structure to keep track of the number of children plugged into the
> >>>>> bus. It will be incremented when a child is plugged, and decremented when a
> >>>>> child is unplugged.
> >>>>>
> >>>>> Signed-off-by: Tony Krowiak <[e-mail]>
> >>>>> Reviewed-by: Pierre Morel<[e-mail]>
> >>>>> Reviewed-by: Halil Pasic <[e-mail]>
> >>>>> ---
> >>>>>    hw/core/qdev.c         | 3 +++
> >>>>>    include/hw/qdev-core.h | 1 +
> >>>>>    qdev-monitor.c         | 2 +-
> >>>>>    3 files changed, 5 insertions(+), 1 deletion(-)  
> >>>>
> >>>> Ping ...
> >>>> I could not determine who the maintainer is for the three files
> >>>> listed above. I checked the MAINTAINERS file and the prologue of each
> >>>> individual file. Can someone please tell me who is responsible
> >>>> for merging these changes? Any additional review comments?
> >>>>      
> >>>>>
> >>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>>>> index 6b3cc55b27c2..956923f33520 100644
> >>>>> --- a/hw/core/qdev.c
> >>>>> +++ b/hw/core/qdev.c
> >>>>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
> >>>>>                snprintf(name, sizeof(name), "child[%d]", kid->index);
> >>>>>                QTAILQ_REMOVE(&bus->children, kid, sibling);
> >>>>>    
> >>>>> +            bus->num_children--;
> >>>>> +
> >>>>>                /* This gives back ownership of kid->child back to us.  */
> >>>>>                object_property_del(OBJECT(bus), name, NULL);
> >>>>>                object_unref(OBJECT(kid->child));
> >>>>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> >>>>>        char name[32];
> >>>>>        BusChild *kid = g_malloc0(sizeof(*kid));
> >>>>>    
> >>>>> +    bus->num_children++;
> >>>>>        kid->index = bus->max_index++;  
> >>>
> >>> Hm... I'm wondering what happens for insane numbers of hotplugging
> >>> operations here?
> >>>
> >>> (Preexisting problem for busses without limit, but busses with a limit
> >>> could now run into that as well.)
> >>>      
> >>
> >> How does this patch change things? I mean bus->num_children gets
> >> decremented on unplug.  
> > 
> > We don't stop anymore if max_index >= max_dev, which means that we can
> > now trigger that even if max_dev != 0.  
> 
> I guess I am missing your point. If max_dev == 0, then there is nothing
> stopping an insane number of hot plug operations; either before this
> patch, or with this patch. With the patch, once the number of children
> hot plugged reaches max_dev, the qbus_is_full function will return false
> and no more children can be plugged. If a child device is unplugged,
> the num_children - which counts the number of children attached to the
> bus - will be decremented, so it always reflects the number of children
> added to the bus. Besides, checking max_index against max_dev
> is erroneous, because max_index is incremented every time a child device
> is plugged and is never decremented. It really operates as more of a
> uinique identifier than a counter and is also used to create a unique
> property name when the child device is linked to the bus as a property
> (see bus_add_child function in hw/core/qdev.c).

Checking num_children against max_dev is the right thing to do, no
argument here.

However, max_index continues to be incremented even if devices have
been unplugged again. That means it can overflow, as it is never bound
by the max_dev constraint.

This has been a problem before for busses with an unrestricted number of
devices before, but with your patch, the problem is now triggerable for
all busses.

Not a problem with your patch, but we might want to look into making
max_index overflow/wraparound save.

> 
> >   
> >>
> >> Regards,
> >> Halil
> >>  
> >>>>>        kid->child = child;
> >>>>>        object_ref(OBJECT(kid->child));
> >>>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> >>>>> index a24d0dd566e3..521f0a947ead 100644
> >>>>> --- a/include/hw/qdev-core.h
> >>>>> +++ b/include/hw/qdev-core.h
> >>>>> @@ -206,6 +206,7 @@ struct BusState {
> >>>>>        HotplugHandler *hotplug_handler;
> >>>>>        int max_index;
> >>>>>        bool realized;
> >>>>> +    int num_children;
> >>>>>        QTAILQ_HEAD(ChildrenHead, BusChild) children;
> >>>>>        QLIST_ENTRY(BusState) sibling;
> >>>>>    };
> >>>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
> >>>>> index 07147c63bf8b..45a8ba49644c 100644
> >>>>> --- a/qdev-monitor.c
> >>>>> +++ b/qdev-monitor.c
> >>>>> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
> >>>>>    static inline bool qbus_is_full(BusState *bus)
> >>>>>    {
> >>>>>        BusClass *bus_class = BUS_GET_CLASS(bus);
> >>>>> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> >>>>> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
> >>>>>    }
> >>>>>    
> >>>>>    /*
> >>>>>        
> >>>
> >>> The approach the patch takes looks sane to me.
> >>>      
> >>  
> >   
>

Anthony Krowiak Jan. 9, 2019, 3:36 p.m. UTC | #8

On 1/9/19 5:14 AM, Cornelia Huck wrote:
> On Tue, 8 Jan 2019 15:34:37 -0500
> Tony Krowiak <[e-mail]> wrote:
> 
>> On 1/8/19 12:06 PM, Cornelia Huck wrote:
>>> On Tue, 8 Jan 2019 17:50:21 +0100
>>> Halil Pasic <[e-mail]> wrote:
>>>    
>>>> On Tue, 8 Jan 2019 17:31:13 +0100
>>>> Cornelia Huck <[e-mail]> wrote:
>>>>   
>>>>> On Tue, 8 Jan 2019 11:08:56 -0500
>>>>> Tony Krowiak <[e-mail]> wrote:
>>>>>       
>>>>>> On 12/17/18 10:57 AM, Tony Krowiak wrote:
>>>>>>> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
>>>>>>> value of the BusState structure with the max_dev value of the BusClass structure
>>>>>>> to determine whether the maximum number of children has been reached for the
>>>>>>> bus. The problem is, the max_index field of the BusState structure does not
>>>>>>> necessarily reflect the number of devices that have been plugged into
>>>>>>> the bus.
>>>>>>>
>>>>>>> Whenever a child device is plugged into the bus, the bus's max_index value is
>>>>>>> assigned to the child device and then incremented. If the child is subsequently
>>>>>>> unplugged, the value of the max_index does not change and no longer reflects the
>>>>>>> number of children.
>>>>>>>
>>>>>>> When the bus's max_index value reaches the maximum number of devices
>>>>>>> allowed for the bus (i.e., the max_dev field in the BusClass structure),
>>>>>>> attempts to plug another device will be rejected claiming that the bus is
>>>>>>> full -- even if the bus is actually empty.
>>>>>>>
>>>>>>> To resolve the problem, a new 'num_children' field is being added to the
>>>>>>> BusState structure to keep track of the number of children plugged into the
>>>>>>> bus. It will be incremented when a child is plugged, and decremented when a
>>>>>>> child is unplugged.
>>>>>>>
>>>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>>>>> Reviewed-by: Pierre Morel<[e-mail]>
>>>>>>> Reviewed-by: Halil Pasic <[e-mail]>
>>>>>>> ---
>>>>>>>     hw/core/qdev.c         | 3 +++
>>>>>>>     include/hw/qdev-core.h | 1 +
>>>>>>>     qdev-monitor.c         | 2 +-
>>>>>>>     3 files changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> Ping ...
>>>>>> I could not determine who the maintainer is for the three files
>>>>>> listed above. I checked the MAINTAINERS file and the prologue of each
>>>>>> individual file. Can someone please tell me who is responsible
>>>>>> for merging these changes? Any additional review comments?
>>>>>>       
>>>>>>>
>>>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>>>> index 6b3cc55b27c2..956923f33520 100644
>>>>>>> --- a/hw/core/qdev.c
>>>>>>> +++ b/hw/core/qdev.c
>>>>>>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>>>>>>>                 snprintf(name, sizeof(name), "child[%d]", kid->index);
>>>>>>>                 QTAILQ_REMOVE(&bus->children, kid, sibling);
>>>>>>>     
>>>>>>> +            bus->num_children--;
>>>>>>> +
>>>>>>>                 /* This gives back ownership of kid->child back to us.  */
>>>>>>>                 object_property_del(OBJECT(bus), name, NULL);
>>>>>>>                 object_unref(OBJECT(kid->child));
>>>>>>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>>>>>>>         char name[32];
>>>>>>>         BusChild *kid = g_malloc0(sizeof(*kid));
>>>>>>>     
>>>>>>> +    bus->num_children++;
>>>>>>>         kid->index = bus->max_index++;
>>>>>
>>>>> Hm... I'm wondering what happens for insane numbers of hotplugging
>>>>> operations here?
>>>>>
>>>>> (Preexisting problem for busses without limit, but busses with a limit
>>>>> could now run into that as well.)
>>>>>       
>>>>
>>>> How does this patch change things? I mean bus->num_children gets
>>>> decremented on unplug.
>>>
>>> We don't stop anymore if max_index >= max_dev, which means that we can
>>> now trigger that even if max_dev != 0.
>>
>> I guess I am missing your point. If max_dev == 0, then there is nothing
>> stopping an insane number of hot plug operations; either before this
>> patch, or with this patch. With the patch, once the number of children
>> hot plugged reaches max_dev, the qbus_is_full function will return false
>> and no more children can be plugged. If a child device is unplugged,
>> the num_children - which counts the number of children attached to the
>> bus - will be decremented, so it always reflects the number of children
>> added to the bus. Besides, checking max_index against max_dev
>> is erroneous, because max_index is incremented every time a child device
>> is plugged and is never decremented. It really operates as more of a
>> uinique identifier than a counter and is also used to create a unique
>> property name when the child device is linked to the bus as a property
>> (see bus_add_child function in hw/core/qdev.c).
> 
> Checking num_children against max_dev is the right thing to do, no
> argument here.
> 
> However, max_index continues to be incremented even if devices have
> been unplugged again. That means it can overflow, as it is never bound
> by the max_dev constraint.
> 
> This has been a problem before for busses with an unrestricted number of
> devices before, but with your patch, the problem is now triggerable for
> all busses.
> 
> Not a problem with your patch, but we might want to look into making
> max_index overflow/wraparound save.

I see your point. It does beg the question, what are the chances that
max_index reaches INT_MAX? In the interest of making this code more
bullet proof, I suppose it is something that should be dealt with.

A search reveals that max_index is used in only two places: It is used
to set the index for a child of the bus (i.e., bus_add_child function in
hw/core/qdev.c); and prior to this patch, to determine if max_dev has
been exceeded (i.e., qbus_is_full function in qdev_monitor.c). From
what I can see, the bus child index is used only to generate a property
name of the format "child[%d]" so the child can be linked as a property
of the bus (see bus_add_child and bus_remove_child functions in
hw/core/qdev.c). Wraparound of the max_index would most likely result in
generating a duplicate property name for the child.

I propose two possible solutions:

1. When max_index reaches INT_MAX, do not allow any additional children
    to be added to the bus.

2. Set a max_dev value of INT_MAX for the BusClass instance if the value
    is not set (in the bus_class_init function in hw/core/bus.c).

3. Instead of generating the property name from the BusChild index
    value, generate a UUID string. Since the index field of the BusChild
    appears to be used only to generate the child's name, maybe change
    the index field to a UUID field or a name field.

> 
>>
>>>    
>>>>
>>>> Regards,
>>>> Halil
>>>>   
>>>>>>>         kid->child = child;
>>>>>>>         object_ref(OBJECT(kid->child));
>>>>>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>>>>>> index a24d0dd566e3..521f0a947ead 100644
>>>>>>> --- a/include/hw/qdev-core.h
>>>>>>> +++ b/include/hw/qdev-core.h
>>>>>>> @@ -206,6 +206,7 @@ struct BusState {
>>>>>>>         HotplugHandler *hotplug_handler;
>>>>>>>         int max_index;
>>>>>>>         bool realized;
>>>>>>> +    int num_children;
>>>>>>>         QTAILQ_HEAD(ChildrenHead, BusChild) children;
>>>>>>>         QLIST_ENTRY(BusState) sibling;
>>>>>>>     };
>>>>>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>>>>>> index 07147c63bf8b..45a8ba49644c 100644
>>>>>>> --- a/qdev-monitor.c
>>>>>>> +++ b/qdev-monitor.c
>>>>>>> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>>>>>>>     static inline bool qbus_is_full(BusState *bus)
>>>>>>>     {
>>>>>>>         BusClass *bus_class = BUS_GET_CLASS(bus);
>>>>>>> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
>>>>>>> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>>>>>>>     }
>>>>>>>     
>>>>>>>     /*
>>>>>>>         
>>>>>
>>>>> The approach the patch takes looks sane to me.
>>>>>       
>>>>   
>>>    
>>
>

Halil Pasic Jan. 9, 2019, 5:35 p.m. UTC | #9

On Wed, 9 Jan 2019 10:36:11 -0500
Tony Krowiak <[e-mail]> wrote:

> On 1/9/19 5:14 AM, Cornelia Huck wrote:
> > On Tue, 8 Jan 2019 15:34:37 -0500
> > Tony Krowiak <[e-mail]> wrote:
> > 
> >> On 1/8/19 12:06 PM, Cornelia Huck wrote:
> >>> On Tue, 8 Jan 2019 17:50:21 +0100
> >>> Halil Pasic <[e-mail]> wrote:
> >>>    
> >>>> On Tue, 8 Jan 2019 17:31:13 +0100
> >>>> Cornelia Huck <[e-mail]> wrote:
> >>>>   
> >>>>> On Tue, 8 Jan 2019 11:08:56 -0500
> >>>>> Tony Krowiak <[e-mail]> wrote:
> >>>>>       
> >>>>>> On 12/17/18 10:57 AM, Tony Krowiak wrote:
> >>>>>>> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> >>>>>>> value of the BusState structure with the max_dev value of the BusClass structure
> >>>>>>> to determine whether the maximum number of children has been reached for the
> >>>>>>> bus. The problem is, the max_index field of the BusState structure does not
> >>>>>>> necessarily reflect the number of devices that have been plugged into
> >>>>>>> the bus.
> >>>>>>>
> >>>>>>> Whenever a child device is plugged into the bus, the bus's max_index value is
> >>>>>>> assigned to the child device and then incremented. If the child is subsequently
> >>>>>>> unplugged, the value of the max_index does not change and no longer reflects the
> >>>>>>> number of children.
> >>>>>>>
> >>>>>>> When the bus's max_index value reaches the maximum number of devices
> >>>>>>> allowed for the bus (i.e., the max_dev field in the BusClass structure),
> >>>>>>> attempts to plug another device will be rejected claiming that the bus is
> >>>>>>> full -- even if the bus is actually empty.
> >>>>>>>
> >>>>>>> To resolve the problem, a new 'num_children' field is being added to the
> >>>>>>> BusState structure to keep track of the number of children plugged into the
> >>>>>>> bus. It will be incremented when a child is plugged, and decremented when a
> >>>>>>> child is unplugged.
> >>>>>>>
> >>>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
> >>>>>>> Reviewed-by: Pierre Morel<[e-mail]>
> >>>>>>> Reviewed-by: Halil Pasic <[e-mail]>
> >>>>>>> ---
> >>>>>>>     hw/core/qdev.c         | 3 +++
> >>>>>>>     include/hw/qdev-core.h | 1 +
> >>>>>>>     qdev-monitor.c         | 2 +-
> >>>>>>>     3 files changed, 5 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> Ping ...
> >>>>>> I could not determine who the maintainer is for the three files
> >>>>>> listed above. I checked the MAINTAINERS file and the prologue of each
> >>>>>> individual file. Can someone please tell me who is responsible
> >>>>>> for merging these changes? Any additional review comments?
> >>>>>>       
> >>>>>>>
> >>>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>>>>>> index 6b3cc55b27c2..956923f33520 100644
> >>>>>>> --- a/hw/core/qdev.c
> >>>>>>> +++ b/hw/core/qdev.c
> >>>>>>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
> >>>>>>>                 snprintf(name, sizeof(name), "child[%d]", kid->index);
> >>>>>>>                 QTAILQ_REMOVE(&bus->children, kid, sibling);
> >>>>>>>     
> >>>>>>> +            bus->num_children--;
> >>>>>>> +
> >>>>>>>                 /* This gives back ownership of kid->child back to us.  */
> >>>>>>>                 object_property_del(OBJECT(bus), name, NULL);
> >>>>>>>                 object_unref(OBJECT(kid->child));
> >>>>>>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> >>>>>>>         char name[32];
> >>>>>>>         BusChild *kid = g_malloc0(sizeof(*kid));
> >>>>>>>     
> >>>>>>> +    bus->num_children++;
> >>>>>>>         kid->index = bus->max_index++;
> >>>>>
> >>>>> Hm... I'm wondering what happens for insane numbers of hotplugging
> >>>>> operations here?
> >>>>>
> >>>>> (Preexisting problem for busses without limit, but busses with a limit
> >>>>> could now run into that as well.)
> >>>>>       
> >>>>
> >>>> How does this patch change things? I mean bus->num_children gets
> >>>> decremented on unplug.
> >>>
> >>> We don't stop anymore if max_index >= max_dev, which means that we can
> >>> now trigger that even if max_dev != 0.
> >>
> >> I guess I am missing your point. If max_dev == 0, then there is nothing
> >> stopping an insane number of hot plug operations; either before this
> >> patch, or with this patch. With the patch, once the number of children
> >> hot plugged reaches max_dev, the qbus_is_full function will return false
> >> and no more children can be plugged. If a child device is unplugged,
> >> the num_children - which counts the number of children attached to the
> >> bus - will be decremented, so it always reflects the number of children
> >> added to the bus. Besides, checking max_index against max_dev
> >> is erroneous, because max_index is incremented every time a child device
> >> is plugged and is never decremented. It really operates as more of a
> >> uinique identifier than a counter and is also used to create a unique
> >> property name when the child device is linked to the bus as a property
> >> (see bus_add_child function in hw/core/qdev.c).
> > 
> > Checking num_children against max_dev is the right thing to do, no
> > argument here.
> > 
> > However, max_index continues to be incremented even if devices have
> > been unplugged again. That means it can overflow, as it is never bound
> > by the max_dev constraint.
> > 
> > This has been a problem before for busses with an unrestricted number of
> > devices before, but with your patch, the problem is now triggerable for
> > all busses.
> > 
> > Not a problem with your patch, but we might want to look into making
> > max_index overflow/wraparound save.
> 
> I see your point. It does beg the question, what are the chances that
> max_index reaches INT_MAX? In the interest of making this code more
> bullet proof, I suppose it is something that should be dealt with.
> 
> A search reveals that max_index is used in only two places: It is used
> to set the index for a child of the bus (i.e., bus_add_child function in
> hw/core/qdev.c); and prior to this patch, to determine if max_dev has
> been exceeded (i.e., qbus_is_full function in qdev_monitor.c). From
> what I can see, the bus child index is used only to generate a property
> name of the format "child[%d]" so the child can be linked as a property
> of the bus (see bus_add_child and bus_remove_child functions in
> hw/core/qdev.c). Wraparound of the max_index would most likely result in
> generating a duplicate property name for the child.
> 
> I propose two possible solutions:
> 
> 1. When max_index reaches INT_MAX, do not allow any additional children
>     to be added to the bus.
> 
> 2. Set a max_dev value of INT_MAX for the BusClass instance if the value
>     is not set (in the bus_class_init function in hw/core/bus.c).
> 
> 3. Instead of generating the property name from the BusChild index
>     value, generate a UUID string. Since the index field of the BusChild
>     appears to be used only to generate the child's name, maybe change
>     the index field to a UUID field or a name field.
> 

Separate problem, separate patch, or?

UUID instead of index solves the problem of unique names I guess, but I
can't tell if that would be acceptable (interface stability, etc.).

The max_dev won't help because we can get a collision while maintaining
the invariant 'not more than 2 devices on the bus'.

So if we don't want to limit the number of hotplug operations, and we do
want to keep the allocation scheme before wrapping, the only solution I
see is looking for the first free identifier after we wrap.

BTW I wonder if making max_index and index unsigned make things bit less
ugly.

Regards,
Halil

Anthony Krowiak Jan. 10, 2019, 3:50 p.m. UTC | #10

On 1/9/19 12:35 PM, Halil Pasic wrote:
> On Wed, 9 Jan 2019 10:36:11 -0500
> Tony Krowiak <[e-mail]> wrote:
> 
>> On 1/9/19 5:14 AM, Cornelia Huck wrote:
>>> On Tue, 8 Jan 2019 15:34:37 -0500
>>> Tony Krowiak <[e-mail]> wrote:
>>>
>>>> On 1/8/19 12:06 PM, Cornelia Huck wrote:
>>>>> On Tue, 8 Jan 2019 17:50:21 +0100
>>>>> Halil Pasic <[e-mail]> wrote:
>>>>>     
>>>>>> On Tue, 8 Jan 2019 17:31:13 +0100
>>>>>> Cornelia Huck <[e-mail]> wrote:
>>>>>>    
>>>>>>> On Tue, 8 Jan 2019 11:08:56 -0500
>>>>>>> Tony Krowiak <[e-mail]> wrote:
>>>>>>>        
>>>>>>>> On 12/17/18 10:57 AM, Tony Krowiak wrote:
>>>>>>>>> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
>>>>>>>>> value of the BusState structure with the max_dev value of the BusClass structure
>>>>>>>>> to determine whether the maximum number of children has been reached for the
>>>>>>>>> bus. The problem is, the max_index field of the BusState structure does not
>>>>>>>>> necessarily reflect the number of devices that have been plugged into
>>>>>>>>> the bus.
>>>>>>>>>
>>>>>>>>> Whenever a child device is plugged into the bus, the bus's max_index value is
>>>>>>>>> assigned to the child device and then incremented. If the child is subsequently
>>>>>>>>> unplugged, the value of the max_index does not change and no longer reflects the
>>>>>>>>> number of children.
>>>>>>>>>
>>>>>>>>> When the bus's max_index value reaches the maximum number of devices
>>>>>>>>> allowed for the bus (i.e., the max_dev field in the BusClass structure),
>>>>>>>>> attempts to plug another device will be rejected claiming that the bus is
>>>>>>>>> full -- even if the bus is actually empty.
>>>>>>>>>
>>>>>>>>> To resolve the problem, a new 'num_children' field is being added to the
>>>>>>>>> BusState structure to keep track of the number of children plugged into the
>>>>>>>>> bus. It will be incremented when a child is plugged, and decremented when a
>>>>>>>>> child is unplugged.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>>>>>>> Reviewed-by: Pierre Morel<[e-mail]>
>>>>>>>>> Reviewed-by: Halil Pasic <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>      hw/core/qdev.c         | 3 +++
>>>>>>>>>      include/hw/qdev-core.h | 1 +
>>>>>>>>>      qdev-monitor.c         | 2 +-
>>>>>>>>>      3 files changed, 5 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> Ping ...
>>>>>>>> I could not determine who the maintainer is for the three files
>>>>>>>> listed above. I checked the MAINTAINERS file and the prologue of each
>>>>>>>> individual file. Can someone please tell me who is responsible
>>>>>>>> for merging these changes? Any additional review comments?
>>>>>>>>        
>>>>>>>>>
>>>>>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>>>>>> index 6b3cc55b27c2..956923f33520 100644
>>>>>>>>> --- a/hw/core/qdev.c
>>>>>>>>> +++ b/hw/core/qdev.c
>>>>>>>>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>>>>>>>>>                  snprintf(name, sizeof(name), "child[%d]", kid->index);
>>>>>>>>>                  QTAILQ_REMOVE(&bus->children, kid, sibling);
>>>>>>>>>      
>>>>>>>>> +            bus->num_children--;
>>>>>>>>> +
>>>>>>>>>                  /* This gives back ownership of kid->child back to us.  */
>>>>>>>>>                  object_property_del(OBJECT(bus), name, NULL);
>>>>>>>>>                  object_unref(OBJECT(kid->child));
>>>>>>>>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>>>>>>>>>          char name[32];
>>>>>>>>>          BusChild *kid = g_malloc0(sizeof(*kid));
>>>>>>>>>      
>>>>>>>>> +    bus->num_children++;
>>>>>>>>>          kid->index = bus->max_index++;
>>>>>>>
>>>>>>> Hm... I'm wondering what happens for insane numbers of hotplugging
>>>>>>> operations here?
>>>>>>>
>>>>>>> (Preexisting problem for busses without limit, but busses with a limit
>>>>>>> could now run into that as well.)
>>>>>>>        
>>>>>>
>>>>>> How does this patch change things? I mean bus->num_children gets
>>>>>> decremented on unplug.
>>>>>
>>>>> We don't stop anymore if max_index >= max_dev, which means that we can
>>>>> now trigger that even if max_dev != 0.
>>>>
>>>> I guess I am missing your point. If max_dev == 0, then there is nothing
>>>> stopping an insane number of hot plug operations; either before this
>>>> patch, or with this patch. With the patch, once the number of children
>>>> hot plugged reaches max_dev, the qbus_is_full function will return false
>>>> and no more children can be plugged. If a child device is unplugged,
>>>> the num_children - which counts the number of children attached to the
>>>> bus - will be decremented, so it always reflects the number of children
>>>> added to the bus. Besides, checking max_index against max_dev
>>>> is erroneous, because max_index is incremented every time a child device
>>>> is plugged and is never decremented. It really operates as more of a
>>>> uinique identifier than a counter and is also used to create a unique
>>>> property name when the child device is linked to the bus as a property
>>>> (see bus_add_child function in hw/core/qdev.c).
>>>
>>> Checking num_children against max_dev is the right thing to do, no
>>> argument here.
>>>
>>> However, max_index continues to be incremented even if devices have
>>> been unplugged again. That means it can overflow, as it is never bound
>>> by the max_dev constraint.
>>>
>>> This has been a problem before for busses with an unrestricted number of
>>> devices before, but with your patch, the problem is now triggerable for
>>> all busses.
>>>
>>> Not a problem with your patch, but we might want to look into making
>>> max_index overflow/wraparound save.
>>
>> I see your point. It does beg the question, what are the chances that
>> max_index reaches INT_MAX? In the interest of making this code more
>> bullet proof, I suppose it is something that should be dealt with.
>>
>> A search reveals that max_index is used in only two places: It is used
>> to set the index for a child of the bus (i.e., bus_add_child function in
>> hw/core/qdev.c); and prior to this patch, to determine if max_dev has
>> been exceeded (i.e., qbus_is_full function in qdev_monitor.c). From
>> what I can see, the bus child index is used only to generate a property
>> name of the format "child[%d]" so the child can be linked as a property
>> of the bus (see bus_add_child and bus_remove_child functions in
>> hw/core/qdev.c). Wraparound of the max_index would most likely result in
>> generating a duplicate property name for the child.
>>
>> I propose two possible solutions:
>>
>> 1. When max_index reaches INT_MAX, do not allow any additional children
>>      to be added to the bus.
>>
>> 2. Set a max_dev value of INT_MAX for the BusClass instance if the value
>>      is not set (in the bus_class_init function in hw/core/bus.c).
>>
>> 3. Instead of generating the property name from the BusChild index
>>      value, generate a UUID string. Since the index field of the BusChild
>>      appears to be used only to generate the child's name, maybe change
>>      the index field to a UUID field or a name field.
>>
> 
> Separate problem, separate patch, or?

Good question.

> 
> UUID instead of index solves the problem of unique names I guess, but I
> can't tell if that would be acceptable (interface stability, etc.).

I may have missed something, but as currently used, the BusChild index
is accessed in only two places; when the child is added to the bus and
when it is removed from the bus. In both cases, it is used to derive
a unique property name for the child device.

Speaking of index, it implies ordering of the bus's children. IMHO, it
only makes semantical sense if the index changes when a child device
with a lower index value is removed from the bus. If a child device
has an index of 5 - i.e., the fifth child on the bus - and the child
device with index 4 is removed, then the child device with index 5 is
no longer the fifth child on the bus. Maybe its just the fact that
these fields are named 'index'. The fact that they are not really used
as indexes caused a bit of confusion for me when analyzing this code and
seems like a misnomer to me.

> 
> The max_dev won't help because we can get a collision while maintaining
> the invariant 'not more than 2 devices on the bus'.

I don't understand, can you better explain what you mean? When you
say "we can get a collision", are you talking about the property
name? What does max_dev have to do with that? Please explain. What do
you mean by "maintaining the invariant 'not more than 2 devices on the
bus'"?

> 
> So if we don't want to limit the number of hotplug operations, and we do
> want to keep the allocation scheme before wrapping, the only solution I
> see is looking for the first free identifier after we wrap.

Are you are saying to look for the first index value that is not
assigned to a BusChild object?

> 
> BTW I wonder if making max_index and index unsigned make things bit less
> ugly.

I thought the same. They could also be made unsigned long or
unsigned long long to increase the number of child devices that can be
plugged in before having to deal with exceeding the index value.

> 
> Regards,
> Halil
>

Cornelia Huck Jan. 10, 2019, 4:57 p.m. UTC | #11

On Thu, 10 Jan 2019 10:50:30 -0500
Tony Krowiak <[e-mail]> wrote:

> On 1/9/19 12:35 PM, Halil Pasic wrote:
> > On Wed, 9 Jan 2019 10:36:11 -0500
> > Tony Krowiak <[e-mail]> wrote:
> >   
> >> On 1/9/19 5:14 AM, Cornelia Huck wrote:  
> >>> On Tue, 8 Jan 2019 15:34:37 -0500
> >>> Tony Krowiak <[e-mail]> wrote:
> >>>  
> >>>> On 1/8/19 12:06 PM, Cornelia Huck wrote:  
> >>>>> On Tue, 8 Jan 2019 17:50:21 +0100
> >>>>> Halil Pasic <[e-mail]> wrote:
> >>>>>       
> >>>>>> On Tue, 8 Jan 2019 17:31:13 +0100
> >>>>>> Cornelia Huck <[e-mail]> wrote:
> >>>>>>      
> >>>>>>> On Tue, 8 Jan 2019 11:08:56 -0500
> >>>>>>> Tony Krowiak <[e-mail]> wrote:
> >>>>>>>          
> >>>>>>>> On 12/17/18 10:57 AM, Tony Krowiak wrote:  

> >>>>>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>>>>>>>> index 6b3cc55b27c2..956923f33520 100644
> >>>>>>>>> --- a/hw/core/qdev.c
> >>>>>>>>> +++ b/hw/core/qdev.c
> >>>>>>>>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
> >>>>>>>>>                  snprintf(name, sizeof(name), "child[%d]", kid->index);
> >>>>>>>>>                  QTAILQ_REMOVE(&bus->children, kid, sibling);
> >>>>>>>>>      
> >>>>>>>>> +            bus->num_children--;
> >>>>>>>>> +
> >>>>>>>>>                  /* This gives back ownership of kid->child back to us.  */
> >>>>>>>>>                  object_property_del(OBJECT(bus), name, NULL);
> >>>>>>>>>                  object_unref(OBJECT(kid->child));
> >>>>>>>>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> >>>>>>>>>          char name[32];
> >>>>>>>>>          BusChild *kid = g_malloc0(sizeof(*kid));
> >>>>>>>>>      
> >>>>>>>>> +    bus->num_children++;
> >>>>>>>>>          kid->index = bus->max_index++;  
> >>>>>>>
> >>>>>>> Hm... I'm wondering what happens for insane numbers of hotplugging
> >>>>>>> operations here?
> >>>>>>>
> >>>>>>> (Preexisting problem for busses without limit, but busses with a limit
> >>>>>>> could now run into that as well.)
> >>>>>>>          
> >>>>>>
> >>>>>> How does this patch change things? I mean bus->num_children gets
> >>>>>> decremented on unplug.  
> >>>>>
> >>>>> We don't stop anymore if max_index >= max_dev, which means that we can
> >>>>> now trigger that even if max_dev != 0.  
> >>>>
> >>>> I guess I am missing your point. If max_dev == 0, then there is nothing
> >>>> stopping an insane number of hot plug operations; either before this
> >>>> patch, or with this patch. With the patch, once the number of children
> >>>> hot plugged reaches max_dev, the qbus_is_full function will return false
> >>>> and no more children can be plugged. If a child device is unplugged,
> >>>> the num_children - which counts the number of children attached to the
> >>>> bus - will be decremented, so it always reflects the number of children
> >>>> added to the bus. Besides, checking max_index against max_dev
> >>>> is erroneous, because max_index is incremented every time a child device
> >>>> is plugged and is never decremented. It really operates as more of a
> >>>> uinique identifier than a counter and is also used to create a unique
> >>>> property name when the child device is linked to the bus as a property
> >>>> (see bus_add_child function in hw/core/qdev.c).  
> >>>
> >>> Checking num_children against max_dev is the right thing to do, no
> >>> argument here.
> >>>
> >>> However, max_index continues to be incremented even if devices have
> >>> been unplugged again. That means it can overflow, as it is never bound
> >>> by the max_dev constraint.
> >>>
> >>> This has been a problem before for busses with an unrestricted number of
> >>> devices before, but with your patch, the problem is now triggerable for
> >>> all busses.
> >>>
> >>> Not a problem with your patch, but we might want to look into making
> >>> max_index overflow/wraparound save.  
> >>
> >> I see your point. It does beg the question, what are the chances that
> >> max_index reaches INT_MAX? In the interest of making this code more
> >> bullet proof, I suppose it is something that should be dealt with.

Yes, there's a low chance of hitting this problem during normal
operation, but you could do scripted plug/unplug to reach the limit
(and test a possible fix).

> >>
> >> A search reveals that max_index is used in only two places: It is used
> >> to set the index for a child of the bus (i.e., bus_add_child function in
> >> hw/core/qdev.c); and prior to this patch, to determine if max_dev has
> >> been exceeded (i.e., qbus_is_full function in qdev_monitor.c). From
> >> what I can see, the bus child index is used only to generate a property
> >> name of the format "child[%d]" so the child can be linked as a property
> >> of the bus (see bus_add_child and bus_remove_child functions in
> >> hw/core/qdev.c). Wraparound of the max_index would most likely result in
> >> generating a duplicate property name for the child.
> >>
> >> I propose two possible solutions:
> >>
> >> 1. When max_index reaches INT_MAX, do not allow any additional children
> >>      to be added to the bus.
> >>
> >> 2. Set a max_dev value of INT_MAX for the BusClass instance if the value
> >>      is not set (in the bus_class_init function in hw/core/bus.c).
> >>
> >> 3. Instead of generating the property name from the BusChild index
> >>      value, generate a UUID string. Since the index field of the BusChild
> >>      appears to be used only to generate the child's name, maybe change
> >>      the index field to a UUID field or a name field.
> >>  
> > 
> > Separate problem, separate patch, or?  
> 
> Good question.

I'd say it's definitely something that can be fixed separately, no need
to hold up this patch.

> 
> > 
> > UUID instead of index solves the problem of unique names I guess, but I
> > can't tell if that would be acceptable (interface stability, etc.).  
> 
> I may have missed something, but as currently used, the BusChild index
> is accessed in only two places; when the child is added to the bus and
> when it is removed from the bus. In both cases, it is used to derive
> a unique property name for the child device.
> 
> Speaking of index, it implies ordering of the bus's children. IMHO, it
> only makes semantical sense if the index changes when a child device
> with a lower index value is removed from the bus. If a child device
> has an index of 5 - i.e., the fifth child on the bus - and the child
> device with index 4 is removed, then the child device with index 5 is
> no longer the fifth child on the bus. Maybe its just the fact that
> these fields are named 'index'. The fact that they are not really used
> as indexes caused a bit of confusion for me when analyzing this code and
> seems like a misnomer to me.

Maybe the index tries to imply the order when they were added to the
bus. But we probably should not read too much into the name; I would
mainly expect them to use unique identifiers.

> 
> > 
> > The max_dev won't help because we can get a collision while maintaining
> > the invariant 'not more than 2 devices on the bus'.  
> 
> I don't understand, can you better explain what you mean? When you
> say "we can get a collision", are you talking about the property
> name? What does max_dev have to do with that? Please explain. What do
> you mean by "maintaining the invariant 'not more than 2 devices on the
> bus'"?
> 
> > 
> > So if we don't want to limit the number of hotplug operations, and we do
> > want to keep the allocation scheme before wrapping, the only solution I
> > see is looking for the first free identifier after we wrap.  
> 
> Are you are saying to look for the first index value that is not
> assigned to a BusChild object?

This seems to be the most sane solution.

> 
> > 
> > BTW I wonder if making max_index and index unsigned make things bit less
> > ugly.  
> 
> I thought the same. They could also be made unsigned long or
> unsigned long long to increase the number of child devices that can be
> plugged in before having to deal with exceeding the index value.

Making them unsigned long long would push the problem out far enough to
be irrelevant in practice. Not sure if we care about fixing it
completely, though.

Halil Pasic Jan. 11, 2019, 10:21 a.m. UTC | #12

On Thu, 10 Jan 2019 10:50:30 -0500
Tony Krowiak <[e-mail]> wrote:

> On 1/9/19 12:35 PM, Halil Pasic wrote:
> > On Wed, 9 Jan 2019 10:36:11 -0500
> > Tony Krowiak <[e-mail]> wrote:
> > 
> >> On 1/9/19 5:14 AM, Cornelia Huck wrote:

[..]

> >> A search reveals that max_index is used in only two places: It is used
> >> to set the index for a child of the bus (i.e., bus_add_child function in
> >> hw/core/qdev.c); and prior to this patch, to determine if max_dev has
> >> been exceeded (i.e., qbus_is_full function in qdev_monitor.c). From
> >> what I can see, the bus child index is used only to generate a property
> >> name of the format "child[%d]" so the child can be linked as a property
> >> of the bus (see bus_add_child and bus_remove_child functions in
> >> hw/core/qdev.c). Wraparound of the max_index would most likely result in
> >> generating a duplicate property name for the child.
> >>
> >> I propose two possible solutions:
> >>
> >> 1. When max_index reaches INT_MAX, do not allow any additional children
> >>      to be added to the bus.
> >>
> >> 2. Set a max_dev value of INT_MAX for the BusClass instance if the value
> >>      is not set (in the bus_class_init function in hw/core/bus.c).
> >>
> >> 3. Instead of generating the property name from the BusChild index
> >>      value, generate a UUID string. Since the index field of the BusChild
> >>      appears to be used only to generate the child's name, maybe change
> >>      the index field to a UUID field or a name field.
> >>
> > 
> > Separate problem, separate patch, or?
> 
> Good question.
>

IMHO this patch should not be delayed because of the mostly unrelated
discussion on max_index wrapping. BTW Tony do you want to do a patch on
max_index?

> > 
> > UUID instead of index solves the problem of unique names I guess, but I
> > can't tell if that would be acceptable (interface stability, etc.).
> 
> I may have missed something, but as currently used, the BusChild index
> is accessed in only two places; when the child is added to the bus and
> when it is removed from the bus. In both cases, it is used to derive
> a unique property name for the child device.
> 

The name is probably externally observable (via QMP). It could be also a
stable part of an (external) API. We would need damn good reasons to
break an external API.

> Speaking of index, it implies ordering of the bus's children. IMHO, it
> only makes semantical sense if the index changes when a child device
> with a lower index value is removed from the bus. If a child device
> has an index of 5 - i.e., the fifth child on the bus - and the child
> device with index 4 is removed, then the child device with index 5 is
> no longer the fifth child on the bus. Maybe its just the fact that
> these fields are named 'index'. The fact that they are not really used
> as indexes caused a bit of confusion for me when analyzing this code and
> seems like a misnomer to me.
> 

No comment.

> > 
> > The max_dev won't help because we can get a collision while maintaining
> > the invariant 'not more than 2 devices on the bus'.
> 
> I don't understand, can you better explain what you mean? When you
> say "we can get a collision", are you talking about the property
> name? What does max_dev have to do with that? Please explain. 

AFAIU the whole point of the exercise with max_index is to generate bus
unique names. By we can get a collision (please see
https://en.oxforddictionaries.com/definition/collision), I mean we can
end up assigning the same name to more than one device on the very same
bus.

> What do
> you mean by "maintaining the invariant 'not more than 2 devices on the
> bus'"?
> 

Let me describe the scenario. Let's have a bus with dev_max == 2. We
first add one device, then another to that bus. Then we unplug and replug
the second device. The name goes from 'child[n]' to 'child[n+1]' with
each iteration of unplug/replug with int arithmetic. That is after a
number of iterations we will reach the name 'child[0]'. But we already
have a child[0] on the bus.

> > 
> > So if we don't want to limit the number of hotplug operations, and we do
> > want to keep the allocation scheme before wrapping, the only solution I
> > see is looking for the first free identifier after we wrap.
> 
> Are you are saying to look for the first index value that is not
> assigned to a BusChild object?
> 

Only after we reach the biggest integer. We want to keep everything as
is for the cases that work today.

> > 
> > BTW I wonder if making max_index and index unsigned make things bit less
> > ugly.
> 
> I thought the same. They could also be made unsigned long or
> unsigned long long to increase the number of child devices that can be
> plugged in before having to deal with exceeding the index value.
> 

My rationale is: names with the negatives would look weird.

Regards,
Halil

Halil Pasic Jan. 11, 2019, 10:31 a.m. UTC | #13

On Thu, 10 Jan 2019 17:57:22 +0100
Cornelia Huck <[e-mail]> wrote:

> > I thought the same. They could also be made unsigned long or
> > unsigned long long to increase the number of child devices that can be
> > plugged in before having to deal with exceeding the index value.  
> 
> Making them unsigned long long would push the problem out far enough to
> be irrelevant in practice. Not sure if we care about fixing it
> completely, though.

My intuition says that INT_MAX hotplug's on a bus is already an
'academic' thing. The rationale behind asking about unsigned is that
I would consider something like 'child[-42]' weird.

My intuition was that this is something that the community considers
not important enough to invest in. If we don't want to leave it as is, I
would prefer some proper fix (explicit limit on the number of hotplug
operations, or scanning for the first free one).

Regards,
Halil

Anthony Krowiak Jan. 28, 2019, 8:35 p.m. UTC | #14

On 12/17/18 10:57 AM, Tony Krowiak wrote:
> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> value of the BusState structure with the max_dev value of the BusClass structure
> to determine whether the maximum number of children has been reached for the
> bus. The problem is, the max_index field of the BusState structure does not
> necessarily reflect the number of devices that have been plugged into
> the bus.
> 
> Whenever a child device is plugged into the bus, the bus's max_index value is
> assigned to the child device and then incremented. If the child is subsequently
> unplugged, the value of the max_index does not change and no longer reflects the
> number of children.
> 
> When the bus's max_index value reaches the maximum number of devices
> allowed for the bus (i.e., the max_dev field in the BusClass structure),
> attempts to plug another device will be rejected claiming that the bus is
> full -- even if the bus is actually empty.
> 
> To resolve the problem, a new 'num_children' field is being added to the
> BusState structure to keep track of the number of children plugged into the
> bus. It will be incremented when a child is plugged, and decremented when a
> child is unplugged.
> 
> Signed-off-by: Tony Krowiak <[e-mail]>
> Reviewed-by: Pierre Morel<[e-mail]>
> Reviewed-by: Halil Pasic <[e-mail]>
> ---
>   hw/core/qdev.c         | 3 +++
>   include/hw/qdev-core.h | 1 +
>   qdev-monitor.c         | 2 +-
>   3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55b27c2..956923f33520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>               snprintf(name, sizeof(name), "child[%d]", kid->index);
>               QTAILQ_REMOVE(&bus->children, kid, sibling);
>   
> +            bus->num_children--;
> +
>               /* This gives back ownership of kid->child back to us.  */
>               object_property_del(OBJECT(bus), name, NULL);
>               object_unref(OBJECT(kid->child));
> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>       char name[32];
>       BusChild *kid = g_malloc0(sizeof(*kid));
>   
> +    bus->num_children++;
>       kid->index = bus->max_index++;
>       kid->child = child;
>       object_ref(OBJECT(kid->child));
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566e3..521f0a947ead 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -206,6 +206,7 @@ struct BusState {
>       HotplugHandler *hotplug_handler;
>       int max_index;
>       bool realized;
> +    int num_children;
>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>       QLIST_ENTRY(BusState) sibling;
>   };
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 07147c63bf8b..45a8ba49644c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>   static inline bool qbus_is_full(BusState *bus)
>   {
>       BusClass *bus_class = BUS_GET_CLASS(bus);
> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>   }
>   
>   /*

Just checking back on this one. Do we want to merge this patch and deal
with the max_index issue in another patch, in this patch, or not at all?

>

Igor Mammedov Feb. 6, 2019, 8:34 a.m. UTC | #15

On Mon, 28 Jan 2019 15:35:09 -0500
Tony Krowiak <[e-mail]> wrote:

> On 12/17/18 10:57 AM, Tony Krowiak wrote:
> > The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> > value of the BusState structure with the max_dev value of the BusClass structure
> > to determine whether the maximum number of children has been reached for the
> > bus. The problem is, the max_index field of the BusState structure does not
> > necessarily reflect the number of devices that have been plugged into
> > the bus.
> > 
> > Whenever a child device is plugged into the bus, the bus's max_index value is
> > assigned to the child device and then incremented. If the child is subsequently
> > unplugged, the value of the max_index does not change and no longer reflects the
> > number of children.
> > 
> > When the bus's max_index value reaches the maximum number of devices
> > allowed for the bus (i.e., the max_dev field in the BusClass structure),
> > attempts to plug another device will be rejected claiming that the bus is
> > full -- even if the bus is actually empty.
> > 
> > To resolve the problem, a new 'num_children' field is being added to the
> > BusState structure to keep track of the number of children plugged into the
> > bus. It will be incremented when a child is plugged, and decremented when a
> > child is unplugged.
> > 
> > Signed-off-by: Tony Krowiak <[e-mail]>
> > Reviewed-by: Pierre Morel<[e-mail]>
> > Reviewed-by: Halil Pasic <[e-mail]>
> > ---
> >   hw/core/qdev.c         | 3 +++
> >   include/hw/qdev-core.h | 1 +
> >   qdev-monitor.c         | 2 +-
> >   3 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 6b3cc55b27c2..956923f33520 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
> >               snprintf(name, sizeof(name), "child[%d]", kid->index);
> >               QTAILQ_REMOVE(&bus->children, kid, sibling);
> >   
> > +            bus->num_children--;
> > +
> >               /* This gives back ownership of kid->child back to us.  */
> >               object_property_del(OBJECT(bus), name, NULL);
> >               object_unref(OBJECT(kid->child));
> > @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> >       char name[32];
> >       BusChild *kid = g_malloc0(sizeof(*kid));
> >   
> > +    bus->num_children++;
> >       kid->index = bus->max_index++;
> >       kid->child = child;
> >       object_ref(OBJECT(kid->child));
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index a24d0dd566e3..521f0a947ead 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -206,6 +206,7 @@ struct BusState {
> >       HotplugHandler *hotplug_handler;
> >       int max_index;
> >       bool realized;
> > +    int num_children;
> >       QTAILQ_HEAD(ChildrenHead, BusChild) children;
> >       QLIST_ENTRY(BusState) sibling;
> >   };
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 07147c63bf8b..45a8ba49644c 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
> >   static inline bool qbus_is_full(BusState *bus)
> >   {
> >       BusClass *bus_class = BUS_GET_CLASS(bus);
> > -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> > +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
> >   }
> >   
> >   /*
> 
> Just checking back on this one. Do we want to merge this patch and deal
> with the max_index issue in another patch, in this patch, or not at all?
I think there were consensus that max_index was a separate problem that should
be addressed by another patch.

Maybe Paolo (CCed) could take this generic patch.

Anthony Krowiak Feb. 18, 2019, 5:02 p.m. UTC | #16

On 2/6/19 3:34 AM, Igor Mammedov wrote:
> On Mon, 28 Jan 2019 15:35:09 -0500
> Tony Krowiak <[e-mail]> wrote:

ping!

Who is the maintainer responsible for merging this?

Eduardo Habkost Feb. 28, 2019, 5:17 p.m. UTC | #17

On Mon, Dec 17, 2018 at 10:57:30AM -0500, Tony Krowiak wrote:
> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> value of the BusState structure with the max_dev value of the BusClass structure
> to determine whether the maximum number of children has been reached for the
> bus. The problem is, the max_index field of the BusState structure does not
> necessarily reflect the number of devices that have been plugged into
> the bus.
> 
> Whenever a child device is plugged into the bus, the bus's max_index value is
> assigned to the child device and then incremented. If the child is subsequently
> unplugged, the value of the max_index does not change and no longer reflects the
> number of children.
> 
> When the bus's max_index value reaches the maximum number of devices
> allowed for the bus (i.e., the max_dev field in the BusClass structure),
> attempts to plug another device will be rejected claiming that the bus is
> full -- even if the bus is actually empty.
> 
> To resolve the problem, a new 'num_children' field is being added to the
> BusState structure to keep track of the number of children plugged into the
> bus. It will be incremented when a child is plugged, and decremented when a
> child is unplugged.
> 
> Signed-off-by: Tony Krowiak <[e-mail]>
> Reviewed-by: Pierre Morel<[e-mail]>
> Reviewed-by: Halil Pasic <[e-mail]>

Queued on machine-next, thanks!

Anthony Krowiak March 4, 2019, 5:35 p.m. UTC | #18

On 12/17/18 10:57 AM, Tony Krowiak wrote:

Ping!! I'm wondering who might be responsible for merging this fix?

> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> value of the BusState structure with the max_dev value of the BusClass structure
> to determine whether the maximum number of children has been reached for the
> bus. The problem is, the max_index field of the BusState structure does not
> necessarily reflect the number of devices that have been plugged into
> the bus.
> 
> Whenever a child device is plugged into the bus, the bus's max_index value is
> assigned to the child device and then incremented. If the child is subsequently
> unplugged, the value of the max_index does not change and no longer reflects the
> number of children.
> 
> When the bus's max_index value reaches the maximum number of devices
> allowed for the bus (i.e., the max_dev field in the BusClass structure),
> attempts to plug another device will be rejected claiming that the bus is
> full -- even if the bus is actually empty.
> 
> To resolve the problem, a new 'num_children' field is being added to the
> BusState structure to keep track of the number of children plugged into the
> bus. It will be incremented when a child is plugged, and decremented when a
> child is unplugged.
> 
> Signed-off-by: Tony Krowiak <[e-mail]>
> Reviewed-by: Pierre Morel<[e-mail]>
> Reviewed-by: Halil Pasic <[e-mail]>
> ---
>   hw/core/qdev.c         | 3 +++
>   include/hw/qdev-core.h | 1 +
>   qdev-monitor.c         | 2 +-
>   3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55b27c2..956923f33520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>               snprintf(name, sizeof(name), "child[%d]", kid->index);
>               QTAILQ_REMOVE(&bus->children, kid, sibling);
>   
> +            bus->num_children--;
> +
>               /* This gives back ownership of kid->child back to us.  */
>               object_property_del(OBJECT(bus), name, NULL);
>               object_unref(OBJECT(kid->child));
> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>       char name[32];
>       BusChild *kid = g_malloc0(sizeof(*kid));
>   
> +    bus->num_children++;
>       kid->index = bus->max_index++;
>       kid->child = child;
>       object_ref(OBJECT(kid->child));
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566e3..521f0a947ead 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -206,6 +206,7 @@ struct BusState {
>       HotplugHandler *hotplug_handler;
>       int max_index;
>       bool realized;
> +    int num_children;
>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>       QLIST_ENTRY(BusState) sibling;
>   };
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 07147c63bf8b..45a8ba49644c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>   static inline bool qbus_is_full(BusState *bus)
>   {
>       BusClass *bus_class = BUS_GET_CLASS(bus);
> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>   }
>   
>   /*
>

Pierre Morel March 5, 2019, 8:01 a.m. UTC | #19

On 04/03/2019 18:35, Tony Krowiak wrote:
> On 12/17/18 10:57 AM, Tony Krowiak wrote:
> 
> Ping!! I'm wondering who might be responsible for merging this fix?

To alert the maintainers, you must send the patch with putting them or 
part of them in CC and put the qemu mailing list in CC too.

use get_maintainer.pl like:

$ ./scripts/get_maintainer.pl -f hw/core/qdev.c
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches!  Use common sense.

Markus Armbruster <[e-mail]> (commit_signer:4/6=67%)
Paolo Bonzini <[e-mail]> (commit_signer:3/6=50%)
Stefan Hajnoczi <[e-mail]> (commit_signer:2/6=33%)
Peter Xu <[e-mail]> (commit_signer:1/6=17%)
"Philippe Mathieu-Daudé" <[e-mail]> (commit_signer:1/6=17%)
[e-mail] (open list:All patches CC here)


Regards,
Pierre


> 
>> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the 
>> max_index
>> value of the BusState structure with the max_dev value of the BusClass 
>> structure
>> to determine whether the maximum number of children has been reached 
>> for the
>> bus. The problem is, the max_index field of the BusState structure 
>> does not
>> necessarily reflect the number of devices that have been plugged into
>> the bus.
>>
>> Whenever a child device is plugged into the bus, the bus's max_index 
>> value is
>> assigned to the child device and then incremented. If the child is 
>> subsequently
>> unplugged, the value of the max_index does not change and no longer 
>> reflects the
>> number of children.
>>
>> When the bus's max_index value reaches the maximum number of devices
>> allowed for the bus (i.e., the max_dev field in the BusClass structure),
>> attempts to plug another device will be rejected claiming that the bus is
>> full -- even if the bus is actually empty.
>>
>> To resolve the problem, a new 'num_children' field is being added to the
>> BusState structure to keep track of the number of children plugged 
>> into the
>> bus. It will be incremented when a child is plugged, and decremented 
>> when a
>> child is unplugged.
>>
>> Signed-off-by: Tony Krowiak <[e-mail]>
>> Reviewed-by: Pierre Morel<[e-mail]>
>> Reviewed-by: Halil Pasic <[e-mail]>
>> ---
>>   hw/core/qdev.c         | 3 +++
>>   include/hw/qdev-core.h | 1 +
>>   qdev-monitor.c         | 2 +-
>>   3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 6b3cc55b27c2..956923f33520 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, 
>> DeviceState *child)
>>               snprintf(name, sizeof(name), "child[%d]", kid->index);
>>               QTAILQ_REMOVE(&bus->children, kid, sibling);
>> +            bus->num_children--;
>> +
>>               /* This gives back ownership of kid->child back to us.  */
>>               object_property_del(OBJECT(bus), name, NULL);
>>               object_unref(OBJECT(kid->child));
>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState 
>> *child)
>>       char name[32];
>>       BusChild *kid = g_malloc0(sizeof(*kid));
>> +    bus->num_children++;
>>       kid->index = bus->max_index++;
>>       kid->child = child;
>>       object_ref(OBJECT(kid->child));
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index a24d0dd566e3..521f0a947ead 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -206,6 +206,7 @@ struct BusState {
>>       HotplugHandler *hotplug_handler;
>>       int max_index;
>>       bool realized;
>> +    int num_children;
>>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>>       QLIST_ENTRY(BusState) sibling;
>>   };
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 07147c63bf8b..45a8ba49644c 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, 
>> char *elem)
>>   static inline bool qbus_is_full(BusState *bus)
>>   {
>>       BusClass *bus_class = BUS_GET_CLASS(bus);
>> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
>> +    return bus_class->max_dev && bus->num_children >= 
>> bus_class->max_dev;
>>   }
>>   /*
>>
>

Igor Mammedov March 5, 2019, 8:28 a.m. UTC | #20

On Mon, 4 Mar 2019 12:35:32 -0500
Tony Krowiak <[e-mail]> wrote:

> On 12/17/18 10:57 AM, Tony Krowiak wrote:
> 
> Ping!! I'm wondering who might be responsible for merging this fix?
See reply from Eduardo,

It's queued and will be in his next pull request

> 
> > The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index
> > value of the BusState structure with the max_dev value of the BusClass structure
> > to determine whether the maximum number of children has been reached for the
> > bus. The problem is, the max_index field of the BusState structure does not
> > necessarily reflect the number of devices that have been plugged into
> > the bus.
> > 
> > Whenever a child device is plugged into the bus, the bus's max_index value is
> > assigned to the child device and then incremented. If the child is subsequently
> > unplugged, the value of the max_index does not change and no longer reflects the
> > number of children.
> > 
> > When the bus's max_index value reaches the maximum number of devices
> > allowed for the bus (i.e., the max_dev field in the BusClass structure),
> > attempts to plug another device will be rejected claiming that the bus is
> > full -- even if the bus is actually empty.
> > 
> > To resolve the problem, a new 'num_children' field is being added to the
> > BusState structure to keep track of the number of children plugged into the
> > bus. It will be incremented when a child is plugged, and decremented when a
> > child is unplugged.
> > 
> > Signed-off-by: Tony Krowiak <[e-mail]>
> > Reviewed-by: Pierre Morel<[e-mail]>
> > Reviewed-by: Halil Pasic <[e-mail]>
> > ---
> >   hw/core/qdev.c         | 3 +++
> >   include/hw/qdev-core.h | 1 +
> >   qdev-monitor.c         | 2 +-
> >   3 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 6b3cc55b27c2..956923f33520 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
> >               snprintf(name, sizeof(name), "child[%d]", kid->index);
> >               QTAILQ_REMOVE(&bus->children, kid, sibling);
> >   
> > +            bus->num_children--;
> > +
> >               /* This gives back ownership of kid->child back to us.  */
> >               object_property_del(OBJECT(bus), name, NULL);
> >               object_unref(OBJECT(kid->child));
> > @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> >       char name[32];
> >       BusChild *kid = g_malloc0(sizeof(*kid));
> >   
> > +    bus->num_children++;
> >       kid->index = bus->max_index++;
> >       kid->child = child;
> >       object_ref(OBJECT(kid->child));
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index a24d0dd566e3..521f0a947ead 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -206,6 +206,7 @@ struct BusState {
> >       HotplugHandler *hotplug_handler;
> >       int max_index;
> >       bool realized;
> > +    int num_children;
> >       QTAILQ_HEAD(ChildrenHead, BusChild) children;
> >       QLIST_ENTRY(BusState) sibling;
> >   };
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 07147c63bf8b..45a8ba49644c 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
> >   static inline bool qbus_is_full(BusState *bus)
> >   {
> >       BusClass *bus_class = BUS_GET_CLASS(bus);
> > -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> > +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
> >   }
> >   
> >   /*
> >   
>

Patch

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6b3cc55b27c2..956923f33520 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -59,6 +59,8 @@  static void bus_remove_child(BusState *bus, DeviceState *child)
             snprintf(name, sizeof(name), "child[%d]", kid->index);
             QTAILQ_REMOVE(&bus->children, kid, sibling);
 
+            bus->num_children--;
+
             /* This gives back ownership of kid->child back to us.  */
             object_property_del(OBJECT(bus), name, NULL);
             object_unref(OBJECT(kid->child));
@@ -73,6 +75,7 @@  static void bus_add_child(BusState *bus, DeviceState *child)
     char name[32];
     BusChild *kid = g_malloc0(sizeof(*kid));
 
+    bus->num_children++;
     kid->index = bus->max_index++;
     kid->child = child;
     object_ref(OBJECT(kid->child));
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index a24d0dd566e3..521f0a947ead 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -206,6 +206,7 @@  struct BusState {
     HotplugHandler *hotplug_handler;
     int max_index;
     bool realized;
+    int num_children;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
 };
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 07147c63bf8b..45a8ba49644c 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -414,7 +414,7 @@  static DeviceState *qbus_find_dev(BusState *bus, char *elem)
 static inline bool qbus_is_full(BusState *bus)
 {
     BusClass *bus_class = BUS_GET_CLASS(bus);
-    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
+    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
 }
 
 /*