[PATCH 0/3] qdev: Fix "info qtree" to show links

[PATCH 0/3] qdev: Fix "info qtree" to show links

[Markus Armbruster]

This is an alternative to Marc-André's "[PATCH] RFC: qdev: add legacy
properties only for those print()-able"
Message-ID: <20251015105419.2975542-1-marcandre.lureau@redhat.com>

Markus Armbruster (3):
  qdev: Change PropertyInfo method print() to return malloc'ed string
  qdev: Fix "info qtree" to show links
  qdev: Legacy properties are now unused, drop

 include/hw/qdev-properties.h     |  2 +-
 hw/core/qdev-properties-system.c |  7 ++---
 hw/core/qdev-properties.c        | 47 --------------------------------
 system/qdev-monitor.c            |  7 ++---
 4 files changed, 7 insertions(+), 56 deletions(-)

-- 
2.49.0

[PATCH 1/3] qdev: Change PropertyInfo method print() to return malloc'ed string

[Markus Armbruster]

Simpler (more so after the next commit), and no risk of truncation
because the caller's buffer is too small.  Performance doesn't matter;
the method is only used for "info qdev".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/qdev-properties.h     | 2 +-
 hw/core/qdev-properties-system.c | 7 +++----
 hw/core/qdev-properties.c        | 9 ++++-----
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0197aa4995..60b8133009 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -34,7 +34,7 @@ struct PropertyInfo {
     const char *description;
     const QEnumLookup *enum_table;
     bool realized_set_allowed; /* allow setting property on realized device */
-    int (*print)(Object *obj, const Property *prop, char *dest, size_t len);
+    char *(*print)(Object *obj, const Property *prop);
     void (*set_default_value)(ObjectProperty *op, const Property *prop);
     ObjectProperty *(*create)(ObjectClass *oc, const char *name,
                               const Property *prop);
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 1f810b7ddf..4e1afaac82 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -865,15 +865,14 @@ out:
     visit_end_alternate(v, (void **) &alt);
 }
 
