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

[Qemu-devel] [PATCH 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.

The series is based on my machine-next branch, available at:
  https://github.com/ehabkost/qemu.git machine-next

The series itself can be found at:
  https://github.com/ehabkost/qemu-hacks.git work/global-error-handling

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: Use error_prepend() for errors applying globals
  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
  machine: Skip global registration for non-existing classes

 hw/core/machine.c                | 27 +++++++++++++++++++++++
 hw/core/qdev-properties-system.c | 21 +-----------------
 hw/core/qdev-properties.c        | 46 ++++++----------------------------------
 include/hw/boards.h              |  1 +
 include/hw/qdev-core.h           |  9 ++++----
 include/hw/qdev-properties.h     |  1 -
 include/qemu/config-file.h       |  1 -
 vl.c                             | 38 ++++++++++++++++++++++++++-------
 8 files changed, 70 insertions(+), 74 deletions(-)

-- 
2.5.5

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

[Eduardo Habkost]

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>
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

[Qemu-devel] [PATCH 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.

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 45eff56..d2d756a 100644
--- a/vl.c
+++ b/vl.c
@@ -2932,6 +2932,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;
@@ -4466,7 +4479,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

[Qemu-devel] [PATCH 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.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/qdev-properties.c |  7 -------
 vl.c                      | 21 ++++++++++++++++++---
 2 files changed, 18 insertions(+), 10 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/vl.c b/vl.c
index d2d756a..d88ddba 100644
--- a/vl.c
+++ b/vl.c
@@ -2935,10 +2935,21 @@ 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);
+    oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
+    if (!oc) {
+        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);
@@ -4480,7 +4491,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

[Qemu-devel] [PATCH 04/10] qdev: Use error_prepend() for errors applying globals

[Eduardo Habkost]

The same Error* will be used in an error_propagate() call in the
future, so prepend a "can't apply global" prefix to it.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/qdev-properties.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 64e17aa..cd19603 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1079,8 +1079,9 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
         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);
+            error_reportf_err(err, "Warning: ");
         }
     }
 }
-- 
2.5.5

[Qemu-devel] [PATCH 05/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.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/qdev-properties.c | 8 ++++++--
 include/hw/qdev-core.h    | 4 ++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index cd19603..0fe7214 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1078,10 +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_prepend(&err, "can't apply global %s.%s=%s: ",
                           prop->driver, prop->property, prop->value);
-            error_reportf_err(err, "Warning: ");
+            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..16e7cc0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -256,6 +256,9 @@ struct PropertyInfo {
 
 /**
  * GlobalProperty:
+ * @errp: If set, error_propagate() will be called on errors when applying
+ *        the property. &error_abort or &error_fatal may be used to make
+ *        errors automatically abort or exit QEMU.
  * @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.
@@ -264,6 +267,7 @@ typedef struct GlobalProperty {
     const char *driver;
     const char *property;
     const char *value;
+    Error **errp;
     bool user_provided;
     bool used;
 } GlobalProperty;
-- 
2.5.5

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

[Eduardo Habkost]

Move the compat_props handling to core machine code.

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 d268bd0..ee71dc0 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 d88ddba..86d53ca 100644
--- a/vl.c
+++ b/vl.c
@@ -4483,13 +4483,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

[Qemu-devel] [PATCH 07/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>
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

[Qemu-devel] [PATCH 08/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.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/qdev-properties.c    | 27 ---------------------------
 include/hw/qdev-properties.h |  1 -
 vl.c                         |  1 -
 3 files changed, 29 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/vl.c b/vl.c
index 86d53ca..bf3bfa3 100644
--- a/vl.c
+++ b/vl.c
@@ -4623,7 +4623,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

[Qemu-devel] [PATCH 09/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>
---
 hw/core/qdev-properties.c | 2 --
 include/hw/qdev-core.h    | 5 -----
 vl.c                      | 1 -
 3 files changed, 8 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 16e7cc0..5bbd4b0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -259,17 +259,12 @@ struct PropertyInfo {
  * @errp: If set, error_propagate() will be called on errors when applying
  *        the property. &error_abort or &error_fatal may be used to make
  *        errors automatically abort or exit QEMU.
- * @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.
  */
 typedef struct GlobalProperty {
     const char *driver;
     const char *property;
     const char *value;
     Error **errp;
-    bool user_provided;
-    bool used;
 } GlobalProperty;
 
 /*** Board API.  This should go away once we have a machine config file.  ***/
diff --git a/vl.c b/vl.c
index bf3bfa3..ee3e3c8 100644
--- a/vl.c
+++ b/vl.c
@@ -2951,7 +2951,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

[Qemu-devel] [PATCH 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>
---
 hw/core/machine.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index d6f6be4..51697cb 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,14 @@ 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);
+        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
+        if (!oc) {
+            continue;
+        }
+
         /* Machine compat_props must never cause errors: */
         p->errp = &error_abort;
         qdev_prop_register_global(p);
-- 
2.5.5

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

[Marcel Apfelbaum]

On 06/15/2016 11:32 PM, Eduardo Habkost wrote:
> Move the compat_props handling to core machine code.
>
> 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 d268bd0..ee71dc0 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 d88ddba..86d53ca 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4483,13 +4483,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) {
>

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

Thanks,
Marcel

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

[Marcel Apfelbaum]

On 06/15/2016 11:32 PM, Eduardo Habkost wrote:
> 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>
> 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);
>       }
>   }
>

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

