[Qemu-devel] [PATCH 0/2] fix startup crash on sparc/ppc guests

[Qemu-devel] [PATCH 0/2] fix startup crash on sparc/ppc guests

[Paolo Bonzini]

The problem happens when qdev_set_prop_* is passed a NULL pointer.
The first patch allows this; the second improves the error message
if there are problems with qdev properties, as requested by Peter.

Blue Swirl, can you apply?

Paolo Bonzini (2):
  qdev: allow setting properties to NULL
  qdev: print error message before aborting

 hw/qdev-properties.c |   41 ++++++++++++++++++++++-------------------
 qerror.c             |    8 ++++++++
 qerror.h             |    1 +
 3 files changed, 31 insertions(+), 19 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/2] qdev: allow setting properties to NULL

[Paolo Bonzini]

SPARC and PPC set properties to NULL.  This can be done with an
empty string value.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index b6d6fcf..6f09a35 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -613,7 +613,7 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop,
     }
     if (!*str) {
         g_free(str);
-        error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
+        *ptr = NULL;
         return;
     }
     ret = parse(dev, str, ptr);
@@ -1120,7 +1120,8 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, char *value)
 int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
 {
     Error *errp = NULL;
-    object_property_set_str(OBJECT(dev), bdrv_get_device_name(value),
+    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
+    object_property_set_str(OBJECT(dev), bdrv_name,
                             name, &errp);
     if (errp) {
         qerror_report_err(errp);
@@ -1139,16 +1140,18 @@ void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverS
 void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
 {
     Error *errp = NULL;
-    assert(value->label);
-    object_property_set_str(OBJECT(dev), value->label, name, &errp);
+    assert(!value || value->label);
+    object_property_set_str(OBJECT(dev),
+                            value ? value->label : "", name, &errp);
     assert(!errp);
 }
 
 void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value)
 {
     Error *errp = NULL;
-    assert(value->name);
-    object_property_set_str(OBJECT(dev), value->name, name, &errp);
+    assert(!value || value->name);
+    object_property_set_str(OBJECT(dev),
+                            value ? value->name : "", name, &errp);
     assert(!errp);
 }
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/2] qdev: print error message before aborting

[Paolo Bonzini]

qdev_prop_set_* functions are always called by machine init functions
that should know what they're doing, so they abort on error.  Still,
an assert(!errp) does not aid debugging.  Print an error before aborting.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   26 +++++++++++++-------------
 qerror.c             |    8 ++++++++
 qerror.h             |    1 +
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 6f09a35..7b74dd5 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1072,49 +1072,49 @@ void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
 {
     Error *errp = NULL;
     object_property_set_bool(OBJECT(dev), value, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value)
 {
     Error *errp = NULL;
     object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value)
 {
     Error *errp = NULL;
     object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value)
 {
     Error *errp = NULL;
     object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_int32(DeviceState *dev, const char *name, int32_t value)
 {
     Error *errp = NULL;
     object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value)
 {
     Error *errp = NULL;
     object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_string(DeviceState *dev, const char *name, char *value)
 {
     Error *errp = NULL;
     object_property_set_str(OBJECT(dev), value, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
@@ -1143,7 +1143,7 @@ void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *valu
     assert(!value || value->label);
     object_property_set_str(OBJECT(dev),
                             value ? value->label : "", name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value)
@@ -1152,14 +1152,14 @@ void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *v
     assert(!value || value->name);
     object_property_set_str(OBJECT(dev),
                             value ? value->name : "", name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value)
 {
     Error *errp = NULL;
     object_property_set_int(OBJECT(dev), value ? value->id : -1, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
@@ -1170,7 +1170,7 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
              value[0], value[1], value[2], value[3], value[4], value[5]);
 
     object_property_set_str(OBJECT(dev), str, name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
@@ -1181,7 +1181,7 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
     prop = qdev_prop_find(dev, name);
     object_property_set_str(OBJECT(dev), prop->info->enum_table[value],
                             name, &errp);
-    assert(!errp);
+    assert_no_error(errp);
 }
 
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
@@ -1213,7 +1213,7 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props)
         } else if (props->qtype == QTYPE_QINT) {
             object_property_set_int(obj, props->defval, props->name, &errp);
         }
-        assert(!errp);
+        assert_no_error(errp);
     }
 }
 
diff --git a/qerror.c b/qerror.c
index 8e6efaf..f55d435 100644
--- a/qerror.c
+++ b/qerror.c
@@ -572,6 +572,14 @@ void qerror_report_err(Error *err)
     }
 }
 
+void assert_no_error(Error *err)
+{
+    if (err) {
+        qerror_report_err(err);
+        abort();
+    }
+}
+
 /**
  * qobject_to_qerror(): Convert a QObject into a QError
  */
diff --git a/qerror.h b/qerror.h
index e8718bf..e26c635 100644
--- a/qerror.h
+++ b/qerror.h
@@ -41,6 +41,7 @@ void qerror_print(QError *qerror);
 void qerror_report_internal(const char *file, int linenr, const char *func,
                             const char *fmt, ...) GCC_FMT_ATTR(4, 5);
 void qerror_report_err(Error *err);
+void assert_no_error(Error *err);
 QString *qerror_format(const char *fmt, QDict *error);
 #define qerror_report(fmt, ...) \
     qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 0/2] fix startup crash on sparc/ppc guests

[Blue Swirl]

On Tue, Feb 14, 2012 at 09:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The problem happens when qdev_set_prop_* is passed a NULL pointer.
> The first patch allows this; the second improves the error message
> if there are problems with qdev properties, as requested by Peter.
>
> Blue Swirl, can you apply?

Thanks, applied both.

> Paolo Bonzini (2):
>  qdev: allow setting properties to NULL
>  qdev: print error message before aborting
>
>  hw/qdev-properties.c |   41 ++++++++++++++++++++++-------------------
>  qerror.c             |    8 ++++++++
>  qerror.h             |    1 +
>  3 files changed, 31 insertions(+), 19 deletions(-)
>
> --
> 1.7.7.6
>
>

Re: [Qemu-devel] [PATCH 2/2] qdev: print error message before aborting

[Peter Maydell]

On 14 February 2012 09:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> qdev_prop_set_* functions are always called by machine init functions
> that should know what they're doing, so they abort on error.  Still,
> an assert(!errp) does not aid debugging.  Print an error before aborting.

So now you get:
qemu-system-arm: Insufficient permission to perform this operation
Aborted

This still isn't really very diagnostic: I had to run the whole
thing under a debugger and look at the backtrace anyway before
I realised I'd hit the same problem again. Isn't there some way
to supply a more specific message than this? (some string that
actuall mentions at least one of "object", "property" and
"after initialization", for example :-))

-- PMM

Re: [Qemu-devel] [PATCH 2/2] qdev: print error message before aborting

[Paolo Bonzini]

On 02/17/2012 05:30 PM, Peter Maydell wrote:
> So now you get:
> qemu-system-arm: Insufficient permission to perform this operation
> Aborted
> 
> This still isn't really very diagnostic: I had to run the whole
> thing under a debugger and look at the backtrace anyway before
> I realised I'd hit the same problem again.

Sorry. :)

> Isn't there some way
> to supply a more specific message than this? (some string that
> actuall mentions at least one of "object", "property" and
> "after initialization", for example :-))

Yes, of course.  This should have been done from the beginning.  I just
cut-and-pasted the error message but that doesn't mean it is a good one.

Paolo