Re: [libvirt] [PATCH v1 03/11] qemu: Introduce nbd-server-start command
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.