Avi Kivity wrote:
> On 05/22/2010 11:18 AM, Jan Kiszka wrote:>> From: Jan Kiszka<jan.kiszka@siemens.com>>>>> This introduces device_show, a monitor command that saves the vmstate of>> a qdev device and visualizes it. QMP is also supported. Buffers are cut>> after 16 byte by default, but the full content can be requested via>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start>> index name. A new qerror is introduced to signal a missing vmstate. And>> it comes with documentation.>>>> +>> +Dump a snapshot of the device state. Buffers are cut after 16 bytes>> unless>> +a full dump is requested.>> +>> +Arguments:>> +>> +- "path": the device's qtree path or unique ID (json-string)>> > > This may be ambiguous.
Can your elaborate what precisely is ambiguous?
> >> +- "full": report full state (json-bool, optional)>> > > Is this needed for QMP? The client can always truncate it to any length.
The effect may not be needed for QMP, but I do need this channel from
the command line to the monitor pretty-printer. I could just stick
"full": json-bool back into the return dict, but that would look somehow
strange IMO.
> >> +>> +Schema of returned object:>> +>> +{ "device": json-string, "id": json-string, "fields" : [>> field-objects ] }>> +>> +The field object array may be empty, otherwise it consists of>> +>> +{ "name": json-string, "size": json-int, "elems": [ element-objects ] }>> +>> +"size" describes the real number of bytes required for a binary>> representation>> +of a single field element in the array. The actually transfered>> amount may be>> +smaller unless a full dump was requested.>> > > This converts the entire qdev tree into an undocumented stable protocol> (the qdev paths were already in this state I believe). This really> worries me.
Being primarily a debugging tool, device_show exports the entire
(qdev'ified) vmstates via QMP. Unlike the migration protocol, it does
not provide something like backward compatibility. This would be
overkill for the intended purpose (though someone may find a different
use case one day).
I think we have the following options:
- disable device_show via QMP, limit it to the monitor console
- declare its output inherently unstable, maybe at least adding the
vmstate version to each device so that potential QMP consumers notice
that they may have to update their tools or switch to a different
processing function
Given that vmstate annotations will most probably require some work on
the output structure (and I don't have a QMP use case ATM anyway), I
would be fine with the first option for now. Still, I don't think we
will ever get beyond the second option because this service is tight to
some internals of QEMU we don't want to freeze.
> >> +>> +The element object array may be empty, otherwise it can contain>> +>> +- json-int objects>> +- QMP buffer objects>> +- field objects>> +- arrays of json-ints, QMP buffers, or field objects>> +>> +Example:>> +>> +-> { "execute": "device_show", "arguments": { "path": "isa.0/i8042" } }>> +<- { "return": { "device": "i8042", "id": "", "fields":>> + [ { "name": "kbd", "size": 4, "elems":>> + [ { "name": "write_cmd", "size": 1, "elems": [0] },>> + { "name": "status", "size": 1, "elems": [25] },>> + { "name": "mode", "size": 1, "elems": [3] },>> + { "name": "pending", "size": 1, "elems": [1] }>> + ] }>> + ]>> + }>> + }>> +>> +EQMP>> > > Looks good. I am only worried about long term stability and documentation.>
Thanks,
Jan

On 05/23/2010 10:57 AM, Jan Kiszka wrote:
> Avi Kivity wrote:> >> On 05/22/2010 11:18 AM, Jan Kiszka wrote:>> >>> From: Jan Kiszka<jan.kiszka@siemens.com>>>>>>> This introduces device_show, a monitor command that saves the vmstate of>>> a qdev device and visualizes it. QMP is also supported. Buffers are cut>>> after 16 byte by default, but the full content can be requested via>>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start>>> index name. A new qerror is introduced to signal a missing vmstate. And>>> it comes with documentation.>>>>>> +>>> +Dump a snapshot of the device state. Buffers are cut after 16 bytes>>> unless>>> +a full dump is requested.>>> +>>> +Arguments:>>> +>>> +- "path": the device's qtree path or unique ID (json-string)>>>>>> >> This may be ambiguous.>> > Can your elaborate what precisely is ambiguous?>
Can't the user choose the unique ID so that it aliases an unrelated
qtree path?
I prefer having mutually exclusive 'path' and 'ref' arguments.
>>> +- "full": report full state (json-bool, optional)>>>>>> >> Is this needed for QMP? The client can always truncate it to any length.>> > The effect may not be needed for QMP, but I do need this channel from> the command line to the monitor pretty-printer. I could just stick> "full": json-bool back into the return dict, but that would look somehow> strange IMO.>
So we could disallow it as a QMP input, but allow it as an HMP input.
>>> +>>> +Schema of returned object:>>> +>>> +{ "device": json-string, "id": json-string, "fields" : [>>> field-objects ] }>>> +>>> +The field object array may be empty, otherwise it consists of>>> +>>> +{ "name": json-string, "size": json-int, "elems": [ element-objects ] }>>> +>>> +"size" describes the real number of bytes required for a binary>>> representation>>> +of a single field element in the array. The actually transfered>>> amount may be>>> +smaller unless a full dump was requested.>>>>>> >> This converts the entire qdev tree into an undocumented stable protocol>> (the qdev paths were already in this state I believe). This really>> worries me.>> > Being primarily a debugging tool, device_show exports the entire> (qdev'ified) vmstates via QMP. Unlike the migration protocol, it does> not provide something like backward compatibility.
Should be explicitly documented. All QMP commands should be backwards
and forwards compatible unless noted.
> This would be> overkill for the intended purpose (though someone may find a different> use case one day).>
Even for simply showing things, a GUI may depend on the presence of
certain fields. If we document that the fields may change, a correctly
written GUI can fall back to a simpler display.
> I think we have the following options:> - disable device_show via QMP, limit it to the monitor console> - declare its output inherently unstable, maybe at least adding the> vmstate version to each device so that potential QMP consumers notice> that they may have to update their tools or switch to a different> processing function>> Given that vmstate annotations will most probably require some work on> the output structure (and I don't have a QMP use case ATM anyway), I> would be fine with the first option for now. Still, I don't think we> will ever get beyond the second option because this service is tight to> some internals of QEMU we don't want to freeze.>
I agree. This feature is very useful as a debugging aid, and as I don't
think we'll have debugging GUIs any time soon, it's better to defer the
problem until we really need to solve it.

Avi Kivity wrote:
> On 05/23/2010 10:57 AM, Jan Kiszka wrote:>> Avi Kivity wrote:>> >>> On 05/22/2010 11:18 AM, Jan Kiszka wrote:>>> >>>> From: Jan Kiszka<jan.kiszka@siemens.com>>>>>>>>> This introduces device_show, a monitor command that saves the>>>> vmstate of>>>> a qdev device and visualizes it. QMP is also supported. Buffers are cut>>>> after 16 byte by default, but the full content can be requested via>>>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the>>>> start>>>> index name. A new qerror is introduced to signal a missing vmstate. And>>>> it comes with documentation.>>>>>>>> +>>>> +Dump a snapshot of the device state. Buffers are cut after 16 bytes>>>> unless>>>> +a full dump is requested.>>>> +>>>> +Arguments:>>>> +>>>> +- "path": the device's qtree path or unique ID (json-string)>>>>>>>> >>> This may be ambiguous.>>> >> Can your elaborate what precisely is ambiguous?>> > > Can't the user choose the unique ID so that it aliases an unrelated> qtree path?
True. I'll swap the search order and document this. Qtree paths should
always rule.
> > I prefer having mutually exclusive 'path' and 'ref' arguments.
That would be unhandy.
> >>>> +- "full": report full state (json-bool, optional)>>>>>>>> >>> Is this needed for QMP? The client can always truncate it to any>>> length.>>> >> The effect may not be needed for QMP, but I do need this channel from>> the command line to the monitor pretty-printer. I could just stick>> "full": json-bool back into the return dict, but that would look somehow>> strange IMO.>> > > So we could disallow it as a QMP input, but allow it as an HMP input.> >>>> +>>>> +Schema of returned object:>>>> +>>>> +{ "device": json-string, "id": json-string, "fields" : [>>>> field-objects ] }>>>> +>>>> +The field object array may be empty, otherwise it consists of>>>> +>>>> +{ "name": json-string, "size": json-int, "elems": [ element-objects>>>> ] }>>>> +>>>> +"size" describes the real number of bytes required for a binary>>>> representation>>>> +of a single field element in the array. The actually transfered>>>> amount may be>>>> +smaller unless a full dump was requested.>>>>>>>> >>> This converts the entire qdev tree into an undocumented stable protocol>>> (the qdev paths were already in this state I believe). This really>>> worries me.>>> >> Being primarily a debugging tool, device_show exports the entire>> (qdev'ified) vmstates via QMP. Unlike the migration protocol, it does>> not provide something like backward compatibility.> > Should be explicitly documented. All QMP commands should be backwards> and forwards compatible unless noted.> >> This would be>> overkill for the intended purpose (though someone may find a different>> use case one day).>> > > Even for simply showing things, a GUI may depend on the presence of> certain fields. If we document that the fields may change, a correctly> written GUI can fall back to a simpler display.> >> I think we have the following options:>> - disable device_show via QMP, limit it to the monitor console>> - declare its output inherently unstable, maybe at least adding the>> vmstate version to each device so that potential QMP consumers notice>> that they may have to update their tools or switch to a different>> processing function>>>> Given that vmstate annotations will most probably require some work on>> the output structure (and I don't have a QMP use case ATM anyway), I>> would be fine with the first option for now. Still, I don't think we>> will ever get beyond the second option because this service is tight to>> some internals of QEMU we don't want to freeze.>> > > I agree. This feature is very useful as a debugging aid, and as I don't> think we'll have debugging GUIs any time soon, it's better to defer the> problem until we really need to solve it.
I introduced .user_only as a monitor command tag and applied it on
device_show. But I also added the vmstate version to the device output,
maybe already helpful for users. All this will come with v3.
Jan

On 05/23/2010 01:03 PM, Jan Kiszka wrote:
>>>>>> Can your elaborate what precisely is ambiguous?>>>>>> >> Can't the user choose the unique ID so that it aliases an unrelated>> qtree path?>> > True. I'll swap the search order and document this. Qtree paths should> always rule.>
Well, I guess the user could avoid ambiguity by avoiding /es.
> >> I prefer having mutually exclusive 'path' and 'ref' arguments.>> > That would be unhandy.>
Don't really see why.
>> I agree. This feature is very useful as a debugging aid, and as I don't>> think we'll have debugging GUIs any time soon, it's better to defer the>> problem until we really need to solve it.>> > I introduced .user_only as a monitor command tag and applied it on> device_show. But I also added the vmstate version to the device output,> maybe already helpful for users. All this will come with v3.>
Thanks.

On Sun, 23 May 2010 09:57:43 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:
> Avi Kivity wrote:
[...]
> > > >> +- "full": report full state (json-bool, optional)> >> > > > > Is this needed for QMP? The client can always truncate it to any length.> > The effect may not be needed for QMP, but I do need this channel from> the command line to the monitor pretty-printer. I could just stick> "full": json-bool back into the return dict, but that would look somehow> strange IMO.
We could have a form of optional key which is only present internally,
we have that async commands.

On 05/24/2010 07:51 AM, Luiz Capitulino wrote:
> On Sun, 23 May 2010 09:57:43 +0200> Jan Kiszka<jan.kiszka@web.de> wrote:>> >> Avi Kivity wrote:>> > [...]>> >>> >>>> +- "full": report full state (json-bool, optional)>>>>>>>> >>> Is this needed for QMP? The client can always truncate it to any length.>>> >> The effect may not be needed for QMP, but I do need this channel from>> the command line to the monitor pretty-printer. I could just stick>> "full": json-bool back into the return dict, but that would look somehow>> strange IMO.>> > We could have a form of optional key which is only present internally,> we have that async commands.>
I'd rather see two separate commands with an internal mechanism to make
direct QMP calls. The human monitor can be implemented in terms of the
QMP call but it should be in terms of the human monitor invoking the QMP
command such that it can munge the output as it sees appropriate.
Regards,
Anthony Liguori

On 05/22/2010 01:55 PM, Avi Kivity wrote:
> On 05/22/2010 11:18 AM, Jan Kiszka wrote:>> From: Jan Kiszka<jan.kiszka@siemens.com>>>>> This introduces device_show, a monitor command that saves the vmstate of>> a qdev device and visualizes it. QMP is also supported. Buffers are cut>> after 16 byte by default, but the full content can be requested via>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start>> index name. A new qerror is introduced to signal a missing vmstate. And>> it comes with documentation.>>>> +>> +Dump a snapshot of the device state. Buffers are cut after 16 bytes >> unless>> +a full dump is requested.>> +>> +Arguments:>> +>> +- "path": the device's qtree path or unique ID (json-string)>> This may be ambiguous.>>> +- "full": report full state (json-bool, optional)>> Is this needed for QMP? The client can always truncate it to any length.>>> +>> +Schema of returned object:>> +>> +{ "device": json-string, "id": json-string, "fields" : [ >> field-objects ] }>> +>> +The field object array may be empty, otherwise it consists of>> +>> +{ "name": json-string, "size": json-int, "elems": [ element-objects ] }>> +>> +"size" describes the real number of bytes required for a binary >> representation>> +of a single field element in the array. The actually transfered >> amount may be>> +smaller unless a full dump was requested.>> This converts the entire qdev tree into an undocumented stable > protocol (the qdev paths were already in this state I believe). This > really worries me.
N.B. the association with qdev is only in identifying the device. The
contents of the device's state are not part of qdev but rather part of
vmstate. vmstate is something that we already guarantee to be stable
since that's required for live migration compatibility.
I don't think that qdev device names and paths are something we have to
worry much about changing over time since they reflect logical bus
layout. They should remain static provided the devices remain static.
The qdev properties are a different matter entirely. A command like
'info qdm' would be potentially difficult to support as part of QMP but
the proposed command's output is actually already part of a backward
compatible interface (vmstate).
Regards,
Anthony Liguori
>> +>> +The element object array may be empty, otherwise it can contain>> +>> +- json-int objects>> +- QMP buffer objects>> +- field objects>> +- arrays of json-ints, QMP buffers, or field objects>> +>> +Example:>> +>> +-> { "execute": "device_show", "arguments": { "path": "isa.0/i8042" >> } }>> +<- { "return": { "device": "i8042", "id": "", "fields":>> + [ { "name": "kbd", "size": 4, "elems":>> + [ { "name": "write_cmd", "size": 1, "elems": >> [0] },>> + { "name": "status", "size": 1, "elems": [25] },>> + { "name": "mode", "size": 1, "elems": [3] },>> + { "name": "pending", "size": 1, "elems": [1] }>> + ] }>> + ]>> + }>> + }>> +>> +EQMP>> Looks good. I am only worried about long term stability and > documentation.>

Anthony Liguori wrote:
> On 05/22/2010 01:55 PM, Avi Kivity wrote:>> On 05/22/2010 11:18 AM, Jan Kiszka wrote:>>> From: Jan Kiszka<jan.kiszka@siemens.com>>>>>>> This introduces device_show, a monitor command that saves the vmstate of>>> a qdev device and visualizes it. QMP is also supported. Buffers are cut>>> after 16 byte by default, but the full content can be requested via>>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start>>> index name. A new qerror is introduced to signal a missing vmstate. And>>> it comes with documentation.>>>>>> +>>> +Dump a snapshot of the device state. Buffers are cut after 16 bytes>>> unless>>> +a full dump is requested.>>> +>>> +Arguments:>>> +>>> +- "path": the device's qtree path or unique ID (json-string)>>>> This may be ambiguous.>>>>> +- "full": report full state (json-bool, optional)>>>> Is this needed for QMP? The client can always truncate it to any length.>>>>> +>>> +Schema of returned object:>>> +>>> +{ "device": json-string, "id": json-string, "fields" : [>>> field-objects ] }>>> +>>> +The field object array may be empty, otherwise it consists of>>> +>>> +{ "name": json-string, "size": json-int, "elems": [ element-objects ] }>>> +>>> +"size" describes the real number of bytes required for a binary>>> representation>>> +of a single field element in the array. The actually transfered>>> amount may be>>> +smaller unless a full dump was requested.>>>> This converts the entire qdev tree into an undocumented stable>> protocol (the qdev paths were already in this state I believe). This>> really worries me.> > N.B. the association with qdev is only in identifying the device. The> contents of the device's state are not part of qdev but rather part of> vmstate. vmstate is something that we already guarantee to be stable> since that's required for live migration compatibility.
In contrast to save/loadvm, device_show does not provide a
backward-compatible output. Not only field names can change (as a result
of internal refactoring), fields may even disappear, new ones may pop
up. vmstate makes this transparent for reading of old states, but not
for visualization.
That said, I see no use case yet that would justify the effort for
making state dumps as stable as vmstates.
Jan

On 05/24/2010 03:35 PM, Jan Kiszka wrote:
> In contrast to save/loadvm, device_show does not provide a> backward-compatible output. Not only field names can change (as a result> of internal refactoring),
Field names could change, but that seems unlikely and unnecessary.
> fields may even disappear,
That would break live migration. The output of device_show when
specifying -M pc-0.13 should always be the same. If it's not, I believe
that's a bug.
The output of device_show across multiple qemu versions without
specifying a -M could certainly be different but that's to be expected
since you have a different hardware model.
> new ones may pop> up. vmstate makes this transparent for reading of old states, but not> for visualization.>> That said, I see no use case yet that would justify the effort for> making state dumps as stable as vmstates.>
They already are. It's just a matter of whether we declare it as such
AFAICT.
Regards,
Anthony Liguori
> Jan>

Anthony Liguori wrote:
> On 05/24/2010 03:35 PM, Jan Kiszka wrote:>> In contrast to save/loadvm, device_show does not provide a>> backward-compatible output. Not only field names can change (as a result>> of internal refactoring),> > Field names could change, but that seems unlikely and unnecessary.
Actually, this may happen to improve the output format for device_show,
either via renaming the fields or annotating it (once we have the latter
mechanism).
> >> fields may even disappear,> > That would break live migration.
Nope. We can (and I think we did before) perfectly read some field up to
version X and convert it into the new format but skip that field for X+1
upward.
> The output of device_show when> specifying -M pc-0.13 should always be the same. If it's not, I believe> that's a bug.> > The output of device_show across multiple qemu versions without> specifying a -M could certainly be different but that's to be expected> since you have a different hardware model.
It will be different as soon as the version_id of a dumped vmstate
changes, even independent of -M.
Jan

On 05/24/2010 05:12 PM, Jan Kiszka wrote:
> Anthony Liguori wrote:> >> On 05/24/2010 03:35 PM, Jan Kiszka wrote:>> >>> In contrast to save/loadvm, device_show does not provide a>>> backward-compatible output. Not only field names can change (as a result>>> of internal refactoring),>>> >> Field names could change, but that seems unlikely and unnecessary.>> > Actually, this may happen to improve the output format for device_show,> either via renaming the fields or annotating it (once we have the latter> mechanism).
I don't think it's a huge win vs. stability.
>> The output of device_show when>> specifying -M pc-0.13 should always be the same. If it's not, I believe>> that's a bug.>>>> The output of device_show across multiple qemu versions without>> specifying a -M could certainly be different but that's to be expected>> since you have a different hardware model.>> > It will be different as soon as the version_id of a dumped vmstate> changes, even independent of -M.>
But QMP stability is only guaranteed for releases and -M pc-0.12 should
(in a perfect world) always dump out the same versions regardless of
whether it's qemu-0.12, qemu-0.13, etc.
Regards,
Anthony Liguori
> Jan>>

On 05/24/2010 11:22 PM, Anthony Liguori wrote:
>> This converts the entire qdev tree into an undocumented stable >> protocol (the qdev paths were already in this state I believe). This >> really worries me.>>> N.B. the association with qdev is only in identifying the device. The > contents of the device's state are not part of qdev but rather part of > vmstate. vmstate is something that we already guarantee to be stable > since that's required for live migration compatibility.
That removes out ability to deprecate older vmstate as time passes. Not
a blocker but something to consider.
> I don't think that qdev device names and paths are something we have > to worry much about changing over time since they reflect logical bus > layout. They should remain static provided the devices remain static.
Modulo mistakes. We already saw one (lack of pci domains). To reduce
the possibility of mistakes, we need reviewable documentation.
Note sysfs had similar assumptions and problems.
> The qdev properties are a different matter entirely. A command like > 'info qdm' would be potentially difficult to support as part of QMP > but the proposed command's output is actually already part of a > backward compatible interface (vmstate).
That's all good. But documentation is critical for this. Not only to
improve quality, but also so that tool authors would have something to
code against instead of trial and error (which invariably misses some
corner cases).

On 05/25/2010 02:23 AM, Avi Kivity wrote:
> On 05/24/2010 11:22 PM, Anthony Liguori wrote:>>> This converts the entire qdev tree into an undocumented stable >>> protocol (the qdev paths were already in this state I believe). >>> This really worries me.>>>>>> N.B. the association with qdev is only in identifying the device. >> The contents of the device's state are not part of qdev but rather >> part of vmstate. vmstate is something that we already guarantee to >> be stable since that's required for live migration compatibility.>> That removes out ability to deprecate older vmstate as time passes. > Not a blocker but something to consider.>>> I don't think that qdev device names and paths are something we have >> to worry much about changing over time since they reflect logical bus >> layout. They should remain static provided the devices remain static.>> Modulo mistakes. We already saw one (lack of pci domains). To reduce > the possibility of mistakes, we need reviewable documentation.
pci domains was only a mistake as a nice-to-have. We can add pci
domains in a backwards compatible way.
The arguments you're making about the importance of backwards
compatibility and what's needed to strongly guarantee it are equally
applicable to the live migration protocol. We really do need to
formally document the live migration protocol in such a way that it's
reviewable if we hope to truly make it compatible across versions.
Regards,
Anthony Liguori
> Note sysfs had similar assumptions and problems.>>> The qdev properties are a different matter entirely. A command like >> 'info qdm' would be potentially difficult to support as part of QMP >> but the proposed command's output is actually already part of a >> backward compatible interface (vmstate).>> That's all good. But documentation is critical for this. Not only to > improve quality, but also so that tool authors would have something to > code against instead of trial and error (which invariably misses some > corner cases).>

On 05/25/2010 04:03 PM, Anthony Liguori wrote:
>>>>> I don't think that qdev device names and paths are something we have >>> to worry much about changing over time since they reflect logical >>> bus layout. They should remain static provided the devices remain >>> static.>>>> Modulo mistakes. We already saw one (lack of pci domains). To >> reduce the possibility of mistakes, we need reviewable documentation.>>> pci domains was only a mistake as a nice-to-have. We can add pci > domains in a backwards compatible way.
It adds a new level to the qdev tree. Of course we can hide the new
level for older clients, and newer clients can drop the level for older
qemus, but it will be oh-so-painful.
>> The arguments you're making about the importance of backwards > compatibility and what's needed to strongly guarantee it are equally > applicable to the live migration protocol. We really do need to > formally document the live migration protocol in such a way that it's > reviewable if we hope to truly make it compatible across versions.
Mostly agreed. I think live migration has a faster/easier deprecation
schedule (easier not to support migration from 0.n-k to 0.n than to
remove qmp support for a feature introduced in 0.n-k when releasing
0.n). But that's a minor concern, improving our externally visible
interface documentation is a good thing and badly needed.

On 05/25/2010 08:19 AM, Avi Kivity wrote:
> On 05/25/2010 04:03 PM, Anthony Liguori wrote:>>>>>>> I don't think that qdev device names and paths are something we >>>> have to worry much about changing over time since they reflect >>>> logical bus layout. They should remain static provided the devices >>>> remain static.>>>>>> Modulo mistakes. We already saw one (lack of pci domains). To >>> reduce the possibility of mistakes, we need reviewable documentation.>>>>>> pci domains was only a mistake as a nice-to-have. We can add pci >> domains in a backwards compatible way.>> It adds a new level to the qdev tree.
The tree is not organized like that today. IOW, the PCI hierarchy is
not reflected in the qdev hierarchy. All PCI devices (regardless of
whether they're a function or a full slot) simply sit below the PCI bus.
>>>> The arguments you're making about the importance of backwards >> compatibility and what's needed to strongly guarantee it are equally >> applicable to the live migration protocol. We really do need to >> formally document the live migration protocol in such a way that it's >> reviewable if we hope to truly make it compatible across versions.>> Mostly agreed. I think live migration has a faster/easier deprecation > schedule (easier not to support migration from 0.n-k to 0.n than to > remove qmp support for a feature introduced in 0.n-k when releasing > 0.n). But that's a minor concern, improving our externally visible > interface documentation is a good thing and badly needed.>
Regards,
Anthony Liguori

On 05/25/2010 04:31 PM, Anthony Liguori wrote:
> On 05/25/2010 08:19 AM, Avi Kivity wrote:>> On 05/25/2010 04:03 PM, Anthony Liguori wrote:>>>>>>>>> I don't think that qdev device names and paths are something we >>>>> have to worry much about changing over time since they reflect >>>>> logical bus layout. They should remain static provided the >>>>> devices remain static.>>>>>>>> Modulo mistakes. We already saw one (lack of pci domains). To >>>> reduce the possibility of mistakes, we need reviewable documentation.>>>>>>>>> pci domains was only a mistake as a nice-to-have. We can add pci >>> domains in a backwards compatible way.>>>> It adds a new level to the qdev tree.>> The tree is not organized like that today. IOW, the PCI hierarchy is > not reflected in the qdev hierarchy. All PCI devices (regardless of > whether they're a function or a full slot) simply sit below the PCI bus.
That's a bug IMO, but regardless, s/qdev tree/pci device component of
the qdev path/.

Avi Kivity <avi@redhat.com> writes:
> On 05/23/2010 10:57 AM, Jan Kiszka wrote:>> Avi Kivity wrote:>> >>> On 05/22/2010 11:18 AM, Jan Kiszka wrote:>>> >>>> From: Jan Kiszka<jan.kiszka@siemens.com>>>>>>>>> This introduces device_show, a monitor command that saves the vmstate of>>>> a qdev device and visualizes it. QMP is also supported. Buffers are cut>>>> after 16 byte by default, but the full content can be requested via>>>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start>>>> index name. A new qerror is introduced to signal a missing vmstate. And>>>> it comes with documentation.>>>>>>>> +>>>> +Dump a snapshot of the device state. Buffers are cut after 16 bytes>>>> unless>>>> +a full dump is requested.>>>> +>>>> +Arguments:>>>> +>>>> +- "path": the device's qtree path or unique ID (json-string)>>>>>>>> >>> This may be ambiguous.>>> >> Can your elaborate what precisely is ambiguous?>> >> Can't the user choose the unique ID so that it aliases an unrelated> qtree path?>> I prefer having mutually exclusive 'path' and 'ref' arguments.
Don't think that's necessary. If the string starts with '/', it's an
absolute path. Else, it's a relative path rooted at the node with the
ID equal to the first component.
Currently breaks down when IDs contain '/', but permitting that is a
bug. There may be more problems; the path lookup code is way too
clever.

Markus Armbruster wrote:
> Avi Kivity <avi@redhat.com> writes:> >> On 05/23/2010 10:57 AM, Jan Kiszka wrote:>>> Avi Kivity wrote:>>> >>>> On 05/22/2010 11:18 AM, Jan Kiszka wrote:>>>> >>>>> From: Jan Kiszka<jan.kiszka@siemens.com>>>>>>>>>>> This introduces device_show, a monitor command that saves the vmstate of>>>>> a qdev device and visualizes it. QMP is also supported. Buffers are cut>>>>> after 16 byte by default, but the full content can be requested via>>>>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start>>>>> index name. A new qerror is introduced to signal a missing vmstate. And>>>>> it comes with documentation.>>>>>>>>>> +>>>>> +Dump a snapshot of the device state. Buffers are cut after 16 bytes>>>>> unless>>>>> +a full dump is requested.>>>>> +>>>>> +Arguments:>>>>> +>>>>> +- "path": the device's qtree path or unique ID (json-string)>>>>>>>>>> >>>> This may be ambiguous.>>>> >>> Can your elaborate what precisely is ambiguous?>>> >> Can't the user choose the unique ID so that it aliases an unrelated>> qtree path?>>>> I prefer having mutually exclusive 'path' and 'ref' arguments.> > Don't think that's necessary. If the string starts with '/', it's an> absolute path. Else, it's a relative path rooted at the node with the> ID equal to the first component.
'pci.0' can be both a (badly chosen) ID as well as an abbreviated qtree
path.
> > Currently breaks down when IDs contain '/', but permitting that is a> bug. There may be more problems; the path lookup code is way too> clever.
Indeed. Less can sometimes be more. My impression is that some of the
cleverness was motivated by lacking qtree completion for the HMP.
Jan

On 05/29/2010 11:14 AM, Jan Kiszka wrote:
>> Currently breaks down when IDs contain '/', but permitting that is a>> bug. There may be more problems; the path lookup code is way too>> clever.>> > Indeed. Less can sometimes be more. My impression is that some of the> cleverness was motivated by lacking qtree completion for the HMP.>
Can we disable abbreviations for QMP and only allow them for HMP?
We can support that by adding a hidden argument to commands specifying
whether the input comes from a human or machine. Abbreviations are
dangerous if they become ambiguous; a human can recover while a machine
can't.

Avi Kivity wrote:
> On 05/29/2010 11:14 AM, Jan Kiszka wrote:>>> Currently breaks down when IDs contain '/', but permitting that is a>>> bug. There may be more problems; the path lookup code is way too>>> clever.>>> >> Indeed. Less can sometimes be more. My impression is that some of the>> cleverness was motivated by lacking qtree completion for the HMP.>> > > Can we disable abbreviations for QMP and only allow them for HMP?> > We can support that by adding a hidden argument to commands specifying> whether the input comes from a human or machine. Abbreviations are> dangerous if they become ambiguous; a human can recover while a machine> can't.>
Both QMP _and_ HMP suffer from ambitious and inconsistent addressing
schemes. Therefore, I'm more and more in favor of [1]. We just need to
add command line completion for option values, something that would be
beneficial for 'drive_add file=...' as well.
Jan
[1] http://article.gmane.org/gmane.comp.emulators.qemu/72152

Jan Kiszka <jan.kiszka@web.de> writes:
> Avi Kivity wrote:>> On 05/29/2010 11:14 AM, Jan Kiszka wrote:>>>> Currently breaks down when IDs contain '/', but permitting that is a>>>> bug. There may be more problems; the path lookup code is way too>>>> clever.>>>> >>> Indeed. Less can sometimes be more. My impression is that some of the>>> cleverness was motivated by lacking qtree completion for the HMP.>>> >> >> Can we disable abbreviations for QMP and only allow them for HMP?>> >> We can support that by adding a hidden argument to commands specifying>> whether the input comes from a human or machine. Abbreviations are>> dangerous if they become ambiguous; a human can recover while a machine>> can't.>> >> Both QMP _and_ HMP suffer from ambitious and inconsistent addressing> schemes. Therefore, I'm more and more in favor of [1]. We just need to> add command line completion for option values, something that would be> beneficial for 'drive_add file=...' as well.>> Jan>> [1] http://article.gmane.org/gmane.comp.emulators.qemu/72152
[1] = abolish the clever abbreviations in path lookup. I agree we
should do that.

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:> >> Avi Kivity wrote:>>> On 05/29/2010 11:14 AM, Jan Kiszka wrote:>>>>> Currently breaks down when IDs contain '/', but permitting that is a>>>>> bug. There may be more problems; the path lookup code is way too>>>>> clever.>>>>> >>>> Indeed. Less can sometimes be more. My impression is that some of the>>>> cleverness was motivated by lacking qtree completion for the HMP.>>>> >>> Can we disable abbreviations for QMP and only allow them for HMP?>>>>>> We can support that by adding a hidden argument to commands specifying>>> whether the input comes from a human or machine. Abbreviations are>>> dangerous if they become ambiguous; a human can recover while a machine>>> can't.>>>>> Both QMP _and_ HMP suffer from ambitious and inconsistent addressing>> schemes. Therefore, I'm more and more in favor of [1]. We just need to>> add command line completion for option values, something that would be>> beneficial for 'drive_add file=...' as well.>>>> Jan>>>> [1] http://article.gmane.org/gmane.comp.emulators.qemu/72152> > [1] = abolish the clever abbreviations in path lookup. I agree we> should do that.
Fine. No concerns regarding overlaying IDs and path specifications as well?
Jan

Jan Kiszka <jan.kiszka@siemens.com> writes:
> Markus Armbruster wrote:>> Jan Kiszka <jan.kiszka@web.de> writes:>> >>> Avi Kivity wrote:>>>> On 05/29/2010 11:14 AM, Jan Kiszka wrote:>>>>>> Currently breaks down when IDs contain '/', but permitting that is a>>>>>> bug. There may be more problems; the path lookup code is way too>>>>>> clever.>>>>>> >>>>> Indeed. Less can sometimes be more. My impression is that some of the>>>>> cleverness was motivated by lacking qtree completion for the HMP.>>>>> >>>> Can we disable abbreviations for QMP and only allow them for HMP?>>>>>>>> We can support that by adding a hidden argument to commands specifying>>>> whether the input comes from a human or machine. Abbreviations are>>>> dangerous if they become ambiguous; a human can recover while a machine>>>> can't.>>>>>>> Both QMP _and_ HMP suffer from ambitious and inconsistent addressing>>> schemes. Therefore, I'm more and more in favor of [1]. We just need to>>> add command line completion for option values, something that would be>>> beneficial for 'drive_add file=...' as well.>>>>>> Jan>>>>>> [1] http://article.gmane.org/gmane.comp.emulators.qemu/72152>> >> [1] = abolish the clever abbreviations in path lookup. I agree we>> should do that.>> Fine. No concerns regarding overlaying IDs and path specifications as well?
I'm fine with that.
Calling the overlayed argument "id" is not so nice. We got a bunch of
other not-so-nice names in QMP, maybe we'll have a flag day to clean
them all up.

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:> >> Markus Armbruster wrote:>>> Jan Kiszka <jan.kiszka@web.de> writes:>>>>>>> Avi Kivity wrote:>>>>> On 05/29/2010 11:14 AM, Jan Kiszka wrote:>>>>>>> Currently breaks down when IDs contain '/', but permitting that is a>>>>>>> bug. There may be more problems; the path lookup code is way too>>>>>>> clever.>>>>>>> >>>>>> Indeed. Less can sometimes be more. My impression is that some of the>>>>>> cleverness was motivated by lacking qtree completion for the HMP.>>>>>> >>>>> Can we disable abbreviations for QMP and only allow them for HMP?>>>>>>>>>> We can support that by adding a hidden argument to commands specifying>>>>> whether the input comes from a human or machine. Abbreviations are>>>>> dangerous if they become ambiguous; a human can recover while a machine>>>>> can't.>>>>>>>>> Both QMP _and_ HMP suffer from ambitious and inconsistent addressing>>>> schemes. Therefore, I'm more and more in favor of [1]. We just need to>>>> add command line completion for option values, something that would be>>>> beneficial for 'drive_add file=...' as well.>>>>>>>> Jan>>>>>>>> [1] http://article.gmane.org/gmane.comp.emulators.qemu/72152>>> [1] = abolish the clever abbreviations in path lookup. I agree we>>> should do that.>> Fine. No concerns regarding overlaying IDs and path specifications as well?> > I'm fine with that.> > Calling the overlayed argument "id" is not so nice. We got a bunch of> other not-so-nice names in QMP, maybe we'll have a flag day to clean> them all up.
For this case (device_del and device_show), I think we should simply
call it 'device'.
Jan