gluster-devel
[Top][All Lists]
Advanced

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

Re: [Gluster-devel] [PATCH v3 UPDATED 1/2] Qemu/Gluster: Add Gluster pro


From: Harsh Bora
Subject: Re: [Gluster-devel] [PATCH v3 UPDATED 1/2] Qemu/Gluster: Add Gluster protocol as supported network disk formats.
Date: Thu, 22 Nov 2012 14:19:11 +0530
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1

On 11/16/2012 03:53 AM, Jiri Denemark wrote:
On Fri, Oct 26, 2012 at 22:27:35 +0530, 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='Volume1/image'>
         <host name='example.org' port='6000' transport='tcp'/>
       </source>
       <target dev='vda' bus='virtio'/>
     </disk>

In the <host> element above, transport is a new optional attribute.
Valid transport values are tcp, unix or rdma. If none specified, tcp is assumed.
If transport type is unix, another new optional attribute socket specifies
path to unix socket:

     <disk type='network' device='disk'>
       <driver name='qemu' type='raw'/>
       <source protocol='gluster' name='Volume2/image'>
         <host transport='unix' socket='/path/to/sock'/>
       </source>
       <target dev='vdb' bus='virtio'/>
     </disk>

We agreed with Daniel that this XML schema is just good enough.

...
  docs/formatdomain.html.in     |  24 ++++--
  docs/schemas/domaincommon.rng |  35 +++++++--
  src/conf/domain_conf.c        |  72 ++++++++++++++----
  src/conf/domain_conf.h        |  12 +++
  src/libvirt_private.syms      |   2 +
  src/qemu/qemu_command.c       | 168 +++++++++++++++++++++++++++++++++++++++++-

Separate all changes in src/qemu/qemu_command.c into a new patch that just
implements gluster support in qemu driver. Unless doing so would break the
build in between of course (but I believe it should not break it).

...
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f47fdad..ea6bc54 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
...
@@ -1055,15 +1056,35 @@
                  </optional>
                  <zeroOrMore>
                    <element name="host">
-                    <attribute name="name">
-                      <ref name="dnsName"/>
-                    </attribute>
-                    <attribute name="port">
-                      <ref name="unsignedInt"/>
-                    </attribute>
+                    <choice>
+                      <group>
+                        <optional>
+                          <attribute name="transport">
+                            <choice>
+                              <value>tcp</value>
+                              <value>rdma</value>
+                            </choice>
+                          </attribute>
+                        </optional>
+                        <attribute name="name">
+                          <ref name="dnsName"/>
+                        </attribute>
+                        <attribute name="port">
+                          <ref name="unsignedInt"/>
+                        </attribute>
+                      </group>
+                      <group>
+                        <attribute name="transport">
+                          <value>unix</value>
+                        </attribute>
+                        <attribute name="socket">
+                          <ref name="absFilePath"/>
+                        </attribute>
+                      </group>
+                    </choice>
                    </element>
                  </zeroOrMore>
-                <empty/>
+               <empty/>

Looks like an accidental whitespace change.

                </element>
              </optional>
              <ref name="diskspec"/>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 33e1e7f..52a965d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
...
@@ -3566,19 +3574,43 @@ virDomainDiskDefParseXML(virCapsPtr caps,
                              }
                              hosts[nhosts].name = NULL;
                              hosts[nhosts].port = NULL;
+                            hosts[nhosts].transport = 
VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
+                            hosts[nhosts].socket = NULL;
                              nhosts++;

