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

Re: [libvirt] [PATCH v5 2/4] qemu: Add support for gluster protocol based network storage backend.



On Thu, Nov 22, 2012 at 23:40:39 +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>
> 
>     <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>

Move these XML examples to 1/4.

> 
> Signed-off-by: Harsh Prateek Bora <harsh linux vnet ibm com>
> ---
>  src/qemu/qemu_command.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 183 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 20730a9..a377744 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2021,6 +2021,156 @@ no_memory:
>      return -1;
>  }
>  
> +static int
> +qemuParseGlusterString(virDomainDiskDefPtr def)
> +{
> +    int ret = 0;
> +    char *transp = NULL;
> +    char *sock = NULL;
> +    char *volimg = NULL;
> +    virURIPtr uri = NULL;
> +    if (!(uri = virURIParse(def->src))) {
> +        return -1;
> +    }
> +
> +    if (VIR_ALLOC(def->hosts) < 0) {
> +        ret = -1;

In libvirt, we rather initialize ret = -1 and set it to 0 at the end of the
function when we know it succeeded. That way you don't have to remember to set
ret = -1 on every error.

> +        goto no_memory;
> +    }
> +
> +    if (STREQ(uri->scheme, "gluster")) {
> +        def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
> +    } else if (STRPREFIX(uri->scheme, "gluster+")) {
> +        transp = strstr(uri->scheme, "+") + 1;
> +        def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp);
> +        if (def->hosts->transport < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Invalid gluster transport type '%s'"), transp);
> +            ret = -1;
> +            goto cleanup;
> +
> +        }
> +    } else {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Invalid transport/scheme '%s'"), uri->scheme);
> +        ret = -1;
> +        goto cleanup;
> +    }
> +    def->nhosts = 0; /* set to 1 once everything succeeds */
> +
> +    if (def->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
> +        def->hosts->name = strdup(uri->server);
> +        if (!def->hosts->name) {
> +            ret = -1;
> +            goto no_memory;
> +        }
> +
> +        if (virAsprintf(&def->hosts->port, "%d", uri->port) < 0) {
> +            ret = -1;
> +            goto no_memory;
> +        }
> +    } else {
> +        def->hosts->name = NULL;
> +        def->hosts->port = 0;
> +        if(uri->query) {
> +            if(STRPREFIX(uri->query, "socket=")) {

make syntax-check requires space between "if" and "(".

> +                sock = strstr(uri->query, "=") + 1;
> +                def->hosts->socket = strdup(sock);
> +                if (!def->hosts->socket) {
> +                    ret = -1;
> +                    goto no_memory;
> +                }
> +            } else {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Invalid query parameter '%s'"), uri->query);

And this is the evidence how easy it is to forget ret = -1 :-)

> +                goto cleanup;
> +            }
> +        }
> +    }
> +    volimg = uri->path + 1; /* skip the prefix slash */
> +    def->src = strdup(volimg);
> +    if (!def->src) {
> +        ret = -1;
> +        goto no_memory;
> +    }
> +
> +    def->nhosts = 1;
> +    VIR_FREE(uri);
> +    return ret;
> +
> +no_memory:
> +    virReportOOMError();
> +cleanup:

"cleanup" label is generally used for code which is common to both success and
error paths. Use "error" label for error-path-only code.

> +    VIR_FREE(def->hosts);

Before freeing def->hosts, all strings referenced from it need to be freed by
calling virDomainDiskHostDefFree.

> +    VIR_FREE(uri);
> +
> +    return ret;
> +}
> +
> +static int
> +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt)
> +{
> +    int ret = 0, port = 0;

Initialize ret to -1.

> +    char *tmpscheme = NULL;
> +    char *volimg = NULL;
> +    char *sock = NULL;
> +    char *builturi = NULL;
> +    const char *transp = NULL;
> +    virURI uri = {
> +        .port = port /* just to clear rest of bits */
> +    };
> +
> +    if (disk->nhosts != 1) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("gluster accepts only one host"));
> +        return -1;
> +    }
> +
> +    virBufferAddLit(opt, "file=");
> +    transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport);
> +
> +    if (virAsprintf(&tmpscheme, "gluster+%s", transp) < 0) {
> +        ret = -1;
> +        goto no_memory;
> +    }
> +
> +    if (virAsprintf(&volimg, "/%s", disk->src) < 0) {
> +        ret = -1;
> +        goto no_memory;
> +    }
> +
> +    if (disk->hosts->port) {
> +        port = atoi(disk->hosts->port);
> +    }
> +
> +    if (disk->hosts->socket) {
> +        if (virAsprintf(&sock, "socket=%s", disk->hosts->socket) < 0) {
> +            ret = -1;
> +            goto no_memory;
> +        }
> +    }
> +
> +    uri.scheme = tmpscheme; /* gluster+<transport> */
> +    uri.server = disk->hosts->name;
> +    uri.port = port;
> +    uri.path = volimg;
> +    uri.query = sock;
> +
> +    builturi = virURIFormat(&uri);
> +    virBufferEscape(opt, ',', ",", "%s", builturi);
> +    goto cleanup;
> +
> +no_memory:
> +    virReportOOMError();
> +cleanup:
> +    VIR_FREE(builturi);
> +    VIR_FREE(tmpscheme);
> +    VIR_FREE(volimg);
> +    VIR_FREE(sock);
> +
> +    return ret;