-static int print_pci_devfn(Object *obj, const Property *prop, char *dest,
-                           size_t len)
+static char *print_pci_devfn(Object *obj, const Property *prop)
 {
     int32_t *ptr = object_field_prop_ptr(obj, prop);
 
     if (*ptr == -1) {
-        return snprintf(dest, len, "<unset>");
+        return g_strdup("<unset>");
     } else {
-        return snprintf(dest, len, "%02x.%x", *ptr >> 3, *ptr & 7);
+        return g_strdup_printf("%02x.%x", *ptr >> 3, *ptr & 7);
     }
 }
 
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index b7e8a89ba5..422a486969 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1117,12 +1117,11 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v,
                                      Error **errp)
 {
     const Property *prop = opaque;
+    char *s;
 
-    char buffer[1024];
-    char *ptr = buffer;
-
-    prop->info->print(obj, prop, buffer, sizeof(buffer));
-    visit_type_str(v, name, &ptr, errp);
+    s = prop->info->print(obj, prop);
+    visit_type_str(v, name, &s, errp);
+    g_free(s);
 }
 
 /**
-- 
2.49.0

[PATCH 2/3] qdev: Fix "info qtree" to show links

[Markus Armbruster]

qdev_print_props() retrieves a property's value from its legacy
property if it exists.  A legacy property is created by
qdev_class_add_legacy_property() when the property has a print()
method or does not have a get() method.

If it has a print() method, the legacy property's value is obtained
from the property's print() method.  This is used to format PCI
addresses nicely, i.e. like 01.3 instead of 11.

Else, if doesn't have a get() method, the legacy property is
unreadable.  "info qtree" silently skips unreadable properties.

Link properties don't have a get() method, and are therefore skipped.
This is wrong, because the underlying QOM property *is* readable.

Change qdev_print_props() to simply use a print() method directly if
it exists, else get the value via QOM.

"info qtree" now shows links fine.  For instance, machine "pc" onboard
device "PIIX4_PM" property "bus" is now visible.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 system/qdev-monitor.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 2ac92d0a07..850f0c6606 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -745,19 +745,18 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, DeviceClass *dc,
     for (int i = 0, n = dc->props_count_; i < n; ++i) {
         const Property *prop = &dc->props_[i];
         char *value;
-        char *legacy_name = g_strdup_printf("legacy-%s", prop->name);
 
-        if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
-            value = object_property_get_str(OBJECT(dev), legacy_name, NULL);
+        if (prop->info->print) {
+            value = prop->info->print(OBJECT(dev), prop);
         } else {
             value = object_property_print(OBJECT(dev), prop->name, true,
                                           NULL);
         }
-        g_free(legacy_name);
 
         if (!value) {
             continue;
         }
+
         qdev_printf("%s = %s\n", prop->name,
                     *value ? value : "<null>");
         g_free(value);
-- 
2.49.0

[PATCH 3/3] qdev: Legacy properties are now unused, drop

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/qdev-properties.c | 46 ---------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 422a486969..46a12652f4 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1108,51 +1108,6 @@ static void qdev_class_add_property(DeviceClass *klass, const char *name,
     object_class_property_set_description(oc, name, prop->info->description);
 }
 
-/**
- * Legacy property handling
- */
-
-static void qdev_get_legacy_property(Object *obj, Visitor *v,
-                                     const char *name, void *opaque,
-                                     Error **errp)
-{
-    const Property *prop = opaque;
-    char *s;
-
-    s = prop->info->print(obj, prop);
-    visit_type_str(v, name, &s, errp);
-    g_free(s);
-}
-
-/**
- * qdev_class_add_legacy_property:
- * @dev: Device to add the property to.
- * @prop: The qdev property definition.
- *
- * Add a legacy QOM property to @dev for qdev property @prop.
- *
- * Legacy properties are string versions of QOM properties.  The format of
- * the string depends on the property type.  Legacy properties are only
- * needed for "info qtree".
- *
- * Do not use this in new code!  QOM Properties added through this interface
- * will be given names in the "legacy" namespace.
- */
-static void qdev_class_add_legacy_property(DeviceClass *dc, const Property *prop)
-{
-    g_autofree char *name = NULL;
-
-    /* Register pointer properties as legacy properties */
-    if (!prop->info->print && prop->info->get) {
-        return;
-    }
-
-    name = g_strdup_printf("legacy-%s", prop->name);
-    object_class_property_add(OBJECT_CLASS(dc), name, "str",
-        prop->info->print ? qdev_get_legacy_property : prop->info->get,
-        NULL, NULL, (Property *)prop);
-}
-
 void device_class_set_props_n(DeviceClass *dc, const Property *props, size_t n)
 {
     /* We used a hole in DeviceClass because that's still a lot. */
@@ -1165,7 +1120,6 @@ void device_class_set_props_n(DeviceClass *dc, const Property *props, size_t n)
     for (size_t i = 0; i < n; ++i) {
         const Property *prop = &props[i];
         assert(prop->name);
-        qdev_class_add_legacy_property(dc, prop);
         qdev_class_add_property(dc, prop->name, prop);
     }
 }
-- 
2.49.0

Re: [PATCH 1/3] qdev: Change PropertyInfo method print() to return malloc'ed string

[Marc-André Lureau]

Hi

