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.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list