Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Il 22/09/2014 13:43, Markus Armbruster ha scritto:
>> Paolo Bonzini <address@hidden> writes:
>> 
>>> Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto:
>>>> Basically this patch brings back historical HMP behaviour.
>>>> As far as I could tell, it wasn't changed intentionally.
>>>
>>> It was changed intentionally.  Or rather, the change was known at the
>>> time Stefan made it.
>> 
>> Commit hash?
>
> There are three commits involved.
>
> The first is commit f4eb32b (qmp: show QOM properties in
> device-list-properties, 2014-05-20).  It was done in preparation for the
> change of virtio-blk-pci.drive from qdev property to QOM property, to
> avoid breaking device-list-properties.

Yes.

Unless there were *no* QOM properties then (other than the implicit ones
listed in the commit message), it also made the help complete again,
which I'd regard as regression fix.

> The actual change took some more time to review, so it went in one month
> later.  Commit caffdac (virtio-blk: use aliases instead of duplicate
> qdev properties, 2014-06-18) is what changed virtio-blk-pci.drive from
> qdev property to QOM property.  This changed the type from 'drive' to
> 'str' in device-list-properties.  (Note that this patch fixed an actual
> bug, it was not just for the sake of cleanup).
>
> I cannot find any reference to the change; perhaps it was discussed only
> on IRC.  However, I'm quite certain that I knew about it.

If memory serves, I didn't.

> Now one thing did slip through; commit caffdac actually dropped
> virtio-blk-pci.drive from -device virtio-blk-pci,help.  Either I
> misremembered that the first commit fixed command-line help too, or I
> just assumed that -device foo,help was implemented on top of
> qmp_device_list_properties.  Which wasn't the case,

Happens.

>                                                     so the third commit
> (9ef52358, qdev-monitor: include QOM properties in -device FOO, help
> output, 2014-07-09) did the right thing and brought back

You mean commit ef52358.

> virtio-blk-pci.drive into the help message.
>
> Now, the cover latter for that patch finally has a hint that the change
> was known.  http://thread.gmane.org/gmane.comp.emulators.qemu/285819 has
> this text:
>
>    We simply need to update -device FOO,help code to use both qdev and
>    QOM properties.  Note that types change because a 'drive' qdev type
>    is actually a 'str' QOM type.  We're moving more and more to QOM
>    properties where the final type for this property would be
>    'link<Drive>' or similar.
>
> It is only in the cover letter and thus not part of any commit message.

I missed it.  Of course I might have missed it even if the commit
message had spelled it out.

The cover letter shows the people involved in the patch were aware of
the help change.  The clause "the final type for this property would be
'link<Drive>' or similar" can perhaps be read as "yes, the change is a
degradation, but it's a temporary one".  Sounds sensible, except our
path to this "final type" is still unclear, and we have no ETA.  Makes
the degradation rather less temporary.

>> I haven't got this part of the code present in my mind at the moment,
>> but I'm willing to take your assertion of a layering violation for
>> granted.  Is this violation necessary for fixing the regression, or is
>> it just an artifact of this particular fix?
>
> I guess we could always add some ad-hoc mechanism for
> device-list-properties to get to the "drive" string, and smuggle it as a
> QOM feature.  Then, aliases would just forward that mechanism to the
> aliased property, which would just work.
>
> Of course, with every new feature we would most likely have yet another
> unfinished transition.  In the lack of a clear user complaint (or even
> of a clear indication that human users ever used -device foo,help...)
> the tempation to pass the buck is strong.

I use it all the time, both interactively to get help, and
programmatically to answer questions about the code or a configuration.

I think elsewhere in this thread, we found a way to improve help that
doesn't involve ad hoc hackery to object.  It does start yet another
transition, though.

Thanks for your explanation, and your patience.