On Thu, Aug 23, 2012 at 16:31:51 +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='socket'/>
> </source>
> <target dev='vda' bus='virtio'/>
> </disk>
>
> Note: In the <host> element above, transport is an optional attribute.
> Valid transport types for a network based disk can be socket, unix or rdma.
>
> TODO:
> - Add support for IPv6 format based server addr
> - Support for transport types other than socket.
Overall, this patch set looks fine. See my comments inline.
> Signed-off-by: Harsh Prateek Bora <harsh@xxxxxxxxxxxxxxxxxx>
> ---
> docs/schemas/domaincommon.rng | 8 +++
> src/conf/domain_conf.c | 14 ++++-
> src/conf/domain_conf.h | 3 +-
> src/qemu/qemu_command.c | 123 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 145 insertions(+), 3 deletions(-)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 145caf7..30c0d8c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1029,6 +1029,7 @@
> <value>nbd</value>
> <value>rbd</value>
> <value>sheepdog</value>
> + <value>gluster</value>
> </choice>
> </attribute>
> <optional>
> @@ -1042,6 +1043,13 @@
> <attribute name="port">
> <ref name="unsignedInt"/>
> </attribute>
> + <attribute name="transport">
> + <choice>
> + <value>socket</value>
> + <value>unix</value>
> + <value>rdma</value>
This could be a bit confusing as socket is too generic, after all unix is also
a socket. Could we change the values "tcp", "unix", "rdma" or something
similar depending on what "socket" was supposed to mean?
> + </choice>
> + </attribute>
> </element>
> </zeroOrMore>
> <empty/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 419088c..c89035e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -200,7 +200,8 @@ VIR_ENUM_IMPL(virDomainDiskErrorPolicy, VIR_DOMAIN_DISK_ERROR_POLICY_LAST,
> VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST,
> "nbd",
> "rbd",
> - "sheepdog")
> + "sheepdog",
> + "gluster")
We want to define a new enum for transport attribute in the same way we have
it for protocol and use that enum instead of any free from string we parse
from the XML or qemu command line.
> VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST,
> "none",
> @@ -994,6 +995,7 @@ void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def)
>
> VIR_FREE(def->name);
> VIR_FREE(def->port);
> + VIR_FREE(def->transport);
Then, there's no need to free it here.
> }
>
> void virDomainControllerDefFree(virDomainControllerDefPtr def)
> @@ -3489,6 +3491,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> }
> hosts[nhosts].name = NULL;
> hosts[nhosts].port = NULL;
> + hosts[nhosts].transport = NULL;
> nhosts++;
>
> hosts[nhosts - 1].name = virXMLPropString(child, "name");
> @@ -3503,6 +3506,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> "%s", _("missing port for host"));
> goto error;
> }
> + /* transport can be socket, unix, rdma, etc. */
> + hosts[nhosts - 1].transport = virXMLPropString(child, "transport");
We would need to change this into calling the appropriate TypeFromString().
> }
> child = child->next;
> }
> @@ -11479,8 +11484,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) {
> + virBufferEscapeString(buf, " transport='%s'",
> + def->hosts[i].transport);
Call the appropriate TypeToString(def->hosts[i].transport) instead and align
it with the first character after "virBufferEscapeString(".
> + }
> + virBufferAddLit(buf, "/>\n");
> }
> virBufferAddLit(buf, " </source>\n");
> }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 0c3824e..67e023f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -442,7 +442,7 @@ 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
> };
Do no remove the empty line above *PROTOCOL_LAST. Just add the new item above
it.
>
> @@ -467,6 +467,7 @@ typedef virDomainDiskHostDef *virDomainDiskHostDefPtr;
> struct _virDomainDiskHostDef {
> char *name;
> char *port;
> + char *transport;
This would be int rather than char *.
> };
>
> enum virDomainDiskIo {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ca62f0c..c8a0f27 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2068,6 +2068,86 @@ no_memory:
> return -1;
> }
>
> +static int qemuParseGlusterString(virDomainDiskDefPtr def)
> +{
> + char *port, *volimg, *transp, *marker;
> +
> + marker = strchr(def->src, ':');
> + if (marker) {
> + /* port found */
> + port = marker;
> + *port++ = '\0';
> + marker = port;
> + } else {
> + /* port not given, assume port = 0 */
> + port = NULL;
> + marker = def->src;
> + }
> +
> + volimg = strchr(marker, '/');
> + if (!volimg) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot parse gluster filename '%s'"), def->src);
> + return -1;
> + }
> + *volimg++ = '\0';
> + transp = strchr(volimg, '?');
> + if (transp) {
> + *transp++ = '\0';
> + transp = strchr(transp, '=');
> + transp++;
> + }
> + if (VIR_ALLOC(def->hosts) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> + def->nhosts = 1;
> + def->hosts->name = def->src;
> + if (port) {
> + def->hosts->port = strdup(port);
> + } else {
> + def->hosts->port = strdup("0");
> + }
> + if (transp) {
> + def->hosts->transport = strdup(transp);
> + if (!def->hosts->transport) {
> + virReportOOMError();
> + return -1;
> + }
> + } else {
> + def->hosts->transport = NULL;
> + }
Again, call the right TypeFromString() instead of just copying what we got.
> + if (!def->hosts->port) {
> + virReportOOMError();
> + return -1;
> + }
Also this check for non-NULL port should go above your new code.
> + 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:%s/%s",
> + disk->hosts->name, disk->hosts->port, disk->src);
> + if (disk->hosts->transport)
> + virBufferAsprintf(opt, "?transport=%s", disk->hosts->transport);
*TypeToString(disk->hosts->transport)
> + }
> + return ret;
> +}
> +
> char *
> qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
> virDomainDiskDefPtr disk,
> @@ -2209,6 +2289,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,",
> @@ -5135,6 +5221,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) {
> @@ -6811,6 +6909,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;
> @@ -7976,6 +8089,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;
> @@ -8061,6 +8178,12 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> goto no_memory;
> }
> break;
> + case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
> +
Remove this empty line here.
> + if (qemuParseGlusterString(disk) < 0)
> + goto error;
> +
> + break;
> }
> }
Hopefully this gluster functionality will be committed to qemu soon after 1.2
release so that we can include this set in libvirt-0.10.2.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list