[Qemu-devel] qdev property listing broken

[Qemu-devel] qdev property listing broken

[Jan Kiszka]

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

Hi Anthony,

I noticed that only properties with legacy names are printed via info
qtree. After digging through the qdev and qom property maze, it turned
out the property registration in qdev_property_add_legacy and
qdev_property_add_static is not consistent with the access in
qdev_print_props. The latter assumes all properties are strings, the
former generate the full set of types - and add_legacy obviously an
inconsistent one, dependent on the existence of print/parse handlers. I
fail to see the right direction, ie. where to fix this. Can you provide
a hint?

Jan

PS: It's really no fun to understand and debug this code anymore.
Hopefully, the removal of the qdev layer can improve this again.


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

Re: [Qemu-devel] qdev property listing broken

[Paolo Bonzini]

Il 02/04/2012 21:33, Jan Kiszka ha scritto:
> Hi Anthony,
> 
> I noticed that only properties with legacy names are printed via info
> qtree. After digging through the qdev and qom property maze, it turned
> out the property registration in qdev_property_add_legacy and
> qdev_property_add_static is not consistent with the access in
> qdev_print_props. The latter assumes all properties are strings, the
> former generate the full set of types - and add_legacy obviously an
> inconsistent one, dependent on the existence of print/parse handlers. I
> fail to see the right direction, ie. where to fix this. Can you provide
> a hint?

Actually the patch is trivial.  Pardon the likely whitespace damage,
I'll send it properly tomorrow morning (it was ready but today I
didn't have the time to test the whole series properly).

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index a310cc7..923519c 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -157,7 +157,7 @@ int qdev_device_help(QemuOpts *opts)
          * for removal.  This conditional should be removed along with
          * it.
          */