-                            hosts[nhosts - 1].name = virXMLPropString(child, 
"name");
-                            if (!hosts[nhosts - 1].name) {
-                                virReportError(VIR_ERR_INTERNAL_ERROR,
-                                               "%s", _("missing name for 
host"));
-                                goto error;
+                            /* transport can be tcp (default), unix or rdma.  
*/
+                            protocol_transport = virXMLPropString(child, 
"transport");
+                            if (protocol_transport != NULL) {
+                                hosts[nhosts - 1].transport = 
virDomainDiskProtocolTransportTypeFromString(protocol_transport);
+                                if (hosts[nhosts - 1].transport < 0) {
+                                    virReportError(VIR_ERR_INTERNAL_ERROR,

I know we use VIR_ERR_INTERNAL_ERROR for this kind of error in most of our
current code but that's not right and we should not be adding more of them.
The correct error code for this is VIR_ERR_XML_ERROR.

+                                                   _("unknown protocol transport 
type '%s'"),
+                                                   protocol_transport);
+                                    goto error;
+                                }
                              }
-                            hosts[nhosts - 1].port = virXMLPropString(child, 
"port");
-                            if (!hosts[nhosts - 1].port) {
+                            hosts[nhosts - 1].socket = virXMLPropString(child, 
"socket");
+
+                            if (hosts[nhosts - 1].transport == 
VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
+                                hosts[nhosts - 1].socket == NULL) {
                                  virReportError(VIR_ERR_INTERNAL_ERROR,

VIR_ERR_XML_ERROR

-                                               "%s", _("missing port for 
host"));
-                                goto error;
+                                               "%s", _("missing socket for unix 
transport"));
+                            }

Since the RNG schema forbids socket attribute with transport != unix, we
should forbid that in the code as well.

+
+                            if (hosts[nhosts - 1].transport != 
VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
+

We don't need this empty line.

+                                hosts[nhosts - 1].name = virXMLPropString(child, 
"name");
+                                if (!hosts[nhosts - 1].name) {
+                                    virReportError(VIR_ERR_INTERNAL_ERROR,

VIR_ERR_XML_ERROR

+                                                   "%s", _("missing name for 
host"));
+                                    goto error;
+                                }
+                                hosts[nhosts - 1].port = virXMLPropString(child, 
"port");
+                                if (!hosts[nhosts - 1].port) {
+                                    virReportError(VIR_ERR_INTERNAL_ERROR,

VIR_ERR_XML_ERROR

+                                                   "%s", _("missing port for 
host"));
+                                    goto error;
+                                }
                              }
                          }
                          child = child->next;
@@ -11754,10 +11786,22 @@ virDomainDiskDefFormat(virBufferPtr buf,

                  virBufferAddLit(buf, ">\n");
                  for (i = 0; i < def->nhosts; i++) {
-                    virBufferEscapeString(buf, "        <host name='%s'",
-                                          def->hosts[i].name);
-                    virBufferEscapeString(buf, " port='%s'/>\n",
-                                          def->hosts[i].port);
+                    virBufferAddLit(buf, "        <host");
+                    if (def->hosts[i].name) {
+                        virBufferEscapeString(buf, " name='%s'", 
def->hosts[i].name);
+                    }
+                    if (def->hosts[i].port) {
+                        virBufferEscapeString(buf, " port='%s'",
+                                              def->hosts[i].port);
+                    }
+                    if (def->hosts[i].transport >= 0) {

Just change this to ``if (def->hosts[i].transport)'' to avoid the need for
using magic -1 value for initializing transport further in the code.

+                        virBufferAsprintf(buf, " transport='%s'",
+                                          
virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport));
+                    }
+                    if (def->hosts[i].socket) {
+                        virBufferEscapeString(buf, " socket='%s'", 
def->hosts[i].socket);
+                    }
+                    virBufferAddLit(buf, "/>\n");
                  }
                  virBufferAddLit(buf, "      </source>\n");
              }
...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 20730a9..f4fdd67 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1940,6 +1940,10 @@ static int qemuAddRBDHost(virDomainDiskDefPtr disk, char 
*hostport)
      disk->hosts[disk->nhosts-1].name = strdup(hostport);
      if (!disk->hosts[disk->nhosts-1].name)
          goto no_memory;
+
+    disk->hosts[disk->nhosts-1].transport = -1;

Just set it to TRANS_TCP. It uses tcp transport, after all.

+    disk->hosts[disk->nhosts-1].socket = NULL;
+
      return 0;

  no_memory:
@@ -2021,6 +2025,127 @@ no_memory:
      return -1;
  }

+static int qemuParseGlusterString(virDomainDiskDefPtr def)

New line after "static int".

+{
+    char *uritoparse = strdup(def->src);
+    char *transp = NULL;
+    char *sock = NULL;
+    virURIPtr uri = NULL;
+    uri = virURIParse(uritoparse);

This code does not check for strdup or virURIParse failures. And the function
leaks uritoparse and def->hosts on error and leaks uri in all cases. It should
be rewritten to use cleanup label. And why do you need to strdup(def->src) in
the first place? Just using it directly in virURIParse should work or am I
missing something?

+
+    if (VIR_ALLOC(def->hosts) < 0) {
+        virReportOOMError();
+        return -1;
+    }
+
+    if (STREQ(uri->scheme, "gluster")) {
+        def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
+    } else if (strstr(uri->scheme, "+")) {

This should rather check for STRPREFIX(uri->scheme, "gluster+")

+        transp = strstr(uri->scheme, "+");
+        transp++;

You could even squash the increment into the previous line :-)

That would give me:

qemu/qemu_command.c: In function 'qemuParseGlusterString':
qemu/qemu_command.c:2048:18: error: lvalue required as left operand of assignment.

I have updated patches for the rest of comments and shall be posting soon.

Harsh




reply via email to

[Prev in Thread] Current Thread [Next in Thread]