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@xxxxxxxxxxxxx> > --- > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list