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(a)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