Re: [Qemu-devel] [PATCHv3] add "info ioapic" monitor command

[Gleb Natapov]

On Wed, Jan 06, 2010 at 01:50:21PM -0600, Anthony Liguori wrote:
> On 01/06/2010 10:35 AM, Gleb Natapov wrote:
> >Knowing ioapic configuration is very useful for the poor soles
> >how need to debug guest occasionally.
> 
> This needs to be implemented in terms of VMState.  There is no
> reason for it not to be.
Except that this has nothing to do with VMState.

> 
> I've just gone through the code in great detail.  You just need to
> implement a vmstate_get_field that looks up a field given an
> instance name and instance id.  You then need to add a to_str
> callback in VMStateInfo.  The vmstate_get_field code will look very
> much like the vmstate_save code except it calls to_str().
> 
> The new monitor command should be 'device_info name [instance_id] [field]'.
> 
What is instance_id? And 'field' is implementation detail that should
not be exposed to user at all. And what happens when VMState will start
to support subsections?

> For the ioapic, you can introduce a new VMStateInfo type that
> duplicates the vmstate_info_uint64 type except it has a different
> to_str callback that decodes the apic fields.  It's just then a
> matter of a few macros and updating the VMState description for the
> ioapic.
> 
You just overdesigning!  What is the point in all those to_str() callbacks
instead of only one that knows device internals? What if a meaning of
one field depends on values of other fields? If you want to have a way
to dump VMState then just dump it in a "field_name = value" form. This
is great for migration debugging.  But to present device state in human
readable from is much simpler done by having only one function that
gets device and prints nice info about its current state. If anything,
the devise state print callback should be part of a device structure.

> It's the same or less code than your current patch and it lays the
> ground work for a consistent command to look up arbitrary device
> info.
> 
Your description is almost the size of my patch, and you can't seriously
say that what you propose is "the same or less code than my current patch"
since all of my patch will be a subset of what you propose, because all
it does is parse ioapic state into readable form. And this is really
what my patch is about: parsing ioapic state into readable form. Now you
are saying that we can call this code from a different place (that
doesn't exist currently), or we can spread it over multiple to_str functions,
but it can't just disappear.
 
--
                        Gleb.