[libvirt] [PATCH v1 03/11] qemu: Introduce nbd-server-start command

[Michal Privoznik]

On 28.11.2012 00:26, Eric Blake wrote:
> ----- Original Message -----
>> This will be used with new migration scheme.
>> This patch creates basically just monitor stub
>> functions. Wiring them into something useful
>> is done in later patches.
> 
> Reasonable way to break up the review.
> 
>> ---
>>  src/qemu/qemu_monitor.c      |   22 ++++++++++++++++++
>>  src/qemu/qemu_monitor.h      |    3 ++
>>  src/qemu/qemu_monitor_json.c |   49
>>  ++++++++++++++++++++++++++++++++++++++++++
>>  src/qemu/qemu_monitor_json.h |    3 ++
>>  4 files changed, 77 insertions(+), 0 deletions(-)
> 
>> +    if (!(data = virJSONValueNewObject()) ||
>> +        !(addr = virJSONValueNewObject()) ||
>> +        virAsprintf(&port_str, "%d", port) < 0) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virJSONValueObjectAppendString(data, "host", host) < 0 ||
>> +        virJSONValueObjectAppendString(data, "port", port_str) < 0
> 
> Is 'port' really a string rather than a JSON integer?  (goes and checks...
> yep - you really did match the JSON here to the documentation in
> qemu.git:qapi-schema.json)

I was surprised as well. Maybe a comment just before would be nice.

> 
>> ||
>> +        virJSONValueObjectAppendString(addr, "type", "inet") < 0 ||
>> +        virJSONValueObjectAppend(addr, "data", data) < 0) {
> 
> Hmm, you aren't supplying anything for the optional 'ipv4' and 'ipv6'
> portions of the address; do we always want the defaults of always
> trying both families, or are we going to need to make this configurable?
> But I guess we can add that later if we find we need it.

Yeah. This is just internal code - no need to make it as general as
possible for now. We can change it whenever somebody needs it. Where if
this is be public API I would be all ten for as general as possible.

> 
> ACK.
>