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@xxxxxxxxxxxxxxxxxx> --- 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
