[Qemu-devel] [PATCH v4 0/2] qdev: Display warning about unused -global

[Qemu-devel] [PATCH v4 0/2] qdev: Display warning about unused -global

[Don Slutz]

I might have named this v2, but since this is a split out of:

[PATCH v3 2/4] GlobalProperty: Display warning about unused -global

From:

[PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

I feel v4 is better.

Changes v3 to v4:
  Add a new patch to add a check in test-qdev-global-props.c
  Changed qdev_prop_check_global() to return state.  Only used
  by unit test.
  Change to use error_report().

  Andreas Färber:
    Added hotpluggable checking.
    Renamed to qdev:

  Did not do:
    Add a separate linked list:
      This looked to me to increase the complexity without any benefit.
    Adjust where vl.c calls qdev_prop_check_global().
      Since this is just before migration starts, I still think this
      is the best place.  I did a quick look into adding some test
      that checks that this is still working, but only found qemu-iotest
      doing the QEMU output check for expected output.  It did not
      make sense to me to add a global property check there.

  Paolo Bonzini:
    Added a comment before definition of the not_used field.



Don Slutz (2):
  qdev: Display warning about unused -global
  qdev: Add test of qdev_prop_check_global

 hw/core/qdev-properties-system.c | 16 ++++++++++++++++
 hw/core/qdev-properties.c        | 18 ++++++++++++++++++
 include/hw/qdev-core.h           |  8 ++++++++
 include/hw/qdev-properties.h     |  1 +
 tests/test-qdev-global-props.c   |  4 ++++
 vl.c                             |  2 ++
 6 files changed, 49 insertions(+)

-- 
1.8.4

[Qemu-devel] [PATCH v4 1/2] qdev: Display warning about unused -global

[Don Slutz]

This can help a user understand why -global was ignored.

For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
ignored when "-global cirrus-vga.vgamem_mb=16" is not.

This is currently clear when the wrong property is provided:

out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
char device redirected to /dev/pts/20 (label compat_monitor0)
qemu-system-x86_64: Property '.vram_size_mb' not found
Aborted (core dumped)

vs

out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16 -monitor pty -vga cirrus
char device redirected to /dev/pts/20 (label compat_monitor0)
VNC server running on `::1:5900'
^Cqemu: terminating on signal 2

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v4:
  Changed qdev_prop_check_global() to return state.
  Change to use error_report().
  Added hotpluggable checking.
  Renamed to qdev:
  Added a comment before definition of the not_used field.


 hw/core/qdev-properties-system.c | 16 ++++++++++++++++
 hw/core/qdev-properties.c        | 18 ++++++++++++++++++
 include/hw/qdev-core.h           |  8 ++++++++
 include/hw/qdev-properties.h     |  1 +
 vl.c                             |  2 ++
 5 files changed, 45 insertions(+)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index de83561..1b124fd 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -439,11 +439,27 @@ PropertyInfo qdev_prop_iothread = {
 static int qdev_add_one_global(QemuOpts *opts, void *opaque)
 {
     GlobalProperty *g;
+    ObjectClass *oc;
 
     g = g_malloc0(sizeof(*g));
     g->driver   = qemu_opt_get(opts, "driver");
     g->property = qemu_opt_get(opts, "property");
     g->value    = qemu_opt_get(opts, "value");
+    oc = object_class_by_name(g->driver);
+    if (oc) {
+        DeviceClass *dc = DEVICE_CLASS(oc);
+
+        if (dc->hotpluggable) {
+            /* If hotpluggable then skip not_used checking. */
+            g->not_used = false;
+        } else {
+            /* Maybe a typo. */
+            g->not_used = true;
+        }
+    } else {
+        /* Maybe a typo. */
+        g->not_used = true;
+    }
     qdev_prop_register_global(g);
     return 0;
 }
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 585a8e9..566dab1 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -952,6 +952,23 @@ void qdev_prop_register_global_list(GlobalProperty *props)
     }
 }
 
+int qdev_prop_check_global(void)
+{
+    GlobalProperty *prop;
+    int ret = 0;
+
+    QTAILQ_FOREACH(prop, &global_props, next) {
+        if (!prop->not_used) {
+            continue;
+        }
+        ret = 1;
+        error_report("Warning: \"-global %s.%s=%s\" not used",
+                     prop->driver, prop->property, prop->value);
+
+    }
+    return ret;
+}
+
 void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
                                     Error **errp)
 {
@@ -963,6 +980,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
         if (strcmp(typename, prop->driver) != 0) {
             continue;
         }
+        prop->not_used = false;
         object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
         if (err != NULL) {
             error_propagate(errp, err);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index dbe473c..bbed829 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -231,10 +231,18 @@ struct PropertyInfo {
     ObjectPropertyRelease *release;
 };
 
+/**
+ * GlobalProperty:
+ * @not_used: Track use of a global property.  Defaults to false in all C99
+ * struct initializations.
+ *
+ * This prevents reports of .compat_props when they are not used.
+ */
 typedef struct GlobalProperty {
     const char *driver;
     const char *property;
     const char *value;
+    bool not_used;
     QTAILQ_ENTRY(GlobalProperty) next;
 } GlobalProperty;
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index c46e908..c962b6b 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -180,6 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
 void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
+int qdev_prop_check_global(void);
 void qdev_prop_set_globals(DeviceState *dev, Error **errp);
 void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
                                     Error **errp);
diff --git a/vl.c b/vl.c
index 236f95e..032fa8c 100644
--- a/vl.c
+++ b/vl.c
@@ -4518,6 +4518,8 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+    qdev_prop_check_global();
+
     if (incoming) {
         Error *local_err = NULL;
         qemu_start_incoming_migration(incoming, &local_err);
-- 
1.8.4

[Qemu-devel] [PATCH v4 2/2] qdev: Add test of qdev_prop_check_global

[Don Slutz]

This will generate a warning from "make check":

...
GTESTER tests/test-qdev-global-props
Warning: "-global dynamic-prop-type-bad.prop3=103" not used
GTESTER tests/check-qom-interface
...

If the warning is not generated, the test will fail.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tests/test-qdev-global-props.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index e4ad173..2bef04c 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -150,8 +150,10 @@ static void test_dynamic_globalprop(void)
     static GlobalProperty props[] = {
         { TYPE_DYNAMIC_PROPS, "prop1", "101" },
         { TYPE_DYNAMIC_PROPS, "prop2", "102" },
+        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
         {}
     };
+    int all_used;
 
     qdev_prop_register_global_list(props);
 
@@ -160,6 +162,8 @@ static void test_dynamic_globalprop(void)
 
     g_assert_cmpuint(mt->prop1, ==, 101);
     g_assert_cmpuint(mt->prop2, ==, 102);
+    all_used = qdev_prop_check_global();
+    g_assert_cmpuint(all_used, ==, 1);
 }
 
 int main(int argc, char **argv)
-- 
1.8.4

Re: [Qemu-devel] [ping PATCH v4 0/2] qdev: Display warning about unused -global

[Don Slutz]

ping.

On 05/05/14 14:03, Don Slutz wrote:
> I might have named this v2, but since this is a split out of:
>
> [PATCH v3 2/4] GlobalProperty: Display warning about unused -global
>
> From:
>
> [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)
>
> I feel v4 is better.
>
> Changes v3 to v4:
>    Add a new patch to add a check in test-qdev-global-props.c
>    Changed qdev_prop_check_global() to return state.  Only used
>    by unit test.
>    Change to use error_report().
>
>    Andreas Färber:
>      Added hotpluggable checking.
>      Renamed to qdev:
>
>    Did not do:
>      Add a separate linked list:
>        This looked to me to increase the complexity without any benefit.
>      Adjust where vl.c calls qdev_prop_check_global().
>        Since this is just before migration starts, I still think this
>        is the best place.  I did a quick look into adding some test
>        that checks that this is still working, but only found qemu-iotest
>        doing the QEMU output check for expected output.  It did not
>        make sense to me to add a global property check there.
>
>    Paolo Bonzini:
>      Added a comment before definition of the not_used field.
>
>
>
> Don Slutz (2):
>    qdev: Display warning about unused -global
>    qdev: Add test of qdev_prop_check_global
>
>   hw/core/qdev-properties-system.c | 16 ++++++++++++++++
>   hw/core/qdev-properties.c        | 18 ++++++++++++++++++
>   include/hw/qdev-core.h           |  8 ++++++++
>   include/hw/qdev-properties.h     |  1 +
>   tests/test-qdev-global-props.c   |  4 ++++
>   vl.c                             |  2 ++
>   6 files changed, 49 insertions(+)
>

Re: [Qemu-devel] [PATCH v4 0/2] qdev: Display warning about unused -global

[Michael S. Tsirkin]

On Mon, May 05, 2014 at 02:03:05PM -0400, Don Slutz wrote:
> I might have named this v2, but since this is a split out of:
> 
> [PATCH v3 2/4] GlobalProperty: Display warning about unused -global
> 
> From:
> 
> [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)
> 
> I feel v4 is better.

Applied, thanks!

> Changes v3 to v4:
>   Add a new patch to add a check in test-qdev-global-props.c
>   Changed qdev_prop_check_global() to return state.  Only used
>   by unit test.
>   Change to use error_report().
> 
>   Andreas Färber:
>     Added hotpluggable checking.
>     Renamed to qdev:
> 
>   Did not do:
>     Add a separate linked list:
>       This looked to me to increase the complexity without any benefit.
>     Adjust where vl.c calls qdev_prop_check_global().
>       Since this is just before migration starts, I still think this
>       is the best place.  I did a quick look into adding some test
>       that checks that this is still working, but only found qemu-iotest
>       doing the QEMU output check for expected output.  It did not
>       make sense to me to add a global property check there.
> 
>   Paolo Bonzini:
>     Added a comment before definition of the not_used field.
> 
> 
> 
> Don Slutz (2):
>   qdev: Display warning about unused -global
>   qdev: Add test of qdev_prop_check_global
> 
>  hw/core/qdev-properties-system.c | 16 ++++++++++++++++
>  hw/core/qdev-properties.c        | 18 ++++++++++++++++++
>  include/hw/qdev-core.h           |  8 ++++++++
>  include/hw/qdev-properties.h     |  1 +
>  tests/test-qdev-global-props.c   |  4 ++++
>  vl.c                             |  2 ++
>  6 files changed, 49 insertions(+)
> 
> -- 
> 1.8.4

[Qemu-devel] Should not abort on -global <nonexistant dev prop> (was: [PATCH v4 1/2] qdev: Display warning about unused -global)

[Markus Armbruster]

Nothing wrong with Don's patch as far as I can see, but...

Don Slutz <dslutz@verizon.com> writes:

> This can help a user understand why -global was ignored.
>
> For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
> ignored when "-global cirrus-vga.vgamem_mb=16" is not.
>
> This is currently clear when the wrong property is provided:
>
> out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
> char device redirected to /dev/pts/20 (label compat_monitor0)
> qemu-system-x86_64: Property '.vram_size_mb' not found
> Aborted (core dumped)

... dumping core here is not nice.

Looks like this regressed in Eduardo's commit 99a0b03 qdev: Set globals
in instance_post_init function.

Before, we exited cleanly on this error, in device_initfn():

        qdev_prop_set_globals(dev, &err);
        if (err != NULL) {
            qerror_report_err(err);
            error_free(err);
            exit(1);
        }

The commit asserts qdev_prop_set_globals() can't fail, which is wrong.

    static void device_post_init(Object *obj)
    {
        DeviceState *dev = DEVICE(obj);
        Error *err = NULL;

        qdev_prop_set_globals(dev, &err);
        assert_no_error(err);
    }

Later simplified to:

    static void device_post_init(Object *obj)
    {
        qdev_prop_set_globals(DEVICE(obj), &error_abort);
    }

Any takers?

[...]

Re: [Qemu-devel] Should not abort on -global <nonexistant dev prop>

[Paolo Bonzini]

Il 06/06/2014 09:09, Markus Armbruster ha scritto:
> Looks like this regressed in Eduardo's commit 99a0b03 qdev: Set globals
> in instance_post_init function.
>
> Before, we exited cleanly on this error, in device_initfn():
>
>         qdev_prop_set_globals(dev, &err);
>         if (err != NULL) {
>             qerror_report_err(err);
>             error_free(err);
>             exit(1);
>         }

Hmm, right.  I had noticed this abort in the past, but I wasn't sure if 
it was a regression or not.

However, the above is not hotplug-friendly either.  In this sense, I 
prefer an assertion that clearly says "gotta fix something here".

> The commit asserts qdev_prop_set_globals() can't fail, which is wrong.
>
>     static void device_post_init(Object *obj)
>     {
>         DeviceState *dev = DEVICE(obj);
>         Error *err = NULL;
>
>         qdev_prop_set_globals(dev, &err);
>         assert_no_error(err);
>     }
>
> Later simplified to:
>
>     static void device_post_init(Object *obj)
>     {
>         qdev_prop_set_globals(DEVICE(obj), &error_abort);
>     }

I think the bug is that the global property should have been filtered 
out early.  Or alternatively we need something better than 
device_post_init to set the globals... no ideas for now.

Paolo

Re: [Qemu-devel] Should not abort on -global <nonexistant dev prop>

[Eduardo Habkost]

On Fri, Jun 06, 2014 at 10:01:34AM +0200, Paolo Bonzini wrote:
> Il 06/06/2014 09:09, Markus Armbruster ha scritto:
> >Looks like this regressed in Eduardo's commit 99a0b03 qdev: Set globals
> >in instance_post_init function.
> >
> >Before, we exited cleanly on this error, in device_initfn():
> >
> >        qdev_prop_set_globals(dev, &err);
> >        if (err != NULL) {
> >            qerror_report_err(err);
> >            error_free(err);
> >            exit(1);
> >        }
> 
> Hmm, right.  I had noticed this abort in the past, but I wasn't sure
> if it was a regression or not.
> 
> However, the above is not hotplug-friendly either.  In this sense, I
> prefer an assertion that clearly says "gotta fix something here".

The assertion can be triggered by user error (a non-existing property or
invalid property value). Sounds like a bug to me.

I will work on a patch to get back to using exit(), except when it is
just a non-existing property. It will still be hotplug-unfriendly, but
abort() is not hotplug-friendly either...

> 
> >The commit asserts qdev_prop_set_globals() can't fail, which is wrong.
> >
> >    static void device_post_init(Object *obj)
> >    {
> >        DeviceState *dev = DEVICE(obj);
> >        Error *err = NULL;
> >
> >        qdev_prop_set_globals(dev, &err);
> >        assert_no_error(err);
> >    }
> >
> >Later simplified to:
> >
> >    static void device_post_init(Object *obj)
> >    {
> >        qdev_prop_set_globals(DEVICE(obj), &error_abort);
> >    }
> 
> I think the bug is that the global property should have been filtered
> out early.

Even if we handle non-existing properties cleanly (without exiting, but
just keeping prop->not_used=true), we still need to handle errors
returned by property setters in a hotplug-friendly way.


> Or alternatively we need something better than
> device_post_init to set the globals... no ideas for now.

The root problem here seems to be that property setters can report
errors, but object_new() can't.

Does that mean we need a variation of object_new() which can report
errors?

-- 
Eduardo