On Wed, Oct 22, 2025 at 2:15 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Simpler (more so after the next commit), and no risk of truncation
> because the caller's buffer is too small.  Performance doesn't matter;
> the method is only used for "info qdev".
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/hw/qdev-properties.h     | 2 +-
>  hw/core/qdev-properties-system.c | 7 +++----
>  hw/core/qdev-properties.c        | 9 ++++-----
>  3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0197aa4995..60b8133009 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -34,7 +34,7 @@ struct PropertyInfo {
>      const char *description;
>      const QEnumLookup *enum_table;
>      bool realized_set_allowed; /* allow setting property on realized device */
> -    int (*print)(Object *obj, const Property *prop, char *dest, size_t len);
> +    char *(*print)(Object *obj, const Property *prop);
>      void (*set_default_value)(ObjectProperty *op, const Property *prop);
>      ObjectProperty *(*create)(ObjectClass *oc, const char *name,
>                                const Property *prop);
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 1f810b7ddf..4e1afaac82 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -865,15 +865,14 @@ out:
>      visit_end_alternate(v, (void **) &alt);
>  }
>
> -static int print_pci_devfn(Object *obj, const Property *prop, char *dest,
> -                           size_t len)
> +static char *print_pci_devfn(Object *obj, const Property *prop)
>  {
>      int32_t *ptr = object_field_prop_ptr(obj, prop);
>
>      if (*ptr == -1) {
> -        return snprintf(dest, len, "<unset>");
> +        return g_strdup("<unset>");
>      } else {
> -        return snprintf(dest, len, "%02x.%x", *ptr >> 3, *ptr & 7);
> +        return g_strdup_printf("%02x.%x", *ptr >> 3, *ptr & 7);
>      }
>  }
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index b7e8a89ba5..422a486969 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1117,12 +1117,11 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v,
>                                       Error **errp)
>  {
>      const Property *prop = opaque;
> +    char *s;

Why not g_autofree ?

>
> -    char buffer[1024];
> -    char *ptr = buffer;
> -
> -    prop->info->print(obj, prop, buffer, sizeof(buffer));
> -    visit_type_str(v, name, &ptr, errp);
> +    s = prop->info->print(obj, prop);
> +    visit_type_str(v, name, &s, errp);
> +    g_free(s);

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

>  }
>
>  /**
> --
> 2.49.0
>
>


-- 
Marc-André Lureau

Re: [PATCH 2/3] qdev: Fix "info qtree" to show links

[Marc-André Lureau]

On Wed, Oct 22, 2025 at 2:15 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> qdev_print_props() retrieves a property's value from its legacy
> property if it exists.  A legacy property is created by
> qdev_class_add_legacy_property() when the property has a print()
> method or does not have a get() method.
>
> If it has a print() method, the legacy property's value is obtained
> from the property's print() method.  This is used to format PCI
> addresses nicely, i.e. like 01.3 instead of 11.
>
> Else, if doesn't have a get() method, the legacy property is
> unreadable.  "info qtree" silently skips unreadable properties.
>
> Link properties don't have a get() method, and are therefore skipped.
> This is wrong, because the underlying QOM property *is* readable.
>
> Change qdev_print_props() to simply use a print() method directly if
> it exists, else get the value via QOM.
>
> "info qtree" now shows links fine.  For instance, machine "pc" onboard
> device "PIIX4_PM" property "bus" is now visible.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

> ---
>  system/qdev-monitor.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 2ac92d0a07..850f0c6606 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -745,19 +745,18 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, DeviceClass *dc,
>      for (int i = 0, n = dc->props_count_; i < n; ++i) {
>          const Property *prop = &dc->props_[i];
>          char *value;
> -        char *legacy_name = g_strdup_printf("legacy-%s", prop->name);
>
> -        if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
> -            value = object_property_get_str(OBJECT(dev), legacy_name, NULL);
> +        if (prop->info->print) {
> +            value = prop->info->print(OBJECT(dev), prop);
>          } else {
>              value = object_property_print(OBJECT(dev), prop->name, true,
>                                            NULL);
>          }
> -        g_free(legacy_name);
>
>          if (!value) {
>              continue;
>          }
> +
>          qdev_printf("%s = %s\n", prop->name,
>                      *value ? value : "<null>");
>          g_free(value);
> --
> 2.49.0
>
>


-- 
Marc-André Lureau

Re: [PATCH 3/3] qdev: Legacy properties are now unused, drop

[Marc-André Lureau]

Hi


On Wed, Oct 22, 2025 at 2:15 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

I don't think we have much reasonable way to use those "legacy-*"
properties from qom-get and similar, so it's probably ok to just
remove them without deprecation.

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

> ---
>  hw/core/qdev-properties.c | 46 ---------------------------------------
>  1 file changed, 46 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 422a486969..46a12652f4 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1108,51 +1108,6 @@ static void qdev_class_add_property(DeviceClass *klass, const char *name,
>      object_class_property_set_description(oc, name, prop->info->description);
>  }
>
> -/**
> - * Legacy property handling
> - */
> -
> -static void qdev_get_legacy_property(Object *obj, Visitor *v,
> -                                     const char *name, void *opaque,
> -                                     Error **errp)
> -{
> -    const Property *prop = opaque;
> -    char *s;
> -
> -    s = prop->info->print(obj, prop);
> -    visit_type_str(v, name, &s, errp);
> -    g_free(s);
> -}
> -
> -/**
> - * qdev_class_add_legacy_property:
> - * @dev: Device to add the property to.
> - * @prop: The qdev property definition.
> - *
> - * Add a legacy QOM property to @dev for qdev property @prop.
> - *
> - * Legacy properties are string versions of QOM properties.  The format of
> - * the string depends on the property type.  Legacy properties are only
> - * needed for "info qtree".
> - *
> - * Do not use this in new code!  QOM Properties added through this interface
> - * will be given names in the "legacy" namespace.
> - */
> -static void qdev_class_add_legacy_property(DeviceClass *dc, const Property *prop)
> -{
> -    g_autofree char *name = NULL;
> -
> -    /* Register pointer properties as legacy properties */
> -    if (!prop->info->print && prop->info->get) {
> -        return;
> -    }
> -
> -    name = g_strdup_printf("legacy-%s", prop->name);
> -    object_class_property_add(OBJECT_CLASS(dc), name, "str",
> -        prop->info->print ? qdev_get_legacy_property : prop->info->get,
> -        NULL, NULL, (Property *)prop);
> -}
> -
>  void device_class_set_props_n(DeviceClass *dc, const Property *props, size_t n)
>  {
>      /* We used a hole in DeviceClass because that's still a lot. */
> @@ -1165,7 +1120,6 @@ void device_class_set_props_n(DeviceClass *dc, const Property *props, size_t n)
>      for (size_t i = 0; i < n; ++i) {
>          const Property *prop = &props[i];
>          assert(prop->name);
> -        qdev_class_add_legacy_property(dc, prop);
>          qdev_class_add_property(dc, prop->name, prop);
>      }
>  }
> --
> 2.49.0
>
>