Thanks,
Marcel

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

[Marcel Apfelbaum]

On 06/15/2016 11:32 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>
> ---
>   hw/core/machine.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d6f6be4..51697cb 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,14 @@ 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);

Hi Eduardo,

> +
> +        /* Skip registering globals for non-existing device classes */
> +        oc = object_class_by_name(p->driver);

This I understand. It ensure a corresponding class is in QEMU binary.
But even so, if such property is not available should we silently continue?
Maybe the user things the property is set...


> +        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);

This I don't understand. Do we check here?

Thanks,
Marcel

> +        if (!oc) {
> +            continue;
> +        }
> +
>           /* Machine compat_props must never cause errors: */
>           p->errp = &error_abort;
>           qdev_prop_register_global(p);
>

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

[Marcel Apfelbaum]

On 06/15/2016 11:32 PM, Eduardo Habkost wrote:
> 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>
> 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;
>           }
>       }
>   }
>

Is always nice to have more warnings.

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

Thanks,
Marcel

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

[Marcel Apfelbaum]

On 06/19/2016 07:39 PM, Marcel Apfelbaum wrote:
> On 06/15/2016 11:32 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>
>> ---
>>   hw/core/machine.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index d6f6be4..51697cb 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,14 @@ 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);
>
> Hi Eduardo,
>
>> +
>> +        /* Skip registering globals for non-existing device classes */
>> +        oc = object_class_by_name(p->driver);
>
> This I understand. It ensure a corresponding class is in QEMU binary.
> But even so, if such property is not available should we silently continue?
> Maybe the user things the property is set...

Please forgive the spelling errors and disregard the above question.

>
>
>> +        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
>
> This I don't understand. Do we check here?
>

Regarding my last question, it seems you followed an existing pattern
here, however I don't quite understand, we found the class,
why do we need to ensure is a TYPE_DEVICE?

What error case do we want to prevent?

Thanks,
Marcel

> Thanks,
> Marcel
>
>> +        if (!oc) {
>> +            continue;
>> +        }
>> +
>>           /* Machine compat_props must never cause errors: */
>>           p->errp = &error_abort;
>>           qdev_prop_register_global(p);
>>
>

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> 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.

Might want to add "Messed up in commit 25f8dd9."

> 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>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

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

Re: [Qemu-devel] [PATCH 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.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/qdev-properties.c |  7 -------
>  vl.c                      | 21 ++++++++++++++++++---
>  2 files changed, 18 insertions(+), 10 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",

qdev_prop_check_globals() runs between machine creation and the main
loop.

> diff --git a/vl.c b/vl.c
> index d2d756a..d88ddba 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2935,10 +2935,21 @@ 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);
> +    oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> +    if (!oc) {

I'd write

       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);
> @@ -4480,7 +4491,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

This runs right before machine creation.

Any types registered between here and qdev_prop_check_globals() are no
longer visible for class name check.  But nothing should register types
then.

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

