[Qemu-devel] Re: [PATCH v2 12/15] monitor: Add basic device state visualization

[Avi Kivity]

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<address@hidden>
> > >
> > > 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.

--
error compiling committee.c: too many arguments to function