[Qemu-devel] [PATCH v2 00/10] globals: Clean up validation and error checking

[Qemu-devel] [PATCH v2 00/10] globals: Clean up validation and error checking

[Eduardo Habkost]

This series includes multiple changes to the way errors are
handled by the global property system.

Git tree:
  https://github.com/ehabkost/qemu-hacks.git work/global-error-handling

Changes v1 -> v2:
* Fix test-qdev-global-props unit test
  * Remove subprocess code and "nouser" test case
  * Reported-by: Igor Mammedov <imammedo@redhat.com>
* Reword doc comments for GlobalProperty::errp
  * Suggested-by: Markus Armbruster <armbru@redhat.com>
* Squashed "Use error_prepend() for errors applying globals"
  and "qdev: GlobalProperty.errp field"
  * Suggested-by: Markus Armbruster <armbru@redhat.com>
* Simplify object_class_dynamic_cast(oc, TYPE_DEVICE) checks
  * Suggested-by: Markus Armbruster <armbru@redhat.com>
* New patch: "qdev: Remove qdev_prop_register_global_list()"

Eduardo Habkost (10):
  qdev: Don't stop applying globals on first error
  qdev: Eliminate qemu_add_globals() function
  vl: Reject invalid class names on -global
  qdev: GlobalProperty.errp field
  machine: Add machine_register_compat_props() function
  vl: Set errp to &error_abort on machine compat_props
  qdev: Eliminate "global not used" warning
  qdev: Eliminate GlobalProperty 'used' and 'user_provided' fields
  qdev: Remove qdev_prop_register_global_list()
  machine: Skip global registration for non-existing classes

 hw/core/machine.c                |  26 +++++++++
 hw/core/qdev-properties-system.c |  21 +------
 hw/core/qdev-properties.c        |  55 +++----------------
 include/hw/boards.h              |   1 +
 include/hw/qdev-core.h           |   9 ++-
 include/hw/qdev-properties.h     |   2 -
 include/qemu/config-file.h       |   1 -
 tests/test-qdev-global-props.c   | 115 ++++-----------------------------------
 vl.c                             |  37 ++++++++++---
 9 files changed, 80 insertions(+), 187 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v2 01/10] qdev: Don't stop applying globals on first error

[Eduardo Habkost]

Since commit 25f8dd9, qdev_prop_set_globals_for_type() stops
applying global properties on the first error. It is a leftover
from when QEMU exited on any error when applying global property.
Now we print a warning about the first error, bug ignore all
other global properties after it.

For example, the following command-line will not set CPUID level
to 3, but will warn only about "x86_64-cpu.vendor" being ignored.

  $ ./x86_64-softmmu/qemu-system-x86_64 \
      -global x86_64-cpu.vendor=x \
      -global x86_64-cpu.level=3
  qemu-system-x86_64: Warning: global x86_64-cpu.vendor=x ignored: Property '.vendor' doesn't take value 'x'

Fix this by not returning from qdev_prop_set_globals_for_type()
on the first error.

Cc: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/qdev-properties.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index e3b2184..c10edee 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1088,7 +1088,6 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
             assert(prop->user_provided);
             error_reportf_err(err, "Warning: global %s.%s=%s ignored: ",
                               prop->driver, prop->property, prop->value);
-            return;
         }
     }
 }
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 01/10] qdev: Don't stop applying globals on first error

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> Since commit 25f8dd9, qdev_prop_set_globals_for_type() stops
> applying global properties on the first error. It is a leftover
> from when QEMU exited on any error when applying global property.
> Now we print a warning about the first error, bug ignore all
> other global properties after it.

Actually, we've stopped since forever.  Before commit 25f8dd9, that was
okay.  Since commit, it isn't.  Suggest:

qdev_prop_set_globals_for_type() stops applying global properties on the
first error.  It is a leftover from when QEMU exited on any error when
applying global property.  Commit 25f8dd9 changed the fatal error to a
warning, but neglected to drop the stopping.  Fix that.

> For example, the following command-line will not set CPUID level
> to 3, but will warn only about "x86_64-cpu.vendor" being ignored.
>
>   $ ./x86_64-softmmu/qemu-system-x86_64 \
>       -global x86_64-cpu.vendor=x \
>       -global x86_64-cpu.level=3
>   qemu-system-x86_64: Warning: global x86_64-cpu.vendor=x ignored: Property '.vendor' doesn't take value 'x'
>
> Fix this by not returning from qdev_prop_set_globals_for_type()
> on the first error.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/qdev-properties.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index e3b2184..c10edee 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1088,7 +1088,6 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
>              assert(prop->user_provided);
>              error_reportf_err(err, "Warning: global %s.%s=%s ignored: ",
>                                prop->driver, prop->property, prop->value);
> -            return;
>          }
>      }
>  }

Preferably with a clarified commit message:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 01/10] qdev: Don't stop applying globals on first error

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 09:40:54AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > Since commit 25f8dd9, qdev_prop_set_globals_for_type() stops
> > applying global properties on the first error. It is a leftover
> > from when QEMU exited on any error when applying global property.
> > Now we print a warning about the first error, bug ignore all
> > other global properties after it.
> 
> Actually, we've stopped since forever.  Before commit 25f8dd9, that was
> okay.  Since commit, it isn't.  Suggest:
> 
> qdev_prop_set_globals_for_type() stops applying global properties on the
> first error.  It is a leftover from when QEMU exited on any error when
> applying global property.  Commit 25f8dd9 changed the fatal error to a
> warning, but neglected to drop the stopping.  Fix that.

Fixed that. Thanks!

-- 
Eduardo

[Qemu-devel] [PATCH v2 02/10] qdev: Eliminate qemu_add_globals() function

[Eduardo Habkost]

