On Fri, Nov 26, 2010 at 05:49:56AM +0900, MORITA Kazutaka wrote:
This patch adds network disk support to libvirt/QEMU. The currently supported protcols are nbd, rbd, and sheepdog. The XML syntax is like this:
<disk type="network" device="disk"> <driver name="qemu" type="raw" /> <source protocol='rbd|sheepdog|nbd' name="...some image identifier..."> <host name="mon1.example.org" port="6000"> <host name="mon2.example.org" port="6000"> <host name="mon3.example.org" port="6000"> </source> <target dev="vda" bus="virtio" /> </disk>
This design looks good to me.
Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> ---
This patch addresses the discussion on https://www.redhat.com/archives/libvir-list/2010-November/msg00759.html
Josh mentioned that the monitor hostnames of RBD can be set through the environment variables, but I couldn't find any documentations about it, so the monitors are not set in this patch. I hope someone who is familiar with RBD implements it.
@@ -1620,6 +1629,30 @@ virDomainDiskDefParseXML(virCapsPtr caps, case VIR_DOMAIN_DISK_TYPE_DIR: source = virXMLPropString(cur, "dir"); break; + case VIR_DOMAIN_DISK_TYPE_NETWORK: + protocol = virXMLPropString(cur, "protocol"); + if (protocol == NULL) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("missing protocol type")); + break; + } + def->protocol = virDomainDiskProtocolTypeFromString(protocol);
Should check for def->protocal < 0 & raise an error, which would indicate that 'protocol' was not one of the expected values.
+ source = virXMLPropString(cur, "name"); + host = cur->children; + while (host != NULL) { + if (host->type == XML_ELEMENT_NODE && + xmlStrEqual(host->name, BAD_CAST "host")) { + if (VIR_REALLOC_N(hosts, def->nhosts + 1) < 0) { + virReportOOMError(); + goto error; + } + hosts[def->nhosts].name = virXMLPropString(host, "name"); + hosts[def->nhosts].port = virXMLPropString(host, "port"); + def->nhosts++;
Ought to check for NULLs returned by virXMLPropString here I think.
+ } + host = host->next; + } + break; default: virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %s"), @@ -1685,7 +1718,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
/* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present */ - if (source == NULL && + if (source == NULL && hosts == NULL && def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { virDomainReportError(VIR_ERR_NO_SOURCE, @@ -1791,6 +1824,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, source = NULL; def->dst = target; target = NULL; + def->hosts = hosts; + hosts = NULL; def->driverName = driverName; driverName = NULL; def->driverType = driverType; @@ -1819,6 +1854,8 @@ cleanup: VIR_FREE(type); VIR_FREE(target); VIR_FREE(source); + VIR_FREE(hosts);
I think you need to free the fields inside 'hosts' too, otherwise we'd leak memory for the port + host strings in the error path.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 35caccc..63abd75 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2714,7 +2714,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, break; }
- if (disk->src) { + if (disk->src || disk->nhosts > 0) {
If you check for nhosts > 0 here
if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) { /* QEMU only supports magic FAT format for now */ if (disk->driverType && @@ -2733,6 +2733,24 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virBufferVSprintf(&opt, "file=fat:floppy:%s,", disk->src); else virBufferVSprintf(&opt, "file=fat:%s,", disk->src); + } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { + switch (disk->protocol) { + case VIR_DOMAIN_DISK_PROTOCOL_NBD: + virBufferVSprintf(&opt, "file=nbd:%s:%s,", + disk->hosts->name, disk->hosts->port); + break; + case VIR_DOMAIN_DISK_PROTOCOL_RBD: + virBufferVSprintf(&opt, "file=rbd:%s,", disk->src); + break; + case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: + if (disk->nhosts > 0) + virBufferVSprintf(&opt, "file=sheepdog:%s:%s:%s,", + disk->hosts->name, disk->hosts->port, + disk->src); + else + virBufferVSprintf(&opt, "file=sheepdog:%s,", disk->src); + break;
This else branch for sheepdog will never execute. So I think it is better to check the nhosts value in each of these conditionals. eg We should report an error if nhosts == 0, or nhosts > 1 for NBD since it only wants 1 host. Likewise for other protocols as nedeed.
@@ -5794,7 +5830,67 @@ qemuParseCommandLineDisk(virCapsPtr caps, values[i] = NULL; if (STRPREFIX(def->src, "/dev/")) def->type = VIR_DOMAIN_DISK_TYPE_BLOCK; - else + else if (STRPREFIX(def->src, "nbd:")) { + char *host, *port; + + def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + host = def->src + strlen("nbd:"); + port = strchr(host, ':'); + if (!port) { + def = NULL; + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), def->src); + goto cleanup; + } + *port++ = '\0'; + if (VIR_ALLOC(def->hosts) < 0) { + virReportOOMError(); + goto cleanup; + } + def->nhosts = 1; + def->hosts->name = strdup(host); + def->hosts->port = strdup(port);
Should check for NULL in both of these strdup cases.
+ + VIR_FREE(def->src); + def->src = NULL; + } else if (STRPREFIX(def->src, "rbd:")) { + char *p = def->src; + + def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->src = strdup(p + strlen("rbd:"));
And this one, and a few more strdup's later in the patch. Regards, Daniel