2:09 p.m.
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(a)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!
[...]
> +
> +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