[Gluster-devel] [PATCH v3 UPDATED 1/2] Qemu/Gluster: Add Gluster protocol as supported network disk formats.
Harsh Bora
harsh at linux.vnet.ibm.com
Thu Nov 22 08:49:11 UTC 2012
On 11/16/2012 03:53 AM, Jiri Denemark wrote:
> 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 :-)
That would give me:
qemu/qemu_command.c: In function 'qemuParseGlusterString':
qemu/qemu_command.c:2048:18: error: lvalue required as left operand of
assignment.
I have updated patches for the rest of comments and shall be posting soon.
Harsh
More information about the Gluster-devel
mailing list