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

[Markus Armbruster]

Avi Kivity <address@hidden> writes:

> 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.
[...]
>> +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.

I like the idea.

>> +
>> +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)?

bytes 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"?
>
>> +- "size": the memory size (json-int)
>>    
>
> In bytes.

All sizes are in bytes.  Any exceptions are bugs.

That said, it's okay for reference documentation to repeat such things
over and over.

[...]
>> +query-pci
>> +---------
>> +
>> +PCI buses and devices information.
>> +
>> +The returned value is a json-array of all buses. Each bus is represented by
>> +a json-object, which has a key with a json-array of all PCI devices attached
>> +to it. Each device is represented by a json-object.
>> +
>> +The bus json-object contains the following:
>> +
>> +- "bus": bus number (json-int)
>> +- "devices": a json-array of json-objects, each json-object represents a
>> +             PCI device
>> +
>> +The PCI device json-object contains the following:
>> +
>> +- "bus": identical to the parent's bus number (json-int)
>> +- "slot": slot number (json-int)
>> +- "function": function number (json-int)
>>    
>
> Would have been nicer as a nested object (list of buses, each
> containing a list of slots, each containing a list of functions).

We have a list of buses, each containing a list of device functions.
Not sure the additional level of nesting you propose buys us anything.

I figure we can still change this if we want.  libvirt uses info pci
only with pre-0.12 QEMU, over the human monitor.

>> +- "class_info": a json-object containing:
>> +     - "desc": device class description (json-string, optional)
>> +     - "class": device class number (json-int)
>> +- "id": a json-object containing:
>> +     - "device": device ID (json-int)
>> +     - "vendor": vendor ID (json-int)
>> +- "irq": device's IRQ if assigned (json-int, optional)
>>    
>
> Please note here that the OS may ignore it, so may not be authoritative.
>
>> +- "qdev_id": qdev id string (json-string)
>> +- "pci_bridge": It's a json-object, only present if this device is a
>> +                PCI bridge, contains:
>> +     - "bus": bus number (json-int)
>> +     - "secondary": secondary bus number (json-int)
>> +     - "subordinate": subordinate bus number (json-int)
>> +     - "io_range": a json-object with memory range information (json-int)
>>    
>
> Object or int?

Delete "(json-int)", add suitable "see below".

>> +     - "memory_range": a json-object with memory range information 
>> (json-int)
>>    
> Object or int?

Delete "(json-int)", add suitable "see below".

>> +     - "prefetchable_range": a json-object with memory range
>> +                             information (json-int)
>>    
>
> Format not understood.

Delete "(json-int)", add suitable "see below".

>> +     - "devices": a json-array of PCI devices if there's any attached 
>> (optional)
>>    
>
> What is the format of an array element?

Yes, that's not clear here.

[...]