The function is just a helper to handle the -global options, it
can stay in vl.c like most qemu_opts_foreach() calls.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/qdev-properties-system.c | 21 +--------------------
 include/qemu/config-file.h       |  1 -
 vl.c                             | 16 +++++++++++++++-
 3 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 891219a..cf7139d 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -1,5 +1,5 @@
 /*
- * qdev property parsing and global properties
+ * qdev property parsing
  * (parts specific for qemu-system-*)
  *
  * This file is based on code from hw/qdev-properties.c from
@@ -394,22 +394,3 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
     }
     nd->instantiated = 1;
 }
-
-static int qdev_add_one_global(void *opaque, QemuOpts *opts, Error **errp)
-{
-    GlobalProperty *g;
-
-    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");
-    g->user_provided = true;
-    qdev_prop_register_global(g);
-    return 0;
-}
-
-void qemu_add_globals(void)
-{
-    qemu_opts_foreach(qemu_find_opts("global"),
-                      qdev_add_one_global, NULL, NULL);
-}
diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 3b8ecb0..8603e86 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -12,7 +12,6 @@ void qemu_add_opts(QemuOptsList *list);
 void qemu_add_drive_opts(QemuOptsList *list);
 int qemu_set_option(const char *str);
 int qemu_global_option(const char *str);
-void qemu_add_globals(void);
 
 void qemu_config_write(FILE *fp);
 int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
diff --git a/vl.c b/vl.c
index 2f63eb4..e9ca733 100644
--- a/vl.c
+++ b/vl.c
@@ -2928,6 +2928,19 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
     loc_pop(&loc);
 }
 
+static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
+{
+    GlobalProperty *g;
+
+    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");
+    g->user_provided = true;
+    qdev_prop_register_global(g);
+    return 0;
+}
+
 int main(int argc, char **argv, char **envp)
 {
     int i;
@@ -4473,7 +4486,8 @@ int main(int argc, char **argv, char **envp)
             qdev_prop_register_global(p);
         }
     }
-    qemu_add_globals();
+    qemu_opts_foreach(qemu_find_opts("global"),
+                      global_init_func, NULL, NULL);
 
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 02/10] qdev: Eliminate qemu_add_globals() function

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> The function is just a helper to handle the -global options, it
> can stay in vl.c like most qemu_opts_foreach() calls.
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 21 +--------------------
>  include/qemu/config-file.h       |  1 -
>  vl.c                             | 16 +++++++++++++++-
>  3 files changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 891219a..cf7139d 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -1,5 +1,5 @@
>  /*
> - * qdev property parsing and global properties
> + * qdev property parsing
>   * (parts specific for qemu-system-*)
>   *
>   * This file is based on code from hw/qdev-properties.c from

Extra points for remembering to update the file comment.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[...]

[Qemu-devel] [PATCH v2 03/10] vl: Reject invalid class names on -global

[Eduardo Habkost]

Instead of just printing a warning very late, reject obviously
invalid -global arguments by validating the class name.

Update test-qdev-global-props to not expect class name validation
to be performed in qdev_prop_check_globals().

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Fix test-qdev-global-props unit test
* Simplify object_dynamic_cast() check
  * Suggested-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/qdev-properties.c      |  7 -------
 tests/test-qdev-global-props.c | 10 ----------
 vl.c                           | 20 +++++++++++++++++---
 3 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index c10edee..64e17aa 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1052,13 +1052,6 @@ int qdev_prop_check_globals(void)
             continue;
         }
         oc = object_class_by_name(prop->driver);
-        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
-        if (!oc) {
-            error_report("Warning: global %s.%s has invalid class name",
-                       prop->driver, prop->property);
-            ret = 1;
-            continue;
-        }
         dc = DEVICE_CLASS(oc);
         if (!dc->hotpluggable && !prop->used) {
             error_report("Warning: global %s.%s=%s not used",
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index 48e5b73..db77ad9 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -201,10 +201,8 @@ static void test_dynamic_globalprop_subprocess(void)
     static GlobalProperty props[] = {
         { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
         { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
-        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
         { TYPE_UNUSED_HOTPLUG, "prop4", "104", true },
         { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
-        { TYPE_NONDEVICE, "prop6", "106", true },
         {}
     };
     int all_used;
@@ -222,8 +220,6 @@ static void test_dynamic_globalprop_subprocess(void)
     g_assert(props[1].used);
     g_assert(!props[2].used);
     g_assert(!props[3].used);
-    g_assert(!props[4].used);
-    g_assert(!props[5].used);
 }
 
 static void test_dynamic_globalprop(void)
@@ -232,10 +228,8 @@ static void test_dynamic_globalprop(void)
     g_test_trap_assert_passed();
     g_test_trap_assert_stderr_unmatched("*prop1*");
     g_test_trap_assert_stderr_unmatched("*prop2*");
-    g_test_trap_assert_stderr("*Warning: global dynamic-prop-type-bad.prop3 has invalid class name\n*");
     g_test_trap_assert_stderr_unmatched("*prop4*");
     g_test_trap_assert_stderr("*Warning: global nohotplug-type.prop5=105 not used\n*");
-    g_test_trap_assert_stderr("*Warning: global nondevice-type.prop6 has invalid class name\n*");
     g_test_trap_assert_stdout("");
 }
 
@@ -246,10 +240,8 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
     static GlobalProperty props[] = {
         { TYPE_DYNAMIC_PROPS, "prop1", "101" },
         { TYPE_DYNAMIC_PROPS, "prop2", "102" },
-        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" },
         { TYPE_UNUSED_HOTPLUG, "prop4", "104" },
         { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
-        { TYPE_NONDEVICE, "prop6", "106" },
         {}
     };
     int all_used;
@@ -267,8 +259,6 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
     g_assert(props[1].used);
     g_assert(!props[2].used);
     g_assert(!props[3].used);
-    g_assert(!props[4].used);
-    g_assert(!props[5].used);
 }
 
 static void test_dynamic_globalprop_nouser(void)
diff --git a/vl.c b/vl.c
index e9ca733..1721a4b 100644
--- a/vl.c
+++ b/vl.c
@@ -2931,10 +2931,20 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
 static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
     GlobalProperty *g;
+    ObjectClass *oc;
+    const char *driver = qemu_opt_get(opts, "driver");
+    const char *prop = qemu_opt_get(opts, "property");
+
+    oc = object_class_by_name(driver);
+    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
+        error_setg(errp, "global %s.%s has invalid class name",
+                   driver, prop);
+        return -1;
+    }
 
     g = g_malloc0(sizeof(*g));
-    g->driver   = qemu_opt_get(opts, "driver");
-    g->property = qemu_opt_get(opts, "property");
+    g->driver   = driver;
+    g->property = prop;
     g->value    = qemu_opt_get(opts, "value");
     g->user_provided = true;
     qdev_prop_register_global(g);
@@ -4487,7 +4497,11 @@ int main(int argc, char **argv, char **envp)
         }
     }
     qemu_opts_foreach(qemu_find_opts("global"),
-                      global_init_func, NULL, NULL);
+                      global_init_func, NULL, &err);
+    if (err) {
+        error_report_err(err);
+        exit(1);
+    }
 
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 03/10] vl: Reject invalid class names on -global

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> Instead of just printing a warning very late, reject obviously
> invalid -global arguments by validating the class name.
>
> Update test-qdev-global-props to not expect class name validation
> to be performed in qdev_prop_check_globals().
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Fix test-qdev-global-props unit test
> * Simplify object_dynamic_cast() check
>   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/core/qdev-properties.c      |  7 -------
>  tests/test-qdev-global-props.c | 10 ----------
>  vl.c                           | 20 +++++++++++++++++---
>  3 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index c10edee..64e17aa 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1052,13 +1052,6 @@ int qdev_prop_check_globals(void)
>              continue;
>          }
>          oc = object_class_by_name(prop->driver);
> -        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> -        if (!oc) {
> -            error_report("Warning: global %s.%s has invalid class name",
> -                       prop->driver, prop->property);
> -            ret = 1;
> -            continue;
> -        }
>          dc = DEVICE_CLASS(oc);
>          if (!dc->hotpluggable && !prop->used) {
>              error_report("Warning: global %s.%s=%s not used",
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 48e5b73..db77ad9 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -201,10 +201,8 @@ static void test_dynamic_globalprop_subprocess(void)
>      static GlobalProperty props[] = {
>          { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
>          { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
> -        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
>          { TYPE_UNUSED_HOTPLUG, "prop4", "104", true },
>          { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
> -        { TYPE_NONDEVICE, "prop6", "106", true },
>          {}
>      };
>      int all_used;
> @@ -222,8 +220,6 @@ static void test_dynamic_globalprop_subprocess(void)
>      g_assert(props[1].used);
>      g_assert(!props[2].used);
>      g_assert(!props[3].used);
> -    g_assert(!props[4].used);
> -    g_assert(!props[5].used);
>  }
>  
>  static void test_dynamic_globalprop(void)
> @@ -232,10 +228,8 @@ static void test_dynamic_globalprop(void)
>      g_test_trap_assert_passed();
>      g_test_trap_assert_stderr_unmatched("*prop1*");
>      g_test_trap_assert_stderr_unmatched("*prop2*");
> -    g_test_trap_assert_stderr("*Warning: global dynamic-prop-type-bad.prop3 has invalid class name\n*");
>      g_test_trap_assert_stderr_unmatched("*prop4*");
>      g_test_trap_assert_stderr("*Warning: global nohotplug-type.prop5=105 not used\n*");
> -    g_test_trap_assert_stderr("*Warning: global nondevice-type.prop6 has invalid class name\n*");
>      g_test_trap_assert_stdout("");
>  }
>  
> @@ -246,10 +240,8 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
>      static GlobalProperty props[] = {
>          { TYPE_DYNAMIC_PROPS, "prop1", "101" },
>          { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> -        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" },
>          { TYPE_UNUSED_HOTPLUG, "prop4", "104" },
>          { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
> -        { TYPE_NONDEVICE, "prop6", "106" },
>          {}
>      };
>      int all_used;
> @@ -267,8 +259,6 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
>      g_assert(props[1].used);
>      g_assert(!props[2].used);
>      g_assert(!props[3].used);
> -    g_assert(!props[4].used);
> -    g_assert(!props[5].used);
>  }
>  

The rest of the patch turns a warning into an error, but the test update
*drops* the affected test cases.  Shouldn't we test the error instead?

To keep my R-by, you can either do that, or mention the loss of test
cases in the commit message.

[...]

Re: [Qemu-devel] [PATCH v2 03/10] vl: Reject invalid class names on -global

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 09:52:12AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > Instead of just printing a warning very late, reject obviously
> > invalid -global arguments by validating the class name.
> >
> > Update test-qdev-global-props to not expect class name validation
> > to be performed in qdev_prop_check_globals().
> >
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> > * Fix test-qdev-global-props unit test
> > * Simplify object_dynamic_cast() check
> >   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  hw/core/qdev-properties.c      |  7 -------
> >  tests/test-qdev-global-props.c | 10 ----------
> >  vl.c                           | 20 +++++++++++++++++---
> >  3 files changed, 17 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index c10edee..64e17aa 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1052,13 +1052,6 @@ int qdev_prop_check_globals(void)
> >              continue;
> >          }
> >          oc = object_class_by_name(prop->driver);
> > -        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > -        if (!oc) {
> > -            error_report("Warning: global %s.%s has invalid class name",
> > -                       prop->driver, prop->property);
> > -            ret = 1;
> > -            continue;
> > -        }
> >          dc = DEVICE_CLASS(oc);
> >          if (!dc->hotpluggable && !prop->used) {
> >              error_report("Warning: global %s.%s=%s not used",
> > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> > index 48e5b73..db77ad9 100644
> > --- a/tests/test-qdev-global-props.c
> > +++ b/tests/test-qdev-global-props.c
> > @@ -201,10 +201,8 @@ static void test_dynamic_globalprop_subprocess(void)
> >      static GlobalProperty props[] = {
> >          { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
> >          { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
> > -        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
> >          { TYPE_UNUSED_HOTPLUG, "prop4", "104", true },
> >          { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
> > -        { TYPE_NONDEVICE, "prop6", "106", true },
> >          {}
> >      };
> >      int all_used;
> > @@ -222,8 +220,6 @@ static void test_dynamic_globalprop_subprocess(void)
> >      g_assert(props[1].used);
> >      g_assert(!props[2].used);
> >      g_assert(!props[3].used);
> > -    g_assert(!props[4].used);
> > -    g_assert(!props[5].used);
> >  }
> >  
> >  static void test_dynamic_globalprop(void)
> > @@ -232,10 +228,8 @@ static void test_dynamic_globalprop(void)
> >      g_test_trap_assert_passed();
> >      g_test_trap_assert_stderr_unmatched("*prop1*");
> >      g_test_trap_assert_stderr_unmatched("*prop2*");
> > -    g_test_trap_assert_stderr("*Warning: global dynamic-prop-type-bad.prop3 has invalid class name\n*");
> >      g_test_trap_assert_stderr_unmatched("*prop4*");
> >      g_test_trap_assert_stderr("*Warning: global nohotplug-type.prop5=105 not used\n*");
> > -    g_test_trap_assert_stderr("*Warning: global nondevice-type.prop6 has invalid class name\n*");
> >      g_test_trap_assert_stdout("");
> >  }
> >  
> > @@ -246,10 +240,8 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
> >      static GlobalProperty props[] = {
> >          { TYPE_DYNAMIC_PROPS, "prop1", "101" },
> >          { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> > -        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" },
> >          { TYPE_UNUSED_HOTPLUG, "prop4", "104" },
> >          { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
> > -        { TYPE_NONDEVICE, "prop6", "106" },
> >          {}
> >      };
> >      int all_used;
> > @@ -267,8 +259,6 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
> >      g_assert(props[1].used);
> >      g_assert(!props[2].used);
> >      g_assert(!props[3].used);
> > -    g_assert(!props[4].used);
> > -    g_assert(!props[5].used);
> >  }
> >  
> 
> The rest of the patch turns a warning into an error, but the test update
> *drops* the affected test cases.  Shouldn't we test the error instead?

We could test the error, but good luck linking vl.o in a unit
test.

> 
> To keep my R-by, you can either do that, or mention the loss of test
> cases in the commit message.

I will mention it in the commit message. Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 03/10] vl: Reject invalid class names on -global

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 08:40:41AM -0300, Eduardo Habkost wrote:
> On Thu, Jun 23, 2016 at 09:52:12AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > 
> > > Instead of just printing a warning very late, reject obviously
> > > invalid -global arguments by validating the class name.
> > >
> > > Update test-qdev-global-props to not expect class name validation
> > > to be performed in qdev_prop_check_globals().
> > >
> > > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Changes v1 -> v2:
> > > * Fix test-qdev-global-props unit test
> > > * Simplify object_dynamic_cast() check
> > >   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > ---
> > >  hw/core/qdev-properties.c      |  7 -------
> > >  tests/test-qdev-global-props.c | 10 ----------
> > >  vl.c                           | 20 +++++++++++++++++---
> > >  3 files changed, 17 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > index c10edee..64e17aa 100644
> > > --- a/hw/core/qdev-properties.c
> > > +++ b/hw/core/qdev-properties.c
> > > @@ -1052,13 +1052,6 @@ int qdev_prop_check_globals(void)
> > >              continue;
> > >          }
> > >          oc = object_class_by_name(prop->driver);
> > > -        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > > -        if (!oc) {
> > > -            error_report("Warning: global %s.%s has invalid class name",
> > > -                       prop->driver, prop->property);
> > > -            ret = 1;
> > > -            continue;
> > > -        }
> > >          dc = DEVICE_CLASS(oc);
> > >          if (!dc->hotpluggable && !prop->used) {
> > >              error_report("Warning: global %s.%s=%s not used",
> > > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> > > index 48e5b73..db77ad9 100644
> > > --- a/tests/test-qdev-global-props.c
> > > +++ b/tests/test-qdev-global-props.c
> > > @@ -201,10 +201,8 @@ static void test_dynamic_globalprop_subprocess(void)
> > >      static GlobalProperty props[] = {
> > >          { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
> > >          { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
> > > -        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
> > >          { TYPE_UNUSED_HOTPLUG, "prop4", "104", true },
> > >          { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
> > > -        { TYPE_NONDEVICE, "prop6", "106", true },
> > >          {}
> > >      };
> > >      int all_used;
> > > @@ -222,8 +220,6 @@ static void test_dynamic_globalprop_subprocess(void)
> > >      g_assert(props[1].used);
> > >      g_assert(!props[2].used);
> > >      g_assert(!props[3].used);
> > > -    g_assert(!props[4].used);
> > > -    g_assert(!props[5].used);
> > >  }
> > >  
> > >  static void test_dynamic_globalprop(void)
> > > @@ -232,10 +228,8 @@ static void test_dynamic_globalprop(void)
> > >      g_test_trap_assert_passed();
> > >      g_test_trap_assert_stderr_unmatched("*prop1*");
> > >      g_test_trap_assert_stderr_unmatched("*prop2*");
> > > -    g_test_trap_assert_stderr("*Warning: global dynamic-prop-type-bad.prop3 has invalid class name\n*");
> > >      g_test_trap_assert_stderr_unmatched("*prop4*");
> > >      g_test_trap_assert_stderr("*Warning: global nohotplug-type.prop5=105 not used\n*");
> > > -    g_test_trap_assert_stderr("*Warning: global nondevice-type.prop6 has invalid class name\n*");
> > >      g_test_trap_assert_stdout("");
> > >  }
> > >  
> > > @@ -246,10 +240,8 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
> > >      static GlobalProperty props[] = {
> > >          { TYPE_DYNAMIC_PROPS, "prop1", "101" },
> > >          { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> > > -        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" },
> > >          { TYPE_UNUSED_HOTPLUG, "prop4", "104" },
> > >          { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
> > > -        { TYPE_NONDEVICE, "prop6", "106" },
> > >          {}
> > >      };
> > >      int all_used;
> > > @@ -267,8 +259,6 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
> > >      g_assert(props[1].used);
> > >      g_assert(!props[2].used);
> > >      g_assert(!props[3].used);
> > > -    g_assert(!props[4].used);
> > > -    g_assert(!props[5].used);
> > >  }
> > >  
> > 
> > The rest of the patch turns a warning into an error, but the test update
> > *drops* the affected test cases.  Shouldn't we test the error instead?
> 
> We could test the error, but good luck linking vl.o in a unit
> test.
> 
> > 
> > To keep my R-by, you can either do that, or mention the loss of test
> > cases in the commit message.
> 
> I will mention it in the commit message. Thanks!

As I am removing the patches that remove
qdev_prop_check_globals() and the used/user_provided fields, I
will change the patch to just add the extra check in vl.c, and
not touch the test code and qdev_prop_check_globals().

I will send v3 soon.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 03/10] vl: Reject invalid class names on -global

[Eduardo Habkost]

On Mon, Jun 20, 2016 at 12:52:56PM -0300, Eduardo Habkost wrote:
> Instead of just printing a warning very late, reject obviously
> invalid -global arguments by validating the class name.
> 
> Update test-qdev-global-props to not expect class name validation
> to be performed in qdev_prop_check_globals().

Added the following to the commit message:

  The checks for the warnings in tests/test-qdev-global-props.c
  are being removed. Unfortunately, adding unit tests for vl.c
  would be quite difficult.

> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Fix test-qdev-global-props unit test
> * Simplify object_dynamic_cast() check
>   * Suggested-by: Markus Armbruster <armbru@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v2 04/10] qdev: GlobalProperty.errp field

[Eduardo Habkost]

The new field will allow error handling to be configured by
qdev_prop_register_global() callers: &error_fatal and
&error_abort can be used to make QEMU exit or abort if any errors
are reported when applying the properties.

While doing it, change the error message from "global %s.%s=%s
ignored" to "can't apply global %s.%s=%s".

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Reword doc comments
  * Suggested-by: Markus Armbruster <armbru@redhat.com>
* Squashed with patch "Use error_prepend() for errors applying
  globals"
  * Suggested-by: Markus Armbruster <armbru@redhat.com>
* Moved to the end of struct to not break test-qdev-global-props
---
 hw/core/qdev-properties.c | 11 ++++++++---
 include/hw/qdev-core.h    |  4 ++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 64e17aa..0fe7214 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1078,9 +1078,14 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
         prop->used = true;
         object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
         if (err != NULL) {
-            assert(prop->user_provided);
-            error_reportf_err(err, "Warning: global %s.%s=%s ignored: ",
-                              prop->driver, prop->property, prop->value);
+            error_prepend(&err, "can't apply global %s.%s=%s: ",
+                          prop->driver, prop->property, prop->value);
+            if (prop->errp) {
+                error_propagate(prop->errp, err);
+            } else {
+                assert(prop->user_provided);
+                error_reportf_err(err, "Warning: ");
+            }
         }
     }
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 24aa0a7..1d1f861 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -259,6 +259,9 @@ struct PropertyInfo {
  * @user_provided: Set to true if property comes from user-provided config
  * (command-line or config file).
  * @used: Set to true if property was used when initializing a device.
+ * @errp: Error destination, used like first argument of error_setg()
+ *        in case property setting fails later. If @errp is NULL, we
+ *        print warnings instead of ignoring errors silently.
  */
 typedef struct GlobalProperty {
     const char *driver;
@@ -266,6 +269,7 @@ typedef struct GlobalProperty {
     const char *value;
     bool user_provided;
     bool used;
+    Error **errp;
 } GlobalProperty;
 
 /*** Board API.  This should go away once we have a machine config file.  ***/
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 04/10] qdev: GlobalProperty.errp field

