[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Il 11/03/2013 04:38, Osier Yang ha scritto:
> On 2013年02月26日 01:44, Paolo Bonzini wrote:
>> Move the code to an external function, and structure it to prepare
>> the addition of new features in the next few patches.
>>
>> Signed-off-by: Paolo Bonzini<pbonzini redhat com>
>> ---
>>   src/qemu/qemu_command.c | 128
>> ++++++++++++++++++++++++++++--------------------
>>   tests/qemuxml2xmltest.c |   1 +
>>   2 files changed, 76 insertions(+), 53 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index a3c5a4e..beb7cfe 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -2128,6 +2128,45 @@ error:
>>   }
>>
>>   static int
>> +qemuParseNBDString(virDomainDiskDefPtr disk)
>> +{
>> +    virDomainDiskHostDefPtr h = NULL;
>> +    char *host, *port;
>> +
>> +    if (VIR_ALLOC(h)<  0)
>> +        goto no_memory;
>> +
>> +    host = disk->src + strlen("nbd:");
>> +    port = strchr(host, ':');
>> +    if (!port) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("cannot parse nbd filename '%s'"), disk->src);
>> +        goto error;
>> +    }
>> +
>> +    *port++ = '\0';
>> +    h->name = strdup(host);
>> +    if (!h->name)
> 
> Trivial, but we prefer:
> 
> if (!(h->name = strdup(host)))
> 
>> +        goto no_memory;
>> +
>> +    h->port = strdup(port);
>> +    if (!h->port)
> 
> Likewise.

Ok.

>> +        goto no_memory;
>> +
>> +    VIR_FREE(disk->src);
> 
> You lost setting defaults when refactoring:
> 
> -                    def->hosts->transport =
> VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
> -                    def->hosts->socket = NULL;

These are both zero in memory, and VIR_ALLOC guarantees that.

>> +    disk->nhosts = 1;
>> +    disk->hosts = h;
>> +    return 0;
>> +
>> +no_memory:
>> +    virReportOOMError();
>> +error:
>> +    virDomainDiskHostDefFree(h);
>> +    VIR_FREE(h);
>> +    return -1;
>> +}
>> +
>> +static int
>>   qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt)
>>   {
>>       int ret = -1;
>> @@ -2188,6 +2227,36 @@ no_memory:
>>       goto cleanup;
>>   }
>>
>> +static int
>> +qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt)
>> +{
>> +    const char *transp;
>> +
>> +    if (disk->nhosts != 1) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("nbd accepts only one host"));
>> +        return -1;
>> +    }
>> +
>> +    virBufferAddLit(opt, "file=nbd:");
>> +
>> +    switch (disk->hosts->transport) {
>> +    case VIR_DOMAIN_DISK_PROTO_TRANS_TCP:
>> +        if (disk->hosts->name)
>> +            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.

Paolo

>> +        break;
>> +    default:
>> +        transp =
>> virDomainDiskProtocolTransportTypeToString(disk->hosts->transport);
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("nbd does not support transport '%s'"),
>> transp);
>> +        break;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   char *
>>   qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>>                     virDomainDiskDefPtr disk,
>> @@ -2314,13 +2383,9 @@ qemuBuildDriveStr(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>>           } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
>>               switch (disk->protocol) {
>>               case VIR_DOMAIN_DISK_PROTOCOL_NBD:
>> -                if (disk->nhosts != 1) {
>> -                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                                   _("NBD accepts only one host"));
>> +                if (qemuBuildNBDString(disk,&opt)<  0)
>>                       goto error;
>> -                }
>> -                virBufferAsprintf(&opt, "file=nbd:%s:%s,",
>> -                                  disk->hosts->name, disk->hosts->port);
>> +                virBufferAddChar(&opt, ',');
>>                   break;
>>               case VIR_DOMAIN_DISK_PROTOCOL_RBD:
>>                   virBufferAddLit(&opt, "file=");
>> @@ -7337,39 +7402,11 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps,
>>                   if (STRPREFIX(def->src, "/dev/"))
>>                       def->type = VIR_DOMAIN_DISK_TYPE_BLOCK;
>>                   else if (STRPREFIX(def->src, "nbd:")) {
>> -                    char *host, *port;
>> -
>>                       def->type = VIR_DOMAIN_DISK_TYPE_NETWORK;
>>                       def->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD;
>> -                    host = def->src + strlen("nbd:");
>> -                    port = strchr(host, ':');
>> -                    if (!port) {
>> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                                       _("cannot parse nbd filename
>> '%s'"),
>> -                                       def->src);
>> -                        goto error;
>> -                    }
>> -                    *port++ = '\0';
>> -                    if (VIR_ALLOC(def->hosts)<  0) {
>> -                        virReportOOMError();
>> -                        goto error;
>> -                    }
>> -                    def->nhosts = 1;
>> -                    def->hosts->name = strdup(host);
>> -                    if (!def->hosts->name) {
>> -                        virReportOOMError();
>> -                        goto error;
>> -                    }
>> -                    def->hosts->port = strdup(port);
>> -                    if (!def->hosts->port) {
>> -                        virReportOOMError();
>> -                        goto error;
>> -                    }
>> -                    def->hosts->transport =
>> VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
>> -                    def->hosts->socket = NULL;
>>
>> -                    VIR_FREE(def->src);
>> -                    def->src = NULL;
>> +                    if (qemuParseNBDString(def)<  0)
>> +                        goto error;
>>                   } else if (STRPREFIX(def->src, "rbd:")) {
>>                       char *p = def->src;
>>
>> @@ -8599,7 +8636,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr
>> qemuCaps,
>>               else if (STRPREFIX(val, "nbd:")) {
>>                   disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK;
>>                   disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD;
>> -                val += strlen("nbd:");
>>               } else if (STRPREFIX(val, "rbd:")) {
>>                   disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK;
>>                   disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD;
>> @@ -8639,26 +8675,12 @@ virDomainDefPtr
>> qemuParseCommandLine(virCapsPtr qemuCaps,
>>                   goto no_memory;
>>
>>               if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
>> -                char *host, *port;
>> +                char *port;
>>
>>                   switch (disk->protocol) {
>>                   case VIR_DOMAIN_DISK_PROTOCOL_NBD:
>> -                    host = disk->src;
>> -                    port = strchr(host, ':');
>> -                    if (!port) {
>> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                                       _("cannot parse nbd filename
>> '%s'"), disk->src);
>> +                    if (qemuParseNBDString(disk)<  0)
>>                           goto error;
>> -                    }
>> -                    *port++ = '\0';
>> -                    if (VIR_ALLOC(disk->hosts)<  0)
>> -                        goto no_memory;
>> -                    disk->nhosts = 1;
>> -                    disk->hosts->name = host;
>> -                    disk->hosts->port = strdup(port);
>> -                    if (!disk->hosts->port)
>> -                        goto no_memory;
>> -                    disk->src = NULL;
>>                       break;
>>                   case VIR_DOMAIN_DISK_PROTOCOL_RBD:
>>                       /* old-style CEPH_ARGS env variable is parsed
>> later */
>> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
>> index 3f36896..2fa93a9 100644
>> --- a/tests/qemuxml2xmltest.c
>> +++ 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");


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]