[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