[Qemu-devel] [PATCH v3 0/2] Fix virtio-*(-non)-transitional crash on 2.6 machine-types

[Qemu-devel] [PATCH v3 0/2] Fix virtio-*(-non)-transitional crash on 2.6 machine-types

[Eduardo Habkost]

Changes v2 -> v3:
* Don't ignore all errors: only ignore optional properties if
  they don't exist
* Patch 1/3 from v2 was already merged on machine-next

Description of v2:

This is a second attempt to fix the crash reported by Thomas[1].

This keeps the compat property array simple, different from my
first attempt[2].

This also avoids extra complexity on the device code: we don't
need interface name, inheritance tricks, or devices overriding a
compat property after the fact.  The simple absence of the QOM
properties on some device types is enough to make the compat code
skip them.

This series is based on my machine-next branch, because of
conflicts with compat property array cleanups that are already
queued.

[1] http://mid.mail-archive.com/a28d196a-e2fe-a013-a6e2-99ac260f6279@redhat.com
[2] http://mid.mail-archive.com/20190104032226.21428-1-ehabkost@redhat.com

Eduardo Habkost (2):
  globals: Allow global properties to be optional
  virtio: Make disable-legacy/disable-modern compat properties optional

 include/hw/qdev-core.h | 3 +++
 hw/core/machine.c      | 5 +++--
 qom/object.c           | 3 +++
 3 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH v3 1/2] globals: Allow global properties to be optional

[Eduardo Habkost]

Making some global properties optional will let us simplify
compat code when a given property works on most (but not all)
subclasses of a given type.

Device types will be able to opt out from optional compat
properties by simply not registering those properties.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2 -> v3:
* Don't ignore all errors, ignore property only if it doesn't
  exist

Changes v1 -> v2:
* (new patch)

Note: the "An error is fatal for non-hotplugged devices [...]"
comment that appears in the diff scope is inaccurate, but I will
fix that in a separate patch because I don't want this to get
into the way of fixing the crash.
---
 include/hw/qdev-core.h | 3 +++
 qom/object.c           | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bc014c1c9f..29874c8611 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -250,6 +250,8 @@ struct PropertyInfo {
 /**
  * GlobalProperty:
  * @used: Set to true if property was used when initializing a device.
+ * @optional: If set to true, GlobalProperty will be skipped without errors
+ *            if the property doesn't exist.
  *
  * An error is fatal for non-hotplugged devices, when the global is applied.
  */
@@ -258,6 +260,7 @@ typedef struct GlobalProperty {
     const char *property;
     const char *value;
     bool used;
+    bool optional;
 } GlobalProperty;
 
 static inline void
diff --git a/qom/object.c b/qom/object.c
index 4e5226ca12..b8c732063b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -385,6 +385,9 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
         if (object_dynamic_cast(obj, p->driver) == NULL) {
             continue;
         }
+        if (p->optional && !object_property_find(obj, p->property, NULL)) {
+            continue;
+        }
         p->used = true;
         object_property_parse(obj, p->value, p->property, &err);
         if (err != NULL) {
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH v3 1/2] globals: Allow global properties to be optional

[Cornelia Huck]

On Thu, 10 Jan 2019 16:04:57 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Making some global properties optional will let us simplify
> compat code when a given property works on most (but not all)
> subclasses of a given type.
> 
> Device types will be able to opt out from optional compat
> properties by simply not registering those properties.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v2 -> v3:
> * Don't ignore all errors, ignore property only if it doesn't
>   exist
> 
> Changes v1 -> v2:
> * (new patch)
> 
> Note: the "An error is fatal for non-hotplugged devices [...]"
> comment that appears in the diff scope is inaccurate, but I will
> fix that in a separate patch because I don't want this to get
> into the way of fixing the crash.
> ---
>  include/hw/qdev-core.h | 3 +++
>  qom/object.c           | 3 +++
>  2 files changed, 6 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH v3 1/2] globals: Allow global properties to be optional

[Marc-André Lureau]

Hi