We like to keep the success path linear...

> +}
> +
>  char *
>  qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>                    virDomainDiskDefPtr disk,
> @@ -2162,6 +2312,12 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>                      goto error;
>                  virBufferAddChar(&opt, ',');
>                  break;
> +            case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
> +                if (qemuBuildGlusterString(disk, &opt) < 0)
> +                    goto error;
> +                virBufferAddChar(&opt, ',');
> +                break;
> +
>              case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
>                  if (disk->nhosts == 0) {
>                      virBufferEscape(&opt, ',', ",", "file=sheepdog:%s,",
> @@ -5242,6 +5398,18 @@ qemuBuildCommandLine(virConnectPtr conn,
>                          file = virBufferContentAndReset(&opt);
>                      }
>                      break;
> +                case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
> +                    {
> +                        virBuffer opt = VIR_BUFFER_INITIALIZER;
> +                        if (qemuBuildGlusterString(disk, &opt) < 0)
> +                            goto error;
> +                        if (virBufferError(&opt)) {
> +                            virReportOOMError();
> +                            goto error;
> +                        }
> +                        file = virBufferContentAndReset(&opt);
> +                    }
> +                    break;
>                  case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
>                      if (disk->nhosts == 0) {
>                          if (virAsprintf(&file, "sheepdog:%s,", disk->src) < 0) {
> @@ -6919,7 +7087,6 @@ qemuParseCommandLineDisk(virCapsPtr caps,
>                          virReportOOMError();
>                          goto cleanup;
>                      }
> -
>                      VIR_FREE(def->src);
>                      def->src = NULL;
>                  } else if (STRPREFIX(def->src, "rbd:")) {

Unrelated whitespace change.

> @@ -6937,6 +7104,12 @@ qemuParseCommandLineDisk(virCapsPtr caps,
>                          goto cleanup;
>  
>                      VIR_FREE(p);
> +                } else if (STRPREFIX(def->src, "gluster")) {
> +                    def->type = VIR_DOMAIN_DISK_TYPE_NETWORK;
> +                    def->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER;
> +
> +                    if (qemuParseGlusterString(def) < 0)
> +                        goto cleanup;
>                  } else if (STRPREFIX(def->src, "sheepdog:")) {
>                      char *p = def->src;
>                      char *port, *vdi;
> @@ -6972,6 +7145,7 @@ qemuParseCommandLineDisk(virCapsPtr caps,
>                              virReportOOMError();
>                              goto cleanup;
>                          }
> +
>                          def->src = strdup(vdi);
>                          if (!def->src) {
>                              virReportOOMError();

Unrelated.

> @@ -8126,6 +8300,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
>                  disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK;
>                  disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD;
>                  val += strlen("rbd:");
> +            } else if (STRPREFIX(val, "gluster")) {
> +                disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK;
> +                disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER;
>              } else if (STRPREFIX(val, "sheepdog:")) {
>                  disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK;
>                  disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG;
> @@ -8211,6 +8388,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
>                              goto no_memory;
>                      }
>                      break;
> +                case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
> +                    if (qemuParseGlusterString(disk) < 0)
> +                        goto error;
> +
> +                    break;
>                  }
>              }
>  

ACK with the following patch squashed in:

    diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
    index 886347c..63e187a 100644
    --- a/src/libvirt_private.syms
    +++ b/src/libvirt_private.syms
    @@ -346,6 +346,7 @@ virDomainDiskErrorPolicyTypeToString;
     virDomainDiskFindControllerModel;
     virDomainDiskGeometryTransTypeFromString;
     virDomainDiskGeometryTransTypeToString;
    +virDomainDiskHostDefFree;
     virDomainDiskIndexByName;
     virDomainDiskInsert;
     virDomainDiskInsertPreAlloced;
    diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
    index 50693a9..e946336 100644
    --- a/src/qemu/qemu_command.c
    +++ b/src/qemu/qemu_command.c
    @@ -2043,93 +2043,85 @@ no_memory:
     static int
     qemuParseGlusterString(virDomainDiskDefPtr def)
     {
    -    int ret = 0;
    +    int ret = -1;
         char *transp = NULL;
         char *sock = NULL;
         char *volimg = NULL;
         virURIPtr uri = NULL;
    +
         if (!(uri = virURIParse(def->src))) {
             return -1;
         }
     
    -    if (VIR_ALLOC(def->hosts) < 0) {
    -        ret = -1;
    +    if (VIR_ALLOC(def->hosts) < 0)
             goto no_memory;
    -    }
     
         if (STREQ(uri->scheme, "gluster")) {
             def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
         } else if (STRPREFIX(uri->scheme, "gluster+")) {
    -        transp = strstr(uri->scheme, "+") + 1;
    +        transp = strchr(uri->scheme, '+') + 1;
             def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp);
             if (def->hosts->transport < 0) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Invalid gluster transport type '%s'"), transp);
    -            ret = -1;
    -            goto cleanup;
    -
    +            goto error;
             }
         } else {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Invalid transport/scheme '%s'"), uri->scheme);
    -        ret = -1;
    -        goto cleanup;
    +        goto error;
         }
         def->nhosts = 0; /* set to 1 once everything succeeds */
     
         if (def->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
             def->hosts->name = strdup(uri->server);
    -        if (!def->hosts->name) {
    -            ret = -1;
    +        if (!def->hosts->name)
                 goto no_memory;
    -        }
     
    -        if (virAsprintf(&def->hosts->port, "%d", uri->port) < 0) {
    -            ret = -1;
    +        if (virAsprintf(&def->hosts->port, "%d", uri->port) < 0)
                 goto no_memory;
    -        }
         } else {
             def->hosts->name = NULL;
             def->hosts->port = 0;
    -        if(uri->query) {
    -            if(STRPREFIX(uri->query, "socket=")) {
    -                sock = strstr(uri->query, "=") + 1;
    +        if (uri->query) {
    +            if (STRPREFIX(uri->query, "socket=")) {
    +                sock = strchr(uri->query, '=') + 1;
                     def->hosts->socket = strdup(sock);
    -                if (!def->hosts->socket) {
    -                    ret = -1;
    +                if (!def->hosts->socket)
                         goto no_memory;
    -                }
                 } else {
                     virReportError(VIR_ERR_INTERNAL_ERROR,
                                    _("Invalid query parameter '%s'"), uri->query);
    -                goto cleanup;
    +                goto error;
                 }
             }
         }
         volimg = uri->path + 1; /* skip the prefix slash */
         def->src = strdup(volimg);
    -    if (!def->src) {
    -        ret = -1;
    +    if (!def->src)
             goto no_memory;
    -    }
     
         def->nhosts = 1;
    -    VIR_FREE(uri);
    +    ret = 0;
    +
    +cleanup:
    +    virURIFree(uri);
    +
         return ret;
     
     no_memory:
         virReportOOMError();
    -cleanup:
    +error:
    +    virDomainDiskHostDefFree(def->hosts);
         VIR_FREE(def->hosts);
    -    VIR_FREE(uri);
    -
    -    return ret;
    +    goto cleanup;
     }
     
     static int
     qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt)
     {
    -    int ret = 0, port = 0;
    +    int ret = -1;
    +    int port = 0;
         char *tmpscheme = NULL;
         char *volimg = NULL;
         char *sock = NULL;
    @@ -2148,26 +2140,19 @@ qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt)
         virBufferAddLit(opt, "file=");
         transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport);
     
    -    if (virAsprintf(&tmpscheme, "gluster+%s", transp) < 0) {
    -        ret = -1;
    +    if (virAsprintf(&tmpscheme, "gluster+%s", transp) < 0)
             goto no_memory;
    -    }
     
    -    if (virAsprintf(&volimg, "/%s", disk->src) < 0) {
    -        ret = -1;
    +    if (virAsprintf(&volimg, "/%s", disk->src) < 0)
             goto no_memory;
    -    }
     
         if (disk->hosts->port) {
             port = atoi(disk->hosts->port);
         }
     
    -    if (disk->hosts->socket) {
    -        if (virAsprintf(&sock, "socket=%s", disk->hosts->socket) < 0) {
    -            ret = -1;
    -            goto no_memory;
    -        }
    -    }
    +    if (disk->hosts->socket &&
    +        virAsprintf(&sock, "socket=%s", disk->hosts->socket) < 0)
    +        goto no_memory;
     
         uri.scheme = tmpscheme; /* gluster+<transport> */
         uri.server = disk->hosts->name;
    @@ -2177,10 +2162,9 @@ qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt)
     
         builturi = virURIFormat(&uri);
         virBufferEscape(opt, ',', ",", "%s", builturi);
    -    goto cleanup;
     
    -no_memory:
    -    virReportOOMError();
    +    ret = 0;
    +
     cleanup:
         VIR_FREE(builturi);
         VIR_FREE(tmpscheme);
    @@ -2188,6 +2172,10 @@ cleanup:
         VIR_FREE(sock);
     
         return ret;
    +
    +no_memory:
    +    virReportOOMError();
    +    goto cleanup;
     }
     
     char *


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