[Sheepdog] [PATCH] add network disk support
Daniel P. Berrange
berrange at redhat.com
Thu Dec 2 14:19:28 CET 2010
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 at 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
More information about the sheepdog
mailing list