[libvirt] [PATCH v2 0/2] Qemu/Gluster support in Libvirt
This patchset provides support for Gluster protocol based network disks. Changelog: v2: - Addressed review comments by Jiri - Updated patcheset as per new URI spec Ref: http://lists.gnu.org/archive/html/qemu-devel/2012-09/msg05199.html v1: - Initial prototype Harsh Prateek Bora (2): Qemu/Gluster: Add Gluster protocol as supported network disk formats. tests: Add tests for gluster protocol based network disks support docs/schemas/domaincommon.rng | 8 + src/conf/domain_conf.c | 28 ++- src/conf/domain_conf.h | 11 ++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 +++++++++++++++++++++ tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-gluster.args | 1 + .../qemuxml2argv-disk-drive-network-gluster.xml | 33 ++++ tests/qemuxml2argvtest.c | 2 + 9 files changed, 288 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.xml -- 1.7.11.4
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this: <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk> Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket. Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..89d9b9f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1048,6 +1048,7 @@ <value>nbd</value> <value>rbd</value> <value>sheepdog</value> + <value>gluster</value> </choice> </attribute> <optional> @@ -1061,6 +1062,13 @@ <attribute name="port"> <ref name="unsignedInt"/> </attribute> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>unix</value> + <value>rdma</value> + </choice> + </attribute> </element> </zeroOrMore> <empty/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..838f079 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -214,7 +214,13 @@ VIR_ENUM_IMPL(virDomainDiskErrorPolicy, VIR_DOMAIN_DISK_ERROR_POLICY_LAST, VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST, "nbd", "rbd", - "sheepdog") + "sheepdog", + "gluster") + +VIR_ENUM_IMPL(virDomainDiskProtocolTransport, VIR_DOMAIN_DISK_PROTO_TRANS_LAST, + "tcp", + "unix", + "rdma") VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST, "none", @@ -3460,6 +3466,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *source = NULL; char *target = NULL; char *protocol = NULL; + char *protocol_transport; char *trans = NULL; virDomainDiskHostDefPtr hosts = NULL; int nhosts = 0; @@ -3566,6 +3573,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, } hosts[nhosts].name = NULL; hosts[nhosts].port = NULL; + hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; nhosts++; hosts[nhosts - 1].name = virXMLPropString(child, "name"); @@ -3580,6 +3588,17 @@ virDomainDiskDefParseXML(virCapsPtr caps, "%s", _("missing port for host")); goto error; } + /* transport can be tcp (default), unix or rdma. */ + protocol_transport = virXMLPropString(child, "transport"); + if (protocol_transport != NULL) { + hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(protocol_transport); + if (hosts[nhosts - 1].transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown protocol transport type '%s'"), + protocol_transport); + goto error; + } + } } child = child->next; } @@ -11756,8 +11775,13 @@ virDomainDiskDefFormat(virBufferPtr buf, for (i = 0; i < def->nhosts; i++) { virBufferEscapeString(buf, " <host name='%s'", def->hosts[i].name); - virBufferEscapeString(buf, " port='%s'/>\n", + virBufferEscapeString(buf, " port='%s'", def->hosts[i].port); + if (def->hosts[i].transport) { + virBufferAsprintf(buf, " transport='%s'", + virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport)); + } + virBufferAddLit(buf, "/>\n"); } virBufferAddLit(buf, " </source>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..7ba7e9b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -456,10 +456,19 @@ enum virDomainDiskProtocol { VIR_DOMAIN_DISK_PROTOCOL_NBD, VIR_DOMAIN_DISK_PROTOCOL_RBD, VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG, + VIR_DOMAIN_DISK_PROTOCOL_GLUSTER, VIR_DOMAIN_DISK_PROTOCOL_LAST }; +enum virDomainDiskProtocolTransport { + VIR_DOMAIN_DISK_PROTO_TRANS_TCP, + VIR_DOMAIN_DISK_PROTO_TRANS_UNIX, + VIR_DOMAIN_DISK_PROTO_TRANS_RDMA, + + VIR_DOMAIN_DISK_PROTO_TRANS_LAST +}; + enum virDomainDiskTray { VIR_DOMAIN_DISK_TRAY_CLOSED, VIR_DOMAIN_DISK_TRAY_OPEN, @@ -481,6 +490,7 @@ typedef virDomainDiskHostDef *virDomainDiskHostDefPtr; struct _virDomainDiskHostDef { char *name; char *port; + int transport; /* enum virDomainDiskProtocolTransport */ }; enum virDomainDiskIo { @@ -2166,6 +2176,7 @@ VIR_ENUM_DECL(virDomainDiskBus) VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) +VIR_ENUM_DECL(virDomainDiskProtocolTransport) VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDiskTray) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dab607a..8e70837 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -350,6 +350,8 @@ virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskPathByName; +virDomainDiskProtocolTransportTypeFromString; +virDomainDiskProtocolTransportTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 20730a9..0bed2bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2021,6 +2021,168 @@ no_memory: return -1; } +static int qemuParseGlusterString(virDomainDiskDefPtr def) +{ + char *host = NULL, *port = NULL, *volimg, *transp = NULL, *marker; + + if (STRPREFIX(def->src, "+")) { + transp = def->src; + transp++; + marker = strstr(def->src, "://"); + *marker++ = '\0'; + marker += 2; + } else { + /* transport type not specified, use tcp as default */ + def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + marker = strstr(def->src, "://"); + marker += 3; + } + + if (transp) { + def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp); + if (def->hosts->transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid gluster transport type '%s'"), transp); + return -1; + } + } + + /* now marker points to string which can start with one of the following: + * IPv6 address in square brackets followed by port (optional) + * <hostname> or <IPv4 address> followed by port (optional) + * '/' if its a unix socket followed by path to gluster disk volume/image + */ + + /* parse server, port */ + if (def->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (STRPREFIX(marker, "[")) { + /* IPv6 addr */ + host = ++marker; + marker = strchr(host, ']'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse IPv6 addr for gluster host %s "), host); + return -1; + } + *marker++ = '\0'; + /* parse port if specified */ + if (STRPREFIX(marker, ":")) { + port = ++marker; + marker = strchr(port, '/'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + } else { + marker++; /* port not specified, skip slash */ + } + } else { + /* IPv4 address / hostname followed by port (optional) */ + host = marker; + marker = strchr(host, '/'); /* skip to path to gluster disk vol/img */ + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + port = strchr(host, ':'); + if (port) { + /* port was specified with host, separate both */ + *port++ = '\0'; + } + } + + /* host points to hostname / IPv4 / IPv6 addr + * port points to port or NULL is port was not specified + */ + } else { + /* transport type is unix, expect one more slash + * followed by path to gluster disk vol/img */ + if (STRPREFIX(marker, "/")) { + marker++; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Gluster unix transport url starts with 3 slashes i.e. gluster+unix:///")); + return -1; + } + } + + /* marker now points to path to gluster disk vol/img */ + volimg = marker; + + /* if transport type = unix, path to gluster disk vol/img + * is followed by ?socket=<path/to/socket> */ + if (def->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(marker, "?socket=")) { + /* In libvirt xml, socket path is to be provided as + * <host name='/path/to/socket' port='0'> + */ + host = strchr(marker, '='); + *host++ = '\0'; + } + } + + if (VIR_ALLOC(def->hosts) < 0) { + virReportOOMError(); + return -1; + } + def->nhosts = 1; + def->hosts->name = host; + if (port) { + def->hosts->port = port; + } else { + def->hosts->port = strdup("0"); + } + if (!def->hosts->port) { + virReportOOMError(); + return -1; + } + def->src = strdup(volimg); + if (!def->src) { + virReportOOMError(); + return -1; + } + + return 0; +} + +static int +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + int ret = 0; + virBufferAddLit(opt, "file="); + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster accepts only one host")); + ret = -1; + } else { + virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { + /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } + } + return ret; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2162,6 +2324,12 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; virBufferAddChar(&opt, ','); break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (qemuBuildGlusterString(disk, &opt) < 0) + goto error; + virBufferAddChar(&opt, ','); + break; + case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { virBufferEscape(&opt, ',', ",", "file=sheepdog:%s,", @@ -5242,6 +5410,18 @@ qemuBuildCommandLine(virConnectPtr conn, file = virBufferContentAndReset(&opt); } break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + { + virBuffer opt = VIR_BUFFER_INITIALIZER; + if (qemuBuildGlusterString(disk, &opt) < 0) + goto error; + if (virBufferError(&opt)) { + virReportOOMError(); + goto error; + } + file = virBufferContentAndReset(&opt); + } + break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { if (virAsprintf(&file, "sheepdog:%s,", disk->src) < 0) { @@ -6937,6 +7117,21 @@ qemuParseCommandLineDisk(virCapsPtr caps, goto cleanup; VIR_FREE(p); + } else if (STRPREFIX(def->src, "gluster")) { + char *p = def->src; + + def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; + def->src = strdup(p + strlen("gluster")); + if (!def->src) { + virReportOOMError(); + goto cleanup; + } + + if (qemuParseGlusterString(def) < 0) + goto cleanup; + + VIR_FREE(p); } else if (STRPREFIX(def->src, "sheepdog:")) { char *p = def->src; char *port, *vdi; @@ -8126,6 +8321,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; val += strlen("rbd:"); + } else if (STRPREFIX(val, "gluster")) { + disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; + val += strlen("gluster"); } else if (STRPREFIX(val, "sheepdog:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG; @@ -8211,6 +8410,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (qemuParseGlusterString(disk) < 0) + goto error; + + break; } } -- 1.7.11.4
On Thu, Oct 04, 2012 at 19:01:58 +0530, Harsh Prateek Bora wrote:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-)
Oh, I just realized that this patch is missing documentation in docs/formatdomain.html.in. Sorry for not noticing that in v1. Anyway, no need to resend the whole series just for this. It's sufficient if you send an additional patch to add just the documentation and it can be squashed while pushing or in v3, depending what the result of v2 review is going to be. I'll do a proper review later. Jirka
On 10/04/2012 07:32 PM, Jiri Denemark wrote:
On Thu, Oct 04, 2012 at 19:01:58 +0530, Harsh Prateek Bora wrote:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-)
Oh, I just realized that this patch is missing documentation in docs/formatdomain.html.in. Sorry for not noticing that in v1. Anyway, no need to resend the whole series just for this. It's sufficient if you send an additional patch to add just the documentation and it can be squashed while pushing or in v3, depending what the result of v2 review is going to be.
Hm, I should have included that in v1 as well, will send a patch or include in v3 if there are other changes required.
I'll do a proper review later.
Thanks, Harsh
Jirka
On 10/04/2012 07:01 PM, Harsh Prateek Bora wrote:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..89d9b9f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1048,6 +1048,7 @@ <value>nbd</value> <value>rbd</value> <value>sheepdog</value> + <value>gluster</value> </choice> </attribute> <optional> @@ -1061,6 +1062,13 @@ <attribute name="port"> <ref name="unsignedInt"/> </attribute> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>unix</value> + <value>rdma</value> + </choice> + </attribute> 'transport' attribute is optional, so it should be placed inside <optional> </optional> ?
</element> </zeroOrMore> <empty/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..838f079 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -214,7 +214,13 @@ VIR_ENUM_IMPL(virDomainDiskErrorPolicy, VIR_DOMAIN_DISK_ERROR_POLICY_LAST, VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST, "nbd", "rbd", - "sheepdog") + "sheepdog", + "gluster") + +VIR_ENUM_IMPL(virDomainDiskProtocolTransport, VIR_DOMAIN_DISK_PROTO_TRANS_LAST, + "tcp", + "unix", + "rdma")
VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST, "none", @@ -3460,6 +3466,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *source = NULL; char *target = NULL; char *protocol = NULL; + char *protocol_transport; char *trans = NULL; virDomainDiskHostDefPtr hosts = NULL; int nhosts = 0; @@ -3566,6 +3573,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, } hosts[nhosts].name = NULL; hosts[nhosts].port = NULL; + hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; nhosts++;
hosts[nhosts - 1].name = virXMLPropString(child, "name"); @@ -3580,6 +3588,17 @@ virDomainDiskDefParseXML(virCapsPtr caps, "%s", _("missing port for host")); goto error; } + /* transport can be tcp (default), unix or rdma. */ + protocol_transport = virXMLPropString(child, "transport"); + if (protocol_transport != NULL) { + hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(protocol_transport); + if (hosts[nhosts - 1].transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown protocol transport type '%s'"), + protocol_transport); + goto error; + } + } } child = child->next; } @@ -11756,8 +11775,13 @@ virDomainDiskDefFormat(virBufferPtr buf, for (i = 0; i < def->nhosts; i++) { virBufferEscapeString(buf, " <host name='%s'", def->hosts[i].name); - virBufferEscapeString(buf, " port='%s'/>\n", + virBufferEscapeString(buf, " port='%s'", def->hosts[i].port); + if (def->hosts[i].transport) { + virBufferAsprintf(buf, " transport='%s'", + virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport)); + } + virBufferAddLit(buf, "/>\n"); } virBufferAddLit(buf, " </source>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..7ba7e9b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -456,10 +456,19 @@ enum virDomainDiskProtocol { VIR_DOMAIN_DISK_PROTOCOL_NBD, VIR_DOMAIN_DISK_PROTOCOL_RBD, VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG, + VIR_DOMAIN_DISK_PROTOCOL_GLUSTER,
VIR_DOMAIN_DISK_PROTOCOL_LAST };
+enum virDomainDiskProtocolTransport { + VIR_DOMAIN_DISK_PROTO_TRANS_TCP, + VIR_DOMAIN_DISK_PROTO_TRANS_UNIX, + VIR_DOMAIN_DISK_PROTO_TRANS_RDMA, + + VIR_DOMAIN_DISK_PROTO_TRANS_LAST +}; + enum virDomainDiskTray { VIR_DOMAIN_DISK_TRAY_CLOSED, VIR_DOMAIN_DISK_TRAY_OPEN, @@ -481,6 +490,7 @@ typedef virDomainDiskHostDef *virDomainDiskHostDefPtr; struct _virDomainDiskHostDef { char *name; char *port; + int transport; /* enum virDomainDiskProtocolTransport */ };
enum virDomainDiskIo { @@ -2166,6 +2176,7 @@ VIR_ENUM_DECL(virDomainDiskBus) VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) +VIR_ENUM_DECL(virDomainDiskProtocolTransport) VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDiskTray) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dab607a..8e70837 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -350,6 +350,8 @@ virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskPathByName; +virDomainDiskProtocolTransportTypeFromString; +virDomainDiskProtocolTransportTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 20730a9..0bed2bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2021,6 +2021,168 @@ no_memory: return -1; }
+static int qemuParseGlusterString(virDomainDiskDefPtr def) +{ + char *host = NULL, *port = NULL, *volimg, *transp = NULL, *marker; + + if (STRPREFIX(def->src, "+")) { + transp = def->src; + transp++; + marker = strstr(def->src, "://"); + *marker++ = '\0'; + marker += 2; + } else { + /* transport type not specified, use tcp as default */ + def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + marker = strstr(def->src, "://"); + marker += 3; + } + + if (transp) { + def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp); + if (def->hosts->transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid gluster transport type '%s'"), transp); + return -1; + } + } + + /* now marker points to string which can start with one of the following: + * IPv6 address in square brackets followed by port (optional) + * <hostname> or <IPv4 address> followed by port (optional) + * '/' if its a unix socket followed by path to gluster disk volume/image + */ + + /* parse server, port */ + if (def->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (STRPREFIX(marker, "[")) { + /* IPv6 addr */ + host = ++marker; + marker = strchr(host, ']'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse IPv6 addr for gluster host %s "), host); + return -1; + } + *marker++ = '\0'; + /* parse port if specified */ + if (STRPREFIX(marker, ":")) { + port = ++marker; + marker = strchr(port, '/'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + } else { + marker++; /* port not specified, skip slash */ + } + } else { + /* IPv4 address / hostname followed by port (optional) */ + host = marker; + marker = strchr(host, '/'); /* skip to path to gluster disk vol/img */ + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + port = strchr(host, ':'); + if (port) { + /* port was specified with host, separate both */ + *port++ = '\0'; + } + } + + /* host points to hostname / IPv4 / IPv6 addr + * port points to port or NULL is port was not specified + */ + } else { + /* transport type is unix, expect one more slash + * followed by path to gluster disk vol/img */ + if (STRPREFIX(marker, "/")) { + marker++; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Gluster unix transport url starts with 3 slashes i.e. gluster+unix:///")); + return -1; + } + } + + /* marker now points to path to gluster disk vol/img */ + volimg = marker; + + /* if transport type = unix, path to gluster disk vol/img + * is followed by ?socket=<path/to/socket> */ + if (def->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(marker, "?socket=")) { + /* In libvirt xml, socket path is to be provided as + * <host name='/path/to/socket' port='0'> + */ + host = strchr(marker, '='); + *host++ = '\0'; + } + } + + if (VIR_ALLOC(def->hosts) < 0) { + virReportOOMError(); + return -1; + } + def->nhosts = 1; + def->hosts->name = host; + if (port) { + def->hosts->port = port; + } else { + def->hosts->port = strdup("0"); + } + if (!def->hosts->port) { + virReportOOMError(); + return -1; + } + def->src = strdup(volimg); + if (!def->src) { + virReportOOMError(); + return -1; + } + + return 0; +} + +static int +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + int ret = 0; + virBufferAddLit(opt, "file="); + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster accepts only one host")); + ret = -1; + } else { + virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) {
Wouldn't it be better to check for ':' and check for absence of "." (and vice versa in the else) so that if someone specified a.b.c:d or a:b:c:d:e.f we can throw error rite away, instead of qemu erroring out later in time ? Its a very primtive check but can help to catch user input error early enuf.
+ /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } This can be clubbed as part of else clause to the above if condn (if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that ways we avoid an un-necessary check of transport here. It also means that disk->src needs to be pulled inside the if & else clauses, which I feel should be ok.
On 10/04/2012 09:17 PM, Deepak C Shetty wrote:
On 10/04/2012 07:01 PM, Harsh Prateek Bora wrote:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..89d9b9f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1048,6 +1048,7 @@ <value>nbd</value> <value>rbd</value> <value>sheepdog</value> + <value>gluster</value> </choice> </attribute> <optional> @@ -1061,6 +1062,13 @@ <attribute name="port"> <ref name="unsignedInt"/> </attribute> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>unix</value> + <value>rdma</value> + </choice> + </attribute> 'transport' attribute is optional, so it should be placed inside <optional> </optional> ?
Not necessarily, <zeroOrMore> works for it. You may want to test the patch without specifying 'transport' attribute!
</element> </zeroOrMore> <empty/>
[...]
+ +static int +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + int ret = 0; + virBufferAddLit(opt, "file="); + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster accepts only one host")); + ret = -1; + } else { + virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { Wouldn't it be better to check for ':' and check for absence of "." (and vice versa in the else) so that if someone specified a.b.c:d or a:b:c:d:e.f we can throw error rite away, instead of qemu erroring out later in time ? Its a very primtive check but can help to catch user input error early enuf.
I chose to check for only ':' to decide if its a IPv6 addr because it doesnt make sense to be partial towards '.' What if someone specifies a host name like 12:12;12,12 or 23:23,23,23 ? A '.' in an IPv6 addr is as bad as any other invalid char.
+ /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } This can be clubbed as part of else clause to the above if condn (if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that ways we avoid an un-necessary check of transport here. It also means that disk->src needs to be pulled inside the if & else clauses, which I feel should be ok.
That was the only reason I kept it out of else. Isn't it better to avoid redundant code than replacing if with an else ? regards, Harsh
On 10/04/2012 01:09 PM, Harsh Bora wrote:
On 10/04/2012 09:17 PM, Deepak C Shetty wrote:
On 10/04/2012 07:01 PM, Harsh Prateek Bora wrote:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/>
+ <attribute name="transport"> + <choice> + <value>tcp</value> + <value>unix</value> + <value>rdma</value> + </choice> + </attribute> 'transport' attribute is optional, so it should be placed inside <optional> </optional> ?
Not necessarily, <zeroOrMore> works for it. You may want to test the patch without specifying 'transport' attribute!
Deepak is right - the <attribute name="transport"> block must be embedded in an <optional> block. <zeroOrMore> only applies to elements, but as written, your zeroOrMore says that it is okay to omit the overall <host> element, but that if you provide a <host> element, it MUST have a transport='...' attribute, and that is not correct. For back-compat reasons, ALL new attributes must be in an <optional> block since older clients of the XML did not provide the new attributes.
I chose to check for only ':' to decide if its a IPv6 addr because it doesnt make sense to be partial towards '.' What if someone specifies a host name like 12:12;12,12 or 23:23,23,23 ? A '.' in an IPv6 addr is as bad as any other invalid char.
'.' is valid in IPv6 addr. But yes, ':' is mandatory in IPv6, and forbidden in IPv4, so it makes a good distinguishing test between the two families. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/05/2012 01:13 AM, Eric Blake wrote:
On 10/04/2012 01:09 PM, Harsh Bora wrote:
On 10/04/2012 09:17 PM, Deepak C Shetty wrote:
On 10/04/2012 07:01 PM, Harsh Prateek Bora wrote:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/>
+ <attribute name="transport"> + <choice> + <value>tcp</value> + <value>unix</value> + <value>rdma</value> + </choice> + </attribute> 'transport' attribute is optional, so it should be placed inside <optional> </optional> ?
Not necessarily, <zeroOrMore> works for it. You may want to test the patch without specifying 'transport' attribute!
Deepak is right - the <attribute name="transport"> block must be embedded in an <optional> block. <zeroOrMore> only applies to elements, but as written, your zeroOrMore says that it is okay to omit the overall <host> element, but that if you provide a <host> element, it MUST have a transport='...' attribute, and that is not correct. For back-compat reasons, ALL new attributes must be in an <optional> block since older clients of the XML did not provide the new attributes.
Looking at the grammar, it appeared to me like what you said, however, when I tested the patch, zeroOrMore worked successfully for the transport attribute as well. However, I am willing to use <optional> block for consistency.
I chose to check for only ':' to decide if its a IPv6 addr because it doesnt make sense to be partial towards '.' What if someone specifies a host name like 12:12;12,12 or 23:23,23,23 ? A '.' in an IPv6 addr is as bad as any other invalid char.
'.' is valid in IPv6 addr. But yes, ':' is mandatory in IPv6, and forbidden in IPv4, so it makes a good distinguishing test between the two families.
So, are you suggesting to validate IPv4 only and that too based on the absence of ':' and presence of '.'? Does that really suffice to validate an IPv4 since any other special character is also an invalid separator for IPv4 ? regards, Harsh
On 10/04/2012 01:56 PM, Harsh Bora wrote:
Deepak is right - the <attribute name="transport"> block must be embedded in an <optional> block. <zeroOrMore> only applies to elements, but as written, your zeroOrMore says that it is okay to omit the overall <host> element, but that if you provide a <host> element, it MUST have a transport='...' attribute, and that is not correct. For back-compat reasons, ALL new attributes must be in an <optional> block since older clients of the XML did not provide the new attributes.
Looking at the grammar, it appeared to me like what you said, however, when I tested the patch, zeroOrMore worked successfully for the transport attribute as well. However, I am willing to use <optional> block for consistency.
Hmm, I guess I'd have to read up on the actual grammar of RNG notation to see if using <zeroOrMore> as an alternate spelling of <optional> for fixed-name attributes is kosher or just something that our particular RNG validation engine permits, but it's certainly not typical usage, when you realize that there will never be more than one of that attribute. At any rate, we've hashed this topic to death :)
I chose to check for only ':' to decide if its a IPv6 addr because it doesnt make sense to be partial towards '.' What if someone specifies a host name like 12:12;12,12 or 23:23,23,23 ? A '.' in an IPv6 addr is as bad as any other invalid char.
'.' is valid in IPv6 addr. But yes, ':' is mandatory in IPv6, and forbidden in IPv4, so it makes a good distinguishing test between the two families.
So, are you suggesting to validate IPv4 only and that too based on the absence of ':' and presence of '.'? Does that really suffice to validate an IPv4 since any other special character is also an invalid separator for IPv4 ?
Ultimately, you want to accept both types of IP addresses. I was just replying to your quote about '.' not being valid in IPv6 as being an incorrect statement; I don't really know what the best separator character is that you want to be using in this context, because I haven't been following the original conversation closely enough. You should also realize that hostnames cannot contain ':' or ';' (I'm not sure about ','), so your question about someone setting a hostname to '12:12;12,12' to confuse the parser is not worth worrying about. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/05/2012 02:07 AM, Eric Blake wrote:
On 10/04/2012 01:56 PM, Harsh Bora wrote:
Deepak is right - the <attribute name="transport"> block must be embedded in an <optional> block. <zeroOrMore> only applies to elements, but as written, your zeroOrMore says that it is okay to omit the overall <host> element, but that if you provide a <host> element, it MUST have a transport='...' attribute, and that is not correct. For back-compat reasons, ALL new attributes must be in an <optional> block since older clients of the XML did not provide the new attributes.
Looking at the grammar, it appeared to me like what you said, however, when I tested the patch, zeroOrMore worked successfully for the transport attribute as well. However, I am willing to use <optional> block for consistency.
Hmm, I guess I'd have to read up on the actual grammar of RNG notation to see if using <zeroOrMore> as an alternate spelling of <optional> for fixed-name attributes is kosher or just something that our particular RNG validation engine permits, but it's certainly not typical usage, when you realize that there will never be more than one of that attribute. At any rate, we've hashed this topic to death :)
Hm, I think its better to have <optional> even if the xml works otherwise, since this is how one would expect the grammar to be written for an optional attribute.
I chose to check for only ':' to decide if its a IPv6 addr because it doesnt make sense to be partial towards '.' What if someone specifies a host name like 12:12;12,12 or 23:23,23,23 ? A '.' in an IPv6 addr is as bad as any other invalid char.
'.' is valid in IPv6 addr. But yes, ':' is mandatory in IPv6, and forbidden in IPv4, so it makes a good distinguishing test between the two families.
So, are you suggesting to validate IPv4 only and that too based on the absence of ':' and presence of '.'? Does that really suffice to validate an IPv4 since any other special character is also an invalid separator for IPv4 ?
Ultimately, you want to accept both types of IP addresses. I was just replying to your quote about '.' not being valid in IPv6 as being an incorrect statement; I don't really know what the best separator character is that you want to be using in this context, because I haven't been following the original conversation closely enough. You should also realize that hostnames cannot contain ':' or ';' (I'm not sure about ','), so your question about someone setting a hostname to '12:12;12,12' to confuse the parser is not worth worrying about.
Ok, I understand that IPv6 can have a '.' though ':' is mandatory in IPv6, and the current code should work for IPv6 addresses with a . as well. However, I am not sure if we need to validate the user provided hostname/IPv4/IPv6 addresses if it contains an invalid separator. Also, the parsing code for IPv4 should work for hostnames as well. By talking about IP address with diff separators, I just meant to say that presence of . and absence of : doesnt necessarily mean a valid IPv4 address since presence of other invalid separator would also mean an invalid IPv4 address. regards, Harsh
I chose to check for only ':' to decide if its a IPv6 addr because it doesnt make sense to be partial towards '.' What if someone specifies a host name like 12:12;12,12 or 23:23,23,23 ? A '.' in an IPv6 addr is as bad as any other invalid char.
'.' is valid in IPv6 addr. But yes, ':' is mandatory in IPv6, and forbidden in IPv4, so it makes a good distinguishing test between the two families.
So, are you suggesting to validate IPv4 only and that too based on the absence of ':' and presence of '.'? Does that really suffice to validate an IPv4 since any other special character is also an invalid separator for IPv4 ?
Ultimately, you want to accept both types of IP addresses. I was just replying to your quote about '.' not being valid in IPv6 as being an incorrect statement; I don't really know what the best separator character is that you want to be using in this context, because I haven't been following the original conversation closely enough. You should also realize that hostnames cannot contain ':' or ';' (I'm not sure about ','), so your question about someone setting a hostname to '12:12;12,12' to confuse the parser is not worth worrying about.
Ok, I understand that IPv6 can have a '.' though ':' is mandatory in IPv6, and the current code should work for IPv6 addresses with a . as well. However, I am not sure if we need to validate the user provided hostname/IPv4/IPv6 addresses if it contains an invalid separator.
Also, the parsing code for IPv4 should work for hostnames as well. By talking about IP address with diff separators, I just meant to say that presence of . and absence of : doesnt necessarily mean a valid IPv4 address since presence of other invalid separator would also mean an invalid IPv4 address.
I didn't knew '.' is allowed in IPv6, if thats the case, then i take back my suggestion here :)
+ /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } This can be clubbed as part of else clause to the above if condn (if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that ways we avoid an un-necessary check of transport here. It also means that disk->src needs to be pulled inside the if & else clauses, which I feel should be ok.
That was the only reason I kept it out of else. Isn't it better to avoid redundant code than replacing if with an else ?
I think it can be coded to look non-redundant too.. you just have to use the disk->src inside the virBufferAsprintf, like you are using disk->hosts->name. I saw Dan's comment abt using virURIPtr, so maybe things change with that.
On Thu, Oct 04, 2012 at 09:17:08PM +0530, Deepak C Shetty wrote:
+ virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { Wouldn't it be better to check for ':' and check for absence of "." (and vice versa in the else) so that if someone specified a.b.c:d or a:b:c:d:e.f we can throw error rite away, instead of qemu erroring out later in time ? Its a very primtive check but can help to catch user input error early enuf.
+ /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } This can be clubbed as part of else clause to the above if condn (if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that ways we avoid an un-necessary check of transport here. It also means that disk->src needs to be pulled inside the if & else clauses, which I feel should be ok.
Since this code is building up a URI, it really should be re-written to use the virURIPtr object, instead of virBufferPtr. This will ensure that things like [] for IPv6 are handled automatically, as well as doing the correct escaping of parameter values. This code is quite possibly exploitable as it stands if the user provided cleverly crafted values for disks->hosts->name which abused URI syntax Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/05/2012 03:13 AM, Daniel P. Berrange wrote:
On Thu, Oct 04, 2012 at 09:17:08PM +0530, Deepak C Shetty wrote:
+ virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { Wouldn't it be better to check for ':' and check for absence of "." (and vice versa in the else) so that if someone specified a.b.c:d or a:b:c:d:e.f we can throw error rite away, instead of qemu erroring out later in time ? Its a very primtive check but can help to catch user input error early enuf.
+ /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } This can be clubbed as part of else clause to the above if condn (if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that ways we avoid an un-necessary check of transport here. It also means that disk->src needs to be pulled inside the if & else clauses, which I feel should be ok.
Since this code is building up a URI, it really should be re-written to use the virURIPtr object, instead of virBufferPtr. This will ensure that things like [] for IPv6 are handled automatically, as well as doing the correct escaping of parameter values. This code is quite possibly exploitable as it stands if the user provided cleverly crafted values for disks->hosts->name which abused URI syntax
Hm, I was thinking of moving to URI infra as a separate patch later, however looks like that is the way to go. I will look into that. regards, Harsh
Daniel
On 10/05/2012 03:13 AM, Daniel P. Berrange wrote:
On Thu, Oct 04, 2012 at 09:17:08PM +0530, Deepak C Shetty wrote:
+ virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { Wouldn't it be better to check for ':' and check for absence of "." (and vice versa in the else) so that if someone specified a.b.c:d or a:b:c:d:e.f we can throw error rite away, instead of qemu erroring out later in time ? Its a very primtive check but can help to catch user input error early enuf.
+ /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } This can be clubbed as part of else clause to the above if condn (if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that ways we avoid an un-necessary check of transport here. It also means that disk->src needs to be pulled inside the if & else clauses, which I feel should be ok.
Since this code is building up a URI, it really should be re-written to use the virURIPtr object, instead of virBufferPtr. This will ensure that things like [] for IPv6 are handled automatically, as well as doing the correct escaping of parameter values. This code is quite possibly exploitable as it stands if the user provided cleverly crafted values for disks->hosts->name which abused URI syntax
Hi Daniel, I had a look at src/util/viruri.c and found that we have virURIParse() which takes a const char* and returns a virURIPtr object, likewise we have virURIFormat() which take a virURIPtr and returns a char*. Are you suggesting to manually populate a virURI struct and pass its address to virURIFormat() to build the cmdline from libvirt XML or are you suggesting to first build a cmdline (virBuffer) from the XML and pass it to virURIParse to validate if the libvirt XML has been provided a valid URI ? Also, IIUC, the usage of virURIPtr is going to be limited inside this function only, since the calling code expects the passed virBuffer to be populated with the required command line, right? regards, Harsh
Daniel
On Mon, Oct 22, 2012 at 05:05:19PM +0530, Harsh Bora wrote:
On 10/05/2012 03:13 AM, Daniel P. Berrange wrote:
On Thu, Oct 04, 2012 at 09:17:08PM +0530, Deepak C Shetty wrote:
+ virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { Wouldn't it be better to check for ':' and check for absence of "." (and vice versa in the else) so that if someone specified a.b.c:d or a:b:c:d:e.f we can throw error rite away, instead of qemu erroring out later in time ? Its a very primtive check but can help to catch user input error early enuf.
+ /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } This can be clubbed as part of else clause to the above if condn (if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that ways we avoid an un-necessary check of transport here. It also means that disk->src needs to be pulled inside the if & else clauses, which I feel should be ok.
Since this code is building up a URI, it really should be re-written to use the virURIPtr object, instead of virBufferPtr. This will ensure that things like [] for IPv6 are handled automatically, as well as doing the correct escaping of parameter values. This code is quite possibly exploitable as it stands if the user provided cleverly crafted values for disks->hosts->name which abused URI syntax
Hi Daniel,
I had a look at src/util/viruri.c and found that we have virURIParse() which takes a const char* and returns a virURIPtr object, likewise we have virURIFormat() which take a virURIPtr and returns a char*.
Are you suggesting to manually populate a virURI struct and pass its address to virURIFormat() to build the cmdline from libvirt XML or are you suggesting to first build a cmdline (virBuffer) from the XML and pass it to virURIParse to validate if the libvirt XML has been provided a valid URI ?
I mean to populate a virURIPtr object and then call virURIFormat to turn it into a string, thus guaranteeing that escaping is properly done. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Il 04/10/2012 15:31, Harsh Prateek Bora ha scritto:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
I would rather add a new attribute "socket" than overload the host name. The host name for Unix sockets is really localhost. Paolo
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..89d9b9f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1048,6 +1048,7 @@ <value>nbd</value> <value>rbd</value> <value>sheepdog</value> + <value>gluster</value> </choice> </attribute> <optional> @@ -1061,6 +1062,13 @@ <attribute name="port"> <ref name="unsignedInt"/> </attribute> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>unix</value> + <value>rdma</value> + </choice> + </attribute> </element> </zeroOrMore> <empty/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..838f079 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -214,7 +214,13 @@ VIR_ENUM_IMPL(virDomainDiskErrorPolicy, VIR_DOMAIN_DISK_ERROR_POLICY_LAST, VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST, "nbd", "rbd", - "sheepdog") + "sheepdog", + "gluster") + +VIR_ENUM_IMPL(virDomainDiskProtocolTransport, VIR_DOMAIN_DISK_PROTO_TRANS_LAST, + "tcp", + "unix", + "rdma")
VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST, "none", @@ -3460,6 +3466,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *source = NULL; char *target = NULL; char *protocol = NULL; + char *protocol_transport; char *trans = NULL; virDomainDiskHostDefPtr hosts = NULL; int nhosts = 0; @@ -3566,6 +3573,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, } hosts[nhosts].name = NULL; hosts[nhosts].port = NULL; + hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; nhosts++;
hosts[nhosts - 1].name = virXMLPropString(child, "name"); @@ -3580,6 +3588,17 @@ virDomainDiskDefParseXML(virCapsPtr caps, "%s", _("missing port for host")); goto error; } + /* transport can be tcp (default), unix or rdma. */ + protocol_transport = virXMLPropString(child, "transport"); + if (protocol_transport != NULL) { + hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(protocol_transport); + if (hosts[nhosts - 1].transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown protocol transport type '%s'"), + protocol_transport); + goto error; + } + } } child = child->next; } @@ -11756,8 +11775,13 @@ virDomainDiskDefFormat(virBufferPtr buf, for (i = 0; i < def->nhosts; i++) { virBufferEscapeString(buf, " <host name='%s'", def->hosts[i].name); - virBufferEscapeString(buf, " port='%s'/>\n", + virBufferEscapeString(buf, " port='%s'", def->hosts[i].port); + if (def->hosts[i].transport) { + virBufferAsprintf(buf, " transport='%s'", + virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport)); + } + virBufferAddLit(buf, "/>\n"); } virBufferAddLit(buf, " </source>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..7ba7e9b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -456,10 +456,19 @@ enum virDomainDiskProtocol { VIR_DOMAIN_DISK_PROTOCOL_NBD, VIR_DOMAIN_DISK_PROTOCOL_RBD, VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG, + VIR_DOMAIN_DISK_PROTOCOL_GLUSTER,
VIR_DOMAIN_DISK_PROTOCOL_LAST };
+enum virDomainDiskProtocolTransport { + VIR_DOMAIN_DISK_PROTO_TRANS_TCP, + VIR_DOMAIN_DISK_PROTO_TRANS_UNIX, + VIR_DOMAIN_DISK_PROTO_TRANS_RDMA, + + VIR_DOMAIN_DISK_PROTO_TRANS_LAST +}; + enum virDomainDiskTray { VIR_DOMAIN_DISK_TRAY_CLOSED, VIR_DOMAIN_DISK_TRAY_OPEN, @@ -481,6 +490,7 @@ typedef virDomainDiskHostDef *virDomainDiskHostDefPtr; struct _virDomainDiskHostDef { char *name; char *port; + int transport; /* enum virDomainDiskProtocolTransport */ };
enum virDomainDiskIo { @@ -2166,6 +2176,7 @@ VIR_ENUM_DECL(virDomainDiskBus) VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) +VIR_ENUM_DECL(virDomainDiskProtocolTransport) VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDiskTray) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dab607a..8e70837 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -350,6 +350,8 @@ virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskPathByName; +virDomainDiskProtocolTransportTypeFromString; +virDomainDiskProtocolTransportTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 20730a9..0bed2bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2021,6 +2021,168 @@ no_memory: return -1; }
+static int qemuParseGlusterString(virDomainDiskDefPtr def) +{ + char *host = NULL, *port = NULL, *volimg, *transp = NULL, *marker; + + if (STRPREFIX(def->src, "+")) { + transp = def->src; + transp++; + marker = strstr(def->src, "://"); + *marker++ = '\0'; + marker += 2; + } else { + /* transport type not specified, use tcp as default */ + def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + marker = strstr(def->src, "://"); + marker += 3; + } + + if (transp) { + def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp); + if (def->hosts->transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid gluster transport type '%s'"), transp); + return -1; + } + } + + /* now marker points to string which can start with one of the following: + * IPv6 address in square brackets followed by port (optional) + * <hostname> or <IPv4 address> followed by port (optional) + * '/' if its a unix socket followed by path to gluster disk volume/image + */ + + /* parse server, port */ + if (def->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (STRPREFIX(marker, "[")) { + /* IPv6 addr */ + host = ++marker; + marker = strchr(host, ']'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse IPv6 addr for gluster host %s "), host); + return -1; + } + *marker++ = '\0'; + /* parse port if specified */ + if (STRPREFIX(marker, ":")) { + port = ++marker; + marker = strchr(port, '/'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + } else { + marker++; /* port not specified, skip slash */ + } + } else { + /* IPv4 address / hostname followed by port (optional) */ + host = marker; + marker = strchr(host, '/'); /* skip to path to gluster disk vol/img */ + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + port = strchr(host, ':'); + if (port) { + /* port was specified with host, separate both */ + *port++ = '\0'; + } + } + + /* host points to hostname / IPv4 / IPv6 addr + * port points to port or NULL is port was not specified + */ + } else { + /* transport type is unix, expect one more slash + * followed by path to gluster disk vol/img */ + if (STRPREFIX(marker, "/")) { + marker++; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Gluster unix transport url starts with 3 slashes i.e. gluster+unix:///")); + return -1; + } + } + + /* marker now points to path to gluster disk vol/img */ + volimg = marker; + + /* if transport type = unix, path to gluster disk vol/img + * is followed by ?socket=<path/to/socket> */ + if (def->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(marker, "?socket=")) { + /* In libvirt xml, socket path is to be provided as + * <host name='/path/to/socket' port='0'> + */ + host = strchr(marker, '='); + *host++ = '\0'; + } + } + + if (VIR_ALLOC(def->hosts) < 0) { + virReportOOMError(); + return -1; + } + def->nhosts = 1; + def->hosts->name = host; + if (port) { + def->hosts->port = port; + } else { + def->hosts->port = strdup("0"); + } + if (!def->hosts->port) { + virReportOOMError(); + return -1; + } + def->src = strdup(volimg); + if (!def->src) { + virReportOOMError(); + return -1; + } + + return 0; +} + +static int +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + int ret = 0; + virBufferAddLit(opt, "file="); + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster accepts only one host")); + ret = -1; + } else { + virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { + /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } + } + return ret; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2162,6 +2324,12 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; virBufferAddChar(&opt, ','); break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (qemuBuildGlusterString(disk, &opt) < 0) + goto error; + virBufferAddChar(&opt, ','); + break; + case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { virBufferEscape(&opt, ',', ",", "file=sheepdog:%s,", @@ -5242,6 +5410,18 @@ qemuBuildCommandLine(virConnectPtr conn, file = virBufferContentAndReset(&opt); } break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + { + virBuffer opt = VIR_BUFFER_INITIALIZER; + if (qemuBuildGlusterString(disk, &opt) < 0) + goto error; + if (virBufferError(&opt)) { + virReportOOMError(); + goto error; + } + file = virBufferContentAndReset(&opt); + } + break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { if (virAsprintf(&file, "sheepdog:%s,", disk->src) < 0) { @@ -6937,6 +7117,21 @@ qemuParseCommandLineDisk(virCapsPtr caps, goto cleanup;
VIR_FREE(p); + } else if (STRPREFIX(def->src, "gluster")) { + char *p = def->src; + + def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; + def->src = strdup(p + strlen("gluster")); + if (!def->src) { + virReportOOMError(); + goto cleanup; + } + + if (qemuParseGlusterString(def) < 0) + goto cleanup; + + VIR_FREE(p); } else if (STRPREFIX(def->src, "sheepdog:")) { char *p = def->src; char *port, *vdi; @@ -8126,6 +8321,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; val += strlen("rbd:"); + } else if (STRPREFIX(val, "gluster")) { + disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; + val += strlen("gluster"); } else if (STRPREFIX(val, "sheepdog:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG; @@ -8211,6 +8410,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (qemuParseGlusterString(disk) < 0) + goto error; + + break; } }
On 10/05/2012 05:27 PM, Paolo Bonzini wrote:
Il 04/10/2012 15:31, Harsh Prateek Bora ha scritto:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
I would rather add a new attribute "socket" than overload the host name. The host name for Unix sockets is really localhost.
Yeh, that makes perfect sense. ~ Harsh
Paolo
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..89d9b9f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1048,6 +1048,7 @@ <value>nbd</value> <value>rbd</value> <value>sheepdog</value> + <value>gluster</value> </choice> </attribute> <optional> @@ -1061,6 +1062,13 @@ <attribute name="port"> <ref name="unsignedInt"/> </attribute> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>unix</value> + <value>rdma</value> + </choice> + </attribute> </element> </zeroOrMore> <empty/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..838f079 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -214,7 +214,13 @@ VIR_ENUM_IMPL(virDomainDiskErrorPolicy, VIR_DOMAIN_DISK_ERROR_POLICY_LAST, VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST, "nbd", "rbd", - "sheepdog") + "sheepdog", + "gluster") + +VIR_ENUM_IMPL(virDomainDiskProtocolTransport, VIR_DOMAIN_DISK_PROTO_TRANS_LAST, + "tcp", + "unix", + "rdma")
VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST, "none", @@ -3460,6 +3466,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *source = NULL; char *target = NULL; char *protocol = NULL; + char *protocol_transport; char *trans = NULL; virDomainDiskHostDefPtr hosts = NULL; int nhosts = 0; @@ -3566,6 +3573,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, } hosts[nhosts].name = NULL; hosts[nhosts].port = NULL; + hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; nhosts++;
hosts[nhosts - 1].name = virXMLPropString(child, "name"); @@ -3580,6 +3588,17 @@ virDomainDiskDefParseXML(virCapsPtr caps, "%s", _("missing port for host")); goto error; } + /* transport can be tcp (default), unix or rdma. */ + protocol_transport = virXMLPropString(child, "transport"); + if (protocol_transport != NULL) { + hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(protocol_transport); + if (hosts[nhosts - 1].transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown protocol transport type '%s'"), + protocol_transport); + goto error; + } + } } child = child->next; } @@ -11756,8 +11775,13 @@ virDomainDiskDefFormat(virBufferPtr buf, for (i = 0; i < def->nhosts; i++) { virBufferEscapeString(buf, " <host name='%s'", def->hosts[i].name); - virBufferEscapeString(buf, " port='%s'/>\n", + virBufferEscapeString(buf, " port='%s'", def->hosts[i].port); + if (def->hosts[i].transport) { + virBufferAsprintf(buf, " transport='%s'", + virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport)); + } + virBufferAddLit(buf, "/>\n"); } virBufferAddLit(buf, " </source>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..7ba7e9b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -456,10 +456,19 @@ enum virDomainDiskProtocol { VIR_DOMAIN_DISK_PROTOCOL_NBD, VIR_DOMAIN_DISK_PROTOCOL_RBD, VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG, + VIR_DOMAIN_DISK_PROTOCOL_GLUSTER,
VIR_DOMAIN_DISK_PROTOCOL_LAST };
+enum virDomainDiskProtocolTransport { + VIR_DOMAIN_DISK_PROTO_TRANS_TCP, + VIR_DOMAIN_DISK_PROTO_TRANS_UNIX, + VIR_DOMAIN_DISK_PROTO_TRANS_RDMA, + + VIR_DOMAIN_DISK_PROTO_TRANS_LAST +}; + enum virDomainDiskTray { VIR_DOMAIN_DISK_TRAY_CLOSED, VIR_DOMAIN_DISK_TRAY_OPEN, @@ -481,6 +490,7 @@ typedef virDomainDiskHostDef *virDomainDiskHostDefPtr; struct _virDomainDiskHostDef { char *name; char *port; + int transport; /* enum virDomainDiskProtocolTransport */ };
enum virDomainDiskIo { @@ -2166,6 +2176,7 @@ VIR_ENUM_DECL(virDomainDiskBus) VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) +VIR_ENUM_DECL(virDomainDiskProtocolTransport) VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDiskTray) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dab607a..8e70837 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -350,6 +350,8 @@ virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskPathByName; +virDomainDiskProtocolTransportTypeFromString; +virDomainDiskProtocolTransportTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 20730a9..0bed2bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2021,6 +2021,168 @@ no_memory: return -1; }
+static int qemuParseGlusterString(virDomainDiskDefPtr def) +{ + char *host = NULL, *port = NULL, *volimg, *transp = NULL, *marker; + + if (STRPREFIX(def->src, "+")) { + transp = def->src; + transp++; + marker = strstr(def->src, "://"); + *marker++ = '\0'; + marker += 2; + } else { + /* transport type not specified, use tcp as default */ + def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + marker = strstr(def->src, "://"); + marker += 3; + } + + if (transp) { + def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp); + if (def->hosts->transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid gluster transport type '%s'"), transp); + return -1; + } + } + + /* now marker points to string which can start with one of the following: + * IPv6 address in square brackets followed by port (optional) + * <hostname> or <IPv4 address> followed by port (optional) + * '/' if its a unix socket followed by path to gluster disk volume/image + */ + + /* parse server, port */ + if (def->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (STRPREFIX(marker, "[")) { + /* IPv6 addr */ + host = ++marker; + marker = strchr(host, ']'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse IPv6 addr for gluster host %s "), host); + return -1; + } + *marker++ = '\0'; + /* parse port if specified */ + if (STRPREFIX(marker, ":")) { + port = ++marker; + marker = strchr(port, '/'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + } else { + marker++; /* port not specified, skip slash */ + } + } else { + /* IPv4 address / hostname followed by port (optional) */ + host = marker; + marker = strchr(host, '/'); /* skip to path to gluster disk vol/img */ + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + port = strchr(host, ':'); + if (port) { + /* port was specified with host, separate both */ + *port++ = '\0'; + } + } + + /* host points to hostname / IPv4 / IPv6 addr + * port points to port or NULL is port was not specified + */ + } else { + /* transport type is unix, expect one more slash + * followed by path to gluster disk vol/img */ + if (STRPREFIX(marker, "/")) { + marker++; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Gluster unix transport url starts with 3 slashes i.e. gluster+unix:///")); + return -1; + } + } + + /* marker now points to path to gluster disk vol/img */ + volimg = marker; + + /* if transport type = unix, path to gluster disk vol/img + * is followed by ?socket=<path/to/socket> */ + if (def->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(marker, "?socket=")) { + /* In libvirt xml, socket path is to be provided as + * <host name='/path/to/socket' port='0'> + */ + host = strchr(marker, '='); + *host++ = '\0'; + } + } + + if (VIR_ALLOC(def->hosts) < 0) { + virReportOOMError(); + return -1; + } + def->nhosts = 1; + def->hosts->name = host; + if (port) { + def->hosts->port = port; + } else { + def->hosts->port = strdup("0"); + } + if (!def->hosts->port) { + virReportOOMError(); + return -1; + } + def->src = strdup(volimg); + if (!def->src) { + virReportOOMError(); + return -1; + } + + return 0; +} + +static int +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + int ret = 0; + virBufferAddLit(opt, "file="); + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster accepts only one host")); + ret = -1; + } else { + virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { + /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } + } + return ret; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2162,6 +2324,12 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; virBufferAddChar(&opt, ','); break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (qemuBuildGlusterString(disk, &opt) < 0) + goto error; + virBufferAddChar(&opt, ','); + break; + case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { virBufferEscape(&opt, ',', ",", "file=sheepdog:%s,", @@ -5242,6 +5410,18 @@ qemuBuildCommandLine(virConnectPtr conn, file = virBufferContentAndReset(&opt); } break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + { + virBuffer opt = VIR_BUFFER_INITIALIZER; + if (qemuBuildGlusterString(disk, &opt) < 0) + goto error; + if (virBufferError(&opt)) { + virReportOOMError(); + goto error; + } + file = virBufferContentAndReset(&opt); + } + break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { if (virAsprintf(&file, "sheepdog:%s,", disk->src) < 0) { @@ -6937,6 +7117,21 @@ qemuParseCommandLineDisk(virCapsPtr caps, goto cleanup;
VIR_FREE(p); + } else if (STRPREFIX(def->src, "gluster")) { + char *p = def->src; + + def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; + def->src = strdup(p + strlen("gluster")); + if (!def->src) { + virReportOOMError(); + goto cleanup; + } + + if (qemuParseGlusterString(def) < 0) + goto cleanup; + + VIR_FREE(p); } else if (STRPREFIX(def->src, "sheepdog:")) { char *p = def->src; char *port, *vdi; @@ -8126,6 +8321,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; val += strlen("rbd:"); + } else if (STRPREFIX(val, "gluster")) { + disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; + val += strlen("gluster"); } else if (STRPREFIX(val, "sheepdog:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG; @@ -8211,6 +8410,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (qemuParseGlusterString(disk) < 0) + goto error; + + break; } }
On 10/05/2012 05:27 PM, Paolo Bonzini wrote:
Il 04/10/2012 15:31, Harsh Prateek Bora ha scritto:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
I would rather add a new attribute "socket" than overload the host name. The host name for Unix sockets is really localhost.
After looking into the URI infra, I realized, it is better to have the new attribute name as 'query' since socket=</path/to/socket> in qemu-gluster commandline is nothing but a query in the whole URI following by a '?' character. Since, the _virURI struct in libvirt also has a member 'query' for the query string in a URI, it makes sense to keep the XML attribute as generic as that so that it can be used by others for related purpose. Needless to say, users will have to specify sockets as <host name='example.org' port='6000' transport='unix' query='socket=/path/to/sock' /> Also, since the libvirt mandates the host element to be provided a name and port, but the qemu-gluster commandline doesnt allow server-port for the unix transport, it will have to be handled differently, otherwise the virURIFormat will add in the server, port info into the URI. If no objections, I will shared the updated patches soon. Harsh
Paolo
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 11 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..89d9b9f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1048,6 +1048,7 @@ <value>nbd</value> <value>rbd</value> <value>sheepdog</value> + <value>gluster</value> </choice> </attribute> <optional> @@ -1061,6 +1062,13 @@ <attribute name="port"> <ref name="unsignedInt"/> </attribute> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>unix</value> + <value>rdma</value> + </choice> + </attribute> </element> </zeroOrMore> <empty/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..838f079 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -214,7 +214,13 @@ VIR_ENUM_IMPL(virDomainDiskErrorPolicy, VIR_DOMAIN_DISK_ERROR_POLICY_LAST, VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST, "nbd", "rbd", - "sheepdog") + "sheepdog", + "gluster") + +VIR_ENUM_IMPL(virDomainDiskProtocolTransport, VIR_DOMAIN_DISK_PROTO_TRANS_LAST, + "tcp", + "unix", + "rdma")
VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST, "none", @@ -3460,6 +3466,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *source = NULL; char *target = NULL; char *protocol = NULL; + char *protocol_transport; char *trans = NULL; virDomainDiskHostDefPtr hosts = NULL; int nhosts = 0; @@ -3566,6 +3573,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, } hosts[nhosts].name = NULL; hosts[nhosts].port = NULL; + hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; nhosts++;
hosts[nhosts - 1].name = virXMLPropString(child, "name"); @@ -3580,6 +3588,17 @@ virDomainDiskDefParseXML(virCapsPtr caps, "%s", _("missing port for host")); goto error; } + /* transport can be tcp (default), unix or rdma. */ + protocol_transport = virXMLPropString(child, "transport"); + if (protocol_transport != NULL) { + hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(protocol_transport); + if (hosts[nhosts - 1].transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown protocol transport type '%s'"), + protocol_transport); + goto error; + } + } } child = child->next; } @@ -11756,8 +11775,13 @@ virDomainDiskDefFormat(virBufferPtr buf, for (i = 0; i < def->nhosts; i++) { virBufferEscapeString(buf, " <host name='%s'", def->hosts[i].name); - virBufferEscapeString(buf, " port='%s'/>\n", + virBufferEscapeString(buf, " port='%s'", def->hosts[i].port); + if (def->hosts[i].transport) { + virBufferAsprintf(buf, " transport='%s'", + virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport)); + } + virBufferAddLit(buf, "/>\n"); } virBufferAddLit(buf, " </source>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..7ba7e9b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -456,10 +456,19 @@ enum virDomainDiskProtocol { VIR_DOMAIN_DISK_PROTOCOL_NBD, VIR_DOMAIN_DISK_PROTOCOL_RBD, VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG, + VIR_DOMAIN_DISK_PROTOCOL_GLUSTER,
VIR_DOMAIN_DISK_PROTOCOL_LAST };
+enum virDomainDiskProtocolTransport { + VIR_DOMAIN_DISK_PROTO_TRANS_TCP, + VIR_DOMAIN_DISK_PROTO_TRANS_UNIX, + VIR_DOMAIN_DISK_PROTO_TRANS_RDMA, + + VIR_DOMAIN_DISK_PROTO_TRANS_LAST +}; + enum virDomainDiskTray { VIR_DOMAIN_DISK_TRAY_CLOSED, VIR_DOMAIN_DISK_TRAY_OPEN, @@ -481,6 +490,7 @@ typedef virDomainDiskHostDef *virDomainDiskHostDefPtr; struct _virDomainDiskHostDef { char *name; char *port; + int transport; /* enum virDomainDiskProtocolTransport */ };
enum virDomainDiskIo { @@ -2166,6 +2176,7 @@ VIR_ENUM_DECL(virDomainDiskBus) VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) +VIR_ENUM_DECL(virDomainDiskProtocolTransport) VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDiskTray) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dab607a..8e70837 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -350,6 +350,8 @@ virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskPathByName; +virDomainDiskProtocolTransportTypeFromString; +virDomainDiskProtocolTransportTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 20730a9..0bed2bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2021,6 +2021,168 @@ no_memory: return -1; }
+static int qemuParseGlusterString(virDomainDiskDefPtr def) +{ + char *host = NULL, *port = NULL, *volimg, *transp = NULL, *marker; + + if (STRPREFIX(def->src, "+")) { + transp = def->src; + transp++; + marker = strstr(def->src, "://"); + *marker++ = '\0'; + marker += 2; + } else { + /* transport type not specified, use tcp as default */ + def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + marker = strstr(def->src, "://"); + marker += 3; + } + + if (transp) { + def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp); + if (def->hosts->transport < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid gluster transport type '%s'"), transp); + return -1; + } + } + + /* now marker points to string which can start with one of the following: + * IPv6 address in square brackets followed by port (optional) + * <hostname> or <IPv4 address> followed by port (optional) + * '/' if its a unix socket followed by path to gluster disk volume/image + */ + + /* parse server, port */ + if (def->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (STRPREFIX(marker, "[")) { + /* IPv6 addr */ + host = ++marker; + marker = strchr(host, ']'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse IPv6 addr for gluster host %s "), host); + return -1; + } + *marker++ = '\0'; + /* parse port if specified */ + if (STRPREFIX(marker, ":")) { + port = ++marker; + marker = strchr(port, '/'); + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + } else { + marker++; /* port not specified, skip slash */ + } + } else { + /* IPv4 address / hostname followed by port (optional) */ + host = marker; + marker = strchr(host, '/'); /* skip to path to gluster disk vol/img */ + if (!marker) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse filename for gluster disk %s "), port); + return -1; + } + *marker++ = '\0'; + port = strchr(host, ':'); + if (port) { + /* port was specified with host, separate both */ + *port++ = '\0'; + } + } + + /* host points to hostname / IPv4 / IPv6 addr + * port points to port or NULL is port was not specified + */ + } else { + /* transport type is unix, expect one more slash + * followed by path to gluster disk vol/img */ + if (STRPREFIX(marker, "/")) { + marker++; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Gluster unix transport url starts with 3 slashes i.e. gluster+unix:///")); + return -1; + } + } + + /* marker now points to path to gluster disk vol/img */ + volimg = marker; + + /* if transport type = unix, path to gluster disk vol/img + * is followed by ?socket=<path/to/socket> */ + if (def->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(marker, "?socket=")) { + /* In libvirt xml, socket path is to be provided as + * <host name='/path/to/socket' port='0'> + */ + host = strchr(marker, '='); + *host++ = '\0'; + } + } + + if (VIR_ALLOC(def->hosts) < 0) { + virReportOOMError(); + return -1; + } + def->nhosts = 1; + def->hosts->name = host; + if (port) { + def->hosts->port = port; + } else { + def->hosts->port = strdup("0"); + } + if (!def->hosts->port) { + virReportOOMError(); + return -1; + } + def->src = strdup(volimg); + if (!def->src) { + virReportOOMError(); + return -1; + } + + return 0; +} + +static int +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + int ret = 0; + virBufferAddLit(opt, "file="); + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("gluster accepts only one host")); + ret = -1; + } else { + virBufferAsprintf(opt, "gluster+%s://", + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)); + + /* if transport type is not unix, specify server:port */ + if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (strstr(disk->hosts->name, ":")) { + /* if IPv6 addr, use square brackets to enclose it */ + virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port); + } else { + virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port); + } + } + + /* append source path to gluster disk image */ + virBufferAsprintf(opt, "/%s", disk->src); + + /* if transport type is unix, server name is path to unix socket, ignore port */ + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + virBufferAsprintf(opt, "?socket=%s", disk->hosts->name); + } + } + return ret; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2162,6 +2324,12 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; virBufferAddChar(&opt, ','); break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (qemuBuildGlusterString(disk, &opt) < 0) + goto error; + virBufferAddChar(&opt, ','); + break; + case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { virBufferEscape(&opt, ',', ",", "file=sheepdog:%s,", @@ -5242,6 +5410,18 @@ qemuBuildCommandLine(virConnectPtr conn, file = virBufferContentAndReset(&opt); } break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + { + virBuffer opt = VIR_BUFFER_INITIALIZER; + if (qemuBuildGlusterString(disk, &opt) < 0) + goto error; + if (virBufferError(&opt)) { + virReportOOMError(); + goto error; + } + file = virBufferContentAndReset(&opt); + } + break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { if (virAsprintf(&file, "sheepdog:%s,", disk->src) < 0) { @@ -6937,6 +7117,21 @@ qemuParseCommandLineDisk(virCapsPtr caps, goto cleanup;
VIR_FREE(p); + } else if (STRPREFIX(def->src, "gluster")) { + char *p = def->src; + + def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; + def->src = strdup(p + strlen("gluster")); + if (!def->src) { + virReportOOMError(); + goto cleanup; + } + + if (qemuParseGlusterString(def) < 0) + goto cleanup; + + VIR_FREE(p); } else if (STRPREFIX(def->src, "sheepdog:")) { char *p = def->src; char *port, *vdi; @@ -8126,6 +8321,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; val += strlen("rbd:"); + } else if (STRPREFIX(val, "gluster")) { + disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; + val += strlen("gluster"); } else if (STRPREFIX(val, "sheepdog:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG; @@ -8211,6 +8410,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; } break; + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: + if (qemuParseGlusterString(disk) < 0) + goto error; + + break; } }
Il 26/10/2012 11:36, Harsh Bora ha scritto:
On 10/05/2012 05:27 PM, Paolo Bonzini wrote:
Il 04/10/2012 15:31, Harsh Prateek Bora ha scritto:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
I would rather add a new attribute "socket" than overload the host name. The host name for Unix sockets is really localhost.
After looking into the URI infra, I realized, it is better to have the new attribute name as 'query' since socket=</path/to/socket> in qemu-gluster commandline is nothing but a query in the whole URI following by a '?' character.
Since, the _virURI struct in libvirt also has a member 'query' for the query string in a URI, it makes sense to keep the XML attribute as generic as that so that it can be used by others for related purpose.
No, each query parameter should map to a separate XML attribute or element.
Needless to say, users will have to specify sockets as <host name='example.org' port='6000' transport='unix' query='socket=/path/to/sock' />
Also, since the libvirt mandates the host element to be provided a name and port
Then change the schema. In RNG it would be like this: <zeroOrMore> <element name="host"> <choice> <group> <optional> <attribute name="transport"> <choice> <value>tcp</value> <value>rdma</value> </choice> </attribute> </optional> <attribute name="name"> <ref name="dnsName"/> </attribute> <attribute name="port"> <ref name="unsignedInt"/> </attribute> </group> <group> <attribute name="transport"> <value>unix</value> </attribute> <attribute name="socket"> <ref name="absFilePath"/> </attribute> </group> </choice> </element> </zeroOrMore>
, but the qemu-gluster commandline doesnt allow server-port for the unix transport, it will have to be handled differently, otherwise the virURIFormat will add in the server, port info into the URI.
An empty host and port will return gluster+unix:///volname?server=/path correctly after virURIFormat. Paolo
On Fri, Oct 26, 2012 at 15:06:19 +0530, Harsh Bora wrote:
On 10/05/2012 05:27 PM, Paolo Bonzini wrote:
Il 04/10/2012 15:31, Harsh Prateek Bora ha scritto:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
I would rather add a new attribute "socket" than overload the host name. The host name for Unix sockets is really localhost.
After looking into the URI infra, I realized, it is better to have the new attribute name as 'query' since socket=</path/to/socket> in qemu-gluster commandline is nothing but a query in the whole URI following by a '?' character.
Since, the _virURI struct in libvirt also has a member 'query' for the query string in a URI, it makes sense to keep the XML attribute as generic as that so that it can be used by others for related purpose.
Needless to say, users will have to specify sockets as <host name='example.org' port='6000' transport='unix' query='socket=/path/to/sock' />
Ah, this looks horrible. We really want to avoid specifying random garbage for hostname and port in case of unix transport. What Paolo is suggesting is to create <host name='example.org' port='6000' transport='tcp'/> or <host socket='/path/to/sock' transport='unix'/> However, I don't like this too much either. What if we add a general socket element? In other words, <host name='example.org' port='6000' transport='tcp|rdma'/> or <socket type='unix' path='/path/to/sock'/> where the type attribute in socket element would determine what other attributes can be used (path for unix sockets). Internally, both elements could be described by a unified socket structure. Jirka
Il 26/10/2012 15:38, Jiri Denemark ha scritto:
<host name='example.org' port='6000' transport='tcp'/> or <host socket='/path/to/sock' transport='unix'/>
However, I don't like this too much either. What if we add a general socket element? In other words,
<host name='example.org' port='6000' transport='tcp|rdma'/> or <socket type='unix' path='/path/to/sock'/>
where the type attribute in socket element would determine what other attributes can be used (path for unix sockets). Internally, both elements could be described by a unified socket structure.
The reason why I suggested reusing <host> is because you could in principle have socket as an attribute even for other transports. libvirt for example has it as a socket parameter for its ssh transport. What about moving the transport to source: <source protocol='gluster' name='Volume2/Image' transport='tcp'> <host name='example.org' port='6000'/> </source> <source protocol='gluster' name='Volume2/Image' transport='unix'> <socket path='/path/to/sock'/> </source> where hypothetically an ssh transport could have both host and socket sub-elements. Paolo
On Fri, Oct 26, 2012 at 15:49:01 +0200, Paolo Bonzini wrote:
Il 26/10/2012 15:38, Jiri Denemark ha scritto:
<host name='example.org' port='6000' transport='tcp'/> or <host socket='/path/to/sock' transport='unix'/>
However, I don't like this too much either. What if we add a general socket element? In other words,
<host name='example.org' port='6000' transport='tcp|rdma'/> or <socket type='unix' path='/path/to/sock'/>
where the type attribute in socket element would determine what other attributes can be used (path for unix sockets). Internally, both elements could be described by a unified socket structure.
The reason why I suggested reusing <host> is because you could in principle have socket as an attribute even for other transports. libvirt for example has it as a socket parameter for its ssh transport.
What about moving the transport to source:
<source protocol='gluster' name='Volume2/Image' transport='tcp'> <host name='example.org' port='6000'/> </source>
<source protocol='gluster' name='Volume2/Image' transport='unix'> <socket path='/path/to/sock'/> </source>
where hypothetically an ssh transport could have both host and socket sub-elements.
Hmm, I'm starting to think we are making it too complicated :-) Let's see what other think. Jirka
Il 26/10/2012 15:55, Jiri Denemark ha scritto:
What about moving the transport to source:
<source protocol='gluster' name='Volume2/Image' transport='tcp'> <host name='example.org' port='6000'/> </source>
<source protocol='gluster' name='Volume2/Image' transport='unix'> <socket path='/path/to/sock'/> </source>
where hypothetically an ssh transport could have both host and socket sub-elements. Hmm, I'm starting to think we are making it too complicated :-) Let's see what other think.
Oh, I'm fine with Harsh's v2. :) Just trying to propose something based on your observations. Paolo
Il 26/10/2012 16:01, Paolo Bonzini ha scritto:
> What about moving the transport to source: > > <source protocol='gluster' name='Volume2/Image' transport='tcp'> > <host name='example.org' port='6000'/> > </source> > > <source protocol='gluster' name='Volume2/Image' transport='unix'> > <socket path='/path/to/sock'/> > </source> > > where hypothetically an ssh transport could have both host and socket > sub-elements. Hmm, I'm starting to think we are making it too complicated :-) Let's see what other think. Oh, I'm fine with Harsh's v2. :) Just trying to propose something based on your observations.
I meant his v3. Paolo
On 10/26/2012 07:08 PM, Jiri Denemark wrote:
On Fri, Oct 26, 2012 at 15:06:19 +0530, Harsh Bora wrote:
On 10/05/2012 05:27 PM, Paolo Bonzini wrote:
Il 04/10/2012 15:31, Harsh Prateek Bora ha scritto:
Qemu accepts gluster protocol as supported storage backend beside others. This patch allows users to specify disks on gluster backends like this:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='gluster' name='volume/image'> <host name='example.org' port='6000' transport='tcp'/> </source> <target dev='vda' bus='virtio'/> </disk>
Note: In the <host> element above, transport is an optional attribute. Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed. If transport type is unix, host name specifies path to unix socket.
I would rather add a new attribute "socket" than overload the host name. The host name for Unix sockets is really localhost.
After looking into the URI infra, I realized, it is better to have the new attribute name as 'query' since socket=</path/to/socket> in qemu-gluster commandline is nothing but a query in the whole URI following by a '?' character.
Since, the _virURI struct in libvirt also has a member 'query' for the query string in a URI, it makes sense to keep the XML attribute as generic as that so that it can be used by others for related purpose.
Needless to say, users will have to specify sockets as <host name='example.org' port='6000' transport='unix' query='socket=/path/to/sock' />
Ah, this looks horrible. We really want to avoid specifying random garbage for hostname and port in case of unix transport. What Paolo is suggesting is to create
<host name='example.org' port='6000' transport='tcp'/> or <host socket='/path/to/sock' transport='unix'/>
However, I don't like this too much either. What if we add a general socket element? In other words,
<host name='example.org' port='6000' transport='tcp|rdma'/> or <socket type='unix' path='/path/to/sock'/>
where the type attribute in socket element would determine what other attributes can be used (path for unix sockets). Internally, both elements could be described by a unified socket structure.
IIUC, a separate element for socket makes more sense if there will be more than one socket utilized per connection (source/target pair), which I am not really sure about. Also, if there is a use case in future, we can always introduce a change in schema as we are doing in this patch. regards, Harsh
Jirka
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-gluster.args | 1 + .../qemuxml2argv-disk-drive-network-gluster.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 4 files changed, 37 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.xml diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 5e51d32..bcea8fe 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -183,6 +183,7 @@ mymain(void) DO_TEST("disk-drive-cache-directsync"); DO_TEST("disk-drive-cache-unsafe"); DO_TEST("disk-drive-network-nbd"); + DO_TEST("disk-drive-network-gluster"); DO_TEST("disk-drive-network-rbd"); /* older format using CEPH_ARGS env var */ DO_TEST("disk-drive-network-rbd-ceph-env"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args new file mode 100644 index 0000000..72d4ded --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -no-kqemu -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive file=gluster+tcp://example.org:6000/Volume/Image,if=virtio,format=raw -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.xml new file mode 100644 index 0000000..fe56166 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='gluster' name='Volume/Image'> + <host name='example.org' port='6000' transport='tcp'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0757e37..20ed7cb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -469,6 +469,8 @@ mymain(void) QEMU_CAPS_DRIVE_CACHE_UNSAFE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-gluster", false, + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-rbd", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-sheepdog", -- 1.7.11.4
participants (7)
-
Daniel P. Berrange -
Deepak C Shetty -
Eric Blake -
Harsh Bora -
Harsh Prateek Bora
-
Jiri Denemark -
Paolo Bonzini