-        if (!prop->info->parse) {
+        if (!prop->info->get) {
             continue;           /* no way to set it, don't show */
         }
         error_printf("%s.%s=%s\n", driver, prop->name,
@@ -165,7 +165,7 @@ int qdev_device_help(QemuOpts *opts)
     }
     if (info->bus_info) {
         for (prop = info->bus_info->props; prop && prop->name; prop++) {
-            if (!prop->info->parse) {
+            if (!prop->info->get) {
                 continue;           /* no way to set it, don't show */
             }
             error_printf("%s.%s=%s\n", driver, prop->name,

Paolo

Re: [Qemu-devel] qdev property listing broken

[Anthony Liguori]

On 04/02/2012 02:33 PM, Jan Kiszka wrote:
> Hi Anthony,
>
> I noticed that only properties with legacy names are printed via info
> qtree.

Yes.  Is that not what you expect?

The qom properties and model is not stable for 1.1 so I was very careful in 
making sure they didn't leak into info qtree.

> After digging through the qdev and qom property maze, it turned
> out the property registration in qdev_property_add_legacy and
> qdev_property_add_static is not consistent with the access in
> qdev_print_props.

qdev_print_props will completely die for 1.2 as will almost everything related 
to the human monitor in qdev-monitor.c.

I just wanted to give us a full release to make sure we were happy with the 
various interfaces.

> The latter assumes all properties are strings, the
> former generate the full set of types - and add_legacy obviously an
> inconsistent one, dependent on the existence of print/parse handlers. I
> fail to see the right direction, ie. where to fix this. Can you provide
> a hint?

Is there a reason you're using info qtree instead of qom-list?  qom-list gives 
you much more info than info qtree.

Regards,

Anthony Liguori


> Jan
>
> PS: It's really no fun to understand and debug this code anymore.
> Hopefully, the removal of the qdev layer can improve this again.

Re: [Qemu-devel] qdev property listing broken

[Anthony Liguori]

On 04/02/2012 02:40 PM, Paolo Bonzini wrote:
> Il 02/04/2012 21:33, Jan Kiszka ha scritto:
>> Hi Anthony,
>>
>> I noticed that only properties with legacy names are printed via info
>> qtree. After digging through the qdev and qom property maze, it turned
>> out the property registration in qdev_property_add_legacy and
>> qdev_property_add_static is not consistent with the access in
>> qdev_print_props. The latter assumes all properties are strings, the
>> former generate the full set of types - and add_legacy obviously an
>> inconsistent one, dependent on the existence of print/parse handlers. I
>> fail to see the right direction, ie. where to fix this. Can you provide
>> a hint?
>
> Actually the patch is trivial.  Pardon the likely whitespace damage,
> I'll send it properly tomorrow morning (it was ready but today I
> didn't have the time to test the whole series properly).
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index a310cc7..923519c 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -157,7 +157,7 @@ int qdev_device_help(QemuOpts *opts)
>            * for removal.  This conditional should be removed along with
>            * it.
>            */
> -        if (!prop->info->parse) {
> +        if (!prop->info->get) {
>               continue;           /* no way to set it, don't show */
>           }
>           error_printf("%s.%s=%s\n", driver, prop->name,

Oh, I misunderstood but I understand now.  This is a regression.  Thanks Paolo.

Regards,

Anthony Liguori

> @@ -165,7 +165,7 @@ int qdev_device_help(QemuOpts *opts)
>       }
>       if (info->bus_info) {
>           for (prop = info->bus_info->props; prop&&  prop->name; prop++) {
> -            if (!prop->info->parse) {
> +            if (!prop->info->get) {
>                   continue;           /* no way to set it, don't show */
>               }
>               error_printf("%s.%s=%s\n", driver, prop->name,
>
> Paolo
>
>

Re: [Qemu-devel] qdev property listing broken

[Jan Kiszka]

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

On 2012-04-02 21:41, Anthony Liguori wrote:
> On 04/02/2012 02:33 PM, Jan Kiszka wrote:
>> Hi Anthony,
>>
>> I noticed that only properties with legacy names are printed via info
>> qtree.
> 
> Yes.  Is that not what you expect?
> 
> The qom properties and model is not stable for 1.1 so I was very careful
> in making sure they didn't leak into info qtree.
> 
>> After digging through the qdev and qom property maze, it turned
>> out the property registration in qdev_property_add_legacy and
>> qdev_property_add_static is not consistent with the access in
>> qdev_print_props.
> 
> qdev_print_props will completely die for 1.2 as will almost everything
> related to the human monitor in qdev-monitor.c.
> 
> I just wanted to give us a full release to make sure we were happy with
> the various interfaces.

This is broken as previously listed qdev properties disappeared.

> 
>> The latter assumes all properties are strings, the
>> former generate the full set of types - and add_legacy obviously an
>> inconsistent one, dependent on the existence of print/parse handlers. I
>> fail to see the right direction, ie. where to fix this. Can you provide
>> a hint?
> 
> Is there a reason you're using info qtree instead of qom-list?  qom-list
> gives you much more info than info qtree.

I'm using the monitor.

Jan


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

Re: [Qemu-devel] qdev property listing broken

[Anthony Liguori]

On 04/02/2012 02:44 PM, Jan Kiszka wrote:
> On 2012-04-02 21:41, Anthony Liguori wrote:
>> On 04/02/2012 02:33 PM, Jan Kiszka wrote:
>>> Hi Anthony,
>>>
>>> I noticed that only properties with legacy names are printed via info
>>> qtree.
>>
>> Yes.  Is that not what you expect?
>>
>> The qom properties and model is not stable for 1.1 so I was very careful
>> in making sure they didn't leak into info qtree.
>>
>>> After digging through the qdev and qom property maze, it turned
>>> out the property registration in qdev_property_add_legacy and
>>> qdev_property_add_static is not consistent with the access in
>>> qdev_print_props.
>>
>> qdev_print_props will completely die for 1.2 as will almost everything
>> related to the human monitor in qdev-monitor.c.
>>
>> I just wanted to give us a full release to make sure we were happy with
>> the various interfaces.
>
> This is broken as previously listed qdev properties disappeared.

Yes, I misunderstood "legacy" names to include static properties too.  You meant 
non-legacy static properties are broken.

Pure qom properties are not displayed and never were displayed.

Regards,

Anthony Liguori

>
>>
>>> The latter assumes all properties are strings, the
>>> former generate the full set of types - and add_legacy obviously an
>>> inconsistent one, dependent on the existence of print/parse handlers. I
>>> fail to see the right direction, ie. where to fix this. Can you provide
>>> a hint?
>>
>> Is there a reason you're using info qtree instead of qom-list?  qom-list
>> gives you much more info than info qtree.
>
> I'm using the monitor.
>
> Jan

Re: [Qemu-devel] qdev property listing broken

[Jan Kiszka]

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

On 2012-04-02 21:48, Anthony Liguori wrote:
> On 04/02/2012 02:44 PM, Jan Kiszka wrote:
>> On 2012-04-02 21:41, Anthony Liguori wrote:
>>> On 04/02/2012 02:33 PM, Jan Kiszka wrote:
>>>> Hi Anthony,
>>>>
>>>> I noticed that only properties with legacy names are printed via info
>>>> qtree.
>>>
>>> Yes.  Is that not what you expect?
>>>
>>> The qom properties and model is not stable for 1.1 so I was very careful
>>> in making sure they didn't leak into info qtree.
>>>
>>>> After digging through the qdev and qom property maze, it turned
>>>> out the property registration in qdev_property_add_legacy and
>>>> qdev_property_add_static is not consistent with the access in
>>>> qdev_print_props.
>>>
>>> qdev_print_props will completely die for 1.2 as will almost everything
>>> related to the human monitor in qdev-monitor.c.
>>>
>>> I just wanted to give us a full release to make sure we were happy with
>>> the various interfaces.
>>
>> This is broken as previously listed qdev properties disappeared.
> 
> Yes, I misunderstood "legacy" names to include static properties too. 
> You meant non-legacy static properties are broken.

Precisely. They are no longer listed.

> 
> Pure qom properties are not displayed and never were displayed.

Yes, I'm interested in those properties I can set on VM creation.

Jan


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

Re: [Qemu-devel] qdev property listing broken

[Jan Kiszka]

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

On 2012-04-02 21:41, Anthony Liguori wrote:
> Is there a reason you're using info qtree instead of qom-list?  qom-list
> gives you much more info than info qtree.

BTW, the qom-list output is still, well, sparsely populated. Also, there
is a "-r" (for recursive) switch missing to make it similar useful one
day. And we will likely need a few pretty-printing controls.

Jan


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

Re: [Qemu-devel] qdev property listing broken

[Peter Maydell]

On 2 April 2012 20:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index a310cc7..923519c 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -157,7 +157,7 @@ int qdev_device_help(QemuOpts *opts)
>          * for removal.  This conditional should be removed along with
>          * it.
>          */
> -        if (!prop->info->parse) {
> +        if (!prop->info->get) {
>             continue;           /* no way to set it, don't show */
>         }

This looks really weird: there's no "get" method so we conclude
"no way to *set* it" ?? Is the comment wrong?

-- PMM

Re: [Qemu-devel] qdev property listing broken

[Anthony Liguori]

On 04/02/2012 03:07 PM, Jan Kiszka wrote:
> On 2012-04-02 21:41, Anthony Liguori wrote:
>> Is there a reason you're using info qtree instead of qom-list?  qom-list
>> gives you much more info than info qtree.
>
> BTW, the qom-list output is still, well, sparsely populated. Also, there
> is a "-r" (for recursive) switch missing to make it similar useful one
> day. And we will likely need a few pretty-printing controls.

Yes, I'd like to move this all into the monitor too FWIW for 1.2.

Regards,

Anthony Liguori

>
> Jan
>

Re: [Qemu-devel] qdev property listing broken

[Paolo Bonzini]

Il 02/04/2012 22:07, Peter Maydell ha scritto:
> On 2 April 2012 20:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> index a310cc7..923519c 100644
>> --- a/hw/qdev-monitor.c
>> +++ b/hw/qdev-monitor.c
>> @@ -157,7 +157,7 @@ int qdev_device_help(QemuOpts *opts)
>>          * for removal.  This conditional should be removed along with
>>          * it.
>>          */
>> -        if (!prop->info->parse) {
>> +        if (!prop->info->get) {
>>             continue;           /* no way to set it, don't show */
>>         }
> 
> This looks really weird: there's no "get" method so we conclude
> "no way to *set* it" ?? Is the comment wrong?

See the comment above.

Paolo

Re: [Qemu-devel] qdev property listing broken

[Peter Maydell]

On 2 April 2012 23:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/04/2012 22:07, Peter Maydell ha scritto:
>> On 2 April 2012 20:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>>> index a310cc7..923519c 100644
>>> --- a/hw/qdev-monitor.c
>>> +++ b/hw/qdev-monitor.c
>>> @@ -157,7 +157,7 @@ int qdev_device_help(QemuOpts *opts)
>>>          * for removal.  This conditional should be removed along with
>>>          * it.
>>>          */
>>> -        if (!prop->info->parse) {
>>> +        if (!prop->info->get) {
>>>             continue;           /* no way to set it, don't show */
>>>         }
>>
>> This looks really weird: there's no "get" method so we conclude
>> "no way to *set* it" ?? Is the comment wrong?
>
> See the comment above.

You mean
        /*
         * TODO Properties without a parser are just for dirty hacks.
         * qdev_prop_ptr is the only such PropertyInfo.  It's marked
         * for removal.  This conditional should be removed along with
         * it.
         */
?

That also looks odd now because we're no longer testing whether the
property has a parser... (plus doesn't qdev_prop_vlan also have no
parser ?)

-- PMM

Re: [Qemu-devel] qdev property listing broken

[Jan Kiszka]

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

On 2012-04-02 21:40, Paolo Bonzini wrote:
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index a310cc7..923519c 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -157,7 +157,7 @@ int qdev_device_help(QemuOpts *opts)
>           * for removal.  This conditional should be removed along with
>           * it.
>           */
> -        if (!prop->info->parse) {
> +        if (!prop->info->get) {
>              continue;           /* no way to set it, don't show */
>          }
>          error_printf("%s.%s=%s\n", driver, prop->name,
> @@ -165,7 +165,7 @@ int qdev_device_help(QemuOpts *opts)
>      }
>      if (info->bus_info) {
>          for (prop = info->bus_info->props; prop && prop->name; prop++) {
> -            if (!prop->info->parse) {
> +            if (!prop->info->get) {
>                  continue;           /* no way to set it, don't show */
>              }
>              error_printf("%s.%s=%s\n", driver, prop->name,

[ missed this as CCs were dropped ]

This fixes the -device foo,?, but not the issue I'm seeing. Non-legacy
properties are still not printed in info qtree - which is no surprise
given the inconsistency I pointed out.

Jan


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

Re: [Qemu-devel] qdev property listing broken

[Paolo Bonzini]

Il 03/04/2012 00:27, Peter Maydell ha scritto:
> On 2 April 2012 23:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 02/04/2012 22:07, Peter Maydell ha scritto:
>>> On 2 April 2012 20:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>>>> index a310cc7..923519c 100644
>>>> --- a/hw/qdev-monitor.c
>>>> +++ b/hw/qdev-monitor.c
>>>> @@ -157,7 +157,7 @@ int qdev_device_help(QemuOpts *opts)
>>>>          * for removal.  This conditional should be removed along with
>>>>          * it.
>>>>          */
>>>> -        if (!prop->info->parse) {
>>>> +        if (!prop->info->get) {
>>>>             continue;           /* no way to set it, don't show */
>>>>         }
>>>
>>> This looks really weird: there's no "get" method so we conclude
>>> "no way to *set* it" ?? Is the comment wrong?
>>
>> See the comment above.
> 
> You mean
>         /*
>          * TODO Properties without a parser are just for dirty hacks.
>          * qdev_prop_ptr is the only such PropertyInfo.  It's marked
>          * for removal.  This conditional should be removed along with
>          * it.
>          */
> ?
> 
> That also looks odd now because we're no longer testing whether the
> property has a parser... (plus doesn't qdev_prop_vlan also have no
> parser ?)

Oops... parse is set, print is get.  It was too late...

Paolo