[libvirt] [PATCH 03/13] qemu: rewrite NBD command-line builder and parser

[Eric Blake]

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 at 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 at redhat.com>
    Signed-off-by: Eric Blake <eblake at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130315/e95928c3/attachment-0001.sig>