--
Marc-André Lureau

Re: [PATCH 3/3] qdev: Legacy properties are now unused, drop

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
>
> On Wed, Oct 22, 2025 at 2:15 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> I don't think we have much reasonable way to use those "legacy-*"
> properties from qom-get and similar, so it's probably ok to just
> remove them without deprecation.

Almost all of them are unreadable via qom-get:

    (qemu) qom-get /machine/unattached/device[3]/pm bus
    "/machine/i440fx/pci.0"
    (qemu) qom-get /machine/unattached/device[3]/pm legacy-bus
    Error: Property 'PIIX4_PM.legacy-bus' is not readable

Same for all the other properties where the qdev property doesn't have a
.get().

Only the ones with a .print() are readable, i.e. only PCI address
properties:

    (qemu) qom-get /machine/unattached/device[3]/pm addr
    11
    (qemu) qom-get /machine/unattached/device[3]/pm legacy-addr
    "01.3"

PATCH 2's commit message explains why.

If a deprecation period is wanted, I'll replace this patch.

If not, I can work the above into the commit message.

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

Thanks!

Re: [PATCH 1/3] qdev: Change PropertyInfo method print() to return malloc'ed string

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Wed, Oct 22, 2025 at 2:15 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Simpler (more so after the next commit), and no risk of truncation
>> because the caller's buffer is too small.  Performance doesn't matter;
>> the method is only used for "info qdev".
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

[...]

