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.