[Igor Mammedov]

On Mon, 20 Jun 2016 12:52:57 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The new field will allow error handling to be configured by
> qdev_prop_register_global() callers: &error_fatal and
> &error_abort can be used to make QEMU exit or abort if any errors
> are reported when applying the properties.
> 
> While doing it, change the error message from "global %s.%s=%s
> ignored" to "can't apply global %s.%s=%s".
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Reword doc comments
>   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> * Squashed with patch "Use error_prepend() for errors applying
>   globals"
>   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> * Moved to the end of struct to not break test-qdev-global-props
> ---
>  hw/core/qdev-properties.c | 11 ++++++++---
>  include/hw/qdev-core.h    |  4 ++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 64e17aa..0fe7214 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1078,9 +1078,14 @@ static void
> qdev_prop_set_globals_for_type(DeviceState *dev, prop->used = true;
>          object_property_parse(OBJECT(dev), prop->value,
> prop->property, &err); if (err != NULL) {
> -            assert(prop->user_provided);
> -            error_reportf_err(err, "Warning: global %s.%s=%s
> ignored: ",
> -                              prop->driver, prop->property,
> prop->value);
> +            error_prepend(&err, "can't apply global %s.%s=%s: ",
> +                          prop->driver, prop->property, prop->value);
> +            if (prop->errp) {
> +                error_propagate(prop->errp, err);
> +            } else {
> +                assert(prop->user_provided);
> +                error_reportf_err(err, "Warning: ");
this will never print warning as assert will trigger first, but in
8/10 assert is removed so

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> +            }
>          }
>      }
>  }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 24aa0a7..1d1f861 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -259,6 +259,9 @@ struct PropertyInfo {
>   * @user_provided: Set to true if property comes from user-provided
> config
>   * (command-line or config file).
>   * @used: Set to true if property was used when initializing a
> device.
> + * @errp: Error destination, used like first argument of error_setg()
> + *        in case property setting fails later. If @errp is NULL, we
> + *        print warnings instead of ignoring errors silently.
>   */
>  typedef struct GlobalProperty {
>      const char *driver;
> @@ -266,6 +269,7 @@ typedef struct GlobalProperty {
>      const char *value;
>      bool user_provided;
>      bool used;
> +    Error **errp;
>  } GlobalProperty;
>  
>  /*** Board API.  This should go away once we have a machine config
> file.  ***/

Re: [Qemu-devel] [PATCH v2 04/10] qdev: GlobalProperty.errp field

[Eduardo Habkost]

On Tue, Jun 21, 2016 at 10:11:28AM +0200, Igor Mammedov wrote:
> On Mon, 20 Jun 2016 12:52:57 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > The new field will allow error handling to be configured by
> > qdev_prop_register_global() callers: &error_fatal and
> > &error_abort can be used to make QEMU exit or abort if any errors
> > are reported when applying the properties.
> > 
> > While doing it, change the error message from "global %s.%s=%s
> > ignored" to "can't apply global %s.%s=%s".
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> > * Reword doc comments
> >   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> > * Squashed with patch "Use error_prepend() for errors applying
> >   globals"
> >   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> > * Moved to the end of struct to not break test-qdev-global-props
> > ---
> >  hw/core/qdev-properties.c | 11 ++++++++---
> >  include/hw/qdev-core.h    |  4 ++++
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 64e17aa..0fe7214 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1078,9 +1078,14 @@ static void
> > qdev_prop_set_globals_for_type(DeviceState *dev, prop->used = true;
> >          object_property_parse(OBJECT(dev), prop->value,
> > prop->property, &err); if (err != NULL) {
> > -            assert(prop->user_provided);
> > -            error_reportf_err(err, "Warning: global %s.%s=%s
> > ignored: ",
> > -                              prop->driver, prop->property,
> > prop->value);
> > +            error_prepend(&err, "can't apply global %s.%s=%s: ",
> > +                          prop->driver, prop->property, prop->value);
> > +            if (prop->errp) {
> > +                error_propagate(prop->errp, err);
> > +            } else {
> > +                assert(prop->user_provided);
> > +                error_reportf_err(err, "Warning: ");
> this will never print warning as assert will trigger first, but in
> 8/10 assert is removed so
> 

Assert won't trigger and warning will be printed if
(prop->user_provided && !prop->errp).

> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 04/10] qdev: GlobalProperty.errp field

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> The new field will allow error handling to be configured by
> qdev_prop_register_global() callers: &error_fatal and
> &error_abort can be used to make QEMU exit or abort if any errors
> are reported when applying the properties.
>
> While doing it, change the error message from "global %s.%s=%s
> ignored" to "can't apply global %s.%s=%s".
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 05/10] machine: Add machine_register_compat_props() function

[Eduardo Habkost]

Move the compat_props handling to core machine code.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/machine.c   | 16 ++++++++++++++++
 include/hw/boards.h |  1 +
 vl.c                |  9 ++-------
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ccdd5fa..754a054 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -580,6 +580,22 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
     }
 }
 
+void machine_register_compat_props(MachineState *machine)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    int i;
+    GlobalProperty *p;
+
+    if (!mc->compat_props) {
+        return;
+    }
+
+    for (i = 0; i < mc->compat_props->len; i++) {
+        p = g_array_index(mc->compat_props, GlobalProperty *, i);
+        qdev_prop_register_global(p);
+    }
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 3ed6155..3e69eca 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -40,6 +40,7 @@ int machine_kvm_shadow_mem(MachineState *machine);
 int machine_phandle_start(MachineState *machine);
 bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
+void machine_register_compat_props(MachineState *machine);
 
 /**
  * CPUArchId:
diff --git a/vl.c b/vl.c
index 1721a4b..8c40d56 100644
--- a/vl.c
+++ b/vl.c
@@ -4489,13 +4489,8 @@ int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
-    if (machine_class->compat_props) {
-        GlobalProperty *p;
-        for (i = 0; i < machine_class->compat_props->len; i++) {
-            p = g_array_index(machine_class->compat_props, GlobalProperty *, i);
-            qdev_prop_register_global(p);
-        }
-    }
+    machine_register_compat_props(current_machine);
+
     qemu_opts_foreach(qemu_find_opts("global"),
                       global_init_func, NULL, &err);
     if (err) {
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 05/10] machine: Add machine_register_compat_props() function

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> Move the compat_props handling to core machine code.
>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 06/10] vl: Set errp to &error_abort on machine compat_props

[Eduardo Habkost]

Use the new GlobalProperty.errp field to handle compat_props
errors.

Example output before this change:
(with an intentionally broken entry added to PC_COMPAT_1_3 just
for testing)

  $ qemu-system-x86_64 -machine pc-1.3
  qemu-system-x86_64: hw/core/qdev-properties.c:1091: qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed.
  Aborted (core dumped)

After:

  $ qemu-system-x86_64 -machine pc-1.3
  Unexpected error in x86_cpuid_set_vendor() at /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:1688:
  qemu-system-x86_64: can't apply global cpu.vendor=x: Property '.vendor' doesn't take value 'x'
  Aborted (core dumped)

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/machine.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 754a054..d6f6be4 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -592,6 +592,8 @@ void machine_register_compat_props(MachineState *machine)
 
     for (i = 0; i < mc->compat_props->len; i++) {
         p = g_array_index(mc->compat_props, GlobalProperty *, i);
+        /* Machine compat_props must never cause errors: */
+        p->errp = &error_abort;
         qdev_prop_register_global(p);
     }
 }
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 06/10] vl: Set errp to &error_abort on machine compat_props

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> Use the new GlobalProperty.errp field to handle compat_props
> errors.
>
> Example output before this change:
> (with an intentionally broken entry added to PC_COMPAT_1_3 just
> for testing)
>
>   $ qemu-system-x86_64 -machine pc-1.3
>   qemu-system-x86_64: hw/core/qdev-properties.c:1091: qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed.
>   Aborted (core dumped)
>
> After:
>
>   $ qemu-system-x86_64 -machine pc-1.3
>   Unexpected error in x86_cpuid_set_vendor() at /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:1688:
>   qemu-system-x86_64: can't apply global cpu.vendor=x: Property '.vendor' doesn't take value 'x'
>   Aborted (core dumped)
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 07/10] qdev: Eliminate "global not used" warning