Re: [Qemu-devel] [PATCH 04/10] qdev: Use error_prepend() for errors applying globals

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> The same Error* will be used in an error_propagate() call in the
> future, so prepend a "can't apply global" prefix to it.

What future?  A future patch?

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/qdev-properties.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 64e17aa..cd19603 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1079,8 +1079,9 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
>          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);
> +            error_reportf_err(err, "Warning: ");
>          }
>      }
>  }

You reword the error message.  Should be mentioned in the commit
message.

Why do you need to prepend the "Warning: ..." in two steps, first
error_prepend() for the "...", then error_reportf_err() for "Warning: "?
Perhaps it'll become clear later in the series, but it's not obvious
now.

Re: [Qemu-devel] [PATCH 05/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.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/qdev-properties.c | 8 ++++++--
>  include/hw/qdev-core.h    | 4 ++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index cd19603..0fe7214 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1078,10 +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_prepend(&err, "can't apply global %s.%s=%s: ",
>                            prop->driver, prop->property, prop->value);
> -            error_reportf_err(err, "Warning: ");
> +            if (prop->errp) {
> +                error_propagate(prop->errp, err);
> +            } else {
> +                assert(prop->user_provided);
> +                error_reportf_err(err, "Warning: ");
> +            }
>          }
>      }
>  }

Now I understand.  Suggest to squash the previous patch into this one.

> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 24aa0a7..16e7cc0 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -256,6 +256,9 @@ struct PropertyInfo {
>  
>  /**
>   * GlobalProperty:
> + * @errp: If set, error_propagate() will be called on errors when applying
> + *        the property. &error_abort or &error_fatal may be used to make
> + *        errors automatically abort or exit QEMU.

"If set" is awkward, because it's not what we usually mean when we talk
about an error being set.  Suggest "If non-null".

But what makes null special isn't that errors won't be propagated.  In
fact, the code behaves as if they were (propagating to null frees the
error, which is exactly what the code does), *except* for one thing that
isn't mentioned here, but should be: we print a warning.

What about:

 * @errp: Error destination, used like a first argument of error_setg(),
 * except with null @errp, we print warnings instead of ignoring errors
 * silently.

>   * @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.
> @@ -264,6 +267,7 @@ typedef struct GlobalProperty {
>      const char *driver;
>      const char *property;
>      const char *value;
> +    Error **errp;
>      bool user_provided;
>      bool used;
>  } GlobalProperty;

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

[Igor Mammedov]

On Wed, 15 Jun 2016 17:32:45 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The function is just a helper to handle the -global options, it
> can stay in vl.c like most qemu_opts_foreach() calls.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@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 45eff56..d2d756a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2932,6 +2932,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;
> @@ -4466,7 +4479,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

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

[Igor Mammedov]

On Wed, 15 Jun 2016 17:32:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Instead of just printing a warning very late, reject obviously
> invalid -global arguments by validating the class name.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
You are removing check that's used by tests, I'm getting
after applying this patch

   /qdev/properties/dynamic/global: **
ERROR:tests/test-qdev-global-props.c:232:test_dynamic_globalprop: child
process (/qdev/properties/dynamic/global/subprocess [174257]) failed
unexpectedly FAIL GTester: last random seed:
R02S52e72b8bde6002f6683785d9dbe46e76
(pid=174262) /qdev/properties/dynamic/global/nouser: OK FAIL:
tests/test-qdev-global-props

> ---
>  hw/core/qdev-properties.c |  7 -------
>  vl.c                      | 21 ++++++++++++++++++---
>  2 files changed, 18 insertions(+), 10 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/vl.c b/vl.c
> index d2d756a..d88ddba 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2935,10 +2935,21 @@ 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);
> +    oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> +    if (!oc) {
> +        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);
> @@ -4480,7 +4491,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

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

[Igor Mammedov]

On Wed, 15 Jun 2016 17:32:43 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This series includes multiple changes to the way errors are
> handled by the global property system.
Could you fix "make check",
by the end of series it breaks at compile time.

> 
> The series is based on my machine-next branch, available at:
>   https://github.com/ehabkost/qemu.git machine-next
> 
> The series itself can be found at:
>   https://github.com/ehabkost/qemu-hacks.git
> work/global-error-handling
> 
> 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: Use error_prepend() for errors applying globals
>   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
>   machine: Skip global registration for non-existing classes
> 
>  hw/core/machine.c                | 27 +++++++++++++++++++++++
>  hw/core/qdev-properties-system.c | 21 +-----------------
>  hw/core/qdev-properties.c        | 46
> ++++++----------------------------------
> include/hw/boards.h              |  1 +
> include/hw/qdev-core.h           |  9 ++++----
> include/hw/qdev-properties.h     |  1 -
> include/qemu/config-file.h       |  1 -
> vl.c                             | 38
> ++++++++++++++++++++++++++------- 8 files changed, 70 insertions(+),
> 74 deletions(-)
> 

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

[Eduardo Habkost]

On Sun, Jun 19, 2016 at 07:50:56PM +0300, Marcel Apfelbaum wrote:
> On 06/19/2016 07:39 PM, Marcel Apfelbaum wrote:
> > On 06/15/2016 11:32 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>
> > > ---
> > >   hw/core/machine.c | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index d6f6be4..51697cb 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,14 @@ 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);
> > 
> > Hi Eduardo,
> > 
> > > +
> > > +        /* Skip registering globals for non-existing device classes */
> > > +        oc = object_class_by_name(p->driver);
> > 
> > This I understand. It ensure a corresponding class is in QEMU binary.
> > But even so, if such property is not available should we silently continue?
> > Maybe the user things the property is set...
> 
> Please forgive the spelling errors and disregard the above question.
> 
> > 
> > 
> > > +        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > 
> > This I don't understand. Do we check here?
> > 
> 
> Regarding my last question, it seems you followed an existing pattern
> here, however I don't quite understand, we found the class,
> why do we need to ensure is a TYPE_DEVICE?
> 
> What error case do we want to prevent?

Globals are a TYPE_DEVICE feature (implemented in
device_post_init()). Registering globals for non-TYPE_DEVICE
classes don't work.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 04/10] qdev: Use error_prepend() for errors applying globals

[Eduardo Habkost]

On Mon, Jun 20, 2016 at 10:02:38AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > The same Error* will be used in an error_propagate() call in the
> > future, so prepend a "can't apply global" prefix to it.
> 
> What future?  A future patch?
> 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/core/qdev-properties.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 64e17aa..cd19603 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1079,8 +1079,9 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
> >          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);
> > +            error_reportf_err(err, "Warning: ");
> >          }
> >      }
> >  }
> 
> You reword the error message.  Should be mentioned in the commit
> message.

