[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]