[Eduardo Habkost]

qdev_prop_check_globals() tries to warn the user if a given
-global option was not used. But it does that only if the device
is not hotpluggable.

The warning also makes it harder for management code or people
that write their own scripts or config files: there's no way to
know if a given -global option will trigger a warning or not,
because we don't have any introspection mechanism to check if a
machine-type instantiates a given device or not.

The warning is also the only reason we have the 'user_provided'
and 'used' fields in struct GlobalProperty. Removing the check
will let us simplify the code.

In other words, the warning is nearly useless and gets in our way
and our users' way. Remove it.

With this change, we don't need subprocess tricks in
test-qdev-global-props anymore.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Fix test-qdev-global-props unit test
---
 hw/core/qdev-properties.c      | 27 --------------------
 include/hw/qdev-properties.h   |  1 -
 tests/test-qdev-global-props.c | 57 +++---------------------------------------
 vl.c                           |  1 -
 4 files changed, 4 insertions(+), 82 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 0fe7214..c14791d 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1036,33 +1036,6 @@ void qdev_prop_register_global_list(GlobalProperty *props)
     }
 }
 
-int qdev_prop_check_globals(void)
-{
-    GList *l;
-    int ret = 0;
-
-    for (l = global_props; l; l = l->next) {
-        GlobalProperty *prop = l->data;
-        ObjectClass *oc;
-        DeviceClass *dc;
-        if (prop->used) {
-            continue;
-        }
-        if (!prop->user_provided) {
-            continue;
-        }
-        oc = object_class_by_name(prop->driver);
-        dc = DEVICE_CLASS(oc);
-        if (!dc->hotpluggable && !prop->used) {
-            error_report("Warning: global %s.%s=%s not used",
-                       prop->driver, prop->property, prop->value);
-            ret = 1;
-            continue;
-        }
-    }
-    return ret;
-}
-
 static void qdev_prop_set_globals_for_type(DeviceState *dev,
                                 const char *typename)
 {
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 034b75a..3bd5ea9 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -191,7 +191,6 @@ 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_globals(void);
 void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index db77ad9..c0fea84 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -64,7 +64,7 @@ static const TypeInfo static_prop_type = {
 };
 
 /* Test simple static property setting to default value */
-static void test_static_prop_subprocess(void)
+static void test_static_prop(void)
 {
     MyType *mt;
 
@@ -74,16 +74,8 @@ static void test_static_prop_subprocess(void)
     g_assert_cmpuint(mt->prop1, ==, PROP_DEFAULT);
 }
 
-static void test_static_prop(void)
-{
-    g_test_trap_subprocess("/qdev/properties/static/default/subprocess", 0, 0);
-    g_test_trap_assert_passed();
-    g_test_trap_assert_stderr("");
-    g_test_trap_assert_stdout("");
-}
-
 /* Test setting of static property using global properties */
-static void test_static_globalprop_subprocess(void)
+static void test_static_globalprop(void)
 {
     MyType *mt;
     static GlobalProperty props[] = {
@@ -100,14 +92,6 @@ static void test_static_globalprop_subprocess(void)
     g_assert_cmpuint(mt->prop2, ==, PROP_DEFAULT);
 }
 
-static void test_static_globalprop(void)
-{
-    g_test_trap_subprocess("/qdev/properties/static/global/subprocess", 0, 0);
-    g_test_trap_assert_passed();
-    g_test_trap_assert_stderr("");
-    g_test_trap_assert_stdout("");
-}
-
 #define TYPE_DYNAMIC_PROPS "dynamic-prop-type"
 #define DYNAMIC_TYPE(obj) \
     OBJECT_CHECK(MyType, (obj), TYPE_DYNAMIC_PROPS)
@@ -195,7 +179,7 @@ static const TypeInfo nondevice_type = {
 };
 
 /* Test setting of dynamic properties using global properties */
-static void test_dynamic_globalprop_subprocess(void)
+static void test_dynamic_globalprop(void)
 {
     MyType *mt;
     static GlobalProperty props[] = {
@@ -205,7 +189,6 @@ static void test_dynamic_globalprop_subprocess(void)
         { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
         {}
     };
-    int all_used;
 
     qdev_prop_register_global_list(props);
 
@@ -214,27 +197,14 @@ static void test_dynamic_globalprop_subprocess(void)
 
     g_assert_cmpuint(mt->prop1, ==, 101);
     g_assert_cmpuint(mt->prop2, ==, 102);
-    all_used = qdev_prop_check_globals();
-    g_assert_cmpuint(all_used, ==, 1);
     g_assert(props[0].used);
     g_assert(props[1].used);
     g_assert(!props[2].used);
     g_assert(!props[3].used);
 }
 
-static void test_dynamic_globalprop(void)
-{
-    g_test_trap_subprocess("/qdev/properties/dynamic/global/subprocess", 0, 0);
-    g_test_trap_assert_passed();
-    g_test_trap_assert_stderr_unmatched("*prop1*");
-    g_test_trap_assert_stderr_unmatched("*prop2*");
-    g_test_trap_assert_stderr_unmatched("*prop4*");
-    g_test_trap_assert_stderr("*Warning: global nohotplug-type.prop5=105 not used\n*");
-    g_test_trap_assert_stdout("");
-}
-
 /* Test setting of dynamic properties using user_provided=false properties */
-static void test_dynamic_globalprop_nouser_subprocess(void)
+static void test_dynamic_globalprop_nouser(void)
 {
     MyType *mt;
     static GlobalProperty props[] = {
@@ -244,7 +214,6 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
         { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
         {}
     };
-    int all_used;
 
     qdev_prop_register_global_list(props);
 
@@ -253,22 +222,12 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
 
     g_assert_cmpuint(mt->prop1, ==, 101);
     g_assert_cmpuint(mt->prop2, ==, 102);
-    all_used = qdev_prop_check_globals();
-    g_assert_cmpuint(all_used, ==, 0);
     g_assert(props[0].used);
     g_assert(props[1].used);
     g_assert(!props[2].used);
     g_assert(!props[3].used);
 }
 
-static void test_dynamic_globalprop_nouser(void)
-{
-    g_test_trap_subprocess("/qdev/properties/dynamic/global/nouser/subprocess", 0, 0);
-    g_test_trap_assert_passed();
-    g_test_trap_assert_stderr("");
-    g_test_trap_assert_stdout("");
-}
-
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -280,23 +239,15 @@ int main(int argc, char **argv)
     type_register_static(&nohotplug_type);
     type_register_static(&nondevice_type);
 
-    g_test_add_func("/qdev/properties/static/default/subprocess",
-                    test_static_prop_subprocess);
     g_test_add_func("/qdev/properties/static/default",
                     test_static_prop);
 
-    g_test_add_func("/qdev/properties/static/global/subprocess",
-                    test_static_globalprop_subprocess);
     g_test_add_func("/qdev/properties/static/global",
                     test_static_globalprop);
 
-    g_test_add_func("/qdev/properties/dynamic/global/subprocess",
-                    test_dynamic_globalprop_subprocess);
     g_test_add_func("/qdev/properties/dynamic/global",
                     test_dynamic_globalprop);
 
-    g_test_add_func("/qdev/properties/dynamic/global/nouser/subprocess",
-                    test_dynamic_globalprop_nouser_subprocess);
     g_test_add_func("/qdev/properties/dynamic/global/nouser",
                     test_dynamic_globalprop_nouser);
 
diff --git a/vl.c b/vl.c
index 8c40d56..9472a26 100644
--- a/vl.c
+++ b/vl.c
@@ -4631,7 +4631,6 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
-    qdev_prop_check_globals();
     if (vmstate_dump_file) {
         /* dump and exit */
         dump_vmstate_json_to_file(vmstate_dump_file);
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 07/10] qdev: Eliminate "global not used" warning

[Igor Mammedov]

On Mon, 20 Jun 2016 12:53:00 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> qdev_prop_check_globals() tries to warn the user if a given
> -global option was not used. But it does that only if the device
> is not hotpluggable.
> 
> The warning also makes it harder for management code or people
> that write their own scripts or config files: there's no way to
> know if a given -global option will trigger a warning or not,
> because we don't have any introspection mechanism to check if a
> machine-type instantiates a given device or not.
> 
> The warning is also the only reason we have the 'user_provided'
> and 'used' fields in struct GlobalProperty. Removing the check
> will let us simplify the code.
> 
> In other words, the warning is nearly useless and gets in our way
> and our users' way. Remove it.
> 
> With this change, we don't need subprocess tricks in
> test-qdev-global-props anymore.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> Changes v1 -> v2:
> * Fix test-qdev-global-props unit test
> ---
>  hw/core/qdev-properties.c      | 27 --------------------
>  include/hw/qdev-properties.h   |  1 -
>  tests/test-qdev-global-props.c | 57
> +++---------------------------------------
> vl.c                           |  1 - 4 files changed, 4
> insertions(+), 82 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 0fe7214..c14791d 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1036,33 +1036,6 @@ void
> qdev_prop_register_global_list(GlobalProperty *props) }
>  }
>  
> -int qdev_prop_check_globals(void)
> -{
> -    GList *l;
> -    int ret = 0;
> -
> -    for (l = global_props; l; l = l->next) {
> -        GlobalProperty *prop = l->data;
> -        ObjectClass *oc;
> -        DeviceClass *dc;
> -        if (prop->used) {
> -            continue;
> -        }
> -        if (!prop->user_provided) {
> -            continue;
> -        }
> -        oc = object_class_by_name(prop->driver);
> -        dc = DEVICE_CLASS(oc);
> -        if (!dc->hotpluggable && !prop->used) {
> -            error_report("Warning: global %s.%s=%s not used",
> -                       prop->driver, prop->property, prop->value);
> -            ret = 1;
> -            continue;
> -        }
> -    }
> -    return ret;
> -}
> -
>  static void qdev_prop_set_globals_for_type(DeviceState *dev,
>                                  const char *typename)
>  {
> diff --git a/include/hw/qdev-properties.h
> b/include/hw/qdev-properties.h index 034b75a..3bd5ea9 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -191,7 +191,6 @@ 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_globals(void);
>  void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret,
> DeviceState *dev, Property *prop, const char *value);
> diff --git a/tests/test-qdev-global-props.c
> b/tests/test-qdev-global-props.c index db77ad9..c0fea84 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -64,7 +64,7 @@ static const TypeInfo static_prop_type = {
>  };
>  
>  /* Test simple static property setting to default value */
> -static void test_static_prop_subprocess(void)
> +static void test_static_prop(void)
>  {
>      MyType *mt;
>  
> @@ -74,16 +74,8 @@ static void test_static_prop_subprocess(void)
>      g_assert_cmpuint(mt->prop1, ==, PROP_DEFAULT);
>  }
>  
> -static void test_static_prop(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/static/default/subprocess",
> 0, 0);
> -    g_test_trap_assert_passed();
> -    g_test_trap_assert_stderr("");
> -    g_test_trap_assert_stdout("");
> -}
> -
>  /* Test setting of static property using global properties */
> -static void test_static_globalprop_subprocess(void)
> +static void test_static_globalprop(void)
>  {
>      MyType *mt;
>      static GlobalProperty props[] = {
> @@ -100,14 +92,6 @@ static void
> test_static_globalprop_subprocess(void) g_assert_cmpuint(mt->prop2,
> ==, PROP_DEFAULT); }
>  
> -static void test_static_globalprop(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/static/global/subprocess",
> 0, 0);
> -    g_test_trap_assert_passed();
> -    g_test_trap_assert_stderr("");
> -    g_test_trap_assert_stdout("");
> -}
> -
>  #define TYPE_DYNAMIC_PROPS "dynamic-prop-type"
>  #define DYNAMIC_TYPE(obj) \
>      OBJECT_CHECK(MyType, (obj), TYPE_DYNAMIC_PROPS)
> @@ -195,7 +179,7 @@ static const TypeInfo nondevice_type = {
>  };
>  
>  /* Test setting of dynamic properties using global properties */
> -static void test_dynamic_globalprop_subprocess(void)
> +static void test_dynamic_globalprop(void)
>  {
>      MyType *mt;
>      static GlobalProperty props[] = {
> @@ -205,7 +189,6 @@ static void
> test_dynamic_globalprop_subprocess(void) { TYPE_UNUSED_NOHOTPLUG,
> "prop5", "105", true }, {}
>      };
> -    int all_used;
>  
>      qdev_prop_register_global_list(props);
>  
> @@ -214,27 +197,14 @@ static void
> test_dynamic_globalprop_subprocess(void) 
>      g_assert_cmpuint(mt->prop1, ==, 101);
>      g_assert_cmpuint(mt->prop2, ==, 102);
> -    all_used = qdev_prop_check_globals();
> -    g_assert_cmpuint(all_used, ==, 1);
>      g_assert(props[0].used);
>      g_assert(props[1].used);
>      g_assert(!props[2].used);
>      g_assert(!props[3].used);
>  }
>  
> -static void test_dynamic_globalprop(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/dynamic/global/subprocess",
> 0, 0);
> -    g_test_trap_assert_passed();
> -    g_test_trap_assert_stderr_unmatched("*prop1*");
> -    g_test_trap_assert_stderr_unmatched("*prop2*");
> -    g_test_trap_assert_stderr_unmatched("*prop4*");
> -    g_test_trap_assert_stderr("*Warning: global
> nohotplug-type.prop5=105 not used\n*");
> -    g_test_trap_assert_stdout("");
> -}
> -
>  /* Test setting of dynamic properties using user_provided=false
> properties */ -static void
> test_dynamic_globalprop_nouser_subprocess(void) +static void
> test_dynamic_globalprop_nouser(void) {
>      MyType *mt;
>      static GlobalProperty props[] = {
> @@ -244,7 +214,6 @@ static void
> test_dynamic_globalprop_nouser_subprocess(void)
> { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" }, {}
>      };
> -    int all_used;
>  
>      qdev_prop_register_global_list(props);
>  
> @@ -253,22 +222,12 @@ static void
> test_dynamic_globalprop_nouser_subprocess(void) 
>      g_assert_cmpuint(mt->prop1, ==, 101);
>      g_assert_cmpuint(mt->prop2, ==, 102);
> -    all_used = qdev_prop_check_globals();
> -    g_assert_cmpuint(all_used, ==, 0);
>      g_assert(props[0].used);
>      g_assert(props[1].used);
>      g_assert(!props[2].used);
>      g_assert(!props[3].used);
>  }
>  
> -static void test_dynamic_globalprop_nouser(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/dynamic/global/nouser/subprocess",
> 0, 0);
> -    g_test_trap_assert_passed();
> -    g_test_trap_assert_stderr("");
> -    g_test_trap_assert_stdout("");
> -}
> -
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -280,23 +239,15 @@ int main(int argc, char **argv)
>      type_register_static(&nohotplug_type);
>      type_register_static(&nondevice_type);
>  
> -    g_test_add_func("/qdev/properties/static/default/subprocess",
> -                    test_static_prop_subprocess);
>      g_test_add_func("/qdev/properties/static/default",
>                      test_static_prop);
>  
> -    g_test_add_func("/qdev/properties/static/global/subprocess",
> -                    test_static_globalprop_subprocess);
>      g_test_add_func("/qdev/properties/static/global",
>                      test_static_globalprop);
>  
> -    g_test_add_func("/qdev/properties/dynamic/global/subprocess",
> -                    test_dynamic_globalprop_subprocess);
>      g_test_add_func("/qdev/properties/dynamic/global",
>                      test_dynamic_globalprop);
>  
> -
> g_test_add_func("/qdev/properties/dynamic/global/nouser/subprocess",
> -                    test_dynamic_globalprop_nouser_subprocess);
>      g_test_add_func("/qdev/properties/dynamic/global/nouser",
>                      test_dynamic_globalprop_nouser);
>  
> diff --git a/vl.c b/vl.c
> index 8c40d56..9472a26 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4631,7 +4631,6 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> -    qdev_prop_check_globals();
>      if (vmstate_dump_file) {
>          /* dump and exit */
>          dump_vmstate_json_to_file(vmstate_dump_file);

Re: [Qemu-devel] [PATCH v2 07/10] qdev: Eliminate "global not used" warning

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> qdev_prop_check_globals() tries to warn the user if a given
> -global option was not used. But it does that only if the device
> is not hotpluggable.

Because then the option will never be used.

> The warning also makes it harder for management code or people
> that write their own scripts or config files: there's no way to
> know if a given -global option will trigger a warning or not,
> because we don't have any introspection mechanism to check if a
> machine-type instantiates a given device or not.

Well, there is: create the machine, look for the device.  I guess what
you're trying to say is that there's no way to *predict* what a machine
type will instantiate, at least not until after the fact.

Suggest "will instantiate a given device".

> The warning is also the only reason we have the 'user_provided'
> and 'used' fields in struct GlobalProperty. Removing the check
> will let us simplify the code.
>
> In other words, the warning is nearly useless and gets in our way
> and our users' way. Remove it.

Well, telling me that I gave a non-sensical option isn't always useless.
I might have fat-fingered something, or be confused about my devices.

Say I give -global PIIX4_PM.disable_s3=1.  Does what I want with an
i440FX machine type.  Except it does nothing with -no-acpi or with a Q35
machine type.  The former is harmless enough, but the latter is a
mistake; I need to say -global ICH9-LPC.disable_s3=1.  Before this
patch, I get a warning in either case.

Is this warning really useless?

[...]

Re: [Qemu-devel] [PATCH v2 07/10] qdev: Eliminate "global not used" warning

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 10:17:55AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > qdev_prop_check_globals() tries to warn the user if a given
> > -global option was not used. But it does that only if the device
> > is not hotpluggable.
> 
> Because then the option will never be used.
> 
> > The warning also makes it harder for management code or people
> > that write their own scripts or config files: there's no way to
> > know if a given -global option will trigger a warning or not,
> > because we don't have any introspection mechanism to check if a
> > machine-type instantiates a given device or not.
> 
> Well, there is: create the machine, look for the device.  I guess what
> you're trying to say is that there's no way to *predict* what a machine
> type will instantiate, at least not until after the fact.
> 
> Suggest "will instantiate a given device".

I would fix it, but I will drop the patch from the series instead
(see below).

> 
> > The warning is also the only reason we have the 'user_provided'
> > and 'used' fields in struct GlobalProperty. Removing the check
> > will let us simplify the code.
> >
> > In other words, the warning is nearly useless and gets in our way
> > and our users' way. Remove it.
> 
> Well, telling me that I gave a non-sensical option isn't always useless.
> I might have fat-fingered something, or be confused about my devices.
> 
> Say I give -global PIIX4_PM.disable_s3=1.  Does what I want with an
> i440FX machine type.  Except it does nothing with -no-acpi or with a Q35
> machine type.  The former is harmless enough, but the latter is a
> mistake; I need to say -global ICH9-LPC.disable_s3=1.  Before this
> patch, I get a warning in either case.
> 
> Is this warning really useless?

Not entirely useless. But I was giving more importance to code
simplicity than to the detection of those mistakes.

But the extra fields and extra code don't bother me so much. I
will drop patches 07/10 and 08/10 from the series.

-- 
Eduardo

[Qemu-devel] [PATCH v2 08/10] qdev: Eliminate GlobalProperty 'used' and 'user_provided' fields

[Eduardo Habkost]

Those fields are not used for anyting and not needed anymore.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Remove "nouser" test case from test-qdev-global-props
---
 hw/core/qdev-properties.c      |  2 --
 include/hw/qdev-core.h         |  5 -----
 tests/test-qdev-global-props.c | 40 ++++------------------------------------
 vl.c                           |  1 -
 4 files changed, 4 insertions(+), 44 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index c14791d..733cc45 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1048,7 +1048,6 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
         if (strcmp(typename, prop->driver) != 0) {
             continue;
         }
-        prop->used = true;
         object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
         if (err != NULL) {
             error_prepend(&err, "can't apply global %s.%s=%s: ",
@@ -1056,7 +1055,6 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
             if (prop->errp) {
                 error_propagate(prop->errp, err);
             } else {
-                assert(prop->user_provided);
                 error_reportf_err(err, "Warning: ");
             }
         }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1d1f861..7b6b007 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -256,9 +256,6 @@ struct PropertyInfo {
 
 /**
  * GlobalProperty:
- * @user_provided: Set to true if property comes from user-provided config
- * (command-line or config file).
- * @used: Set to true if property was used when initializing a device.
  * @errp: Error destination, used like first argument of error_setg()
  *        in case property setting fails later. If @errp is NULL, we
  *        print warnings instead of ignoring errors silently.
@@ -267,8 +264,6 @@ typedef struct GlobalProperty {
     const char *driver;
     const char *property;
     const char *value;
-    bool user_provided;
-    bool used;
     Error **errp;
 } GlobalProperty;
 
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index c0fea84..37394fa 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -183,10 +183,10 @@ static void test_dynamic_globalprop(void)
 {
     MyType *mt;
     static GlobalProperty props[] = {
-        { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
-        { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
-        { TYPE_UNUSED_HOTPLUG, "prop4", "104", true },
-        { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
+        { TYPE_DYNAMIC_PROPS, "prop1", "101", },
+        { TYPE_DYNAMIC_PROPS, "prop2", "102", },
+        { TYPE_UNUSED_HOTPLUG, "prop4", "104", },
+        { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", },
         {}
     };
 
@@ -197,35 +197,6 @@ static void test_dynamic_globalprop(void)
 
     g_assert_cmpuint(mt->prop1, ==, 101);
     g_assert_cmpuint(mt->prop2, ==, 102);
-    g_assert(props[0].used);
-    g_assert(props[1].used);
-    g_assert(!props[2].used);
-    g_assert(!props[3].used);
-}
-
-/* Test setting of dynamic properties using user_provided=false properties */
-static void test_dynamic_globalprop_nouser(void)
-{
-    MyType *mt;
-    static GlobalProperty props[] = {
-        { TYPE_DYNAMIC_PROPS, "prop1", "101" },
-        { TYPE_DYNAMIC_PROPS, "prop2", "102" },
-        { TYPE_UNUSED_HOTPLUG, "prop4", "104" },
-        { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
-        {}
-    };
-
-    qdev_prop_register_global_list(props);
-
-    mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
-    qdev_init_nofail(DEVICE(mt));
-
-    g_assert_cmpuint(mt->prop1, ==, 101);
-    g_assert_cmpuint(mt->prop2, ==, 102);
-    g_assert(props[0].used);
-    g_assert(props[1].used);
-    g_assert(!props[2].used);
-    g_assert(!props[3].used);
 }
 
 int main(int argc, char **argv)
@@ -248,9 +219,6 @@ int main(int argc, char **argv)
     g_test_add_func("/qdev/properties/dynamic/global",
                     test_dynamic_globalprop);
 
-    g_test_add_func("/qdev/properties/dynamic/global/nouser",
-                    test_dynamic_globalprop_nouser);
-
     g_test_run();
 
     return 0;
diff --git a/vl.c b/vl.c
index 9472a26..ce28fcc 100644
--- a/vl.c
+++ b/vl.c
@@ -2946,7 +2946,6 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
     g->driver   = driver;
     g->property = prop;
     g->value    = qemu_opt_get(opts, "value");
-    g->user_provided = true;
     qdev_prop_register_global(g);
     return 0;
 }
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 08/10] qdev: Eliminate GlobalProperty 'used' and 'user_provided' fields

[Igor Mammedov]

On Mon, 20 Jun 2016 12:53:01 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Those fields are not used for anyting and not needed anymore.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> Changes v1 -> v2:
> * Remove "nouser" test case from test-qdev-global-props
> ---
>  hw/core/qdev-properties.c      |  2 --
>  include/hw/qdev-core.h         |  5 -----
>  tests/test-qdev-global-props.c | 40
> ++++------------------------------------
> vl.c                           |  1 - 4 files changed, 4
> insertions(+), 44 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index c14791d..733cc45 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1048,7 +1048,6 @@ static void
> qdev_prop_set_globals_for_type(DeviceState *dev, if (strcmp(typename,
> prop->driver) != 0) { continue;
>          }
> -        prop->used = true;
>          object_property_parse(OBJECT(dev), prop->value,
> prop->property, &err); if (err != NULL) {
>              error_prepend(&err, "can't apply global %s.%s=%s: ",
> @@ -1056,7 +1055,6 @@ static void
> qdev_prop_set_globals_for_type(DeviceState *dev, if (prop->errp) {
>                  error_propagate(prop->errp, err);
>              } else {
> -                assert(prop->user_provided);
>                  error_reportf_err(err, "Warning: ");
>              }
>          }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 1d1f861..7b6b007 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -256,9 +256,6 @@ struct PropertyInfo {
>  
>  /**
>   * GlobalProperty:
> - * @user_provided: Set to true if property comes from user-provided
> config
> - * (command-line or config file).
> - * @used: Set to true if property was used when initializing a
> device.
>   * @errp: Error destination, used like first argument of error_setg()
>   *        in case property setting fails later. If @errp is NULL, we
>   *        print warnings instead of ignoring errors silently.
> @@ -267,8 +264,6 @@ typedef struct GlobalProperty {
>      const char *driver;
>      const char *property;
>      const char *value;
> -    bool user_provided;
> -    bool used;
>      Error **errp;
>  } GlobalProperty;
>  
> diff --git a/tests/test-qdev-global-props.c
> b/tests/test-qdev-global-props.c index c0fea84..37394fa 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -183,10 +183,10 @@ static void test_dynamic_globalprop(void)
>  {
>      MyType *mt;
>      static GlobalProperty props[] = {
> -        { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
> -        { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
> -        { TYPE_UNUSED_HOTPLUG, "prop4", "104", true },
> -        { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
> +        { TYPE_DYNAMIC_PROPS, "prop1", "101", },
> +        { TYPE_DYNAMIC_PROPS, "prop2", "102", },
> +        { TYPE_UNUSED_HOTPLUG, "prop4", "104", },
> +        { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", },
>          {}
>      };
>  
> @@ -197,35 +197,6 @@ static void test_dynamic_globalprop(void)
>  
>      g_assert_cmpuint(mt->prop1, ==, 101);
>      g_assert_cmpuint(mt->prop2, ==, 102);
> -    g_assert(props[0].used);
> -    g_assert(props[1].used);
> -    g_assert(!props[2].used);
> -    g_assert(!props[3].used);
> -}
> -
> -/* Test setting of dynamic properties using user_provided=false
> properties */ -static void test_dynamic_globalprop_nouser(void)
> -{
> -    MyType *mt;
> -    static GlobalProperty props[] = {
> -        { TYPE_DYNAMIC_PROPS, "prop1", "101" },
> -        { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> -        { TYPE_UNUSED_HOTPLUG, "prop4", "104" },
> -        { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
> -        {}
> -    };
> -
> -    qdev_prop_register_global_list(props);
> -
> -    mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
> -    qdev_init_nofail(DEVICE(mt));
> -
> -    g_assert_cmpuint(mt->prop1, ==, 101);
> -    g_assert_cmpuint(mt->prop2, ==, 102);
> -    g_assert(props[0].used);
> -    g_assert(props[1].used);
> -    g_assert(!props[2].used);
> -    g_assert(!props[3].used);
>  }
>  
>  int main(int argc, char **argv)
> @@ -248,9 +219,6 @@ int main(int argc, char **argv)
>      g_test_add_func("/qdev/properties/dynamic/global",
>                      test_dynamic_globalprop);
>  
> -    g_test_add_func("/qdev/properties/dynamic/global/nouser",
> -                    test_dynamic_globalprop_nouser);
> -
>      g_test_run();
>  
>      return 0;
> diff --git a/vl.c b/vl.c
> index 9472a26..ce28fcc 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2946,7 +2946,6 @@ static int global_init_func(void *opaque,
> QemuOpts *opts, Error **errp) g->driver   = driver;
>      g->property = prop;
>      g->value    = qemu_opt_get(opts, "value");
> -    g->user_provided = true;
>      qdev_prop_register_global(g);
>      return 0;
>  }

[Qemu-devel] [PATCH v2 09/10] qdev: Remove qdev_prop_register_global_list()

[Eduardo Habkost]

The function is not used by any QEMU code, except for the
test-qdev-global-props unit test.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* (New patch in series)
---
 hw/core/qdev-properties.c      |  9 ---------
 include/hw/qdev-properties.h   |  1 -
 tests/test-qdev-global-props.c | 12 ++++++------
 3 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 733cc45..cf1d47f 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1027,15 +1027,6 @@ void qdev_prop_register_global(GlobalProperty *prop)
     global_props = g_list_append(global_props, prop);
 }
 
-void qdev_prop_register_global_list(GlobalProperty *props)
-{
-    int i;
-
-    for (i = 0; props[i].driver != NULL; i++) {
-        qdev_prop_register_global(props+i);
-    }
-}
-
 static void qdev_prop_set_globals_for_type(DeviceState *dev,
                                 const char *typename)
 {
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 3bd5ea9..3d25c00 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -190,7 +190,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
 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);
 void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index 37394fa..29c704f 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -78,12 +78,9 @@ static void test_static_prop(void)
 static void test_static_globalprop(void)
 {
     MyType *mt;
-    static GlobalProperty props[] = {
-        { TYPE_STATIC_PROPS, "prop1", "200" },
-        {}
-    };
+    static GlobalProperty prop = { TYPE_STATIC_PROPS, "prop1", "200" };
 
-    qdev_prop_register_global_list(props);
+    qdev_prop_register_global(&prop);
 
     mt = STATIC_TYPE(object_new(TYPE_STATIC_PROPS));
     qdev_init_nofail(DEVICE(mt));
@@ -182,6 +179,7 @@ static const TypeInfo nondevice_type = {
 static void test_dynamic_globalprop(void)
 {
     MyType *mt;
+    int i;
     static GlobalProperty props[] = {
         { TYPE_DYNAMIC_PROPS, "prop1", "101", },
         { TYPE_DYNAMIC_PROPS, "prop2", "102", },
@@ -190,7 +188,9 @@ static void test_dynamic_globalprop(void)
         {}
     };
 
-    qdev_prop_register_global_list(props);
+    for (i = 0; props[i].driver; i++) {
+        qdev_prop_register_global(props + i);
+    }
 
     mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
     qdev_init_nofail(DEVICE(mt));
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 09/10] qdev: Remove qdev_prop_register_global_list()

[Marcel Apfelbaum]

On 06/20/2016 06:53 PM, Eduardo Habkost wrote:
> The function is not used by any QEMU code, except for the
> test-qdev-global-props unit test.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * (New patch in series)
> ---
>   hw/core/qdev-properties.c      |  9 ---------
>   include/hw/qdev-properties.h   |  1 -
>   tests/test-qdev-global-props.c | 12 ++++++------
>   3 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 733cc45..cf1d47f 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1027,15 +1027,6 @@ void qdev_prop_register_global(GlobalProperty *prop)
>       global_props = g_list_append(global_props, prop);
>   }
>
> -void qdev_prop_register_global_list(GlobalProperty *props)
> -{
> -    int i;
> -
> -    for (i = 0; props[i].driver != NULL; i++) {
> -        qdev_prop_register_global(props+i);
> -    }
> -}
> -
>   static void qdev_prop_set_globals_for_type(DeviceState *dev,
>                                   const char *typename)
>   {
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 3bd5ea9..3d25c00 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -190,7 +190,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
>   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);
>   void qdev_prop_set_globals(DeviceState *dev);
>   void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                       Property *prop, const char *value);
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 37394fa..29c704f 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -78,12 +78,9 @@ static void test_static_prop(void)
>   static void test_static_globalprop(void)
>   {
>       MyType *mt;
> -    static GlobalProperty props[] = {
> -        { TYPE_STATIC_PROPS, "prop1", "200" },
> -        {}
> -    };
> +    static GlobalProperty prop = { TYPE_STATIC_PROPS, "prop1", "200" };
>
> -    qdev_prop_register_global_list(props);
> +    qdev_prop_register_global(&prop);
>
>       mt = STATIC_TYPE(object_new(TYPE_STATIC_PROPS));
>       qdev_init_nofail(DEVICE(mt));
> @@ -182,6 +179,7 @@ static const TypeInfo nondevice_type = {
>   static void test_dynamic_globalprop(void)
>   {
>       MyType *mt;
> +    int i;
>       static GlobalProperty props[] = {
>           { TYPE_DYNAMIC_PROPS, "prop1", "101", },
>           { TYPE_DYNAMIC_PROPS, "prop2", "102", },
> @@ -190,7 +188,9 @@ static void test_dynamic_globalprop(void)
>           {}
>       };
>
> -    qdev_prop_register_global_list(props);
> +    for (i = 0; props[i].driver; i++) {
> +        qdev_prop_register_global(props + i);
> +    }
>
>       mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
>       qdev_init_nofail(DEVICE(mt));
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 09/10] qdev: Remove qdev_prop_register_global_list()

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> The function is not used by any QEMU code, except for the
> test-qdev-global-props unit test.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 10/10] machine: Skip global registration for non-existing classes

[Eduardo Habkost]

MachineClass::compat_props may point to class names that are not
compiled into the QEMU binary. Skip registering those as global
properties. This will allow the qdev global property code to
implement stricter checks on the global property values in the
future.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Simplify object_class_dynamic_cast(oc, TYPE_DEVICE) check
---
 hw/core/machine.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index d6f6be4..97d223b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -583,6 +583,7 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
 void machine_register_compat_props(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    ObjectClass *oc;
     int i;
     GlobalProperty *p;
 
@@ -592,6 +593,13 @@ void machine_register_compat_props(MachineState *machine)
 
     for (i = 0; i < mc->compat_props->len; i++) {
         p = g_array_index(mc->compat_props, GlobalProperty *, i);
+
+        /* Skip registering globals for non-existing device classes */
+        oc = object_class_by_name(p->driver);
+        if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
+            continue;
+        }
+
         /* Machine compat_props must never cause errors: */
         p->errp = &error_abort;
         qdev_prop_register_global(p);
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 10/10] machine: Skip global registration for non-existing classes

[Marcel Apfelbaum]

On 06/20/2016 06:53 PM, Eduardo Habkost wrote:
> MachineClass::compat_props may point to class names that are not
> compiled into the QEMU binary. Skip registering those as global
> properties. This will allow the qdev global property code to
> implement stricter checks on the global property values in the
> future.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Simplify object_class_dynamic_cast(oc, TYPE_DEVICE) check
> ---
>   hw/core/machine.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d6f6be4..97d223b 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -583,6 +583,7 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
>   void machine_register_compat_props(MachineState *machine)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    ObjectClass *oc;
>       int i;
>       GlobalProperty *p;
>
> @@ -592,6 +593,13 @@ void machine_register_compat_props(MachineState *machine)
>
>       for (i = 0; i < mc->compat_props->len; i++) {
>           p = g_array_index(mc->compat_props, GlobalProperty *, i);
> +
> +        /* Skip registering globals for non-existing device classes */
> +        oc = object_class_by_name(p->driver);
> +        if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> +            continue;
> +        }
> +
>           /* Machine compat_props must never cause errors: */
>           p->errp = &error_abort;
>           qdev_prop_register_global(p);
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 10/10] machine: Skip global registration for non-existing classes

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> MachineClass::compat_props may point to class names that are not
> compiled into the QEMU binary. Skip registering those as global
> properties. This will allow the qdev global property code to
> implement stricter checks on the global property values in the
> future.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Not sure what those stricter checks might be, but that's okay.

Doesn't this suppress detection of programming errors?  Say I fat-finger
a device name in my machine type's global properties.  Before this
patch, I get an error (a nice one since PATCH 06).

Re: [Qemu-devel] [PATCH v2 10/10] machine: Skip global registration for non-existing classes

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 10:26:49AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > MachineClass::compat_props may point to class names that are not
> > compiled into the QEMU binary. Skip registering those as global
> > properties. This will allow the qdev global property code to
> > implement stricter checks on the global property values in the
> > future.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Not sure what those stricter checks might be, but that's okay.
> 
> Doesn't this suppress detection of programming errors?  Say I fat-finger
> a device name in my machine type's global properties.

We can't detect that, because compat_props is allowed to include
classes that are disabled at build time.

> Before this patch, I get an error (a nice one since PATCH 06).

You don't. The class name validation in qdev_prop_check_globals()
is done only on user-provided properties.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 10/10] machine: Skip global registration for non-existing classes

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 10:26:49AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > MachineClass::compat_props may point to class names that are not
> > compiled into the QEMU binary. Skip registering those as global
> > properties. This will allow the qdev global property code to
> > implement stricter checks on the global property values in the
> > future.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Not sure what those stricter checks might be, but that's okay.

I would like to make qdev_prop_register_global() validate the
class and property names. It should be doable for the
!cannot_destroy_with_object_finalize_yet classes.

Maybe even the property value could be validated by actually
setting the property in a temporary object, but I am not looking
forward to audit every single property setter in QEMU to ensure
this is safe.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 10/10] machine: Skip global registration for non-existing classes

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Jun 23, 2016 at 10:26:49AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > MachineClass::compat_props may point to class names that are not
>> > compiled into the QEMU binary. Skip registering those as global
>> > properties. This will allow the qdev global property code to
>> > implement stricter checks on the global property values in the
>> > future.
>> >
>> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> 
>> Not sure what those stricter checks might be, but that's okay.
>
> I would like to make qdev_prop_register_global() validate the
> class and property names. It should be doable for the
> !cannot_destroy_with_object_finalize_yet classes.
>
> Maybe even the property value could be validated by actually
> setting the property in a temporary object, but I am not looking
> forward to audit every single property setter in QEMU to ensure
> this is safe.

Dynamically created types and properties --- the gift that keeps giving.

Re: [Qemu-devel] [PATCH v2 00/10] globals: Clean up validation and error checking

[Eduardo Habkost]

Markus, all patches in this series got at least one Reviewed-by
line. I would like to have your Acked-by to merge it through my
tree, so I can apply Igor's -cpu + global property series that
depend on it.

On Mon, Jun 20, 2016 at 12:52:53PM -0300, Eduardo Habkost wrote:
> This series includes multiple changes to the way errors are
> handled by the global property system.
> 
> Git tree:
>   https://github.com/ehabkost/qemu-hacks.git work/global-error-handling
> 
> Changes v1 -> v2:
> * Fix test-qdev-global-props unit test
>   * Remove subprocess code and "nouser" test case
>   * Reported-by: Igor Mammedov <imammedo@redhat.com>
> * Reword doc comments for GlobalProperty::errp
>   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> * Squashed "Use error_prepend() for errors applying globals"
>   and "qdev: GlobalProperty.errp field"
>   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> * Simplify object_class_dynamic_cast(oc, TYPE_DEVICE) checks
>   * Suggested-by: Markus Armbruster <armbru@redhat.com>
> * New patch: "qdev: Remove qdev_prop_register_global_list()"
> 
> Eduardo Habkost (10):
>   qdev: Don't stop applying globals on first error
>   qdev: Eliminate qemu_add_globals() function
>   vl: Reject invalid class names on -global
>   qdev: GlobalProperty.errp field
>   machine: Add machine_register_compat_props() function
>   vl: Set errp to &error_abort on machine compat_props
>   qdev: Eliminate "global not used" warning
>   qdev: Eliminate GlobalProperty 'used' and 'user_provided' fields
>   qdev: Remove qdev_prop_register_global_list()
>   machine: Skip global registration for non-existing classes
> 
>  hw/core/machine.c                |  26 +++++++++
>  hw/core/qdev-properties-system.c |  21 +------
>  hw/core/qdev-properties.c        |  55 +++----------------
>  include/hw/boards.h              |   1 +
>  include/hw/qdev-core.h           |   9 ++-
>  include/hw/qdev-properties.h     |   2 -
>  include/qemu/config-file.h       |   1 -
>  tests/test-qdev-global-props.c   | 115 ++++-----------------------------------
>  vl.c                             |  37 ++++++++++---
>  9 files changed, 80 insertions(+), 187 deletions(-)
> 
> -- 
> 2.5.5
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 00/10] globals: Clean up validation and error checking

[Eduardo Habkost]

On Mon, Jun 20, 2016 at 12:52:53PM -0300, Eduardo Habkost wrote:
> Eduardo Habkost (10):
>   qdev: Don't stop applying globals on first error

Applied to machine-next, after editing commit message.

>   qdev: Eliminate qemu_add_globals() function

Applied to machine-next.

>   vl: Reject invalid class names on -global

Will send v3.

>   qdev: GlobalProperty.errp field

Applied to machine-next.

>   machine: Add machine_register_compat_props() function

Applied to machine-next.

>   vl: Set errp to &error_abort on machine compat_props

Applied to machine-next.

>   qdev: Eliminate "global not used" warning

Dropped.

>   qdev: Eliminate GlobalProperty 'used' and 'user_provided' fields

Dropped.

>   qdev: Remove qdev_prop_register_global_list()

Will send v3.

>   machine: Skip global registration for non-existing classes

Will send v3, with an update in commit message and comments.

-- 
Eduardo