On Thu, Jan 10, 2019 at 10:05 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> Making some global properties optional will let us simplify
> compat code when a given property works on most (but not all)
> subclasses of a given type.
>
> Device types will be able to opt out from optional compat
> properties by simply not registering those properties.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v2 -> v3:
> * Don't ignore all errors, ignore property only if it doesn't
>   exist
>
> Changes v1 -> v2:
> * (new patch)
>
> Note: the "An error is fatal for non-hotplugged devices [...]"
> comment that appears in the diff scope is inaccurate, but I will
> fix that in a separate patch because I don't want this to get
> into the way of fixing the crash.
> ---
>  include/hw/qdev-core.h | 3 +++
>  qom/object.c           | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bc014c1c9f..29874c8611 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -250,6 +250,8 @@ struct PropertyInfo {
>  /**
>   * GlobalProperty:
>   * @used: Set to true if property was used when initializing a device.
> + * @optional: If set to true, GlobalProperty will be skipped without errors
> + *            if the property doesn't exist.
>   *
>   * An error is fatal for non-hotplugged devices, when the global is applied.
>   */
> @@ -258,6 +260,7 @@ typedef struct GlobalProperty {
>      const char *property;
>      const char *value;
>      bool used;
> +    bool optional;

with optional/skip_if_missing

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>  } GlobalProperty;
>
>  static inline void
> diff --git a/qom/object.c b/qom/object.c
> index 4e5226ca12..b8c732063b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -385,6 +385,9 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
>          if (object_dynamic_cast(obj, p->driver) == NULL) {
>              continue;
>          }
> +        if (p->optional && !object_property_find(obj, p->property, NULL)) {
> +            continue;
> +        }
>          p->used = true;
>          object_property_parse(obj, p->value, p->property, &err);
>          if (err != NULL) {
> --
> 2.18.0.rc1.1.g3f1ff2140
>

[Qemu-devel] [PATCH v3 2/2] virtio: Make disable-legacy/disable-modern compat properties optional

[Eduardo Habkost]

The disable-legacy and disable-modern properties apply only to
some virtio-pci devices.  Make those properties optional.

This fixes the crash introduced by commit f6e501a28ef9 ("virtio: Provide
version-specific variants of virtio PCI devices"):

  $ qemu-system-x86_64 -machine pc-i440fx-2.6 \
    -device virtio-net-pci-non-transitional
  Unexpected error in object_property_find() at qom/object.c:1092:
  qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \
  global virtio-pci.disable-modern=on: Property '.disable-modern' not found
  Aborted (core dumped)

Reported-by: Thomas Huth <thuth@redhat.com>
Fixes: f6e501a28ef9 ("virtio: Provide version-specific variants of virtio PCI devices")
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/machine.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 5530b71981..a19143aa44 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -91,8 +91,9 @@ const size_t hw_compat_2_7_len = G_N_ELEMENTS(hw_compat_2_7);
 
 GlobalProperty hw_compat_2_6[] = {
     { "virtio-mmio", "format_transport_address", "off" },
-    { "virtio-pci", "disable-modern", "on" },
-    { "virtio-pci", "disable-legacy", "off" },
+    /* Optional because not all virtio-pci devices support legacy mode */
+    { "virtio-pci", "disable-modern", "on",  .optional = true },
+    { "virtio-pci", "disable-legacy", "off", .optional = true },
 };
 const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6);
 
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH v3 2/2] virtio: Make disable-legacy/disable-modern compat properties optional

[Cornelia Huck]

On Thu, 10 Jan 2019 16:04:58 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The disable-legacy and disable-modern properties apply only to
> some virtio-pci devices.  Make those properties optional.
> 
> This fixes the crash introduced by commit f6e501a28ef9 ("virtio: Provide
> version-specific variants of virtio PCI devices"):
> 
>   $ qemu-system-x86_64 -machine pc-i440fx-2.6 \
>     -device virtio-net-pci-non-transitional
>   Unexpected error in object_property_find() at qom/object.c:1092:
>   qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \
>   global virtio-pci.disable-modern=on: Property '.disable-modern' not found
>   Aborted (core dumped)
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Fixes: f6e501a28ef9 ("virtio: Provide version-specific variants of virtio PCI devices")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/machine.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>