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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 13 May 2010 09:01:21 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 12 May 2010 18:48:38 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> > +query-block
> >> > +-----------
> >> > +
> >> > +Show the block devices.
> >> > +
> >> > +Each block device information is stored in a json-object and the returned value
> >> > +is a json-array of all devices.
> >> > +
> >> > +Each json-object contain the following:
> >> > +
> >> > +- "device": device name (json-string)
> >> > +- "type": device type (json-string)
> >> 
> >> Possible values?  "hd", "cdrom", "floppy".  Code also has "unknown", but
> >> when it uses that, it's badly broken.
> >
> >  Yes, but you didn't mean we shouldn't use 'unknown', right?
> 
> Shouldn't we reply with some internal error when it happens instead of
> sending a crap value?  Anyway, it's not a problem with this patch, just
> a separate issue I found while reviewing it.

 Separate issue, indeed, but it's good we're talking about them in this
thread.

 I'm not sure returning an error is reasonable, we should only do that
if this invalidates the whole call.

> Documentation for the expected values would be nice.  I'm fine with
> doing that in a follow-up patch.  Same for the other places I flagged.

 Good!

> >> > +- "removable": true if the device is removable, false otherwise (json-bool)
> >> > +- "locked": true if the device is locked, false otherwise (json-bool)
> >> > +- "inserted": only present if the device is inserted, it is a json-object
> >> > +   containing the following:
> >> > +         - "file": device file name (json-string)
> >> > +         - "ro": true if read-only, false otherwise (json-bool)
> >> > +         - "drv": driver format name (json-string)
> >> 
> >> Possible values?
> >
> >  I got the following list by grepping the code. Kevin, can you confirm it's
> > correct?
> >
> >  "blkdebug", "bochs", "cloop", "cow", "dmg", "file", "file", "ftp", "ftps",
> >  "host_cdrom", "host_cdrom", "host_device", "host_device", "host_floppy",
> >  "http", "https", "nbd", "parallels", "qcow", "qcow2", "raw", "tftp", "vdi",
> >  "vmdk", "vpc", "vvfat"
> >
> >> > +query-cpus
> >> > +----------
> >> > +
> >> > +Show CPU information.
> >> > +
> >> > +Return a json-array. Each CPU is represented by a json-object, which contains:
> >> > +
> >> > +- "cpu": CPU index (json-int)
> >> 
> >> It's actually upper case (see example below).  I hate that.
> >
> >  Hm, this one leaked.. But it's quite minor anyway.
> 
> My comment on this patch is "please make it consistent with the code",
> no more.

 Right.

> >> > +- "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)
> >> 
> >> Key name depending on arch: isn't that an extraordinarily bad idea?
> >
> >  Honestly, I don't think it's that bad: it's a form of optional key,
> > but if you think it's so bad I can add a "arch" key with the arch's name.
> >
> >  Don't think anyone is using this, anyway.
> 
> Why not call it "pc" and be done with it?

 Because the interpretation of 'pc' depends on the arch, the printing
code is an example of that.

> Again, this is not a problem with this patch, but a separate issue.
> 
> >> > +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)
> >> 
> >> Possible values?  "active", "completed", "failed", "cancelled".  Note
> >> there's no value for "never attempted"; see below.
> >> 
> >> > +- "ram": only present if "status" is "active", it is a json-object with the
> >> > +  following RAM information (in bytes):
> >> > +         - "transferred": amount transferred (json-int)
> >> > +         - "remaining": amount remaining (json-int)
> >> > +         - "total": total (json-int)
> >> > +- "disk": only present if "status" is "active" and it is a block migration,
> >> > +  it is a json-object with the following disk information (in bytes):
> >> > +         - "transferred": amount transferred (json-int)
> >> > +         - "remaining": amount remaining (json-int)
> >> > +         - "total": total (json-int)
> >> 
> >> Before the first migration, we actually reply with
> >> 
> >> {"return": {}}
> >> 
> >> which contradicts the doc.
> >
> >  Good catch, what would be the best behavior here?
> 
> Three behaviors come to mind:
> 
> * There is no migration status before migration has been attempted, and
>   asking for it is an error.  So send one.

 It's not an error, nothing went wrong.

> * Invent a new value for "status".
> 
> * Pretend the (non-existant) previous migration completed.
> 
> Matter of taste.  Last one is the simplest.

 I like the second best, but I'm not sure. Maybe migration people
should help here, as this can be improved in the user monitor
as well.