Re: [libvirt] [PATCH v3 UPDATED 1/2] Qemu/Gluster: Add Gluster protocol as supported network disk formats.

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 :-)