>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index b7e8a89ba5..422a486969 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -1117,12 +1117,11 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v,
>>                                       Error **errp)
>>  {
>>      const Property *prop = opaque;
>> +    char *s;
>
> Why not g_autofree ?

Old habits...  I think it's a wash here, because there's just one path
through the function.

>> -    char buffer[1024];
>> -    char *ptr = buffer;
>> -
>> -    prop->info->print(obj, prop, buffer, sizeof(buffer));
>> -    visit_type_str(v, name, &ptr, errp);
>> +    s = prop->info->print(obj, prop);
>> +    visit_type_str(v, name, &s, errp);
>> +    g_free(s);
>
> otherwise,
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!

Re: [PATCH 2/3] qdev: Fix "info qtree" to show links

[Paolo Bonzini]

On 10/22/25 12:14, Markus Armbruster wrote:
> qdev_print_props() retrieves a property's value from its legacy
> property if it exists.  A legacy property is created by
> qdev_class_add_legacy_property() when the property has a print()
> method or does not have a get() method.
> 
> If it has a print() method, the legacy property's value is obtained
> from the property's print() method.  This is used to format PCI
> addresses nicely, i.e. like 01.3 instead of 11.
> 
> Else, if doesn't have a get() method, the legacy property is
> unreadable.  "info qtree" silently skips unreadable properties.
> 
> Link properties don't have a get() method, and are therefore skipped.
> This is wrong, because the underlying QOM property *is* readable.
> 
> Change qdev_print_props() to simply use a print() method directly if
> it exists, else get the value via QOM.
> 
> "info qtree" now shows links fine.  For instance, machine "pc" onboard
> device "PIIX4_PM" property "bus" is now visible.

It's been many years, but I think the original idea was that dc->props_ 
would be replaced with walking QOM properties.

I'm not opposed to the patch, but it would put the plan in the coffin so 
I thought I'd point that out.

In the meanwhile I queued patch 1, which is an obviously good idea.

Paolo

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   system/qdev-monitor.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 2ac92d0a07..850f0c6606 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -745,19 +745,18 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, DeviceClass *dc,
>       for (int i = 0, n = dc->props_count_; i < n; ++i) {
>           const Property *prop = &dc->props_[i];
>           char *value;
> -        char *legacy_name = g_strdup_printf("legacy-%s", prop->name);
>   
> -        if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
> -            value = object_property_get_str(OBJECT(dev), legacy_name, NULL);
> +        if (prop->info->print) {
> +            value = prop->info->print(OBJECT(dev), prop);
>           } else {
>               value = object_property_print(OBJECT(dev), prop->name, true,
>                                             NULL);
>           }
> -        g_free(legacy_name);
>   
>           if (!value) {
>               continue;
>           }
> +
>           qdev_printf("%s = %s\n", prop->name,
>                       *value ? value : "<null>");
>           g_free(value);

Re: [PATCH 2/3] qdev: Fix "info qtree" to show links

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/22/25 12:14, Markus Armbruster wrote:
>> qdev_print_props() retrieves a property's value from its legacy
>> property if it exists.  A legacy property is created by
>> qdev_class_add_legacy_property() when the property has a print()
>> method or does not have a get() method.
>>
>> If it has a print() method, the legacy property's value is obtained
>> from the property's print() method.  This is used to format PCI
>> addresses nicely, i.e. like 01.3 instead of 11.
>>
>> Else, if doesn't have a get() method, the legacy property is
>> unreadable.  "info qtree" silently skips unreadable properties.
>>
>> Link properties don't have a get() method, and are therefore skipped.
>> This is wrong, because the underlying QOM property *is* readable.
>>
>> Change qdev_print_props() to simply use a print() method directly if
>> it exists, else get the value via QOM.
>>
>> "info qtree" now shows links fine.  For instance, machine "pc" onboard
>> device "PIIX4_PM" property "bus" is now visible.
>
> It's been many years, but I think the original idea was that dc->props_ would be replaced with walking QOM properties.
>
> I'm not opposed to the patch, but it would put the plan in the coffin so I thought I'd point that out.

I'd argue that legacy properties are a questionable hack to preserve a
specific solution to a problem.

The problem: PCI addresses are integers in C and in QOM.  Makes sense.
But "info qtree" has always displayed PCI addresses in the form DEV.FN,
which also makes sense.

The pre-QOM solution: qdev property method .get() returns the integer,
.print() formats it for humans.  "info qtree" used the latter.

Aside: "format for humans" may well be more widely applicable, if we
care.

The current QOM solution: QOM has no concept "format for humans", it has
only .get().  Instead of introducing the concept, we added "legacy
properties" whose .get() method wraps around qdev's .print() instead of
qdev's .get().

This created yet another accidental external interface.  Fixable the
same way as other accidentally exposed properties: add the means to
restrict access to C.

How legacy properties work and how they're used is less than clear.
Evidence: I was rather confused, and had to dig through quite a bit of
code to unconfuse myself.  I guess that would also be fixable to a
degree with comments.

My proposed solution: bypass QOM, use qdev directly.  Quite a bit
simpler.  No need for additional comments, I hope.  Kills the accidental
external interface.

A possible future solution: add the concept to QOM.  Then we could walk
QOM properties instead of dc->props_.  So, it's not quite the coffin,
more like the freezer.

> In the meanwhile I queued patch 1, which is an obviously good idea.

Thanks!

Re: [PATCH 2/3] qdev: Fix "info qtree" to show links

[Daniel P. Berrangé]

On Tue, Oct 28, 2025 at 11:33:31AM +0100, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 10/22/25 12:14, Markus Armbruster wrote:
> >> qdev_print_props() retrieves a property's value from its legacy
> >> property if it exists.  A legacy property is created by
> >> qdev_class_add_legacy_property() when the property has a print()
> >> method or does not have a get() method.
> >>
> >> If it has a print() method, the legacy property's value is obtained
> >> from the property's print() method.  This is used to format PCI
> >> addresses nicely, i.e. like 01.3 instead of 11.
> >>
> >> Else, if doesn't have a get() method, the legacy property is
> >> unreadable.  "info qtree" silently skips unreadable properties.
> >>
> >> Link properties don't have a get() method, and are therefore skipped.
> >> This is wrong, because the underlying QOM property *is* readable.
> >>
> >> Change qdev_print_props() to simply use a print() method directly if
> >> it exists, else get the value via QOM.
> >>
> >> "info qtree" now shows links fine.  For instance, machine "pc" onboard
> >> device "PIIX4_PM" property "bus" is now visible.
> >
> > It's been many years, but I think the original idea was that dc->props_ would be replaced with walking QOM properties.
> >
> > I'm not opposed to the patch, but it would put the plan in the coffin so I thought I'd point that out.
> 
> I'd argue that legacy properties are a questionable hack to preserve a
> specific solution to a problem.
> 
> The problem: PCI addresses are integers in C and in QOM.  Makes sense.
> But "info qtree" has always displayed PCI addresses in the form DEV.FN,
> which also makes sense.
> 
> The pre-QOM solution: qdev property method .get() returns the integer,
> .print() formats it for humans.  "info qtree" used the latter.
> 
> Aside: "format for humans" may well be more widely applicable, if we
> care.

The scope of the DEV.FN hack is worse than that - with PCI addresses,
while most of the time we just pass DEV, the QAPI also accepts it in
DEV.FN format for the 'addr' property and libvirt relies on that.

Here are two example CLIs that libvirt would generate when configuring
multi-function PCI placement:

For PCI-E (q35)

  -device '{"driver":"pcie-root-port",
            "port":19,
            "chassis":16,
            "id":"pci.16",
            "bus":"pcie.0",
            "multifunction":true,
            "addr":"0x2.0x3"}'

For PCI (i440fx)

  -device '{"driver":"lsi",
            "id":"scsi2",
            "bus":"pci.0",
            "multifunction":true,
            "addr":"0x4.0x1"}'

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/3] qdev: Fix "info qtree" to show links

