qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/2] QMP: Introduce commands doc


From: Luiz Capitulino
Subject: Re: [Qemu-devel] [PATCH 1/2] QMP: Introduce commands doc
Date: Thu, 13 May 2010 11:55:24 -0300

On Thu, 13 May 2010 16:48:13 +0300
Avi Kivity <address@hidden> wrote:

> On 05/05/2010 10:11 PM, Luiz Capitulino wrote:
> > One of the most important missing feature in QMP today is its
> > supported commands documentation.
> >
> > The plan is to make it part of self-description support, however
> > self-description is a big task we have been postponing for a
> > long time now and still don't know when it's going to be done.
> >
> > In order not to compromise QMP adoption and make users' life easier,
> > this commit adds a simple text documentation which fully describes
> > all QMP supported commands.
> >
> > This is not ideal for a number of reasons (harder to maintain,
> > text-only, etc) but does improve the current situation.
> >
> >    
> 
> Excellent.

 Thanks.

> > +change
> > +------
> > +
> > +Change a removable medium or VNC configuration.
> >    
> 
> This is sad.  Would anyone write a C function with a similar description?

 I wouldn't, but someone did for qemu.

> Do we have this in 0.12?  If not we can change (...) it.  If we do, it's 
> a more difficult deprecation process.

 Yes, we do and it's used by libvirt iirc.

> > +cpu
> > +---
> > +
> > +Set the default CPU.
> > +
> > +Arguments:
> > +
> > +- "index": the CPU's index (json-int)
> > +
> > +Example:
> > +
> > +{ "execute": "cpu", "arguments": { "index": 0 } }
> > +
> > +Note: CPUs' indexes are obtained with the 'query-cpus' command.
> >    
> 
> The default cpu is local to the monitor?

 I guess so, someone has reported to me that QMP's and user Monitor's
behavior diverge. Ie, the default cpu set in QMP doesn't show up as
default in the user Monitor.

> Not sure, but it may have been better to omit it and instead require an 
> explicit cpu argument in all commands that rely on it.  As it is, 
> multithreaded management programs will need to lock the monitor when 
> reading cpu registers, etc.
> 
> Or, we can add an optional cpu argument to all commands that depend on it.

 I'll have to think about it, didn't have the time to get into this though.

> > +
> > +migrate
> > +-------
> > +
> > +Migrate to URI.
> > +
> > +Arguments:
> > +
> > +- "blk": block migration, full disk copy (json-bool, optional)
> > +- "inc": incremental disk copy (json-bool, optional)
> >    
> 
> Don't these need to be per-disk?

 You mean the documentation should say that or should we be able to
specify the disk some way?

> > +- "uri": Destination URI (json-string)
> > +
> > +Example:
> > +
> > +{ "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
> > +
> > +Notes:
> > +
> > +(1) The 'query-migrate' command should be used to check migration's 
> > progress
> > +    and final result (this information is provided by the 'status' member)
> >    
> 
> Is there an event the provides completion notification?  I don't see one 
> in qemu-events.txt, but I think there should be one.

 Juan is working on it.

> > +(2) All boolean arguments default to false
> > +(3) The user Monitor's "detach" argument is invalid in QMP and should not
> > +    be used
> > +
> > +
> > +migrate_set_downtime
> > +--------------------
> > +
> > +Set maximum tolerated downtime (in seconds) for migrations.
> > +
> > +Arguments:
> > +
> > +- "value": maximum downtime (json-number)
> > +
> > +Example:
> > +
> > +{ "execute": "migrate_set_downtime", "arguments": { "value": 60 } }
> >    
> 
> The example doesn't match reality well, suggest 0.1.
> 
> Would have been nicer as migrate_set_parameters downtime: 0.1, we can do 
> that when we add more parameters.
> 
> > +
> > +migrate_set_speed
> > +-----------------
> > +
> > +Set maximum speed for migrations.
> > +
> > +Arguments:
> > +
> > +- "value": maximum speed (json-number)
> > +
> > +Example:
> > +
> > +{ "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
> >    
> 
> Oh, we do have more.
> 
> Please document the units for this value (bits per second)?
> 
> > +pmemsave
> > +--------
> > +
> > +Save to disk physical memory dump starting at 'val' of size 'size'.
> > +
> > +Arguments:
> > +
> > +- "val": the starting address (json-int)
> >    
> 
> Why "val" instead of "address" or "physical-address"?

 Mea culpa, for this and most inconsistencies you find in the protocol.

 The reason is that I did a lot of conversions in a hurry, so that
we could get libvirt working under QMP as soon as possible.

 Unfortunately, some bad names and some bad behaviors from the user
Monitor leaked into QMP. Lesson learned, although sometimes it's very
difficult to define what a name/behavior should be in QMP.

> > +query-commands
> > +--------------
> > +
> > +List QMP available commands.
> > +
> > +Each command is represented by a json-object, the returned value is a 
> > json-array
> > +of all commands.
> > +
> > +Each json-object contain:
> > +
> > +- "name": command's name (json-string)
> > +
> > +Example:
> > +
> > +{
> > +   "return":[
> > +      {
> > +         "name":"query-balloon"
> > +      },
> > +      {
> > +         "name":"system_powerdown"
> > +      }
> > +   ]
> > +}
> > +
> > +Note: This example has been shortened as the real response is too long.
> >    
> 
> Presumably, this will be extended with a description of the arguments 
> and return types?

 Yes, we'll have one for events as well.

 This is self-description, our next big feature.

> > +query-cpus
> > +----------
> > +
> > +Show CPU information.
> > +
> > +Return a json-array. Each CPU is represented by a json-object, which 
> > contains:
> > +
> > +- "cpu": CPU index (json-int)
> > +- "current": true if this is the current CPU, false otherwise (json-bool)
> > +- "halted": true if the cpu is halted, false otherwise (json-bool)
> > +- Current program counter. The key's name depends on the architecture:
> > +     "pc": i386/x86_64 (json-int)
> > +     "nip": PPC (json-int)
> > +     "pc" and "npc": sparc (json-int)
> > +     "PC": mips (json-int)
> >    
> 
> That's a bit sad.  It makes it difficult to use with stub generators.
> 
> Can we force everything to "pc"?  It will break non-x86, but there 
> aren't likely to be many QMP users using for those archs at present.

 Ok, Markus seems to think the same so I'll change it, a "arch" key
can be added later if needed.

> > +query-mice
> > +----------
> > +
> > +Show VM mice information.
> > +
> > +Each mouse is represented by a json-object, the returned value is a 
> > json-array
> > +of all mice.
> > +
> > +The mouse json-object contains the following:
> > +
> > +- "name": mouse's name (json-string)
> > +- "index": mouse's index (json-int)
> > +- "current": true if this mouse is receiving events, false otherwise 
> > (json-bool)
> >    
> 
> What does this mean?

 It's the mouse that will move, I guess.

> > +- "absolute": true if the mouse generates absolute input events (json-bool)
> > +
> >
> > +
> > +query-migrate
> > +-------------
> > +
> > +Migration status.
> > +
> > +Return a json-object. If migration is active there will be another 
> > json-object
> > +with RAM migration status and if block migration is active another one with
> > +block migration status.
> > +
> > +The main json-object contains the following:
> > +
> > +- "status": migration status (json-string)
> >    
> 
> Please list all possible values here (what's the status before the first 
> migration is started?

 The command returns "void", Markus has some suggestions on how to fix that.

> Very nice overall.  It is *way* easier to review documentation than 
> piles of code, so I suggest starting with documentation from now on.  
> I've adopted this approach in kvm.

 Absolutely, will work on v3 and resubmit.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]