Re: [libvirt] [PATCHv3 19/36] qemu: json: Add format strings for optional command arguments

On 05/30/2014 02:37 AM, Peter Krempa wrote: > This patch adds option to specify that a json qemu command argument is > optional without the need to use if's or ternary operators to pass the > list. Additionally all the modifier characters are documented to avoid > user confusion. > --- > src/qemu/qemu_monitor_json.c | 122 +++++++++++++++++++++++++++++-------------- > 1 file changed, 84 insertions(+), 38 deletions(-) > The diffstat is misleading - the bulk of the addition is documentation, and the bulk of the deletions are compression taking advantage of the new feature. Overall, I like the patch! > + * > + * i: signed integer value > + * z: signed integer value, omitted if zero > + * > + * I: signed long integer value > + * Z: integer value, signed, omitted if zero Looks more consistent as: "signed long integer value, omitted if zero" > + * > + * u: unsigned integer value > + * p: unsigned integer value, omitted if zero > + * > + * U: unsigned long integer value (see below for quirks) > + * P: unsigned long integer value, omitted if zero, drop the trailing comma > + * > + * d: double precision floating point number > + * b: boolean value Is it worth a B for a boolean value that is omitted if false?

> + * n: json null value > + * a: json array > + */ > type = key[0]; > key += 2; > > @@ -3557,7 +3603,7 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon, > > cmd = qemuMonitorJSONMakeCommand("send-key", > "a:keys", keys, > - holdtime ? "U:hold-time" : NULL, holdtime, > + "P:hold-time", holdtime, > NULL); Fix the indentation while you are at it. ACK with nits addressed.