[Gluster-devel] [PATCH v2 1/2] Qemu/Gluster: Add Gluster protocol as supported network disk formats.

[Harsh Bora]

On 10/04/2012 09:17 PM, Deepak C Shetty wrote:
> On 10/04/2012 07:01 PM, 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='tcp'/>
>>        </source>
>>        <target dev='vda' bus='virtio'/>
>>      </disk>
>>
>> Note: In the <host> element above, transport is an optional attribute.
>> Valid transport values are tcp, unix or rdma. If none specified, tcp
>> is assumed.
>> If transport type is unix, host name specifies path to unix socket.
>>
>> Signed-off-by: Harsh Prateek Bora <harsh at linux.vnet.ibm.com>
>> ---
>>   docs/schemas/domaincommon.rng |   8 ++
>>   src/conf/domain_conf.c        |  28 +++++-
>>   src/conf/domain_conf.h        |  11 +++
>>   src/libvirt_private.syms      |   2 +
>>   src/qemu/qemu_command.c       | 204
>> ++++++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 251 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/schemas/domaincommon.rng
>> b/docs/schemas/domaincommon.rng
>> index f47fdad..89d9b9f 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -1048,6 +1048,7 @@
>>                       <value>nbd</value>
>>                       <value>rbd</value>
>>                       <value>sheepdog</value>
>> +                    <value>gluster</value>
>>                     </choice>
>>                   </attribute>
>>                   <optional>
>> @@ -1061,6 +1062,13 @@
>>                       <attribute name="port">
>>                         <ref name="unsignedInt"/>
>>                       </attribute>
>> +                    <attribute name="transport">
>> +                      <choice>
>> +                        <value>tcp</value>
>> +                        <value>unix</value>
>> +                        <value>rdma</value>
>> +                      </choice>
>> +                    </attribute>
> 'transport' attribute is optional, so it should be placed inside
> <optional> </optional> ?

Not necessarily, <zeroOrMore> works for it. You may want to test the 
patch without specifying 'transport' attribute!

>
>>                     </element>
>>                   </zeroOrMore>
>>                   <empty/>

[...]

>> +
>> +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://",
>> +
>> virDomainDiskProtocolTransportTypeToString(disk->hosts->transport));
>> +
>> +        /* if transport type is not unix, specify server:port */
>> +        if (disk->hosts->transport !=
>> VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
>> +            if (strstr(disk->hosts->name, ":")) {
> Wouldn't it be better to check for ':' and check for absence of "." (and
> vice versa in the else) so that if someone specified a.b.c:d
> or a:b:c:d:e.f we can throw error rite away, instead of qemu erroring
> out later in time ? Its a very primtive check but
> can help to catch user input error early enuf.

I chose to check for only ':' to decide if its a IPv6 addr because it 
doesnt make sense to be partial towards '.' What if someone specifies a 
host name like 12:12;12,12 or 23:23,23,23 ? A '.' in an IPv6 addr is as 
bad as any other invalid char.

>
>> +                /* if IPv6 addr, use square brackets to enclose it */
>> +                virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name,
>> disk->hosts->port);
>> +            } else {
>> +                virBufferAsprintf(opt, "%s:%s", disk->hosts->name,
>> disk->hosts->port);
>> +            }
>> +        }
>> +
>> +        /* append source path to gluster disk image */
>> +        virBufferAsprintf(opt, "/%s", disk->src);
>> +
>> +        /* if transport type is unix, server name is path to unix
>> socket, ignore port */
>> +        if (disk->hosts->transport ==
>> VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
>> +            virBufferAsprintf(opt, "?socket=%s", disk->hosts->name);
>> +        }
> This can be clubbed as part of else clause to the above if condn (if
> (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that ways
> we avoid an un-necessary check of transport here.
> It also means that disk->src needs to be pulled inside the if & else
> clauses, which I feel should be ok.

That was the only reason I kept it out of else. Isn't it better to avoid 
redundant code than replacing if with an else ?

regards,
Harsh

>