[Peter Maydell]

On Tue, 28 Oct 2025 at 10:34, Markus Armbruster <armbru@redhat.com> wrote:
> The problem: PCI addresses are integers in C and in QOM.  Makes sense.
> But "info qtree" has always displayed PCI addresses in the form DEV.FN,
> which also makes sense.
>
> The pre-QOM solution: qdev property method .get() returns the integer,
> .print() formats it for humans.  "info qtree" used the latter.
>
> Aside: "format for humans" may well be more widely applicable, if we
> care.

Relatedly, there are various places where we define a "string" QOM
property and then format that into an underlying enum (though
it is also nice not to have the enum values / representation
not be public facing ABI)...

-- PMM

Re: [PATCH 2/3] qdev: Fix "info qtree" to show links

[Paolo Bonzini]

On Tue, Oct 28, 2025 at 12:04 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 28 Oct 2025 at 10:34, Markus Armbruster <armbru@redhat.com> wrote:
> > The problem: PCI addresses are integers in C and in QOM.  Makes sense.
> > But "info qtree" has always displayed PCI addresses in the form DEV.FN,
> > which also makes sense.
> >
> > The pre-QOM solution: qdev property method .get() returns the integer,
> > .print() formats it for humans.  "info qtree" used the latter.
> >
> > Aside: "format for humans" may well be more widely applicable, if we
> > care.
>
> Relatedly, there are various places where we define a "string" QOM
> property and then format that into an underlying enum

That is intended, a QOM property type can be any QAPI type (in
addition to link<class> and child<class>) and therefore it can be the
name of an enum.

The PCI address case was done like this (I imagine) to avoid exposing
it as a string.

Paolo

Re: [PATCH 2/3] qdev: Fix "info qtree" to show links

[Paolo Bonzini]

On Tue, Oct 28, 2025 at 11:59 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Tue, Oct 28, 2025 at 11:33:31AM +0100, Markus Armbruster wrote:
> > Aside: "format for humans" may well be more widely applicable, if we
> > care.
>
> The scope of the DEV.FN hack is worse than that - with PCI addresses,
> while most of the time we just pass DEV, the QAPI also accepts it in
> DEV.FN format for the 'addr' property and libvirt relies on that.

That's fine, it's "just" a QAPI alternate type. A bit weird, but it
works and is used by other QAPI side.

The getter (printing) side is where the mess is.

Paolo