Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

2021-10-05 Thread Kevin Wolf 
Am 27.09.2021 um 12:33 hat Damien Hedde geschrieben:
> Hi Kevin,
> 
> I proposed a very similar patch in our rfc series because we needed some of
> the cleaning you do here.
> https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05679.html
> I've added a bit of doc for the function, feel free to take it if you want.

Thanks, I'm replacing my patch with yours for v2.

Kevin

Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

2021-09-27 Thread Damien Hedde 

Hi Kevin,

I proposed a very similar patch in our rfc series because we needed some 
of the cleaning you do here.

https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05679.html
I've added a bit of doc for the function, feel free to take it if you want.

On 9/24/21 16:09, Vladimir Sementsov-Ogievskiy wrote:

24.09.2021 12:04, Kevin Wolf wrote:

object_property_add_child() fails (with &error_abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.

Signed-off-by: Kevin Wolf 
---
  include/monitor/qdev.h  |  2 +-
  hw/xen/xen-legacy-backend.c |  3 ++-
  softmmu/qdev-monitor.c  | 16 ++--
  3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, 
Error **errp);

  int qdev_device_help(QemuOpts *opts);
  DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
  #endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice 
*xen_be_get_xendev(const char *type, int dom,

  xendev = g_malloc0(ops->size);
  object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
  OBJECT(xendev)->free = g_free;
-    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, 
dev));

+    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+    &error_abort);
  qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
  object_unref(OBJECT(xendev));
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, 
Error **errp)

  }
  /* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)


According to recommendations in error.h, worth adding also return value 
(for example true=success false=failure), so [..]



  {
  if (id) {
  dev->id = id;
  }


Unrelated but.. What's the strange logic?

Is it intended that with passed id=NULL we don't update dev->id variable 
but try to do following logic with old dev->id?


dev->id is expected to be NULL. The object is created just before 
calling this function so it is always the case. We could probably assert 
this.





  if (dev->id) {
-    object_property_add_child(qdev_get_peripheral(), dev->id,
-  OBJECT(dev));
+    object_property_try_add_child(qdev_get_peripheral(), dev->id,
+  OBJECT(dev), errp);
  } else {
  static int anon_count;
  gchar *name = g_strdup_printf("device[%d]", anon_count++);
-    object_property_add_child(qdev_get_peripheral_anon(), name,
-  OBJECT(dev));
+    object_property_try_add_child(qdev_get_peripheral_anon(), name,
+  OBJECT(dev), errp);
  g_free(name);
  }
  }
  DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
  {
+    ERRP_GUARD();
  DeviceClass *dc;
  const char *driver, *path;
  DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, 
Error **errp)

  }
  }
-    qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+    qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+    if (*errp) {
+    goto err_del_dev;
+    }


[..] here we'll have

if (!qdev_set_id(...)) {
   goto err_del_dev;
}

and no need for ERRP_GUARD.


  /* set properties */
  if (qemu_opt_foreach(opts, set_property, dev, errp)) {

Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

2021-09-24 Thread Vladimir Sementsov-Ogievskiy 

24.09.2021 12:04, Kevin Wolf wrote:

object_property_add_child() fails (with &error_abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.

Signed-off-by: Kevin Wolf 
---
  include/monitor/qdev.h  |  2 +-
  hw/xen/xen-legacy-backend.c |  3 ++-
  softmmu/qdev-monitor.c  | 16 ++--
  3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error 
**errp);
  
  int qdev_device_help(QemuOpts *opts);

  DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
  
  #endif

diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char 
*type, int dom,
  xendev = g_malloc0(ops->size);
  object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
  OBJECT(xendev)->free = g_free;
-qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+&error_abort);
  qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
  object_unref(OBJECT(xendev));
  
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c

index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, Error **errp)
  }
  
  /* Takes ownership of @id, will be freed when deleting the device */

-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)


According to recommendations in error.h, worth adding also return value (for 
example true=success false=failure), so [..]


  {
  if (id) {
  dev->id = id;
  }


Unrelated but.. What's the strange logic?

Is it intended that with passed id=NULL we don't update dev->id variable but try 
to do following logic with old dev->id?

  
  if (dev->id) {

-object_property_add_child(qdev_get_peripheral(), dev->id,
-  OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral(), dev->id,
+  OBJECT(dev), errp);
  } else {
  static int anon_count;
  gchar *name = g_strdup_printf("device[%d]", anon_count++);
-object_property_add_child(qdev_get_peripheral_anon(), name,
-  OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral_anon(), name,
+  OBJECT(dev), errp);
  g_free(name);
  }
  }
  
  DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)

  {
+ERRP_GUARD();
  DeviceClass *dc;
  const char *driver, *path;
  DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
  }
  }
  
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));

+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+if (*errp) {
+goto err_del_dev;
+}


[..] here we'll have

if (!qdev_set_id(...)) {
  goto err_del_dev;
}

and no need for ERRP_GUARD.

  
  /* set properties */

  if (qemu_opt_foreach(opts, set_property, dev, errp)) {




--
Best regards,
Vladimir

[PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

2021-09-24 Thread Kevin Wolf 
object_property_add_child() fails (with &error_abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.

Signed-off-by: Kevin Wolf 
---
 include/monitor/qdev.h  |  2 +-
 hw/xen/xen-legacy-backend.c |  3 ++-
 softmmu/qdev-monitor.c  | 16 ++--
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error 
**errp);
 
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
 #endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char 
*type, int dom,
 xendev = g_malloc0(ops->size);
 object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
 OBJECT(xendev)->free = g_free;
-qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+&error_abort);
 qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
 object_unref(OBJECT(xendev));
 
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, Error **errp)
 }
 
 /* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)
 {
 if (id) {
 dev->id = id;
 }
 
 if (dev->id) {
-object_property_add_child(qdev_get_peripheral(), dev->id,
-  OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral(), dev->id,
+  OBJECT(dev), errp);
 } else {
 static int anon_count;
 gchar *name = g_strdup_printf("device[%d]", anon_count++);
-object_property_add_child(qdev_get_peripheral_anon(), name,
-  OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral_anon(), name,
+  OBJECT(dev), errp);
 g_free(name);
 }
 }
 
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 {
+ERRP_GUARD();
 DeviceClass *dc;
 const char *driver, *path;
 DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 }
 }
 
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+if (*errp) {
+goto err_del_dev;
+}
 
 /* set properties */
 if (qemu_opt_foreach(opts, set_property, dev, errp)) {
-- 
2.31.1