Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Wed, 12 May 2010 18:48:38 +0200
> Markus Armbruster <arm...@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.
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.
>> > +- "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.
>> > +- "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?
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.
* Invent a new value for "status".
* Pretend the (non-existant) previous migration completed.
Matter of taste. Last one is the simplest.
Anyway, separate issue.
>> > +Example:
>> > +
>> > +{
>> > + "return":[
>> > + {
>> > + "bus":0,
>> > + "devices":[
>> > + {
>> > + "bus":0,
>> > + "qdev_id":"",
>> > + "slot":0,
>> > + "class_info":{
>> > + "class":1536,
>> > + "desc":"Host bridge"
>> > + },
>> > + "id":{
>> > + "device":32902,
>> > + "vendor":4663
>> > + },
>> > + "function":0,
>> > + "regions":[
>> > +
>> > + ]
>>
>> Suggest to simply write
>>
>> "regions":[]
>
> I could, and I agree it's better, but I'm using a formatting tool, so
> editing by hand would be a PITA.
>
>> > +Note: This example has been shortened as the real response is too long.
>>
>> Actually, I get a shorter response for my minimal guest: just slots
>> 0..3. Suggest to omit slot 4 and this note.
>
> What's the command-line for it?
$ qemu-system-x86_64 -nodefaults -enable-kvm -m 1024 -vga cirrus -S -vnc :0
-usb -readconfig ~/work/qemu/test.cfg
with the following test.cfg (created by -writeconfig):
# qemu config file
[drive "hda"]
if = "none"
file = "f12.img"
[chardev "monitor"]
backend = "stdio"
[chardev "qmp"]
backend = "socket"
path = "f12-qmp"
server = "on"
wait = "off"
[device]
driver = "ide-drive"
drive = "hda"
bus = "ide.0"
unit = "0"
[device "nic.0"]
driver = "virtio-net-pci"
netdev = "net.0"
[netdev "net.0"]
type = "user"
[mon "monitor"]
mode = "readline"
chardev = "monitor"
default = "on"
[mon "qmp"]
mode = "control"
chardev = "qmp"
>> > +query-vnc
>> > +---------
>> > +
>> > +Show VNC server information.
>> > +
>> > +Return a json-object with server information. Connected clients are
>> > returned
>> > +as a json-array of json-objects.
>> > +
>> > +The main json-object contains the following:
>> > +
>> > +- "enabled": true or false (json-bool)
>> > +- "host": server's IP address (json-string)
>>
>> Wouldn't hurt to mention it can be a wildcard address. The example
>> below shows the IPv4 wildcard address "0.0.0.0".
>>
>> > +- "family": address family (json-string, "ipv4" or "ipv6")
>>
>> inet_strfamily() can also return "unix" and "unknown".
>>
>> By the way, we use PF_INET6, PF_INET and PF_UNIX there. To be
>> pedantically correct, we should use AF_INET6, AF_INET and AF_LOCAL.
>>
>> > +- "service": server's port number (json-string)
>>
>> Why isn't this json-int?
>
> Because getnameinfo() returns a string and I didn't bother using it,
> do you?
>From a protocol point of view, transmitting a number as a string is
wrong. How we find the number in QEMU is an irrelevant implementation
detail.
Another separate issue.
