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

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

[Eduardo Habkost]

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.

While working on the fix, I stumbled upon another minor bug in
object_apply_global_props(), which I fixed in patch 1/3.

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 (3):
  qom: Don't keep error value between object_property_parse() calls
  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           | 6 ++++--
 3 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH v2 1/3] qom: Don't keep error value between object_property_parse() calls

[Eduardo Habkost]

When handling errp==NULL at object_apply_global_props(), we are
leaving the old error value in `err` after printing a warning.
This makes QEMU crash if two global properties generate warnings:

  $ echo device_add rtl8139 | qemu-system-x86_64 -monitor stdio -global rtl8139.xxx=yyy -global rtl8139.xxx=zzz
  warning: can't apply global rtl8139.xxx=yyy: Property '.xxx' not found
  qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == NULL' failed.
  Aborted (core dumped)

Fix that by making `err` go out of scope immediately after the
warn_report_err() call.

Fixes: 50545b2cc029 "qdev-props: call object_apply_global_props()"
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qom/object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index aa6f3a2a71..4e5226ca12 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -372,7 +372,6 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
 
 void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp)
 {
-    Error *err = NULL;
     int i;
 
     if (!props) {
@@ -381,6 +380,7 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
 
     for (i = 0; i < props->len; i++) {
         GlobalProperty *p = g_ptr_array_index(props, i);
+        Error *err = NULL;
 
         if (object_dynamic_cast(obj, p->driver) == NULL) {
             continue;
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH v2 1/3] qom: Don't keep error value between object_property_parse() calls

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]

On 1/9/19 8:02 PM, Eduardo Habkost wrote:
> When handling errp==NULL at object_apply_global_props(), we are
> leaving the old error value in `err` after printing a warning.
> This makes QEMU crash if two global properties generate warnings:
> 
>   $ echo device_add rtl8139 | qemu-system-x86_64 -monitor stdio -global rtl8139.xxx=yyy -global rtl8139.xxx=zzz
>   warning: can't apply global rtl8139.xxx=yyy: Property '.xxx' not found
>   qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == NULL' failed.
>   Aborted (core dumped)
> 
> Fix that by making `err` go out of scope immediately after the
> warn_report_err() call.
> 
> Fixes: 50545b2cc029 "qdev-props: call object_apply_global_props()"
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qom/object.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index aa6f3a2a71..4e5226ca12 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -372,7 +372,6 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
>  
>  void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp)
>  {
> -    Error *err = NULL;

Could also have been fixed by leaving this line at this scope,...

>      int i;
>  
>      if (!props) {
> @@ -381,6 +380,7 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
>  
>      for (i = 0; i < props->len; i++) {
>          GlobalProperty *p = g_ptr_array_index(props, i);
> +        Error *err = NULL;
>  
>          if (object_dynamic_cast(obj, p->driver) == NULL) {
>              continue;
> 

...and doing 'err = NULL;' after warn_report_err().  That is, it's not
the going out of scope that fixes it per se, but the fact that you
changed to resetting it to NULL on each loop invocation rather than
leaving it pointing at freed memory.  Whether you set to NULL by a
tighter scope initializer or by an assignment doesn't matter, so no need
to respin since your way works.

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 1/3] qom: Don't keep error value between object_property_parse() calls

[Marc-André Lureau]

On Thu, Jan 10, 2019 at 6:04 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> When handling errp==NULL at object_apply_global_props(), we are
> leaving the old error value in `err` after printing a warning.
> This makes QEMU crash if two global properties generate warnings:
>
>   $ echo device_add rtl8139 | qemu-system-x86_64 -monitor stdio -global rtl8139.xxx=yyy -global rtl8139.xxx=zzz
>   warning: can't apply global rtl8139.xxx=yyy: Property '.xxx' not found
>   qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == NULL' failed.
>   Aborted (core dumped)
>
> Fix that by making `err` go out of scope immediately after the
> warn_report_err() call.
>
> Fixes: 50545b2cc029 "qdev-props: call object_apply_global_props()"
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

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

> ---
>  qom/object.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index aa6f3a2a71..4e5226ca12 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -372,7 +372,6 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
>
>  void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp)
>  {
> -    Error *err = NULL;
>      int i;
>
>      if (!props) {
> @@ -381,6 +380,7 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
>
>      for (i = 0; i < props->len; i++) {
>          GlobalProperty *p = g_ptr_array_index(props, i);
> +        Error *err = NULL;
>
>          if (object_dynamic_cast(obj, p->driver) == NULL) {
>              continue;
> --
> 2.18.0.rc1.1.g3f1ff2140
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 1/3] qom: Don't keep error value between object_property_parse() calls

[Cornelia Huck]

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

> When handling errp==NULL at object_apply_global_props(), we are
> leaving the old error value in `err` after printing a warning.
> This makes QEMU crash if two global properties generate warnings:
> 
>   $ echo device_add rtl8139 | qemu-system-x86_64 -monitor stdio -global rtl8139.xxx=yyy -global rtl8139.xxx=zzz
>   warning: can't apply global rtl8139.xxx=yyy: Property '.xxx' not found
>   qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == NULL' failed.
>   Aborted (core dumped)
> 
> Fix that by making `err` go out of scope immediately after the
> warn_report_err() call.
> 
> Fixes: 50545b2cc029 "qdev-props: call object_apply_global_props()"
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qom/object.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[Qemu-devel] [PATCH v2 2/3] 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, or by
making the property setter report an error.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
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           | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bc014c1c9f..463e1ddb1e 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, errors when applying the property won't be
+ *            reported by object_apply_global_props().
  *
  * 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..226ddf0189 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -387,7 +387,9 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
         }
         p->used = true;
         object_property_parse(obj, p->value, p->property, &err);
-        if (err != NULL) {
+        if (p->optional) {
+            error_free(err);
+        } else if (err != NULL) {
             error_prepend(&err, "can't apply global %s.%s=%s: ",
                           p->driver, p->property, p->value);
             /*
-- 
2.18.0.rc1.1.g3f1ff2140

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

[Cornelia Huck]

On Thu, 10 Jan 2019 00:02:58 -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, or by
> making the property setter report an error.

I'm not sure whether we want to conflate "property does not exist" and
"setter returns an error". I can see the value in "apply a setting if
the property exists; if it doesn't exist, the setting is not
applicable". But if you also allow the setter to return an error, you
will not notice if you e.g. have been providing junk in the value and
it errors out everywhere.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> 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           | 4 +++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bc014c1c9f..463e1ddb1e 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, errors when applying the property won't be
> + *            reported by object_apply_global_props().
>   *
>   * 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..226ddf0189 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -387,7 +387,9 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
>          }
>          p->used = true;
>          object_property_parse(obj, p->value, p->property, &err);
> -        if (err != NULL) {
> +        if (p->optional) {
> +            error_free(err);
> +        } else if (err != NULL) {
>              error_prepend(&err, "can't apply global %s.%s=%s: ",
>                            p->driver, p->property, p->value);
>              /*

[Qemu-devel] [PATCH v2 3/3] 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 v2 3/3] virtio: Make disable-legacy/disable-modern compat properties optional

[Marc-André Lureau]

Hi

On Thu, Jan 10, 2019 at 6:05 AM 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(-)
>
> 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 },

Could the generic devices implement a specific interface instead?
virtio-pci-generic?

Adding "optional" handling looks like it may hide some other errors.

>  };
>  const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6);
>
> --
> 2.18.0.rc1.1.g3f1ff2140
>
>


-- 
Marc-André Lureau

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

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> Hi
> 
> On Thu, Jan 10, 2019 at 6:05 AM 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(-)
> >
> > 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 },
> 
> Could the generic devices implement a specific interface instead?
> virtio-pci-generic?
> 
> Adding "optional" handling looks like it may hide some other errors.

I think it would be OK if it only ignored the non-existent property
errors (or better, tested for their existence and then skipped if
they weren't there).

Dave

> >  };
> >  const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6);
> >
> > --
> > 2.18.0.rc1.1.g3f1ff2140
> >
> >
> 
> 
> -- 
> Marc-André Lureau
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Eduardo Habkost]

On Thu, Jan 10, 2019 at 12:35:26PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jan 10, 2019 at 6:05 AM 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(-)
> >
> > 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 },
> 
> Could the generic devices implement a specific interface instead?
> virtio-pci-generic?

This is the kind of complexity I wanted to avoid.  We already
have too many interface names for subsets of PCI and virtio
devices.

> 
> Adding "optional" handling looks like it may hide some other errors.

What if we just make "optional" mean "skip if the property
doesn't exist", as suggested by Cornelia and Dave?

-- 
Eduardo

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

[Michael S. Tsirkin]

On Thu, Jan 10, 2019 at 11:31:23AM -0200, Eduardo Habkost wrote:
> On Thu, Jan 10, 2019 at 12:35:26PM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > On Thu, Jan 10, 2019 at 6:05 AM 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(-)
> > >
> > > 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 },
> > 
> > Could the generic devices implement a specific interface instead?
> > virtio-pci-generic?
> 
> This is the kind of complexity I wanted to avoid.  We already
> have too many interface names for subsets of PCI and virtio
> devices.
> 
> > 
> > Adding "optional" handling looks like it may hide some other errors.
> 
> What if we just make "optional" mean "skip if the property
> doesn't exist", as suggested by Cornelia and Dave?

That's the more standard meaning of the word optional, isn't it?

> -- 
> Eduardo

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

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Thu, Jan 10, 2019 at 11:31:23AM -0200, Eduardo Habkost wrote:
> > On Thu, Jan 10, 2019 at 12:35:26PM +0400, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > On Thu, Jan 10, 2019 at 6:05 AM 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(-)
> > > >
> > > > 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 },
> > > 
> > > Could the generic devices implement a specific interface instead?
> > > virtio-pci-generic?
> > 
> > This is the kind of complexity I wanted to avoid.  We already
> > have too many interface names for subsets of PCI and virtio
> > devices.
> > 
> > > 
> > > Adding "optional" handling looks like it may hide some other errors.
> > 
> > What if we just make "optional" mean "skip if the property
> > doesn't exist", as suggested by Cornelia and Dave?
> 
> That's the more standard meaning of the word optional, isn't it?

Well the word is a separate issue; 'optional' is probably a bad choice
but that's less important than what it actually does.
I'd go for skip_if_missing  which is explicit.

Dave

> 
> > -- 
> > Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Eduardo Habkost]

On Thu, Jan 10, 2019 at 03:01:25PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Thu, Jan 10, 2019 at 11:31:23AM -0200, Eduardo Habkost wrote:
> > > On Thu, Jan 10, 2019 at 12:35:26PM +0400, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Thu, Jan 10, 2019 at 6:05 AM 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(-)
> > > > >
> > > > > 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 },
> > > > 
> > > > Could the generic devices implement a specific interface instead?
> > > > virtio-pci-generic?
> > > 
> > > This is the kind of complexity I wanted to avoid.  We already
> > > have too many interface names for subsets of PCI and virtio
> > > devices.
> > > 
> > > > 
> > > > Adding "optional" handling looks like it may hide some other errors.
> > > 
> > > What if we just make "optional" mean "skip if the property
> > > doesn't exist", as suggested by Cornelia and Dave?
> > 
> > That's the more standard meaning of the word optional, isn't it?
> 
> Well the word is a separate issue; 'optional' is probably a bad choice
> but that's less important than what it actually does.
> I'd go for skip_if_missing  which is explicit.

I'm unsure about that.  To me "optional" means "it can be
missing" and carries the same information as "skip if missing".

What do others think?

-- 
Eduardo

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

[Marc-André Lureau]

On Thu, Jan 10, 2019 at 10:07 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Thu, Jan 10, 2019 at 03:01:25PM +0000, Dr. David Alan Gilbert wrote:
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > On Thu, Jan 10, 2019 at 11:31:23AM -0200, Eduardo Habkost wrote:
> > > > On Thu, Jan 10, 2019 at 12:35:26PM +0400, Marc-André Lureau wrote:
> > > > > Hi
> > > > >
> > > > > On Thu, Jan 10, 2019 at 6:05 AM 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(-)
> > > > > >
> > > > > > 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 },
> > > > >
> > > > > Could the generic devices implement a specific interface instead?
> > > > > virtio-pci-generic?
> > > >
> > > > This is the kind of complexity I wanted to avoid.  We already
> > > > have too many interface names for subsets of PCI and virtio
> > > > devices.
> > > >
> > > > >
> > > > > Adding "optional" handling looks like it may hide some other errors.
> > > >
> > > > What if we just make "optional" mean "skip if the property
> > > > doesn't exist", as suggested by Cornelia and Dave?
> > >
> > > That's the more standard meaning of the word optional, isn't it?
> >
> > Well the word is a separate issue; 'optional' is probably a bad choice
> > but that's less important than what it actually does.
> > I'd go for skip_if_missing  which is explicit.
>
> I'm unsure about that.  To me "optional" means "it can be
> missing" and carries the same information as "skip if missing".
>
> What do others think?

skip_if_missing is more explicit.


-- 
Marc-André Lureau

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

[Cornelia Huck]

On Fri, 11 Jan 2019 01:06:45 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> On Thu, Jan 10, 2019 at 10:07 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Thu, Jan 10, 2019 at 03:01:25PM +0000, Dr. David Alan Gilbert wrote:  
> > > * Michael S. Tsirkin (mst@redhat.com) wrote:  
> > > > On Thu, Jan 10, 2019 at 11:31:23AM -0200, Eduardo Habkost wrote:  
> > > > > On Thu, Jan 10, 2019 at 12:35:26PM +0400, Marc-André Lureau wrote:  
> > > > > > Hi
> > > > > >
> > > > > > On Thu, Jan 10, 2019 at 6:05 AM 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(-)
> > > > > > >
> > > > > > > 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 },  
> > > > > >
> > > > > > Could the generic devices implement a specific interface instead?
> > > > > > virtio-pci-generic?  
> > > > >
> > > > > This is the kind of complexity I wanted to avoid.  We already
> > > > > have too many interface names for subsets of PCI and virtio
> > > > > devices.
> > > > >  
> > > > > >
> > > > > > Adding "optional" handling looks like it may hide some other errors.  
> > > > >
> > > > > What if we just make "optional" mean "skip if the property
> > > > > doesn't exist", as suggested by Cornelia and Dave?  
> > > >
> > > > That's the more standard meaning of the word optional, isn't it?  
> > >
> > > Well the word is a separate issue; 'optional' is probably a bad choice
> > > but that's less important than what it actually does.
> > > I'd go for skip_if_missing  which is explicit.  
> >
> > I'm unsure about that.  To me "optional" means "it can be
> > missing" and carries the same information as "skip if missing".
> >
> > What do others think?  
> 
> skip_if_missing is more explicit.

I'd slightly prefer optional, but skip_if_missing is fine.