From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=38528 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OCYGh-0002Or-2L for qemu-devel@nongnu.org; Thu, 13 May 2010 09:16:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OCYGf-0003BN-3E for qemu-devel@nongnu.org; Thu, 13 May 2010 09:16:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37082) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OCYGe-0003BG-QV for qemu-devel@nongnu.org; Thu, 13 May 2010 09:16:01 -0400 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o4DDFwNx025850 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 13 May 2010 09:15:59 -0400 Date: Thu, 13 May 2010 10:15:50 -0300 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH 1/2] QMP: Introduce commands doc Message-ID: <20100513101550.75a7cb50@redhat.com> In-Reply-To: References: <1273086712-29163-1-git-send-email-lcapitulino@redhat.com> <1273086712-29163-2-git-send-email-lcapitulino@redhat.com> <20100512181712.5637f26f@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: bazulay@redhat.com, juzhang@redhat.com, qemu-devel@nongnu.org On Thu, 13 May 2010 09:01:21 +0200 Markus Armbruster wrote: > Luiz Capitulino writes: > > > On Wed, 12 May 2010 18:48:38 +0200 > > Markus Armbruster 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.