[libvirt] [PATCH 03/13] qemu: rewrite NBD command-line builder and parser
Paolo Bonzini
pbonzini at redhat.com
Mon Mar 11 13:18:05 UTC 2013
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 at 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");
More information about the libvir-list
mailing list