Re: [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 <address@hidden>
> > ---
> >   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

>