Re: [Qemu-devel] [PATCH] qdev: Use GList for global properties

[Eduardo Habkost]

On Thu, May 12, 2016 at 06:22:36PM +0200, Andreas Färber wrote:
> Am 12.05.2016 um 14:26 schrieb Eduardo Habkost:
> > On Fri, Jan 29, 2016 at 12:15:12PM -0200, Eduardo Habkost wrote:
> >> On Thu, Jan 28, 2016 at 07:01:12PM +0200, Michael S. Tsirkin wrote:
> >>> On Thu, Jan 28, 2016 at 01:02:26PM -0200, Eduardo Habkost wrote:
>  If the same GlobalProperty struct is registered twice, the list
>  entry gets corrupted, making tqe_next points to itself, and
>  qdev_prop_set_globals() gets stuck in a loop. The bug can be
>  easily reproduced by running:
> 
>    $ qemu-system-x86_64 -rtc-td-hack -rtc-td-hack
> 
>  Change global_props to use GList instead of queue.h, making the
>  code simpler and able to deal with properties being registered
>  twice.
> 
>  Signed-off-by: Eduardo Habkost 
> >>>
> >>> Reviewed-by: Michael S. Tsirkin 
> >>
> >> Thanks! It seems we don't have a maintainer for hw/core/qdev*.
> >> Who should merge this?
> > 
> > I've just noticed that this didn't get included. Andreas, is it
> > OK if I merge this through the machine core tree?
> 
> In general I don't mind about those lists, but I still have the bus
> refactoring sitting in my tree that didn't make it into 2.6 because
> someone made last-minute changes to osdep.h that resulted in hundreds of
> errors in my tree... If you make sure it doesn't conflict, feel free to
> take it.

Which branch should I look at, to ensure it doesn't conflict?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qdev: Use GList for global properties

[Eduardo Habkost]

On Fri, Jan 29, 2016 at 12:15:12PM -0200, Eduardo Habkost wrote:
> On Thu, Jan 28, 2016 at 07:01:12PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Jan 28, 2016 at 01:02:26PM -0200, Eduardo Habkost wrote:
> > > If the same GlobalProperty struct is registered twice, the list
> > > entry gets corrupted, making tqe_next points to itself, and
> > > qdev_prop_set_globals() gets stuck in a loop. The bug can be
> > > easily reproduced by running:
> > > 
> > >   $ qemu-system-x86_64 -rtc-td-hack -rtc-td-hack
> > > 
> > > Change global_props to use GList instead of queue.h, making the
> > > code simpler and able to deal with properties being registered
> > > twice.
> > > 
> > > Signed-off-by: Eduardo Habkost 
> > 
> > Reviewed-by: Michael S. Tsirkin 
> 
> Thanks! It seems we don't have a maintainer for hw/core/qdev*.
> Who should merge this?

I've just noticed that this didn't get included. Andreas, is it
OK if I merge this through the machine core tree?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qdev: Use GList for global properties

[Eduardo Habkost]

On Thu, Jan 28, 2016 at 07:01:12PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 28, 2016 at 01:02:26PM -0200, Eduardo Habkost wrote:
> > If the same GlobalProperty struct is registered twice, the list
> > entry gets corrupted, making tqe_next points to itself, and
> > qdev_prop_set_globals() gets stuck in a loop. The bug can be
> > easily reproduced by running:
> > 
> >   $ qemu-system-x86_64 -rtc-td-hack -rtc-td-hack
> > 
> > Change global_props to use GList instead of queue.h, making the
> > code simpler and able to deal with properties being registered
> > twice.
> > 
> > Signed-off-by: Eduardo Habkost 
> 
> Reviewed-by: Michael S. Tsirkin 

Thanks! It seems we don't have a maintainer for hw/core/qdev*.
Who should merge this?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qdev: Use GList for global properties

[Michael S. Tsirkin]

On Thu, Jan 28, 2016 at 01:02:26PM -0200, Eduardo Habkost wrote:
> If the same GlobalProperty struct is registered twice, the list
> entry gets corrupted, making tqe_next points to itself, and
> qdev_prop_set_globals() gets stuck in a loop. The bug can be
> easily reproduced by running:
> 
>   $ qemu-system-x86_64 -rtc-td-hack -rtc-td-hack
> 
> Change global_props to use GList instead of queue.h, making the
> code simpler and able to deal with properties being registered
> twice.
> 
> Signed-off-by: Eduardo Habkost 

Reviewed-by: Michael S. Tsirkin 

