[libvirt] [PATCH 1/3] storage: add gluster backend initialization support for multiple servers

[Peter Krempa]

On Thu, Oct 08, 2015 at 17:25:51 +0530, Prasanna Kumar Kalever wrote:
> This patch adds support for initialization of gluster backend
> with multiple servers which acts as gluster volfile servers for
> the gluster storage backend.
> 
> This will help in achieving high availability of gluster backend
> connectivity via libgfapi i.e. when the first volfile server fails,
> then other servers specified are used as volfile server to re-establish
> the backend gluster connectivity.
> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever at redhat.com>
> ---
>  src/storage/storage_backend_gluster.c | 77 +++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
> index d2e79bc..53474c1 100644
> --- a/src/storage/storage_backend_gluster.c
> +++ b/src/storage/storage_backend_gluster.c
> @@ -32,6 +32,9 @@
>  #include "virstring.h"
>  #include "viruri.h"
>  
> +#define QEMU_DEFAULT_GLUSTER_PORT 24007
> +#define QEMU_DEFAULT_GLUSTER_TRANSPORT "tcp"
> +
>  #define VIR_FROM_THIS VIR_FROM_STORAGE
>  
>  VIR_LOG_INIT("storage.storage_backend_gluster");
> @@ -570,62 +573,64 @@ static int
>  virStorageFileBackendGlusterInit(virStorageSourcePtr src)
>  {
>      virStorageFileBackendGlusterPrivPtr priv = NULL;
> -    virStorageNetHostDefPtr host = &(src->hosts[0]);
> -    const char *hostname;
> +    const char *transport = NULL;

'transport' doesn't need to be initialized here since it's used in a
loop. This might cover up bugs.

>      int port = 0;
> +    size_t i = 0;
>  
> -    if (src->nhosts != 1) {

This still needs to check that we have at least one host.

> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Expected exactly 1 host for the gluster volume"));
> -        return -1;
> -    }
> -
> -    hostname = host->name;
> -
> -    VIR_DEBUG("initializing gluster storage file %p (gluster://%s:%s/%s%s)[%u:%u]",
> -              src, hostname, host->port ? host->port : "0",
> -              NULLSTR(src->volume), src->path,
> -              (unsigned int)src->drv->uid, (unsigned int)src->drv->gid);
> +    VIR_DEBUG("Initializing gluster volume=%s image-path=%s with uid=%u gid=%u",
> +            NULLSTR(src->volume), src->path,
> +            (unsigned int)src->drv->uid, (unsigned int)src->drv->gid);

Wrong alignment ...

>  
>      if (!src->volume) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("missing gluster volume name for path '%s'"),
> -                       src->path);
> +                _("missing gluster volume name for path '%s'"),
> +                src->path);

... and here you deliberately break alignment of existing code without
any useful change. Please adhere to the libvirt coding style when
posting patches. I will not complain further in this review about this.

>          return -1;
>      }
>  
>      if (VIR_ALLOC(priv) < 0)
>          return -1;
>  
> -    if (host->port &&
> -        virStrToLong_i(host->port, NULL, 10, &port) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("failed to parse port number '%s'"),
> -                       host->port);
> -        goto error;
> -    }
> -
> -    if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX)
> -        hostname = host->socket;
> -
>      if (!(priv->vol = glfs_new(src->volume))) {
>          virReportOOMError();
>          goto error;
>      }
>  
> -    if (glfs_set_volfile_server(priv->vol,
> -                                virStorageNetHostTransportTypeToString(host->transport),
> -                                hostname, port) < 0) {
> -        virReportSystemError(errno,
> -                             _("failed to set gluster volfile server '%s'"),
> -                             hostname);
> -        goto error;
> +    for (i = 0; i < src->nhosts; i++) {
> +        if (!src->hosts[i].port) {
> +            port = QEMU_DEFAULT_GLUSTER_PORT;
> +        } else if (virStrToLong_i(src->hosts[i].port, NULL, 10, &port) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                    _("failed to parse port number '%s'"), src->hosts[i].port);
> +            goto error;
> +        }
> +
> +        if (src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) {
> +            transport = src->hosts[i].socket;
> +        } else if (!src->hosts[i].transport) {
> +            transport = QEMU_DEFAULT_GLUSTER_TRANSPORT;
> +        } else {
> +            transport = virStorageNetHostTransportTypeToString(src->hosts[i].transport);
> +        }

The whole above condition is breaking code for unix socket transport,
sice you assign the socket path as the transport string. Additionally
the case for default transport should not be necessary since AFAIK the
parser code sets the default to TCP.

> +
> +        if (glfs_set_volfile_server(priv->vol, transport, src->hosts[i].name,
> +                    port) < 0) {
> +            virReportSystemError(errno,
> +                    _("failed to set gluster volfile server '%s'"),
> +                    src->hosts[i].name);
> +            goto error;
> +        } else {
> +            VIR_DEBUG("Successfully set volfile server with server:'%s' "
> +                    "port:'%d' transport:'%s'", src->hosts[i].name,
> +                    port, transport);
> +        }
>      }
>  
>      if (glfs_init(priv->vol) < 0) {
>          virReportSystemError(errno,
> -                             _("failed to initialize gluster connection to "
> -                               "server: '%s'"), hostname);
> +                _("failed to initialize gluster connection with given hosts "
> +                    "volume : '%s' image-path : '%s'"),

I'd prefer that the error message contained at least the first
server.

> +                NULLSTR(src->volume), src->path);
>          goto error;
>      }
>  

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151013/44307761/attachment-0001.sig>