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

[Paolo Bonzini]

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.

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.

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, 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
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 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.

Paolo