> ---
>  hw/core/qdev-properties.c | 17 ++---
>  include/hw/qdev-core.h|  1 -
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index aacad66..180e0da 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1,3 +1,5 @@
> +#include 
> +
>  #include "net/net.h"
>  #include "hw/qdev.h"
>  #include "qapi/qmp/qerror.h"
> @@ -1009,12 +1011,11 @@ void qdev_prop_set_ptr(DeviceState *dev, const char 
> *name, void *value)
>  *ptr = value;
>  }
>  
> -static QTAILQ_HEAD(, GlobalProperty) global_props =
> -QTAILQ_HEAD_INITIALIZER(global_props);
> +static GList *global_props;
>  
>  void qdev_prop_register_global(GlobalProperty *prop)
>  {
> -QTAILQ_INSERT_TAIL(_props, prop, next);
> +global_props = g_list_append(global_props, prop);
>  }
>  
>  void qdev_prop_register_global_list(GlobalProperty *props)
> @@ -1028,10 +1029,11 @@ void qdev_prop_register_global_list(GlobalProperty 
> *props)
>  
>  int qdev_prop_check_globals(void)
>  {
> -GlobalProperty *prop;
> +GList *l;
>  int ret = 0;
>  
> -QTAILQ_FOREACH(prop, _props, next) {
> +for (l = global_props; l; l = l->next) {
> +GlobalProperty *prop = l->data;
>  ObjectClass *oc;
>  DeviceClass *dc;
>  if (prop->used) {
> @@ -1062,9 +1064,10 @@ int qdev_prop_check_globals(void)
>  static void qdev_prop_set_globals_for_type(DeviceState *dev,
>  const char *typename)
>  {
> -GlobalProperty *prop;
> +GList *l;
>  
> -QTAILQ_FOREACH(prop, _props, next) {
> +for (l = global_props; l; l = l->next) {
> +GlobalProperty *prop = l->data;
>  Error *err = NULL;
>  
>  if (strcmp(typename, prop->driver) != 0) {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index abcdee8..1fdbaaa 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -268,7 +268,6 @@ typedef struct GlobalProperty {
>  const char *value;
>  bool user_provided;
>  bool used;
> -QTAILQ_ENTRY(GlobalProperty) next;
>  } GlobalProperty;
>  
>  /*** Board API.  This should go away once we have a machine config file.  
> ***/
> -- 
> 2.1.0

[Qemu-devel] [PATCH] qdev: Use GList for global properties

[Eduardo Habkost]

If the same GlobalProperty struct is registered twice, the list
entry gets corrupted, making tqe_next points to itself, and
qdev_prop_set_globals() gets stuck in a loop. The bug can be
easily reproduced by running:

  $ qemu-system-x86_64 -rtc-td-hack -rtc-td-hack

Change global_props to use GList instead of queue.h, making the
code simpler and able to deal with properties being registered
twice.

Signed-off-by: Eduardo Habkost 
---
 hw/core/qdev-properties.c | 17 ++---
 include/hw/qdev-core.h|  1 -
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index aacad66..180e0da 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1,3 +1,5 @@
+#include 
+
 #include "net/net.h"
 #include "hw/qdev.h"
 #include "qapi/qmp/qerror.h"
@@ -1009,12 +1011,11 @@ void qdev_prop_set_ptr(DeviceState *dev, const char 
*name, void *value)
 *ptr = value;
 }
 
-static QTAILQ_HEAD(, GlobalProperty) global_props =
-QTAILQ_HEAD_INITIALIZER(global_props);
+static GList *global_props;
 
 void qdev_prop_register_global(GlobalProperty *prop)
 {
-QTAILQ_INSERT_TAIL(_props, prop, next);
+global_props = g_list_append(global_props, prop);
 }
 
 void qdev_prop_register_global_list(GlobalProperty *props)
@@ -1028,10 +1029,11 @@ void qdev_prop_register_global_list(GlobalProperty 
*props)
 
 int qdev_prop_check_globals(void)
 {
-GlobalProperty *prop;
+GList *l;
 int ret = 0;
 
-QTAILQ_FOREACH(prop, _props, next) {
+for (l = global_props; l; l = l->next) {
+GlobalProperty *prop = l->data;
 ObjectClass *oc;
 DeviceClass *dc;
 if (prop->used) {
@@ -1062,9 +1064,10 @@ int qdev_prop_check_globals(void)
 static void qdev_prop_set_globals_for_type(DeviceState *dev,
 const char *typename)
 {
-GlobalProperty *prop;
+GList *l;
 
-QTAILQ_FOREACH(prop, _props, next) {
+for (l = global_props; l; l = l->next) {
+GlobalProperty *prop = l->data;
 Error *err = NULL;
 
 if (strcmp(typename, prop->driver) != 0) {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index abcdee8..1fdbaaa 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -268,7 +268,6 @@ typedef struct GlobalProperty {
 const char *value;
 bool user_provided;
 bool used;
-QTAILQ_ENTRY(GlobalProperty) next;
 } GlobalProperty;
 
 /*** Board API.  This should go away once we have a machine config file.  ***/
-- 
2.1.0