On Fri, Oct 26, 2012 at 22:27:35 +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='Volume1/image'>
> <host name='example.org' port='6000' transport='tcp'/>
> </source>
> <target dev='vda' bus='virtio'/>
> </disk>
>
> In the <host> element above, transport is a new optional attribute.
> Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed.
> If transport type is unix, another new optional attribute socket specifies
> path to unix socket:
>
> <disk type='network' device='disk'>
> <driver name='qemu' type='raw'/>
> <source protocol='gluster' name='Volume2/image'>
> <host transport='unix' socket='/path/to/sock'/>
> </source>
> <target dev='vdb' bus='virtio'/>
> </disk>
We agreed with Daniel that this XML schema is just good enough.
...
> docs/formatdomain.html.in | 24 ++++--
> docs/schemas/domaincommon.rng | 35 +++++++--
> src/conf/domain_conf.c | 72 ++++++++++++++----
> src/conf/domain_conf.h | 12 +++
> src/libvirt_private.syms | 2 +
> src/qemu/qemu_command.c | 168 +++++++++++++++++++++++++++++++++++++++++-
Separate all changes in src/qemu/qemu_command.c into a new patch that just
implements gluster support in qemu driver. Unless doing so would break the
build in between of course (but I believe it should not break it).
...
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f47fdad..ea6bc54 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
...
> @@ -1055,15 +1056,35 @@
> </optional>
> <zeroOrMore>
> <element name="host">
> - <attribute name="name">
> - <ref name="dnsName"/>
> - </attribute>
> - <attribute name="port">
> - <ref name="unsignedInt"/>
> - </attribute>
> + <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>
> - <empty/>
> + <empty/>
Looks like an accidental whitespace change.
> </element>
> </optional>
> <ref name="diskspec"/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 33e1e7f..52a965d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -3566,19 +3574,43 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> }
> hosts[nhosts].name = NULL;
> hosts[nhosts].port = NULL;
> + hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
> + hosts[nhosts].socket = NULL;
> nhosts++;
>
> - hosts[nhosts - 1].name = virXMLPropString(child, "name");
> - if (!hosts[nhosts - 1].name) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("missing name 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,
I know we use VIR_ERR_INTERNAL_ERROR for this kind of error in most of our
current code but that's not right and we should not be adding more of them.
The correct error code for this is VIR_ERR_XML_ERROR.
> + _("unknown protocol transport type '%s'"),
> + protocol_transport);
> + goto error;
> + }
> }
> - hosts[nhosts - 1].port = virXMLPropString(child, "port");
> - if (!hosts[nhosts - 1].port) {
> + hosts[nhosts - 1].socket = virXMLPropString(child, "socket");
> +
> + if (hosts[nhosts - 1].transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
> + hosts[nhosts - 1].socket == NULL) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
VIR_ERR_XML_ERROR
> - "%s", _("missing port for host"));
> - goto error;
> + "%s", _("missing socket for unix transport"));
> + }
Since the RNG schema forbids socket attribute with transport != unix, we
should forbid that in the code as well.
> +
> + if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
> +
We don't need this empty line.
> + hosts[nhosts - 1].name = virXMLPropString(child, "name");
> + if (!hosts[nhosts - 1].name) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
VIR_ERR_XML_ERROR
> + "%s", _("missing name for host"));
> + goto error;
> + }
> + hosts[nhosts - 1].port = virXMLPropString(child, "port");
> + if (!hosts[nhosts - 1].port) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
VIR_ERR_XML_ERROR
> + "%s", _("missing port for host"));
> + goto error;
> + }
> }
> }
> child = child->next;
> @@ -11754,10 +11786,22 @@ virDomainDiskDefFormat(virBufferPtr buf,
>
> virBufferAddLit(buf, ">\n");
> for (i = 0; i < def->nhosts; i++) {
> - virBufferEscapeString(buf, " <host name='%s'",
> - def->hosts[i].name);
> - virBufferEscapeString(buf, " port='%s'/>\n",
> - def->hosts[i].port);
> + virBufferAddLit(buf, " <host");
> + if (def->hosts[i].name) {
> + virBufferEscapeString(buf, " name='%s'", def->hosts[i].name);
> + }
> + if (def->hosts[i].port) {
> + virBufferEscapeString(buf, " port='%s'",
> + def->hosts[i].port);
> + }
> + if (def->hosts[i].transport >= 0) {
Just change this to ``if (def->hosts[i].transport)'' to avoid the need for
using magic -1 value for initializing transport further in the code.
> + virBufferAsprintf(buf, " transport='%s'",
> + virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport));
> + }
> + if (def->hosts[i].socket) {
> + virBufferEscapeString(buf, " socket='%s'", def->hosts[i].socket);
> + }
> + virBufferAddLit(buf, "/>\n");
> }
> virBufferAddLit(buf, " </source>\n");
> }
...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 20730a9..f4fdd67 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1940,6 +1940,10 @@ static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
> disk->hosts[disk->nhosts-1].name = strdup(hostport);
> if (!disk->hosts[disk->nhosts-1].name)
> goto no_memory;
> +
> + disk->hosts[disk->nhosts-1].transport = -1;
Just set it to TRANS_TCP. It uses tcp transport, after all.
> + disk->hosts[disk->nhosts-1].socket = NULL;
> +
> return 0;
>
> no_memory:
> @@ -2021,6 +2025,127 @@ no_memory:
> return -1;
> }
>
> +static int qemuParseGlusterString(virDomainDiskDefPtr def)
New line after "static int".
> +{
> + char *uritoparse = strdup(def->src);
> + char *transp = NULL;
> + char *sock = NULL;
> + virURIPtr uri = NULL;
> + uri = virURIParse(uritoparse);
This code does not check for strdup or virURIParse failures. And the function
leaks uritoparse and def->hosts on error and leaks uri in all cases. It should
be rewritten to use cleanup label. And why do you need to strdup(def->src) in
the first place? Just using it directly in virURIParse should work or am I
missing something?
> +
> + if (VIR_ALLOC(def->hosts) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + if (STREQ(uri->scheme, "gluster")) {
> + def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
> + } else if (strstr(uri->scheme, "+")) {
This should rather check for STRPREFIX(uri->scheme, "gluster+")
> + transp = strstr(uri->scheme, "+");
> + transp++;
You could even squash the increment into the previous line :-)
> + def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp);
> + if (def->hosts->transport < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Invalid gluster transport type '%s'"), transp);
> + return -1;
> +
Unneeded empty line.
> + }
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Invalid transport/scheme '%s'"), uri->scheme);
Four spaces of indentation would have been enough :-)
> + }
> +
> + def->nhosts = 1;
I'd move this line just before ret = 0 (after you modify the code to use
cleanup label) so that you can always free def->hosts on error path without
confusing the caller with hosts == NULL and nhosts == 1.
> + def->hosts->name = strdup(uri->server);
> + if (!def->hosts->name) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + if (virAsprintf(&def->hosts->port, "%d", uri->port) < 0) {
> + virReportOOMError();
> + return -1;
> + }
I'd move one of the extra empty lines here :-)
> + if(STRPREFIX(uri->query, "socket=")) {
> + sock = strstr(uri->query, "=");
> + sock++;
> + def->hosts->socket = strdup(sock);
> + if (!def->hosts->socket) {
> + virReportOOMError();
> + return -1;
> + }
> +
Extra empty line.
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Invalid query parameter '%s'"), uri->query);
> +
Extra empty line.
> + }
> +
> + def->src = strdup(uri->path);
> + if (!def->src) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + VIR_FREE(uritoparse);
> +
> + return 0;
> +}
> +
> +static int
> +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt)
> +{
> + int ret = 0, port = 0;
> + char *tmpscheme = NULL;
> + char *volimg = NULL;
> + char *sock = NULL;
> + char *builturi = NULL;
> +
> + virBufferAddLit(opt, "file=");
> + if (disk->nhosts != 1) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("gluster accepts only one host"));
Extra space, "_" should be aligned with "V" in VIR_ERR_INTERNAL_ERROR.
> + ret = -1;
Heh, why don't you just return -1 to avoid the need for else branch and
additional indentation level? Also it would make more sense to do this check
before adding "file=" to the buffer although it does not really matter.
> + } else {
> +
Extra empty line.
> + if (virAsprintf(&tmpscheme, "gluster+%s",
> + virDomainDiskProtocolTransportTypeToString(disk->hosts->transport)) < 0) {
One space is missing, "virDomainDisk..." should be aligned with "&". But
anyway, I'd use a temporary variable for transport to make this long line
shorter.
> + virReportOOMError();
> + return -1;
> + }
> + if (virAsprintf(&volimg, "/%s", disk->src) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + if (disk->hosts->port) {
> + port = atoi(disk->hosts->port);
> + }
> +
> + if (disk->hosts->socket) {
> + if (virAsprintf(&sock, "socket=%s", disk->hosts->socket) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> + }
> + virURI uri = {
> + .scheme = tmpscheme, /* gluster+<transport> */
> + .server = disk->hosts->name,
> + .port = port,
> + .path = volimg,
> + .query = sock,
> + };
> +
> + builturi = virURIFormat(&uri);
> + virBufferAsprintf(opt, "%s", builturi);
I think this should rather call
virBufferEscape(opt, ',', ",", "%s", builturi);
so that a possible ',' in volume name does not confuse qemu.
> +
> + VIR_FREE(tmpscheme);
> + VIR_FREE(volimg);
> + VIR_FREE(sock);
> + }
> + return ret;
Again, the function may leak several strings in error case and always leaks
builturi.
> +}
> +
> char *
> qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
> virDomainDiskDefPtr disk,
...
> @@ -6919,7 +7062,8 @@ qemuParseCommandLineDisk(virCapsPtr caps,
> virReportOOMError();
> goto cleanup;
> }
> -
> + def->hosts->transport = -1;
Initialize to TRANSP_TCP.
> + def->hosts->socket = NULL;
> VIR_FREE(def->src);
> def->src = NULL;
> } else if (STRPREFIX(def->src, "rbd:")) {
> @@ -6937,6 +7081,14 @@ qemuParseCommandLineDisk(virCapsPtr caps,
> goto cleanup;
>
> VIR_FREE(p);
> + } else if (STRPREFIX(def->src, "gluster")) {
> +
Extra empty line.
> + def->type = VIR_DOMAIN_DISK_TYPE_NETWORK;
> + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER;
> +
> + if (qemuParseGlusterString(def) < 0)
> + goto cleanup;
> +
Extra empty line.
> } else if (STRPREFIX(def->src, "sheepdog:")) {
> char *p = def->src;
> char *port, *vdi;
> @@ -6972,6 +7124,9 @@ qemuParseCommandLineDisk(virCapsPtr caps,
> virReportOOMError();
> goto cleanup;
> }
> + def->hosts->transport = -1;
TRANSP_TCP
> + def->hosts->socket = NULL;
> +
> def->src = strdup(vdi);
> if (!def->src) {
> virReportOOMError();
...
> @@ -8666,6 +8829,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> VIR_FREE(hosts);
> goto no_memory;
> }
> + first_rbd_disk->hosts[first_rbd_disk->nhosts].transport = -1;
TRANSP_TCP
> + first_rbd_disk->hosts[first_rbd_disk->nhosts].socket = NULL;
> +
> first_rbd_disk->nhosts++;
> token = strtok_r(NULL, ",", &saveptr);
> }
OK, quite a lot of non-trivial changes make me request a new version.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list