[Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Eduardo Habkost]

This avoids QEMU from aborting on cases like this:

  $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5
  qemu-system-x86_64: Property '.foobar' not found
  Aborted (core dumped)

The code sets dev->not_used if the property is not found as an effort to
to allow errors to be reported even if the device is hotpluggable, but
it won't catch all errors. We can't know the property is not going to be
available for hotpluggable devices, unless we actually try to create the
device.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/qdev-properties.c      | 10 +++++++++-
 tests/test-qdev-global-props.c | 14 ++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 3d12560..8cd7c2a 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -976,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
                                     Error **errp)
 {
     GlobalProperty *prop;
+    Object *obj = OBJECT(dev);
 
     QTAILQ_FOREACH(prop, &global_props, next) {
         Error *err = NULL;
@@ -983,8 +984,15 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
         if (strcmp(typename, prop->driver) != 0) {
             continue;
         }
+        if (!object_property_find(obj, prop->property, &err)) {
+            /* not_used doesn't default to true for hotpluggable devices,
+             * but in this case we know the property wasn't used when it could.
+             */
+            prop->not_used = true;
+            continue;
+        }
         prop->not_used = false;
-        object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
+        object_property_parse(obj, prop->value, prop->property, &err);
         if (err != NULL) {
             error_propagate(errp, err);
             return;
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index 2bef04c..1ccc3e5 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -148,8 +148,9 @@ static void test_dynamic_globalprop(void)
 {
     MyType *mt;
     static GlobalProperty props[] = {
-        { TYPE_DYNAMIC_PROPS, "prop1", "101" },
-        { TYPE_DYNAMIC_PROPS, "prop2", "102" },
+        { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
+        { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
+        { TYPE_DYNAMIC_PROPS, "prop3", "103", true },
         { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
         {}
     };
@@ -164,6 +165,15 @@ static void test_dynamic_globalprop(void)
     g_assert_cmpuint(mt->prop2, ==, 102);
     all_used = qdev_prop_check_global();
     g_assert_cmpuint(all_used, ==, 1);
+
+    /* prop1 */
+    g_assert(!props[0].not_used);
+    /* prop2 */
+    g_assert(!props[1].not_used);
+    /* TYPE_DYNAMIC_PROPS.prop3: non-existing property */
+    g_assert(props[2].not_used);
+    /* TYPE_DYNAMIC_PROPS-bad.prop3: non-existing class */
+    g_assert(props[3].not_used);
 }
 
 int main(int argc, char **argv)
-- 
1.9.0

Re: [Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Don Slutz]

On 06/06/14 16:14, Eduardo Habkost wrote:
> This avoids QEMU from aborting on cases like this:
>
>    $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5
>    qemu-system-x86_64: Property '.foobar' not found
>    Aborted (core dumped)
>
> The code sets dev->not_used if the property is not found as an effort to
> to allow errors to be reported even if the device is hotpluggable, but
> it won't catch all errors. We can't know the property is not going to be
> available for hotpluggable devices, unless we actually try to create the
> device.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/core/qdev-properties.c      | 10 +++++++++-
>   tests/test-qdev-global-props.c | 14 ++++++++++++--
>   2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 3d12560..8cd7c2a 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -976,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>                                       Error **errp)
>   {
>       GlobalProperty *prop;
> +    Object *obj = OBJECT(dev);
>   
>       QTAILQ_FOREACH(prop, &global_props, next) {
>           Error *err = NULL;
> @@ -983,8 +984,15 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>           if (strcmp(typename, prop->driver) != 0) {
>               continue;
>           }
> +        if (!object_property_find(obj, prop->property, &err)) {
> +            /* not_used doesn't default to true for hotpluggable devices,
> +             * but in this case we know the property wasn't used when it could.
> +             */
> +            prop->not_used = true;
> +            continue;
> +        }
>           prop->not_used = false;
> -        object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> +        object_property_parse(obj, prop->value, prop->property, &err);
>           if (err != NULL) {
>               error_propagate(errp, err);
>               return;
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 2bef04c..1ccc3e5 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -148,8 +148,9 @@ static void test_dynamic_globalprop(void)
>   {
>       MyType *mt;
>       static GlobalProperty props[] = {
> -        { TYPE_DYNAMIC_PROPS, "prop1", "101" },
> -        { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> +        { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
> +        { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
> +        { TYPE_DYNAMIC_PROPS, "prop3", "103", true },

I think the test would be better if this started out as false.

     -Don Slutz

>           { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
>           {}
>       };
> @@ -164,6 +165,15 @@ static void test_dynamic_globalprop(void)
>       g_assert_cmpuint(mt->prop2, ==, 102);
>       all_used = qdev_prop_check_global();
>       g_assert_cmpuint(all_used, ==, 1);
> +
> +    /* prop1 */
> +    g_assert(!props[0].not_used);
> +    /* prop2 */
> +    g_assert(!props[1].not_used);
> +    /* TYPE_DYNAMIC_PROPS.prop3: non-existing property */
> +    g_assert(props[2].not_used);
> +    /* TYPE_DYNAMIC_PROPS-bad.prop3: non-existing class */
> +    g_assert(props[3].not_used);
>   }
>   
>   int main(int argc, char **argv)

Re: [Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Igor Mammedov]

On Fri, 6 Jun 2014 17:14:29 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This avoids QEMU from aborting on cases like this:
> 
>   $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5
>   qemu-system-x86_64: Property '.foobar' not found
>   Aborted (core dumped)
That is expected behavior.

> 
> The code sets dev->not_used if the property is not found as an effort to
> to allow errors to be reported even if the device is hotpluggable, but
> it won't catch all errors. We can't know the property is not going to be
> available for hotpluggable devices, unless we actually try to create the
> device.
Instead of ignoring users errors, DeviceState should have async_error
field which could be set by device_post_init() instead of aborting and
later device_add could gracefully fail hotadd operation if error is set.

PS:
initfn-s could also reuse this, instead of ignoring errors as they do now.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/qdev-properties.c      | 10 +++++++++-
>  tests/test-qdev-global-props.c | 14 ++++++++++++--
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 3d12560..8cd7c2a 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -976,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>                                      Error **errp)
>  {
>      GlobalProperty *prop;
> +    Object *obj = OBJECT(dev);
>  
>      QTAILQ_FOREACH(prop, &global_props, next) {
>          Error *err = NULL;
> @@ -983,8 +984,15 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>          if (strcmp(typename, prop->driver) != 0) {
>              continue;
>          }
> +        if (!object_property_find(obj, prop->property, &err)) {
> +            /* not_used doesn't default to true for hotpluggable devices,
> +             * but in this case we know the property wasn't used when it could.
> +             */
> +            prop->not_used = true;
> +            continue;
> +        }
>          prop->not_used = false;
> -        object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> +        object_property_parse(obj, prop->value, prop->property, &err);
>          if (err != NULL) {
>              error_propagate(errp, err);
>              return;
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 2bef04c..1ccc3e5 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -148,8 +148,9 @@ static void test_dynamic_globalprop(void)
>  {
>      MyType *mt;
>      static GlobalProperty props[] = {
> -        { TYPE_DYNAMIC_PROPS, "prop1", "101" },
> -        { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> +        { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
> +        { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
> +        { TYPE_DYNAMIC_PROPS, "prop3", "103", true },
>          { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
>          {}
>      };
> @@ -164,6 +165,15 @@ static void test_dynamic_globalprop(void)
>      g_assert_cmpuint(mt->prop2, ==, 102);
>      all_used = qdev_prop_check_global();
>      g_assert_cmpuint(all_used, ==, 1);
> +
> +    /* prop1 */
> +    g_assert(!props[0].not_used);
> +    /* prop2 */
> +    g_assert(!props[1].not_used);
> +    /* TYPE_DYNAMIC_PROPS.prop3: non-existing property */
> +    g_assert(props[2].not_used);
> +    /* TYPE_DYNAMIC_PROPS-bad.prop3: non-existing class */
> +    g_assert(props[3].not_used);
>  }
>  
>  int main(int argc, char **argv)
> -- 
> 1.9.0
> 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Eduardo Habkost]

On Fri, Jun 06, 2014 at 11:38:58PM +0200, Igor Mammedov wrote:
> On Fri, 6 Jun 2014 17:14:29 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This avoids QEMU from aborting on cases like this:
> > 
> >   $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5
> >   qemu-system-x86_64: Property '.foobar' not found
> >   Aborted (core dumped)
> That is expected behavior.

Why?

QEMU should never dump core due to user error.

QEMU should not abort when handling a device_add command due to user
error.

> 
> > 
> > The code sets dev->not_used if the property is not found as an effort to
> > to allow errors to be reported even if the device is hotpluggable, but
> > it won't catch all errors. We can't know the property is not going to be
> > available for hotpluggable devices, unless we actually try to create the
> > device.
> Instead of ignoring users errors, DeviceState should have async_error
> field which could be set by device_post_init() instead of aborting and
> later device_add could gracefully fail hotadd operation if error is set.
> 
> PS:
> initfn-s could also reuse this, instead of ignoring errors as they do now.

Your proposal sounds good, and would allow reporting error without
creating an object_new() variation that accepts Error**.

But I believe we need to choose what to do in the meantime, while we
don't have that mechanism implemented. Dumping core is not acceptable.
Exiting QEMU while handling device_add doesn't seem acceptable to me,
either.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Igor Mammedov]

On Fri, 6 Jun 2014 19:21:36 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jun 06, 2014 at 11:38:58PM +0200, Igor Mammedov wrote:
> > On Fri, 6 Jun 2014 17:14:29 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > This avoids QEMU from aborting on cases like this:
> > > 
> > >   $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5
> > >   qemu-system-x86_64: Property '.foobar' not found
> > >   Aborted (core dumped)
> > That is expected behavior.
> 
> Why?
> 
> QEMU should never dump core due to user error.
> 
> QEMU should not abort when handling a device_add command due to user
> error.
I've meant QEMU shouldn't start if CLI has error. whether it's abort or
exit(FAIL) doesn't matter much.

> 
> > 
> > > 
> > > The code sets dev->not_used if the property is not found as an effort to
> > > to allow errors to be reported even if the device is hotpluggable, but
> > > it won't catch all errors. We can't know the property is not going to be
> > > available for hotpluggable devices, unless we actually try to create the
> > > device.
> > Instead of ignoring users errors, DeviceState should have async_error
> > field which could be set by device_post_init() instead of aborting and
> > later device_add could gracefully fail hotadd operation if error is set.
> > 
> > PS:
> > initfn-s could also reuse this, instead of ignoring errors as they do now.
> 
> Your proposal sounds good, and would allow reporting error without
> creating an object_new() variation that accepts Error**.
> 
> But I believe we need to choose what to do in the meantime, while we
> don't have that mechanism implemented. Dumping core is not acceptable.
> Exiting QEMU while handling device_add doesn't seem acceptable to me,
> either.
Exiting at startup is fine and allows to filter out user errors early.

During hotplug exiting is certainly not an option, that's why I'm
suggesting add async_error so that hotplug operation could fail gracefully.
It's a cleaner approach and not much more complex.

Ignoring errors on the other side would introduce relaxed CLI interface that
we would have to support forever for compatibility reasons.

> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Peter Maydell]

On 7 June 2014 00:22, Igor Mammedov <imammedo@redhat.com> wrote:
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> On Fri, Jun 06, 2014 at 11:38:58PM +0200, Igor Mammedov wrote:
>> > Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > >   $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5
>> > >   qemu-system-x86_64: Property '.foobar' not found
>> > >   Aborted (core dumped)
>> > That is expected behavior.
>>
>> Why?
>>
>> QEMU should never dump core due to user error.
>>
>> QEMU should not abort when handling a device_add command due to user
>> error.
> I've meant QEMU shouldn't start if CLI has error. whether it's abort or
> exit(FAIL) doesn't matter much.

I'm with Eduardo on this one -- if the user passes us a bad
command line we should diagnose it helpfully and exit with
a failure code; abort() is for programming errors. (If nothing
else, using abort() for user-triggerable conditions tends to
mean we get bug reports about core dumps, so it's in our
own interest to not do that :-) )

thanks
-- PMM

Re: [Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Eduardo Habkost]

On Sat, Jun 07, 2014 at 12:45:26AM +0100, Peter Maydell wrote:
> On 7 June 2014 00:22, Igor Mammedov <imammedo@redhat.com> wrote:
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> On Fri, Jun 06, 2014 at 11:38:58PM +0200, Igor Mammedov wrote:
> >> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > >   $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5
> >> > >   qemu-system-x86_64: Property '.foobar' not found
> >> > >   Aborted (core dumped)
> >> > That is expected behavior.
> >>
> >> Why?
> >>
> >> QEMU should never dump core due to user error.
> >>
> >> QEMU should not abort when handling a device_add command due to user
> >> error.
> > I've meant QEMU shouldn't start if CLI has error. whether it's abort or
> > exit(FAIL) doesn't matter much.
> 
> I'm with Eduardo on this one -- if the user passes us a bad
> command line we should diagnose it helpfully and exit with
> a failure code; abort() is for programming errors. (If nothing
> else, using abort() for user-triggerable conditions tends to
> mean we get bug reports about core dumps, so it's in our
> own interest to not do that :-) )

So you are agreeing with Igor, as well. He is just suggesting we keep
terminating QEMU on that case, and we can use exit() for that. My patch,
on the other hand, makes QEMU not terminate on those cases.

Note that we have two different bugs:

1) QEMU core dumps in case of user error. This is a regression
   introduced in 1.7.0. It can be fixed easily by changing existing code
   to use exit() instead of aborting.
2) QEMU terminating on device_add in case of user error. This bug was
   always present. It can be addressed using the async_error approach
   suggested by Igor.

I will submit a new patch to address (1), first.

-- 
Eduardo

[Qemu-devel] [PATCH] qdev: Don't abort() in case globals can't be set

[Eduardo Habkost]

It would be much better if we didn't terminate QEMU inside
device_post_init(), but at least exiting cleanly is better than aborting
and dumping core.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/qdev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e65a5aa..74862c2 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -901,7 +901,13 @@ static void device_initfn(Object *obj)
 
 static void device_post_init(Object *obj)
 {
-    qdev_prop_set_globals(DEVICE(obj), &error_abort);
+    Error *err = NULL;
+    qdev_prop_set_globals(DEVICE(obj), &err);
+    if (err) {
+        qerror_report_err(err);
+        error_free(err);
+        exit(EXIT_FAILURE);
+    }
 }
 
 /* Unlink device from bus and free the structure.  */
-- 
1.9.0

Re: [Qemu-devel] [PATCH] qdev: Skip non-existing properties when setting globals

[Peter Maydell]

On 7 June 2014 02:09, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Sat, Jun 07, 2014 at 12:45:26AM +0100, Peter Maydell wrote:
>> I'm with Eduardo on this one -- if the user passes us a bad
>> command line we should diagnose it helpfully and exit with
>> a failure code; abort() is for programming errors. (If nothing
>> else, using abort() for user-triggerable conditions tends to
>> mean we get bug reports about core dumps, so it's in our
>> own interest to not do that :-) )
>
> So you are agreeing with Igor, as well. He is just suggesting we keep
> terminating QEMU on that case, and we can use exit() for that. My patch,
> on the other hand, makes QEMU not terminate on those cases.

No, I'm happy with not terminating and handling the case
correctly if that makes more sense for this case; I'm just
saying that dumping core is definitely not OK.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] qdev: Don't abort() in case globals can't be set

[Michael S. Tsirkin]

On Fri, Jun 06, 2014 at 10:26:15PM -0300, Eduardo Habkost wrote:
> It would be much better if we didn't terminate QEMU inside
> device_post_init(), but at least exiting cleanly is better than aborting
> and dumping core.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/core/qdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e65a5aa..74862c2 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -901,7 +901,13 @@ static void device_initfn(Object *obj)
>  
>  static void device_post_init(Object *obj)
>  {
> -    qdev_prop_set_globals(DEVICE(obj), &error_abort);
> +    Error *err = NULL;
> +    qdev_prop_set_globals(DEVICE(obj), &err);
> +    if (err) {
> +        qerror_report_err(err);
> +        error_free(err);
> +        exit(EXIT_FAILURE);
> +    }
>  }
>  
>  /* Unlink device from bus and free the structure.  */
> -- 
> 1.9.0

Re: [Qemu-devel] [PATCH] qdev: Don't abort() in case globals can't be set

[Igor Mammedov]

On Fri, 6 Jun 2014 22:26:15 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> It would be much better if we didn't terminate QEMU inside
> device_post_init(), but at least exiting cleanly is better than aborting
> and dumping core.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/qdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e65a5aa..74862c2 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -901,7 +901,13 @@ static void device_initfn(Object *obj)
>  
>  static void device_post_init(Object *obj)
>  {
> -    qdev_prop_set_globals(DEVICE(obj), &error_abort);
> +    Error *err = NULL;
> +    qdev_prop_set_globals(DEVICE(obj), &err);
> +    if (err) {
> +        qerror_report_err(err);
> +        error_free(err);
> +        exit(EXIT_FAILURE);
> +    }
>  }
>  
>  /* Unlink device from bus and free the structure.  */
> -- 
> 1.9.0
> 
> 
Reviewed-By: Igor Mammedov <imammedo@redhat.com>

-- 
Regards,
  Igor