Will mention in in the commit message in v2.

> 
> Why do you need to prepend the "Warning: ..." in two steps, first
> error_prepend() for the "...", then error_reportf_err() for "Warning: "?
> Perhaps it'll become clear later in the series, but it's not obvious
> now.

The split is useful for patch 05/10. I will squash both patches,
as you suggested in another message.

Thanks!

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Jun 20, 2016 at 10:14:55AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
[...]
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 24aa0a7..16e7cc0 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -256,6 +256,9 @@ struct PropertyInfo {
> >  
> >  /**
> >   * GlobalProperty:
> > + * @errp: If set, error_propagate() will be called on errors when applying
> > + *        the property. &error_abort or &error_fatal may be used to make
> > + *        errors automatically abort or exit QEMU.
> 
> "If set" is awkward, because it's not what we usually mean when we talk
> about an error being set.  Suggest "If non-null".

Agreed.

> 
> But what makes null special isn't that errors won't be propagated.  In
> fact, the code behaves as if they were (propagating to null frees the
> error, which is exactly what the code does), *except* for one thing that
> isn't mentioned here, but should be: we print a warning.
> 
> What about:
> 
>  * @errp: Error destination, used like a first argument of error_setg(),
>  * except with null @errp, we print warnings instead of ignoring errors
>  * silently.

Perfect. I will use it. Thanks!

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Jun 20, 2016 at 03:11:00PM +0200, Igor Mammedov wrote:
> On Wed, 15 Jun 2016 17:32:43 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This series includes multiple changes to the way errors are
> > handled by the global property system.
> Could you fix "make check",
> by the end of series it breaks at compile time.

Yes, I will fix it in v2. Thanks!

-- 
Eduardo