On 03/11/2013 07:18 AM, Paolo Bonzini wrote:
+ h->name = strdup(host); + if (!h->name)
Trivial, but we prefer:
if (!(h->name = strdup(host)))
Personally, I don't care enough to rewrite it - adding parenthesis to save a line isn't documented in HACKING as a requirement.
+ virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); + virBufferEscape(opt, ',', ",", ":%s", + disk->hosts->port ? disk->hosts->port : "10809");
What's the magic (10809)? It's not in the old code, I guess it's the default port, but should we have a macro for it instead?
Your choice.
A macro seems nice, so I added one: diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 7232906..9e2d6a9 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -2473,6 +2473,8 @@ no_memory: goto cleanup; } +#define QEMU_DEFAULT_NBD_PORT "10809" + static int qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) { @@ -2491,7 +2493,8 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) if (disk->hosts->name) virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); virBufferEscape(opt, ',', ",", ":%s", - disk->hosts->port ? disk->hosts->port : "10809"); + disk->hosts->port ? disk->hosts->port : + QEMU_DEFAULT_NBD_PORT); break; default: transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); On 03/15/2013 08:32 AM, Daniel P. Berrange wrote:
I would have had the 'parse' method further down near the other parse function which calls it, but no big deal.
I didn't bother with this.
+++ b/tests/qemuxml2xmltest.c @@ -166,6 +166,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wt"); DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); + DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi");
This test case addition could be pushed indepedently of the rest of this patch.
I did just that, with this commit message: commit b96e0c18e161d80c88af354ef952fe2b6013f20f Author: Paolo Bonzini <pbonzini@redhat.com> Date: Mon Feb 25 18:44:22 2013 +0100 qemu: test NBD command-line builder and parser Enable more testing of NBD parsing, to ensure rewrites work. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>
ACK
and pushed, as two patches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org