[v19,5/5] block/gluster: add support for multiple gluster servers

Commit Message

Prasanna Kumar Kalever July 15, 2016, 3 p.m. UTC

This patch adds a way to specify multiple volfile servers to the gluster
block backend of QEMU with tcp|rdma transport types and their port numbers.

Problem:

Currently VM Image on gluster volume is specified like this:

file=gluster[+tcp]://host[:port]/testvol/a.img

Say we have three hosts in a trusted pool with replica 3 volume in action.
When the host mentioned in the command above goes down for some reason,
the other two hosts are still available. But there's currently no way
to tell QEMU about them.

Solution:

New way of specifying VM Image on gluster volume with volfile servers:
(We still support old syntax to maintain backward compatibility)

Basic command line syntax looks like:

Pattern I:
 -drive driver=gluster,
        volume=testvol,path=/path/a.raw,[debug=N,]
        server.0.type=tcp,
        server.0.host=1.2.3.4,
       [server.0.port=24007,]
        server.1.type=unix,
        server.1.socket=/path/socketfile

Pattern II:
 'json:{"driver":"qcow2","file":{"driver":"gluster",
       "volume":"testvol","path":"/path/a.qcow2",["debug":N,]
       "server":[{hostinfo_1}, ...{hostinfo_N}]}}'

   driver      => 'gluster' (protocol name)
   volume      => name of gluster volume where our VM image resides
   path        => absolute path of image in gluster volume
  [debug]      => libgfapi loglevel [(0 - 9) default 4 -> Error]

  {hostinfo}   => {{type:"tcp",host:"1.2.3.4"[,port=24007]},
                   {type:"unix",socket:"/path/sockfile"}}

   type        => transport type used to connect to gluster management daemon,
                  it can be tcp|unix
   host        => host address (hostname/ipv4/ipv6 addresses/socket path)
  [port]       => port number on which glusterd is listening. (default 24007)
   socket      => path to socket file

Examples:
1.
 -drive driver=qcow2,file.driver=gluster,
        file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
        file.server.0.type=tcp,
        file.server.0.host=1.2.3.4,
        file.server.0.port=24007,
        file.server.1.type=tcp,
        file.server.1.socket=/var/run/glusterd.socket
2.
 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
         "path":"/path/a.qcow2","debug":9,"server":
         [{type:"tcp",host:"1.2.3.4",port=24007},
          {type:"unix",socket:"/var/run/glusterd.socket"}] } }'

This patch gives a mechanism to provide all the server addresses, which are in
replica set, so in case host1 is down VM can still boot from any of the
active hosts.

This is equivalent to the backup-volfile-servers option supported by
mount.glusterfs (FUSE way of mounting gluster volume)

credits: sincere thanks to all the supporters

Signed-off-by: Prasanna Kumar Kalever <[e-mail]>
---
 block/gluster.c      | 347 +++++++++++++++++++++++++++++++++++++++++++++------
 qapi/block-core.json |   2 +-
 2 files changed, 307 insertions(+), 42 deletions(-)

Comments

Markus Armbruster July 18, 2016, 10:17 a.m. UTC | #1

Prasanna Kumar Kalever <[e-mail]> writes:

> This patch adds a way to specify multiple volfile servers to the gluster
> block backend of QEMU with tcp|rdma transport types and their port numbers.
>
> Problem:
>
> Currently VM Image on gluster volume is specified like this:
>
> file=gluster[+tcp]://host[:port]/testvol/a.img
>
> Say we have three hosts in a trusted pool with replica 3 volume in action.
> When the host mentioned in the command above goes down for some reason,
> the other two hosts are still available. But there's currently no way
> to tell QEMU about them.
>
> Solution:
>
> New way of specifying VM Image on gluster volume with volfile servers:
> (We still support old syntax to maintain backward compatibility)
>
> Basic command line syntax looks like:
>
> Pattern I:
>  -drive driver=gluster,
>         volume=testvol,path=/path/a.raw,[debug=N,]
>         server.0.type=tcp,
>         server.0.host=1.2.3.4,
>        [server.0.port=24007,]
>         server.1.type=unix,
>         server.1.socket=/path/socketfile
>
> Pattern II:
>  'json:{"driver":"qcow2","file":{"driver":"gluster",
>        "volume":"testvol","path":"/path/a.qcow2",["debug":N,]
>        "server":[{hostinfo_1}, ...{hostinfo_N}]}}'
>
>    driver      => 'gluster' (protocol name)
>    volume      => name of gluster volume where our VM image resides
>    path        => absolute path of image in gluster volume
>   [debug]      => libgfapi loglevel [(0 - 9) default 4 -> Error]
>
>   {hostinfo}   => {{type:"tcp",host:"1.2.3.4"[,port=24007]},
>                    {type:"unix",socket:"/path/sockfile"}}
>
>    type        => transport type used to connect to gluster management daemon,
>                   it can be tcp|unix
>    host        => host address (hostname/ipv4/ipv6 addresses/socket path)
>   [port]       => port number on which glusterd is listening. (default 24007)
>    socket      => path to socket file
>
> Examples:
> 1.
>  -drive driver=qcow2,file.driver=gluster,
>         file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
>         file.server.0.type=tcp,
>         file.server.0.host=1.2.3.4,
>         file.server.0.port=24007,
>         file.server.1.type=tcp,
>         file.server.1.socket=/var/run/glusterd.socket
> 2.
>  'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
>          "path":"/path/a.qcow2","debug":9,"server":
>          [{type:"tcp",host:"1.2.3.4",port=24007},
>           {type:"unix",socket:"/var/run/glusterd.socket"}] } }'
>
> This patch gives a mechanism to provide all the server addresses, which are in
> replica set, so in case host1 is down VM can still boot from any of the
> active hosts.
>
> This is equivalent to the backup-volfile-servers option supported by
> mount.glusterfs (FUSE way of mounting gluster volume)
>
> credits: sincere thanks to all the supporters
>
> Signed-off-by: Prasanna Kumar Kalever <[e-mail]>
> ---
>  block/gluster.c      | 347 +++++++++++++++++++++++++++++++++++++++++++++------
>  qapi/block-core.json |   2 +-
>  2 files changed, 307 insertions(+), 42 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index ff1e783..fd2279d 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -12,8 +12,16 @@
>  #include "block/block_int.h"
>  #include "qapi/error.h"
>  #include "qemu/uri.h"
> +#include "qemu/error-report.h"
>  
>  #define GLUSTER_OPT_FILENAME        "filename"
> +#define GLUSTER_OPT_VOLUME          "volume"
> +#define GLUSTER_OPT_PATH            "path"
> +#define GLUSTER_OPT_TYPE            "type"
> +#define GLUSTER_OPT_SERVER_PATTERN  "server."
> +#define GLUSTER_OPT_HOST            "host"
> +#define GLUSTER_OPT_PORT            "port"
> +#define GLUSTER_OPT_SOCKET          "socket"
>  #define GLUSTER_OPT_DEBUG           "debug"
>  #define GLUSTER_DEFAULT_PORT        24007
>  #define GLUSTER_DEBUG_DEFAULT       4
> @@ -82,6 +90,77 @@ static QemuOptsList runtime_opts = {
>      },
>  };
>  
> +static QemuOptsList runtime_json_opts = {
> +    .name = "gluster_json",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
> +    .desc = {
> +        {
> +            .name = GLUSTER_OPT_VOLUME,
> +            .type = QEMU_OPT_STRING,
> +            .help = "name of gluster volume where VM image resides",
> +        },
> +        {
> +            .name = GLUSTER_OPT_PATH,
> +            .type = QEMU_OPT_STRING,
> +            .help = "absolute path to image file in gluster volume",
> +        },
> +        {
> +            .name = GLUSTER_OPT_DEBUG,
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Gluster log level, valid range is 0-9",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static QemuOptsList runtime_type_opts = {
> +    .name = "gluster_type",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_type_opts.head),
> +    .desc = {
> +        {
> +            .name = GLUSTER_OPT_TYPE,
> +            .type = QEMU_OPT_STRING,
> +            .help = "tcp|unix",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static QemuOptsList runtime_unix_opts = {
> +    .name = "gluster_unix",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_unix_opts.head),
> +    .desc = {
> +        {
> +            .name = GLUSTER_OPT_SOCKET,
> +            .type = QEMU_OPT_STRING,
> +            .help = "socket file path)",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static QemuOptsList runtime_tcp_opts = {
> +    .name = "gluster_tcp",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
> +    .desc = {
> +        {
> +            .name = GLUSTER_OPT_TYPE,
> +            .type = QEMU_OPT_STRING,
> +            .help = "tcp|unix",
> +        },
> +        {
> +            .name = GLUSTER_OPT_HOST,
> +            .type = QEMU_OPT_STRING,
> +            .help = "host address (hostname/ipv4/ipv6 addresses)",
> +        },
> +        {
> +            .name = GLUSTER_OPT_PORT,
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "port number on which glusterd is listening (default 24007)",
> +        },
> +        { /* end of list */ }
> +    },
> +};
>  
>  static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
>  {
> @@ -157,7 +236,8 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
>          return -EINVAL;
>      }
>  
> -    gconf->server = gsconf = g_new0(GlusterServer, 1);
> +    gconf->server = g_new0(GlusterServerList, 1);
> +    gconf->server->value = gsconf = g_new0(GlusterServer, 1);
>  
>      /* transport */
>      if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> @@ -211,38 +291,34 @@ out:
>      return ret;
>  }
>  
> -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
> -                                      const char *filename, Error **errp)
> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
> +                                           Error **errp)
>  {
> -    struct glfs *glfs = NULL;
> +    struct glfs *glfs;
>      int ret;
>      int old_errno;
> -
> -    ret = qemu_gluster_parse_uri(gconf, filename);
> -    if (ret < 0) {
> -        error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
> -                         "volume/path[?socket=...]");
> -        errno = -ret;
> -        goto out;
> -    }
> +    GlusterServerList *server;
>  
>      glfs = glfs_new(gconf->volume);
>      if (!glfs) {
>          goto out;
>      }
>  
> -    if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
> -        ret = glfs_set_volfile_server(glfs,
> -                                      GlusterTransport_lookup[gconf->server->type],
> -                                      gconf->server->u.q_unix.socket, 0);
> -    } else {
> -        ret = glfs_set_volfile_server(glfs,
> -                                      GlusterTransport_lookup[gconf->server->type],
> -                                      gconf->server->u.tcp.host,
> -                                      gconf->server->u.tcp.port);
> -    }
> -    if (ret < 0) {
> -        goto out;
> +    for (server = gconf->server; server; server = server->next) {
> +        if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
> +            ret = glfs_set_volfile_server(glfs,
> +                                          GlusterTransport_lookup[server->value->type],
> +                                          server->value->u.q_unix.socket, 0);
> +        } else {
> +            ret = glfs_set_volfile_server(glfs,
> +                                          GlusterTransport_lookup[server->value->type],
> +                                          server->value->u.tcp.host,
> +                                          server->value->u.tcp.port);

server->value.u.tcp.port is optional.  Using it without checking
server->value.u.tcp.has_port relies on the default value being zero.  We
don't actually document that.  Perhaps we should.

> +        }
> +
> +        if (ret < 0) {
> +            goto out;
> +        }
>      }
>  
>      ret = glfs_set_logging(glfs, "-", gconf->debug_level);
> @@ -252,19 +328,19 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>  
>      ret = glfs_init(glfs);
>      if (ret) {
> -        if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
> -            error_setg(errp,
> -                       "Gluster connection for volume: %s, path: %s "
> -                       "failed to connect on socket %s ",
> -                       gconf->volume, gconf->path,
> -                       gconf->server->u.q_unix.socket);
> -        } else {
> -            error_setg(errp,
> -                       "Gluster connection for volume: %s, path: %s "
> -                       "failed to connect  host  %s and port %d ",
> -                       gconf->volume, gconf->path,
> -                       gconf->server->u.tcp.host, gconf->server->u.tcp.port);
> +        error_setg(errp, "Gluster connection for volume: %s, path: %s ",
> +                   gconf->volume, gconf->path);
> +        for (server = gconf->server; server; server = server->next) {
> +            if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
> +                error_append_hint(errp, "failed to connect on socket %s ",
> +                                  server->value->u.q_unix.socket);
> +            } else {
> +                error_append_hint(errp, "failed to connect host %s and port %d ",
> +                                  server->value->u.tcp.host,
> +                                  server->value->u.tcp.port);
> +            }
>          }
> +        error_append_hint(errp, "Please refer to gluster logs for more info ");

Your code produces the error message "Gluster connection for volume:
VOLUME, path: PATH ", which makes no sense.

It also produces a hint that is a concatenation of one or more "failed
to connect on FOO", followed by "Please refer to ..." without any
punctuation, but with a trailing space.

The error message must make sense on its own, without the hint.

A fixed error message could look like this:

    Gluster connection for volume VOLUME, path PATH failed to connect on FOO, on BAR, and on BAZ

or with a little less effort

    Gluster connection for volume VOLUME, path PATH failed to connect on FOO, BAR, BAZ

or simply

    Can't connect to Gluster volume VOLUME, path PATH at FOO, BAR, BAZ

You can't build up the error message with error_append_hint().  Using it
to append a hint pointing to Gluster logs is fine.

>  
>          /* glfs_init sometimes doesn't set errno although docs suggest that */
>          if (errno == 0) {
> @@ -284,6 +360,195 @@ out:
>      return NULL;
>  }
>  
> +static int qapi_enum_parse(const char *opt)
> +{
> +    int i;
> +
> +    if (!opt) {
> +        return GLUSTER_TRANSPORT__MAX;
> +    }
> +
> +    for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
> +        if (!strcmp(opt, GlusterTransport_lookup[i])) {
> +            return i;
> +        }
> +    }
> +
> +    return i;
> +}
> +
> +/*
> + * Convert the json formatted command line into qapi.
> +*/
> +static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> +                                  QDict *options, Error **errp)
> +{
> +    QemuOpts *opts;
> +    GlusterServer *gsconf;
> +    GlusterServerList *curr = NULL;
> +    QDict *backing_options = NULL;
> +    Error *local_err = NULL;
> +    char *str = NULL;
> +    const char *ptr;
> +    size_t num_servers;
> +    int i;
> +
> +    /* create opts info from runtime_json_opts list */
> +    opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
> +    if (num_servers < 1) {
> +        error_setg(&local_err, "please provide 'server' option with valid "
> +                               "fields in array of hostinfo ");

This isn't an error message, it's instructions what to do.  Such
instructions can be useful when they're correct, but they can't replace
an error message.  The error message should state what's wrong.  No
less, no more.

Moreover, avoid prefixes like "qemu_gluster:".  Usually, the fact that
this is about Gluster is obvious.  When it isn't, providing context is
the caller's job.

> +        goto out;
> +    }
> +
> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
> +    if (!ptr) {
> +        error_setg(&local_err, "please provide 'volume' option ");

Not an error message.

> +        goto out;
> +    }
> +    gconf->volume = g_strdup(ptr);
> +
> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
> +    if (!ptr) {
> +        error_setg(&local_err, "please provide 'path' option ");

Not an error message.

> +        goto out;
> +    }
> +    gconf->path = g_strdup(ptr);
> +    qemu_opts_del(opts);
> +
> +    for (i = 0; i < num_servers; i++) {
> +        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
> +        qdict_extract_subqdict(options, &backing_options, str);
> +
> +        /* create opts info from runtime_type_opts list */
> +        opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort);
> +        qemu_opts_absorb_qdict(opts, backing_options, &local_err);
> +        if (local_err) {
> +            goto out;
> +        }
> +
> +        ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
> +        gsconf = g_new0(GlusterServer, 1);
> +        gsconf->type = qapi_enum_parse(ptr);
> +        if (!ptr) {
> +            error_setg(&local_err, "please provide 'type' in hostinfo.%d as "
> +                                   "tcp|unix ", i);

Not an error message.

> +            goto out;
> +
> +        }
> +        if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
> +            error_setg(&local_err, "'type' in hostinfo.%d can be tcp|unix ", i);
> +            goto out;
> +        }
> +        qemu_opts_del(opts);
> +
> +        if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
> +            /* create opts info from runtime_tcp_opts list */
> +            opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
> +            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
> +            if (local_err) {
> +                goto out;
> +            }
> +
> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
> +            if (!ptr) {
> +                error_setg(&local_err, "please provide 'host' in hostinfo.%d ",
> +                           i);
> +                goto out;
> +            }
> +            gsconf->u.tcp.host = g_strdup(ptr);
> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
> +            gsconf->u.tcp.port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
> +                                               GLUSTER_DEFAULT_PORT);
> +            gsconf->u.tcp.has_port = true;
> +            qemu_opts_del(opts);
> +        } else {
> +            /* create opts info from runtime_unix_opts list */
> +            opts = qemu_opts_create(&runtime_unix_opts, NULL, 0, &error_abort);
> +            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
> +            if (local_err) {
> +                goto out;
> +            }
> +
> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
> +            if (!ptr) {
> +                error_setg(&local_err, "please provide 'socket' in hostinfo.%d ",
> +                           i);
> +                goto out;
> +            }
> +            gsconf->u.q_unix.socket = g_strdup(ptr);
> +            qemu_opts_del(opts);
> +        }
> +
> +        if (gconf->server == NULL) {
> +            gconf->server = g_new0(GlusterServerList, 1);
> +            gconf->server->value = gsconf;
> +            curr = gconf->server;
> +        } else {
> +            curr->next = g_new0(GlusterServerList, 1);
> +            curr->next->value = gsconf;
> +            curr = curr->next;
> +        }
> +
> +        qdict_del(backing_options, str);
> +        g_free(str);
> +        str = NULL;
> +    }
> +
> +    return 0;
> +
> +out:
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }

error_propagate() does the right thing when its second argument is
null.  Please drop the conditional.

> +    qemu_opts_del(opts);
> +    if (str) {
> +        qdict_del(backing_options, str);
> +        g_free(str);
> +    }
> +    errno = EINVAL;
> +    return -errno;
> +}
> +
> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
> +                                      const char *filename,
> +                                      QDict *options, Error **errp)
> +{
> +    int ret;
> +    if (filename) {
> +        ret = qemu_gluster_parse_uri(gconf, filename);
> +        if (ret < 0) {
> +            error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
> +                             "volume/path[?socket=...]");

Not an error message.

> +            errno = -ret;
> +            return NULL;
> +        }
> +    } else {
> +        ret = qemu_gluster_parse_json(gconf, options, errp);
> +        if (ret < 0) {
> +            error_append_hint(errp, "Usage: "
> +                             "-drive driver=qcow2,file.driver=gluster,"
> +                             "file.volume=testvol,file.path=/path/a.qcow2"
> +                             "[,file.debug=9],file.server.0.type=tcp,"
> +                             "file.server.0.host=1.2.3.4,"
> +                             "[file.server.0.port=24007,]"
> +                             "file.server.1.transport=unix,"
> +                             "file.server.1.socket=/var/run/glusterd.socket ...");
> +            errno = -ret;
> +            return NULL;
> +        }
> +
> +    }
> +
> +    return qemu_gluster_glfs_init(gconf, errp);
> +}
> +
>  static void qemu_gluster_complete_aio(void *opaque)
>  {
>      GlusterAIOCB *acb = (GlusterAIOCB *)opaque;
> @@ -383,7 +648,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>      gconf = g_new0(BlockdevOptionsGluster, 1);
>      gconf->debug_level = s->debug_level;
>      gconf->has_debug_level = true;
> -    s->glfs = qemu_gluster_init(gconf, filename, errp);
> +    s->glfs = qemu_gluster_init(gconf, filename, options, errp);
>      if (!s->glfs) {
>          ret = -errno;
>          goto out;
> @@ -454,7 +719,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>      gconf = g_new0(BlockdevOptionsGluster, 1);
>      gconf->debug_level = s->debug_level;
>      gconf->has_debug_level = true;
> -    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
> +    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp);
>      if (reop_s->glfs == NULL) {
>          ret = -errno;
>          goto exit;
> @@ -601,7 +866,7 @@ static int qemu_gluster_create(const char *filename,
>      }
>      gconf->has_debug_level = true;
>  
> -    glfs = qemu_gluster_init(gconf, filename, errp);
> +    glfs = qemu_gluster_init(gconf, filename, NULL, errp);
>      if (!glfs) {
>          ret = -errno;
>          goto out;
> @@ -981,7 +1246,7 @@ static BlockDriver bdrv_gluster = {
>      .format_name                  = "gluster",
>      .protocol_name                = "gluster",
>      .instance_size                = sizeof(BDRVGlusterState),
> -    .bdrv_needs_filename          = true,
> +    .bdrv_needs_filename          = false,
>      .bdrv_file_open               = qemu_gluster_open,
>      .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
>      .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
> @@ -1009,7 +1274,7 @@ static BlockDriver bdrv_gluster_tcp = {
>      .format_name                  = "gluster",
>      .protocol_name                = "gluster+tcp",
>      .instance_size                = sizeof(BDRVGlusterState),
> -    .bdrv_needs_filename          = true,
> +    .bdrv_needs_filename          = false,
>      .bdrv_file_open               = qemu_gluster_open,
>      .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
>      .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d7b5c76..5557f1c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2137,7 +2137,7 @@
>  { 'struct': 'BlockdevOptionsGluster',
>    'data': { 'volume': 'str',
>              'path': 'str',
> -            'server': 'GlusterServer',
> +            'server': ['GlusterServer'],
>              '*debug_level': 'int' } }
>  
>  ##

Prasanna Kalever July 18, 2016, 11:51 a.m. UTC | #2

On Mon, Jul 18, 2016 at 3:47 PM, Markus Armbruster <[e-mail]> wrote:
> Prasanna Kumar Kalever <[e-mail]> writes:
>
>> This patch adds a way to specify multiple volfile servers to the gluster
>> block backend of QEMU with tcp|rdma transport types and their port numbers.
>>
>> Problem:
>>
>> Currently VM Image on gluster volume is specified like this:
>>
>> file=gluster[+tcp]://host[:port]/testvol/a.img
>>
>> Say we have three hosts in a trusted pool with replica 3 volume in action.
>> When the host mentioned in the command above goes down for some reason,
>> the other two hosts are still available. But there's currently no way
>> to tell QEMU about them.
>>
>> Solution:
>>
>> New way of specifying VM Image on gluster volume with volfile servers:
>> (We still support old syntax to maintain backward compatibility)
>>
>> Basic command line syntax looks like:
>>
>> Pattern I:
>>  -drive driver=gluster,
>>         volume=testvol,path=/path/a.raw,[debug=N,]
>>         server.0.type=tcp,
>>         server.0.host=1.2.3.4,
>>        [server.0.port=24007,]
>>         server.1.type=unix,
>>         server.1.socket=/path/socketfile
>>
>> Pattern II:
>>  'json:{"driver":"qcow2","file":{"driver":"gluster",
>>        "volume":"testvol","path":"/path/a.qcow2",["debug":N,]
>>        "server":[{hostinfo_1}, ...{hostinfo_N}]}}'
>>
>>    driver      => 'gluster' (protocol name)
>>    volume      => name of gluster volume where our VM image resides
>>    path        => absolute path of image in gluster volume
>>   [debug]      => libgfapi loglevel [(0 - 9) default 4 -> Error]
>>
>>   {hostinfo}   => {{type:"tcp",host:"1.2.3.4"[,port=24007]},
>>                    {type:"unix",socket:"/path/sockfile"}}
>>
>>    type        => transport type used to connect to gluster management daemon,
>>                   it can be tcp|unix
>>    host        => host address (hostname/ipv4/ipv6 addresses/socket path)
>>   [port]       => port number on which glusterd is listening. (default 24007)
>>    socket      => path to socket file
>>
>> Examples:
>> 1.
>>  -drive driver=qcow2,file.driver=gluster,
>>         file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
>>         file.server.0.type=tcp,
>>         file.server.0.host=1.2.3.4,
>>         file.server.0.port=24007,
>>         file.server.1.type=tcp,
>>         file.server.1.socket=/var/run/glusterd.socket
>> 2.
>>  'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
>>          "path":"/path/a.qcow2","debug":9,"server":
>>          [{type:"tcp",host:"1.2.3.4",port=24007},
>>           {type:"unix",socket:"/var/run/glusterd.socket"}] } }'
>>
>> This patch gives a mechanism to provide all the server addresses, which are in
>> replica set, so in case host1 is down VM can still boot from any of the
>> active hosts.
>>
>> This is equivalent to the backup-volfile-servers option supported by
>> mount.glusterfs (FUSE way of mounting gluster volume)
>>
>> credits: sincere thanks to all the supporters
>>
>> Signed-off-by: Prasanna Kumar Kalever <[e-mail]>
>> ---
>>  block/gluster.c      | 347 +++++++++++++++++++++++++++++++++++++++++++++------
>>  qapi/block-core.json |   2 +-
>>  2 files changed, 307 insertions(+), 42 deletions(-)
>>
>> diff --git a/block/gluster.c b/block/gluster.c
>> index ff1e783..fd2279d 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -12,8 +12,16 @@
>>  #include "block/block_int.h"
>>  #include "qapi/error.h"
>>  #include "qemu/uri.h"
>> +#include "qemu/error-report.h"
>>
>>  #define GLUSTER_OPT_FILENAME        "filename"
>> +#define GLUSTER_OPT_VOLUME          "volume"
>> +#define GLUSTER_OPT_PATH            "path"
>> +#define GLUSTER_OPT_TYPE            "type"
>> +#define GLUSTER_OPT_SERVER_PATTERN  "server."
>> +#define GLUSTER_OPT_HOST            "host"
>> +#define GLUSTER_OPT_PORT            "port"
>> +#define GLUSTER_OPT_SOCKET          "socket"
>>  #define GLUSTER_OPT_DEBUG           "debug"
>>  #define GLUSTER_DEFAULT_PORT        24007
>>  #define GLUSTER_DEBUG_DEFAULT       4
>> @@ -82,6 +90,77 @@ static QemuOptsList runtime_opts = {
>>      },
>>  };
>>
>> +static QemuOptsList runtime_json_opts = {
>> +    .name = "gluster_json",
>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = GLUSTER_OPT_VOLUME,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "name of gluster volume where VM image resides",
>> +        },
>> +        {
>> +            .name = GLUSTER_OPT_PATH,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "absolute path to image file in gluster volume",
>> +        },
>> +        {
>> +            .name = GLUSTER_OPT_DEBUG,
>> +            .type = QEMU_OPT_NUMBER,
>> +            .help = "Gluster log level, valid range is 0-9",
>> +        },
>> +        { /* end of list */ }
>> +    },
>> +};
>> +
>> +static QemuOptsList runtime_type_opts = {
>> +    .name = "gluster_type",
>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_type_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = GLUSTER_OPT_TYPE,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "tcp|unix",
>> +        },
>> +        { /* end of list */ }
>> +    },
>> +};
>> +
>> +static QemuOptsList runtime_unix_opts = {
>> +    .name = "gluster_unix",
>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_unix_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = GLUSTER_OPT_SOCKET,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "socket file path)",
>> +        },
>> +        { /* end of list */ }
>> +    },
>> +};
>> +
>> +static QemuOptsList runtime_tcp_opts = {
>> +    .name = "gluster_tcp",
>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = GLUSTER_OPT_TYPE,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "tcp|unix",
>> +        },
>> +        {
>> +            .name = GLUSTER_OPT_HOST,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "host address (hostname/ipv4/ipv6 addresses)",
>> +        },
>> +        {
>> +            .name = GLUSTER_OPT_PORT,
>> +            .type = QEMU_OPT_NUMBER,
>> +            .help = "port number on which glusterd is listening (default 24007)",
>> +        },
>> +        { /* end of list */ }
>> +    },
>> +};
>>
>>  static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
>>  {
>> @@ -157,7 +236,8 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
>>          return -EINVAL;
>>      }
>>
>> -    gconf->server = gsconf = g_new0(GlusterServer, 1);
>> +    gconf->server = g_new0(GlusterServerList, 1);
>> +    gconf->server->value = gsconf = g_new0(GlusterServer, 1);
>>
>>      /* transport */
>>      if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
>> @@ -211,38 +291,34 @@ out:
>>      return ret;
>>  }
>>
>> -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>> -                                      const char *filename, Error **errp)
>> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>> +                                           Error **errp)
>>  {
>> -    struct glfs *glfs = NULL;
>> +    struct glfs *glfs;
>>      int ret;
>>      int old_errno;
>> -
>> -    ret = qemu_gluster_parse_uri(gconf, filename);
>> -    if (ret < 0) {
>> -        error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>> -                         "volume/path[?socket=...]");
>> -        errno = -ret;
>> -        goto out;
>> -    }
>> +    GlusterServerList *server;
>>
>>      glfs = glfs_new(gconf->volume);
>>      if (!glfs) {
>>          goto out;
>>      }
>>
>> -    if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>> -        ret = glfs_set_volfile_server(glfs,
>> -                                      GlusterTransport_lookup[gconf->server->type],
>> -                                      gconf->server->u.q_unix.socket, 0);
>> -    } else {
>> -        ret = glfs_set_volfile_server(glfs,
>> -                                      GlusterTransport_lookup[gconf->server->type],
>> -                                      gconf->server->u.tcp.host,
>> -                                      gconf->server->u.tcp.port);
>> -    }
>> -    if (ret < 0) {
>> -        goto out;
>> +    for (server = gconf->server; server; server = server->next) {
>> +        if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
>> +            ret = glfs_set_volfile_server(glfs,
>> +                                          GlusterTransport_lookup[server->value->type],
>> +                                          server->value->u.q_unix.socket, 0);
>> +        } else {
>> +            ret = glfs_set_volfile_server(glfs,
>> +                                          GlusterTransport_lookup[server->value->type],
>> +                                          server->value->u.tcp.host,
>> +                                          server->value->u.tcp.port);
>
> server->value.u.tcp.port is optional.  Using it without checking
> server->value.u.tcp.has_port relies on the default value being zero.  We
> don't actually document that.  Perhaps we should.

I have made sure to fill it in the code, also marked
gsconf->u.tcp.has_port = true;

>
>> +        }
>> +
>> +        if (ret < 0) {
>> +            goto out;
>> +        }
>>      }
>>
>>      ret = glfs_set_logging(glfs, "-", gconf->debug_level);
>> @@ -252,19 +328,19 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>
>>      ret = glfs_init(glfs);
>>      if (ret) {
>> -        if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>> -            error_setg(errp,
>> -                       "Gluster connection for volume: %s, path: %s "
>> -                       "failed to connect on socket %s ",
>> -                       gconf->volume, gconf->path,
>> -                       gconf->server->u.q_unix.socket);
>> -        } else {
>> -            error_setg(errp,
>> -                       "Gluster connection for volume: %s, path: %s "
>> -                       "failed to connect  host  %s and port %d ",
>> -                       gconf->volume, gconf->path,
>> -                       gconf->server->u.tcp.host, gconf->server->u.tcp.port);
>> +        error_setg(errp, "Gluster connection for volume: %s, path: %s ",
>> +                   gconf->volume, gconf->path);
>> +        for (server = gconf->server; server; server = server->next) {
>> +            if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
>> +                error_append_hint(errp, "failed to connect on socket %s ",
>> +                                  server->value->u.q_unix.socket);
>> +            } else {
>> +                error_append_hint(errp, "failed to connect host %s and port %d ",
>> +                                  server->value->u.tcp.host,
>> +                                  server->value->u.tcp.port);
>> +            }
>>          }
>> +        error_append_hint(errp, "Please refer to gluster logs for more info ");
>
> Your code produces the error message "Gluster connection for volume:
> VOLUME, path: PATH ", which makes no sense.
>
> It also produces a hint that is a concatenation of one or more "failed
> to connect on FOO", followed by "Please refer to ..." without any
> punctuation, but with a trailing space.
>
> The error message must make sense on its own, without the hint.
>
> A fixed error message could look like this:
>
>     Gluster connection for volume VOLUME, path PATH failed to connect on FOO, on BAR, and on BAZ
>
> or with a little less effort
>
>     Gluster connection for volume VOLUME, path PATH failed to connect on FOO, BAR, BAZ
>
> or simply
>
>     Can't connect to Gluster volume VOLUME, path PATH at FOO, BAR, BAZ
>
> You can't build up the error message with error_append_hint().  Using it
> to append a hint pointing to Gluster logs is fine.

okay.

>
>>
>>          /* glfs_init sometimes doesn't set errno although docs suggest that */
>>          if (errno == 0) {
>> @@ -284,6 +360,195 @@ out:
>>      return NULL;
>>  }
>>
>> +static int qapi_enum_parse(const char *opt)
>> +{
>> +    int i;
>> +
>> +    if (!opt) {
>> +        return GLUSTER_TRANSPORT__MAX;
>> +    }
>> +
>> +    for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
>> +        if (!strcmp(opt, GlusterTransport_lookup[i])) {
>> +            return i;
>> +        }
>> +    }
>> +
>> +    return i;
>> +}
>> +
>> +/*
>> + * Convert the json formatted command line into qapi.
>> +*/
>> +static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>> +                                  QDict *options, Error **errp)
>> +{
>> +    QemuOpts *opts;
>> +    GlusterServer *gsconf;
>> +    GlusterServerList *curr = NULL;
>> +    QDict *backing_options = NULL;
>> +    Error *local_err = NULL;
>> +    char *str = NULL;
>> +    const char *ptr;
>> +    size_t num_servers;
>> +    int i;
>> +
>> +    /* create opts info from runtime_json_opts list */
>> +    opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
>> +    qemu_opts_absorb_qdict(opts, options, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>> +
>> +    num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
>> +    if (num_servers < 1) {
>> +        error_setg(&local_err, "please provide 'server' option with valid "
>> +                               "fields in array of hostinfo ");
>
> This isn't an error message, it's instructions what to do.  Such
> instructions can be useful when they're correct, but they can't replace
> an error message.  The error message should state what's wrong.  No
> less, no more.

Let me know, how to log the hint from a leaf function, where Error
object is not created yet.
I also feel its more like an error in the usage of the json command

>
> Moreover, avoid prefixes like "qemu_gluster:".  Usually, the fact that
> this is about Gluster is obvious.  When it isn't, providing context is
> the caller's job.

I think I have removed almost all 'qemu_gluster:' in v19 by respecting
you comments in v18

>
>> +        goto out;
>> +    }
>> +
>> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
>> +    if (!ptr) {
>> +        error_setg(&local_err, "please provide 'volume' option ");
>
> Not an error message.

Also same here ...

>
>> +        goto out;
>> +    }
>> +    gconf->volume = g_strdup(ptr);
>> +
>> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
>> +    if (!ptr) {
>> +        error_setg(&local_err, "please provide 'path' option ");
>
> Not an error message.

here ...

>
>> +        goto out;
>> +    }
>> +    gconf->path = g_strdup(ptr);
>> +    qemu_opts_del(opts);
>> +
>> +    for (i = 0; i < num_servers; i++) {
>> +        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
>> +        qdict_extract_subqdict(options, &backing_options, str);
>> +
>> +        /* create opts info from runtime_type_opts list */
>> +        opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort);
>> +        qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>> +        if (local_err) {
>> +            goto out;
>> +        }
>> +
>> +        ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>> +        gsconf = g_new0(GlusterServer, 1);
>> +        gsconf->type = qapi_enum_parse(ptr);
>> +        if (!ptr) {
>> +            error_setg(&local_err, "please provide 'type' in hostinfo.%d as "
>> +                                   "tcp|unix ", i);
>
> Not an error message.

and here ...
How do I say whats really wrong in the command, which could be long
(if provides N servers in the list)

>
>> +            goto out;
>> +
>> +        }
>> +        if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
>> +            error_setg(&local_err, "'type' in hostinfo.%d can be tcp|unix ", i);
>> +            goto out;
>> +        }
>> +        qemu_opts_del(opts);
>> +
>> +        if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
>> +            /* create opts info from runtime_tcp_opts list */
>> +            opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
>> +            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>> +            if (local_err) {
>> +                goto out;
>> +            }
>> +
>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
>> +            if (!ptr) {
>> +                error_setg(&local_err, "please provide 'host' in hostinfo.%d ",
>> +                           i);
>> +                goto out;
>> +            }
>> +            gsconf->u.tcp.host = g_strdup(ptr);
>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
>> +            gsconf->u.tcp.port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
>> +                                               GLUSTER_DEFAULT_PORT);
>> +            gsconf->u.tcp.has_port = true;
>> +            qemu_opts_del(opts);
>> +        } else {
>> +            /* create opts info from runtime_unix_opts list */
>> +            opts = qemu_opts_create(&runtime_unix_opts, NULL, 0, &error_abort);
>> +            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>> +            if (local_err) {
>> +                goto out;
>> +            }
>> +
>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
>> +            if (!ptr) {
>> +                error_setg(&local_err, "please provide 'socket' in hostinfo.%d ",
>> +                           i);
>> +                goto out;
>> +            }
>> +            gsconf->u.q_unix.socket = g_strdup(ptr);
>> +            qemu_opts_del(opts);
>> +        }
>> +
>> +        if (gconf->server == NULL) {
>> +            gconf->server = g_new0(GlusterServerList, 1);
>> +            gconf->server->value = gsconf;
>> +            curr = gconf->server;
>> +        } else {
>> +            curr->next = g_new0(GlusterServerList, 1);
>> +            curr->next->value = gsconf;
>> +            curr = curr->next;
>> +        }
>> +
>> +        qdict_del(backing_options, str);
>> +        g_free(str);
>> +        str = NULL;
>> +    }
>> +
>> +    return 0;
>> +
>> +out:
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +    }
>
> error_propagate() does the right thing when its second argument is
> null.  Please drop the conditional.

Sure

>
>> +    qemu_opts_del(opts);
>> +    if (str) {
>> +        qdict_del(backing_options, str);
>> +        g_free(str);
>> +    }
>> +    errno = EINVAL;
>> +    return -errno;
>> +}
>> +
>> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>> +                                      const char *filename,
>> +                                      QDict *options, Error **errp)
>> +{
>> +    int ret;
>> +    if (filename) {
>> +        ret = qemu_gluster_parse_uri(gconf, filename);
>> +        if (ret < 0) {
>> +            error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>> +                             "volume/path[?socket=...]");
>
> Not an error message.

Can you suggest the change required here for displaying hints from a
leaf function, I am worried for missing your intention here, since you
have most of your v18 comments here.

IIRC, the leaf function which wants to display out a hint/msg/error
should have an Error object, which is created by error_setg() and
error_append_hind() appends to it.

Sorry for disappointing you Markus.

Thanks,
--
Prasanna


>
>> +            errno = -ret;
>> +            return NULL;
>> +        }
>> +    } else {
>> +        ret = qemu_gluster_parse_json(gconf, options, errp);
>> +        if (ret < 0) {
>> +            error_append_hint(errp, "Usage: "
>> +                             "-drive driver=qcow2,file.driver=gluster,"
>> +                             "file.volume=testvol,file.path=/path/a.qcow2"
>> +                             "[,file.debug=9],file.server.0.type=tcp,"
>> +                             "file.server.0.host=1.2.3.4,"
>> +                             "[file.server.0.port=24007,]"
>> +                             "file.server.1.transport=unix,"
>> +                             "file.server.1.socket=/var/run/glusterd.socket ...");
>> +            errno = -ret;
>> +            return NULL;
>> +        }
>> +
>> +    }
>> +
>> +    return qemu_gluster_glfs_init(gconf, errp);
>> +}
>> +
>>  static void qemu_gluster_complete_aio(void *opaque)
>>  {
>>      GlusterAIOCB *acb = (GlusterAIOCB *)opaque;
>> @@ -383,7 +648,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>>      gconf = g_new0(BlockdevOptionsGluster, 1);
>>      gconf->debug_level = s->debug_level;
>>      gconf->has_debug_level = true;
>> -    s->glfs = qemu_gluster_init(gconf, filename, errp);
>> +    s->glfs = qemu_gluster_init(gconf, filename, options, errp);
>>      if (!s->glfs) {
>>          ret = -errno;
>>          goto out;
>> @@ -454,7 +719,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>>      gconf = g_new0(BlockdevOptionsGluster, 1);
>>      gconf->debug_level = s->debug_level;
>>      gconf->has_debug_level = true;
>> -    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
>> +    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp);
>>      if (reop_s->glfs == NULL) {
>>          ret = -errno;
>>          goto exit;
>> @@ -601,7 +866,7 @@ static int qemu_gluster_create(const char *filename,
>>      }
>>      gconf->has_debug_level = true;
>>
>> -    glfs = qemu_gluster_init(gconf, filename, errp);
>> +    glfs = qemu_gluster_init(gconf, filename, NULL, errp);
>>      if (!glfs) {
>>          ret = -errno;
>>          goto out;
>> @@ -981,7 +1246,7 @@ static BlockDriver bdrv_gluster = {
>>      .format_name                  = "gluster",
>>      .protocol_name                = "gluster",
>>      .instance_size                = sizeof(BDRVGlusterState),
>> -    .bdrv_needs_filename          = true,
>> +    .bdrv_needs_filename          = false,
>>      .bdrv_file_open               = qemu_gluster_open,
>>      .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
>>      .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
>> @@ -1009,7 +1274,7 @@ static BlockDriver bdrv_gluster_tcp = {
>>      .format_name                  = "gluster",
>>      .protocol_name                = "gluster+tcp",
>>      .instance_size                = sizeof(BDRVGlusterState),
>> -    .bdrv_needs_filename          = true,
>> +    .bdrv_needs_filename          = false,
>>      .bdrv_file_open               = qemu_gluster_open,
>>      .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
>>      .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index d7b5c76..5557f1c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2137,7 +2137,7 @@
>>  { 'struct': 'BlockdevOptionsGluster',
>>    'data': { 'volume': 'str',
>>              'path': 'str',
>> -            'server': 'GlusterServer',
>> +            'server': ['GlusterServer'],
>>              '*debug_level': 'int' } }
>>
>>  ##

Markus Armbruster July 18, 2016, 2:39 p.m. UTC | #3

Prasanna Kalever <[e-mail]> writes:

> On Mon, Jul 18, 2016 at 3:47 PM, Markus Armbruster <[e-mail]> wrote:
>> Prasanna Kumar Kalever <[e-mail]> writes:
>>
>>> This patch adds a way to specify multiple volfile servers to the gluster
>>> block backend of QEMU with tcp|rdma transport types and their port numbers.
>>>
>>> Problem:
>>>
>>> Currently VM Image on gluster volume is specified like this:
>>>
>>> file=gluster[+tcp]://host[:port]/testvol/a.img
>>>
>>> Say we have three hosts in a trusted pool with replica 3 volume in action.
>>> When the host mentioned in the command above goes down for some reason,
>>> the other two hosts are still available. But there's currently no way
>>> to tell QEMU about them.
>>>
>>> Solution:
>>>
>>> New way of specifying VM Image on gluster volume with volfile servers:
>>> (We still support old syntax to maintain backward compatibility)
>>>
>>> Basic command line syntax looks like:
>>>
>>> Pattern I:
>>>  -drive driver=gluster,
>>>         volume=testvol,path=/path/a.raw,[debug=N,]
>>>         server.0.type=tcp,
>>>         server.0.host=1.2.3.4,
>>>        [server.0.port=24007,]
>>>         server.1.type=unix,
>>>         server.1.socket=/path/socketfile
>>>
>>> Pattern II:
>>>  'json:{"driver":"qcow2","file":{"driver":"gluster",
>>>        "volume":"testvol","path":"/path/a.qcow2",["debug":N,]
>>>        "server":[{hostinfo_1}, ...{hostinfo_N}]}}'
>>>
>>>    driver      => 'gluster' (protocol name)
>>>    volume      => name of gluster volume where our VM image resides
>>>    path        => absolute path of image in gluster volume
>>>   [debug]      => libgfapi loglevel [(0 - 9) default 4 -> Error]
>>>
>>>   {hostinfo}   => {{type:"tcp",host:"1.2.3.4"[,port=24007]},
>>>                    {type:"unix",socket:"/path/sockfile"}}
>>>
>>>    type        => transport type used to connect to gluster management daemon,
>>>                   it can be tcp|unix
>>>    host        => host address (hostname/ipv4/ipv6 addresses/socket path)
>>>   [port]       => port number on which glusterd is listening. (default 24007)
>>>    socket      => path to socket file
>>>
>>> Examples:
>>> 1.
>>>  -drive driver=qcow2,file.driver=gluster,
>>>         file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
>>>         file.server.0.type=tcp,
>>>         file.server.0.host=1.2.3.4,
>>>         file.server.0.port=24007,
>>>         file.server.1.type=tcp,
>>>         file.server.1.socket=/var/run/glusterd.socket
>>> 2.
>>>  'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
>>>          "path":"/path/a.qcow2","debug":9,"server":
>>>          [{type:"tcp",host:"1.2.3.4",port=24007},
>>>           {type:"unix",socket:"/var/run/glusterd.socket"}] } }'
>>>
>>> This patch gives a mechanism to provide all the server addresses, which are in
>>> replica set, so in case host1 is down VM can still boot from any of the
>>> active hosts.
>>>
>>> This is equivalent to the backup-volfile-servers option supported by
>>> mount.glusterfs (FUSE way of mounting gluster volume)
>>>
>>> credits: sincere thanks to all the supporters
>>>
>>> Signed-off-by: Prasanna Kumar Kalever <[e-mail]>
>>> ---
>>>  block/gluster.c      | 347 +++++++++++++++++++++++++++++++++++++++++++++------
>>>  qapi/block-core.json |   2 +-
>>>  2 files changed, 307 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/block/gluster.c b/block/gluster.c
>>> index ff1e783..fd2279d 100644
>>> --- a/block/gluster.c
>>> +++ b/block/gluster.c
>>> @@ -12,8 +12,16 @@
>>>  #include "block/block_int.h"
>>>  #include "qapi/error.h"
>>>  #include "qemu/uri.h"
>>> +#include "qemu/error-report.h"
>>>
>>>  #define GLUSTER_OPT_FILENAME        "filename"
>>> +#define GLUSTER_OPT_VOLUME          "volume"
>>> +#define GLUSTER_OPT_PATH            "path"
>>> +#define GLUSTER_OPT_TYPE            "type"
>>> +#define GLUSTER_OPT_SERVER_PATTERN  "server."
>>> +#define GLUSTER_OPT_HOST            "host"
>>> +#define GLUSTER_OPT_PORT            "port"
>>> +#define GLUSTER_OPT_SOCKET          "socket"
>>>  #define GLUSTER_OPT_DEBUG           "debug"
>>>  #define GLUSTER_DEFAULT_PORT        24007
>>>  #define GLUSTER_DEBUG_DEFAULT       4
>>> @@ -82,6 +90,77 @@ static QemuOptsList runtime_opts = {
>>>      },
>>>  };
>>>
>>> +static QemuOptsList runtime_json_opts = {
>>> +    .name = "gluster_json",
>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
>>> +    .desc = {
>>> +        {
>>> +            .name = GLUSTER_OPT_VOLUME,
>>> +            .type = QEMU_OPT_STRING,
>>> +            .help = "name of gluster volume where VM image resides",
>>> +        },
>>> +        {
>>> +            .name = GLUSTER_OPT_PATH,
>>> +            .type = QEMU_OPT_STRING,
>>> +            .help = "absolute path to image file in gluster volume",
>>> +        },
>>> +        {
>>> +            .name = GLUSTER_OPT_DEBUG,
>>> +            .type = QEMU_OPT_NUMBER,
>>> +            .help = "Gluster log level, valid range is 0-9",
>>> +        },
>>> +        { /* end of list */ }
>>> +    },
>>> +};
>>> +
>>> +static QemuOptsList runtime_type_opts = {
>>> +    .name = "gluster_type",
>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_type_opts.head),
>>> +    .desc = {
>>> +        {
>>> +            .name = GLUSTER_OPT_TYPE,
>>> +            .type = QEMU_OPT_STRING,
>>> +            .help = "tcp|unix",
>>> +        },
>>> +        { /* end of list */ }
>>> +    },
>>> +};
>>> +
>>> +static QemuOptsList runtime_unix_opts = {
>>> +    .name = "gluster_unix",
>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_unix_opts.head),
>>> +    .desc = {
>>> +        {
>>> +            .name = GLUSTER_OPT_SOCKET,
>>> +            .type = QEMU_OPT_STRING,
>>> +            .help = "socket file path)",
>>> +        },
>>> +        { /* end of list */ }
>>> +    },
>>> +};
>>> +
>>> +static QemuOptsList runtime_tcp_opts = {
>>> +    .name = "gluster_tcp",
>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
>>> +    .desc = {
>>> +        {
>>> +            .name = GLUSTER_OPT_TYPE,
>>> +            .type = QEMU_OPT_STRING,
>>> +            .help = "tcp|unix",
>>> +        },
>>> +        {
>>> +            .name = GLUSTER_OPT_HOST,
>>> +            .type = QEMU_OPT_STRING,
>>> +            .help = "host address (hostname/ipv4/ipv6 addresses)",
>>> +        },
>>> +        {
>>> +            .name = GLUSTER_OPT_PORT,
>>> +            .type = QEMU_OPT_NUMBER,
>>> +            .help = "port number on which glusterd is listening (default 24007)",
>>> +        },
>>> +        { /* end of list */ }
>>> +    },
>>> +};
>>>
>>>  static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
>>>  {
>>> @@ -157,7 +236,8 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
>>>          return -EINVAL;
>>>      }
>>>
>>> -    gconf->server = gsconf = g_new0(GlusterServer, 1);
>>> +    gconf->server = g_new0(GlusterServerList, 1);
>>> +    gconf->server->value = gsconf = g_new0(GlusterServer, 1);
>>>
>>>      /* transport */
>>>      if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
>>> @@ -211,38 +291,34 @@ out:
>>>      return ret;
>>>  }
>>>
>>> -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>> -                                      const char *filename, Error **errp)
>>> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>> +                                           Error **errp)
>>>  {
>>> -    struct glfs *glfs = NULL;
>>> +    struct glfs *glfs;
>>>      int ret;
>>>      int old_errno;
>>> -
>>> -    ret = qemu_gluster_parse_uri(gconf, filename);
>>> -    if (ret < 0) {
>>> -        error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>>> -                         "volume/path[?socket=...]");
>>> -        errno = -ret;
>>> -        goto out;
>>> -    }
>>> +    GlusterServerList *server;
>>>
>>>      glfs = glfs_new(gconf->volume);
>>>      if (!glfs) {
>>>          goto out;
>>>      }
>>>
>>> -    if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>>> -        ret = glfs_set_volfile_server(glfs,
>>> -                                      GlusterTransport_lookup[gconf->server->type],
>>> -                                      gconf->server->u.q_unix.socket, 0);
>>> -    } else {
>>> -        ret = glfs_set_volfile_server(glfs,
>>> -                                      GlusterTransport_lookup[gconf->server->type],
>>> -                                      gconf->server->u.tcp.host,
>>> -                                      gconf->server->u.tcp.port);
>>> -    }
>>> -    if (ret < 0) {
>>> -        goto out;
>>> +    for (server = gconf->server; server; server = server->next) {
>>> +        if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
>>> +            ret = glfs_set_volfile_server(glfs,
>>> +                                          GlusterTransport_lookup[server->value->type],
>>> +                                          server->value->u.q_unix.socket, 0);
>>> +        } else {
>>> +            ret = glfs_set_volfile_server(glfs,
>>> +                                          GlusterTransport_lookup[server->value->type],
>>> +                                          server->value->u.tcp.host,
>>> +                                          server->value->u.tcp.port);
>>
>> server->value.u.tcp.port is optional.  Using it without checking
>> server->value.u.tcp.has_port relies on the default value being zero.  We
>> don't actually document that.  Perhaps we should.
>
> I have made sure to fill it in the code, also marked
> gsconf->u.tcp.has_port = true;
>
>>
>>> +        }
>>> +
>>> +        if (ret < 0) {
>>> +            goto out;
>>> +        }
>>>      }
>>>
>>>      ret = glfs_set_logging(glfs, "-", gconf->debug_level);
>>> @@ -252,19 +328,19 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>>
>>>      ret = glfs_init(glfs);
>>>      if (ret) {
>>> -        if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>>> -            error_setg(errp,
>>> -                       "Gluster connection for volume: %s, path: %s "
>>> -                       "failed to connect on socket %s ",
>>> -                       gconf->volume, gconf->path,
>>> -                       gconf->server->u.q_unix.socket);
>>> -        } else {
>>> -            error_setg(errp,
>>> -                       "Gluster connection for volume: %s, path: %s "
>>> -                       "failed to connect  host  %s and port %d ",
>>> -                       gconf->volume, gconf->path,
>>> -                       gconf->server->u.tcp.host, gconf->server->u.tcp.port);
>>> +        error_setg(errp, "Gluster connection for volume: %s, path: %s ",
>>> +                   gconf->volume, gconf->path);
>>> +        for (server = gconf->server; server; server = server->next) {
>>> +            if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
>>> +                error_append_hint(errp, "failed to connect on socket %s ",
>>> +                                  server->value->u.q_unix.socket);
>>> +            } else {
>>> +                error_append_hint(errp, "failed to connect host %s and port %d ",
>>> +                                  server->value->u.tcp.host,
>>> +                                  server->value->u.tcp.port);
>>> +            }
>>>          }
>>> +        error_append_hint(errp, "Please refer to gluster logs for more info ");
>>
>> Your code produces the error message "Gluster connection for volume:
>> VOLUME, path: PATH ", which makes no sense.
>>
>> It also produces a hint that is a concatenation of one or more "failed
>> to connect on FOO", followed by "Please refer to ..." without any
>> punctuation, but with a trailing space.
>>
>> The error message must make sense on its own, without the hint.
>>
>> A fixed error message could look like this:
>>
>>     Gluster connection for volume VOLUME, path PATH failed to connect on FOO, on BAR, and on BAZ
>>
>> or with a little less effort
>>
>>     Gluster connection for volume VOLUME, path PATH failed to connect on FOO, BAR, BAZ
>>
>> or simply
>>
>>     Can't connect to Gluster volume VOLUME, path PATH at FOO, BAR, BAZ
>>
>> You can't build up the error message with error_append_hint().  Using it
>> to append a hint pointing to Gluster logs is fine.
>
> okay.
>
>>
>>>
>>>          /* glfs_init sometimes doesn't set errno although docs suggest that */
>>>          if (errno == 0) {
>>> @@ -284,6 +360,195 @@ out:
>>>      return NULL;
>>>  }
>>>
>>> +static int qapi_enum_parse(const char *opt)
>>> +{
>>> +    int i;
>>> +
>>> +    if (!opt) {
>>> +        return GLUSTER_TRANSPORT__MAX;
>>> +    }
>>> +
>>> +    for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
>>> +        if (!strcmp(opt, GlusterTransport_lookup[i])) {
>>> +            return i;
>>> +        }
>>> +    }
>>> +
>>> +    return i;
>>> +}
>>> +
>>> +/*
>>> + * Convert the json formatted command line into qapi.
>>> +*/
>>> +static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>> +                                  QDict *options, Error **errp)
>>> +{
>>> +    QemuOpts *opts;
>>> +    GlusterServer *gsconf;
>>> +    GlusterServerList *curr = NULL;
>>> +    QDict *backing_options = NULL;
>>> +    Error *local_err = NULL;
>>> +    char *str = NULL;
>>> +    const char *ptr;
>>> +    size_t num_servers;
>>> +    int i;
>>> +
>>> +    /* create opts info from runtime_json_opts list */
>>> +    opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
>>> +    qemu_opts_absorb_qdict(opts, options, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
>>> +    if (num_servers < 1) {
>>> +        error_setg(&local_err, "please provide 'server' option with valid "
>>> +                               "fields in array of hostinfo ");
>>
>> This isn't an error message, it's instructions what to do.  Such
>> instructions can be useful when they're correct, but they can't replace
>> an error message.  The error message should state what's wrong.  No
>> less, no more.
>
> Let me know, how to log the hint from a leaf function, where Error
> object is not created yet.
> I also feel its more like an error in the usage of the json command

The error we diagnose here is that @options either lacks a 'server' key,
or its value is invalid.  Since I'm too lazy to come up with an error
message for that, I grep for this kind of error (value of
qdict_array_entries() negative), and find one in quorum.c:

    s->num_children = qdict_array_entries(options, "children.");
    if (s->num_children < 0) {
        error_setg(&local_err, "Option children is not a valid array");
        ret = -EINVAL;
        goto exit;
    }

>> Moreover, avoid prefixes like "qemu_gluster:".  Usually, the fact that
>> this is about Gluster is obvious.  When it isn't, providing context is
>> the caller's job.
>
> I think I have removed almost all 'qemu_gluster:' in v19 by respecting
> you comments in v18

Pasto, sorry.

>>> +        goto out;
>>> +    }
>>> +
>>> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
>>> +    if (!ptr) {
>>> +        error_setg(&local_err, "please provide 'volume' option ");
>>
>> Not an error message.
>
> Also same here ...

The usual error message for a missing mandatory key 'volume' is
"Parameter 'volume' is missing".  For historical reasons, that's
commonly written as

    error_setg(errp, QERR_MISSING_PARAMETER, "volume");

If I didn't know that already, I'd try to find out the same way as for
the previous one: grep for this kind of error (value of qemu_opt_get()
null), stick to the most common way to report it.

Feel free to use a message that's more tailored to this particular
error.  My point here isn't that you should reuse existing error
messages (that's a complete non-requirement), only that an error message
should first and foremost tell the user what's wrong.  Telling him what
he might have to do fix it is secondary.  It might be helpful, but in
practice it's often misleading, because users often screw up in more
ways than you anticipated when writing the error message.

>>> +        goto out;
>>> +    }
>>> +    gconf->volume = g_strdup(ptr);
>>> +
>>> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
>>> +    if (!ptr) {
>>> +        error_setg(&local_err, "please provide 'path' option ");
>>
>> Not an error message.
>
> here ...
>
>>
>>> +        goto out;
>>> +    }
>>> +    gconf->path = g_strdup(ptr);
>>> +    qemu_opts_del(opts);
>>> +
>>> +    for (i = 0; i < num_servers; i++) {
>>> +        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
>>> +        qdict_extract_subqdict(options, &backing_options, str);
>>> +
>>> +        /* create opts info from runtime_type_opts list */
>>> +        opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort);
>>> +        qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>> +        if (local_err) {
>>> +            goto out;
>>> +        }
>>> +
>>> +        ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>>> +        gsconf = g_new0(GlusterServer, 1);
>>> +        gsconf->type = qapi_enum_parse(ptr);
>>> +        if (!ptr) {
>>> +            error_setg(&local_err, "please provide 'type' in hostinfo.%d as "
>>> +                                   "tcp|unix ", i);
>>
>> Not an error message.
>
> and here ...
> How do I say whats really wrong in the command, which could be long
> (if provides N servers in the list)

The usual error message for a key 'type' having a value other than 'tcp'
or 'unix' would be "Parameter 'type' expects 'tcp' or 'unix'".  For
historical reasons, that's commonly written as

    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", "tcp or unix");

If I didn't know that already, I'd again grep.

Once again, feel free to improve on this message.

>>> +            goto out;
>>> +
>>> +        }
>>> +        if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
>>> +            error_setg(&local_err, "'type' in hostinfo.%d can be tcp|unix ", i);
>>> +            goto out;
>>> +        }
>>> +        qemu_opts_del(opts);
>>> +
>>> +        if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
>>> +            /* create opts info from runtime_tcp_opts list */
>>> +            opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
>>> +            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>> +            if (local_err) {
>>> +                goto out;
>>> +            }
>>> +
>>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
>>> +            if (!ptr) {
>>> +                error_setg(&local_err, "please provide 'host' in hostinfo.%d ",
>>> +                           i);
>>> +                goto out;
>>> +            }
>>> +            gsconf->u.tcp.host = g_strdup(ptr);
>>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
>>> +            gsconf->u.tcp.port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
>>> +                                               GLUSTER_DEFAULT_PORT);
>>> +            gsconf->u.tcp.has_port = true;
>>> +            qemu_opts_del(opts);
>>> +        } else {
>>> +            /* create opts info from runtime_unix_opts list */
>>> +            opts = qemu_opts_create(&runtime_unix_opts, NULL, 0, &error_abort);
>>> +            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>> +            if (local_err) {
>>> +                goto out;
>>> +            }
>>> +
>>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
>>> +            if (!ptr) {
>>> +                error_setg(&local_err, "please provide 'socket' in hostinfo.%d ",
>>> +                           i);
>>> +                goto out;
>>> +            }
>>> +            gsconf->u.q_unix.socket = g_strdup(ptr);
>>> +            qemu_opts_del(opts);
>>> +        }
>>> +
>>> +        if (gconf->server == NULL) {
>>> +            gconf->server = g_new0(GlusterServerList, 1);
>>> +            gconf->server->value = gsconf;
>>> +            curr = gconf->server;
>>> +        } else {
>>> +            curr->next = g_new0(GlusterServerList, 1);
>>> +            curr->next->value = gsconf;
>>> +            curr = curr->next;
>>> +        }
>>> +
>>> +        qdict_del(backing_options, str);
>>> +        g_free(str);
>>> +        str = NULL;
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +out:
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +    }
>>
>> error_propagate() does the right thing when its second argument is
>> null.  Please drop the conditional.
>
> Sure
>
>>
>>> +    qemu_opts_del(opts);
>>> +    if (str) {
>>> +        qdict_del(backing_options, str);
>>> +        g_free(str);
>>> +    }
>>> +    errno = EINVAL;
>>> +    return -errno;
>>> +}
>>> +
>>> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>> +                                      const char *filename,
>>> +                                      QDict *options, Error **errp)
>>> +{
>>> +    int ret;
>>> +    if (filename) {
>>> +        ret = qemu_gluster_parse_uri(gconf, filename);
>>> +        if (ret < 0) {
>>> +            error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>>> +                             "volume/path[?socket=...]");
>>
>> Not an error message.
>
> Can you suggest the change required here for displaying hints from a
> leaf function, I am worried for missing your intention here, since you
> have most of your v18 comments here.
>
> IIRC, the leaf function which wants to display out a hint/msg/error
> should have an Error object, which is created by error_setg() and
> error_append_hind() appends to it.

The best you can do here is a generic error message like "invalid URI".
To be more specific, you have to create the error in
qemu_gluster_parse_uri(), because only there you know what exactly is
wrong.  Requires giving it an Error **errp parameter.  I'm *not* asking
you for that.  It's a separate improvement.

"invalid URI" isn't as horrible as you might think, because the actual
error message should come out like this:

    qemu-system-x86_64: -drive file=gluster+tcp://junk: invalid URI

which I find clearer than

    qemu-system-x86_64: -drive file=gluster+tcp://junk: Usage: file=gluster[+transport]://[server[:port]]/volname/image[?socket=...]

I reiterate my plea to *test* error messages.

We generally don't print usage information hints after errors.  If we
did, then the code would look like this:

            error_setg(errp, "invalid URI");
            error_append_hint(errp, "Usage: file=gluster[+transport]:"
                              "//[host[:port]]/volume/path[?socket=...]\n");

Should look like this:

    qemu-system-x86_64: -drive file=gluster+tcp://junk: invalid URI
    Usage: file=gluster[+transport]://[host[:port]]/volume/path[?socket=...]

By the way, not sure what [?socket...] means.

> Sorry for disappointing you Markus.

We'll get there :)

Prasanna Kalever July 18, 2016, 7:02 p.m. UTC | #4

On Mon, Jul 18, 2016 at 8:09 PM, Markus Armbruster <[e-mail]> wrote:
> Prasanna Kalever <[e-mail]> writes:
>
>> On Mon, Jul 18, 2016 at 3:47 PM, Markus Armbruster <[e-mail]> wrote:
>>> Prasanna Kumar Kalever <[e-mail]> writes:
>>>
>>>> This patch adds a way to specify multiple volfile servers to the gluster
>>>> block backend of QEMU with tcp|rdma transport types and their port numbers.
>>>>
>>>> Problem:
>>>>
>>>> Currently VM Image on gluster volume is specified like this:
>>>>
>>>> file=gluster[+tcp]://host[:port]/testvol/a.img
>>>>
>>>> Say we have three hosts in a trusted pool with replica 3 volume in action.
>>>> When the host mentioned in the command above goes down for some reason,
>>>> the other two hosts are still available. But there's currently no way
>>>> to tell QEMU about them.
>>>>
>>>> Solution:
>>>>
>>>> New way of specifying VM Image on gluster volume with volfile servers:
>>>> (We still support old syntax to maintain backward compatibility)
>>>>
>>>> Basic command line syntax looks like:
>>>>
>>>> Pattern I:
>>>>  -drive driver=gluster,
>>>>         volume=testvol,path=/path/a.raw,[debug=N,]
>>>>         server.0.type=tcp,
>>>>         server.0.host=1.2.3.4,
>>>>        [server.0.port=24007,]
>>>>         server.1.type=unix,
>>>>         server.1.socket=/path/socketfile
>>>>
>>>> Pattern II:
>>>>  'json:{"driver":"qcow2","file":{"driver":"gluster",
>>>>        "volume":"testvol","path":"/path/a.qcow2",["debug":N,]
>>>>        "server":[{hostinfo_1}, ...{hostinfo_N}]}}'
>>>>
>>>>    driver      => 'gluster' (protocol name)
>>>>    volume      => name of gluster volume where our VM image resides
>>>>    path        => absolute path of image in gluster volume
>>>>   [debug]      => libgfapi loglevel [(0 - 9) default 4 -> Error]
>>>>
>>>>   {hostinfo}   => {{type:"tcp",host:"1.2.3.4"[,port=24007]},
>>>>                    {type:"unix",socket:"/path/sockfile"}}
>>>>
>>>>    type        => transport type used to connect to gluster management daemon,
>>>>                   it can be tcp|unix
>>>>    host        => host address (hostname/ipv4/ipv6 addresses/socket path)
>>>>   [port]       => port number on which glusterd is listening. (default 24007)
>>>>    socket      => path to socket file
>>>>
>>>> Examples:
>>>> 1.
>>>>  -drive driver=qcow2,file.driver=gluster,
>>>>         file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
>>>>         file.server.0.type=tcp,
>>>>         file.server.0.host=1.2.3.4,
>>>>         file.server.0.port=24007,
>>>>         file.server.1.type=tcp,
>>>>         file.server.1.socket=/var/run/glusterd.socket
>>>> 2.
>>>>  'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
>>>>          "path":"/path/a.qcow2","debug":9,"server":
>>>>          [{type:"tcp",host:"1.2.3.4",port=24007},
>>>>           {type:"unix",socket:"/var/run/glusterd.socket"}] } }'
>>>>
>>>> This patch gives a mechanism to provide all the server addresses, which are in
>>>> replica set, so in case host1 is down VM can still boot from any of the
>>>> active hosts.
>>>>
>>>> This is equivalent to the backup-volfile-servers option supported by
>>>> mount.glusterfs (FUSE way of mounting gluster volume)
>>>>
>>>> credits: sincere thanks to all the supporters
>>>>
>>>> Signed-off-by: Prasanna Kumar Kalever <[e-mail]>
>>>> ---
>>>>  block/gluster.c      | 347 +++++++++++++++++++++++++++++++++++++++++++++------
>>>>  qapi/block-core.json |   2 +-
>>>>  2 files changed, 307 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/block/gluster.c b/block/gluster.c
>>>> index ff1e783..fd2279d 100644
>>>> --- a/block/gluster.c
>>>> +++ b/block/gluster.c
>>>> @@ -12,8 +12,16 @@
>>>>  #include "block/block_int.h"
>>>>  #include "qapi/error.h"
>>>>  #include "qemu/uri.h"
>>>> +#include "qemu/error-report.h"
>>>>
>>>>  #define GLUSTER_OPT_FILENAME        "filename"
>>>> +#define GLUSTER_OPT_VOLUME          "volume"
>>>> +#define GLUSTER_OPT_PATH            "path"
>>>> +#define GLUSTER_OPT_TYPE            "type"
>>>> +#define GLUSTER_OPT_SERVER_PATTERN  "server."
>>>> +#define GLUSTER_OPT_HOST            "host"
>>>> +#define GLUSTER_OPT_PORT            "port"
>>>> +#define GLUSTER_OPT_SOCKET          "socket"
>>>>  #define GLUSTER_OPT_DEBUG           "debug"
>>>>  #define GLUSTER_DEFAULT_PORT        24007
>>>>  #define GLUSTER_DEBUG_DEFAULT       4
>>>> @@ -82,6 +90,77 @@ static QemuOptsList runtime_opts = {
>>>>      },
>>>>  };
>>>>
>>>> +static QemuOptsList runtime_json_opts = {
>>>> +    .name = "gluster_json",
>>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
>>>> +    .desc = {
>>>> +        {
>>>> +            .name = GLUSTER_OPT_VOLUME,
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "name of gluster volume where VM image resides",
>>>> +        },
>>>> +        {
>>>> +            .name = GLUSTER_OPT_PATH,
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "absolute path to image file in gluster volume",
>>>> +        },
>>>> +        {
>>>> +            .name = GLUSTER_OPT_DEBUG,
>>>> +            .type = QEMU_OPT_NUMBER,
>>>> +            .help = "Gluster log level, valid range is 0-9",
>>>> +        },
>>>> +        { /* end of list */ }
>>>> +    },
>>>> +};
>>>> +
>>>> +static QemuOptsList runtime_type_opts = {
>>>> +    .name = "gluster_type",
>>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_type_opts.head),
>>>> +    .desc = {
>>>> +        {
>>>> +            .name = GLUSTER_OPT_TYPE,
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "tcp|unix",
>>>> +        },
>>>> +        { /* end of list */ }
>>>> +    },
>>>> +};
>>>> +
>>>> +static QemuOptsList runtime_unix_opts = {
>>>> +    .name = "gluster_unix",
>>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_unix_opts.head),
>>>> +    .desc = {
>>>> +        {
>>>> +            .name = GLUSTER_OPT_SOCKET,
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "socket file path)",
>>>> +        },
>>>> +        { /* end of list */ }
>>>> +    },
>>>> +};
>>>> +
>>>> +static QemuOptsList runtime_tcp_opts = {
>>>> +    .name = "gluster_tcp",
>>>> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
>>>> +    .desc = {
>>>> +        {
>>>> +            .name = GLUSTER_OPT_TYPE,
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "tcp|unix",
>>>> +        },
>>>> +        {
>>>> +            .name = GLUSTER_OPT_HOST,
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "host address (hostname/ipv4/ipv6 addresses)",
>>>> +        },
>>>> +        {
>>>> +            .name = GLUSTER_OPT_PORT,
>>>> +            .type = QEMU_OPT_NUMBER,
>>>> +            .help = "port number on which glusterd is listening (default 24007)",
>>>> +        },
>>>> +        { /* end of list */ }
>>>> +    },
>>>> +};
>>>>
>>>>  static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
>>>>  {
>>>> @@ -157,7 +236,8 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
>>>>          return -EINVAL;
>>>>      }
>>>>
>>>> -    gconf->server = gsconf = g_new0(GlusterServer, 1);
>>>> +    gconf->server = g_new0(GlusterServerList, 1);
>>>> +    gconf->server->value = gsconf = g_new0(GlusterServer, 1);
>>>>
>>>>      /* transport */
>>>>      if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
>>>> @@ -211,38 +291,34 @@ out:
>>>>      return ret;
>>>>  }
>>>>
>>>> -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>>> -                                      const char *filename, Error **errp)
>>>> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>>> +                                           Error **errp)
>>>>  {
>>>> -    struct glfs *glfs = NULL;
>>>> +    struct glfs *glfs;
>>>>      int ret;
>>>>      int old_errno;
>>>> -
>>>> -    ret = qemu_gluster_parse_uri(gconf, filename);
>>>> -    if (ret < 0) {
>>>> -        error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>>>> -                         "volume/path[?socket=...]");
>>>> -        errno = -ret;
>>>> -        goto out;
>>>> -    }
>>>> +    GlusterServerList *server;
>>>>
>>>>      glfs = glfs_new(gconf->volume);
>>>>      if (!glfs) {
>>>>          goto out;
>>>>      }
>>>>
>>>> -    if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>>>> -        ret = glfs_set_volfile_server(glfs,
>>>> -                                      GlusterTransport_lookup[gconf->server->type],
>>>> -                                      gconf->server->u.q_unix.socket, 0);
>>>> -    } else {
>>>> -        ret = glfs_set_volfile_server(glfs,
>>>> -                                      GlusterTransport_lookup[gconf->server->type],
>>>> -                                      gconf->server->u.tcp.host,
>>>> -                                      gconf->server->u.tcp.port);
>>>> -    }
>>>> -    if (ret < 0) {
>>>> -        goto out;
>>>> +    for (server = gconf->server; server; server = server->next) {
>>>> +        if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
>>>> +            ret = glfs_set_volfile_server(glfs,
>>>> +                                          GlusterTransport_lookup[server->value->type],
>>>> +                                          server->value->u.q_unix.socket, 0);
>>>> +        } else {
>>>> +            ret = glfs_set_volfile_server(glfs,
>>>> +                                          GlusterTransport_lookup[server->value->type],
>>>> +                                          server->value->u.tcp.host,
>>>> +                                          server->value->u.tcp.port);
>>>
>>> server->value.u.tcp.port is optional.  Using it without checking
>>> server->value.u.tcp.has_port relies on the default value being zero.  We
>>> don't actually document that.  Perhaps we should.
>>
>> I have made sure to fill it in the code, also marked
>> gsconf->u.tcp.has_port = true;
>>
>>>
>>>> +        }
>>>> +
>>>> +        if (ret < 0) {
>>>> +            goto out;
>>>> +        }
>>>>      }
>>>>
>>>>      ret = glfs_set_logging(glfs, "-", gconf->debug_level);
>>>> @@ -252,19 +328,19 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>>>
>>>>      ret = glfs_init(glfs);
>>>>      if (ret) {
>>>> -        if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>>>> -            error_setg(errp,
>>>> -                       "Gluster connection for volume: %s, path: %s "
>>>> -                       "failed to connect on socket %s ",
>>>> -                       gconf->volume, gconf->path,
>>>> -                       gconf->server->u.q_unix.socket);
>>>> -        } else {
>>>> -            error_setg(errp,
>>>> -                       "Gluster connection for volume: %s, path: %s "
>>>> -                       "failed to connect  host  %s and port %d ",
>>>> -                       gconf->volume, gconf->path,
>>>> -                       gconf->server->u.tcp.host, gconf->server->u.tcp.port);
>>>> +        error_setg(errp, "Gluster connection for volume: %s, path: %s ",
>>>> +                   gconf->volume, gconf->path);
>>>> +        for (server = gconf->server; server; server = server->next) {
>>>> +            if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
>>>> +                error_append_hint(errp, "failed to connect on socket %s ",
>>>> +                                  server->value->u.q_unix.socket);
>>>> +            } else {
>>>> +                error_append_hint(errp, "failed to connect host %s and port %d ",
>>>> +                                  server->value->u.tcp.host,
>>>> +                                  server->value->u.tcp.port);
>>>> +            }
>>>>          }
>>>> +        error_append_hint(errp, "Please refer to gluster logs for more info ");
>>>
>>> Your code produces the error message "Gluster connection for volume:
>>> VOLUME, path: PATH ", which makes no sense.
>>>
>>> It also produces a hint that is a concatenation of one or more "failed
>>> to connect on FOO", followed by "Please refer to ..." without any
>>> punctuation, but with a trailing space.
>>>
>>> The error message must make sense on its own, without the hint.
>>>
>>> A fixed error message could look like this:
>>>
>>>     Gluster connection for volume VOLUME, path PATH failed to connect on FOO, on BAR, and on BAZ
>>>
>>> or with a little less effort
>>>
>>>     Gluster connection for volume VOLUME, path PATH failed to connect on FOO, BAR, BAZ
>>>
>>> or simply
>>>
>>>     Can't connect to Gluster volume VOLUME, path PATH at FOO, BAR, BAZ
>>>
>>> You can't build up the error message with error_append_hint().  Using it
>>> to append a hint pointing to Gluster logs is fine.
>>
>> okay.
>>
>>>
>>>>
>>>>          /* glfs_init sometimes doesn't set errno although docs suggest that */
>>>>          if (errno == 0) {
>>>> @@ -284,6 +360,195 @@ out:
>>>>      return NULL;
>>>>  }
>>>>
>>>> +static int qapi_enum_parse(const char *opt)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    if (!opt) {
>>>> +        return GLUSTER_TRANSPORT__MAX;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
>>>> +        if (!strcmp(opt, GlusterTransport_lookup[i])) {
>>>> +            return i;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return i;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Convert the json formatted command line into qapi.
>>>> +*/
>>>> +static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>>> +                                  QDict *options, Error **errp)
>>>> +{
>>>> +    QemuOpts *opts;
>>>> +    GlusterServer *gsconf;
>>>> +    GlusterServerList *curr = NULL;
>>>> +    QDict *backing_options = NULL;
>>>> +    Error *local_err = NULL;
>>>> +    char *str = NULL;
>>>> +    const char *ptr;
>>>> +    size_t num_servers;
>>>> +    int i;
>>>> +
>>>> +    /* create opts info from runtime_json_opts list */
>>>> +    opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
>>>> +    qemu_opts_absorb_qdict(opts, options, &local_err);
>>>> +    if (local_err) {
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
>>>> +    if (num_servers < 1) {
>>>> +        error_setg(&local_err, "please provide 'server' option with valid "
>>>> +                               "fields in array of hostinfo ");
>>>
>>> This isn't an error message, it's instructions what to do.  Such
>>> instructions can be useful when they're correct, but they can't replace
>>> an error message.  The error message should state what's wrong.  No
>>> less, no more.
>>
>> Let me know, how to log the hint from a leaf function, where Error
>> object is not created yet.
>> I also feel its more like an error in the usage of the json command
>
> The error we diagnose here is that @options either lacks a 'server' key,
> or its value is invalid.  Since I'm too lazy to come up with an error
> message for that, I grep for this kind of error (value of
> qdict_array_entries() negative), and find one in quorum.c:
>
>     s->num_children = qdict_array_entries(options, "children.");
>     if (s->num_children < 0) {
>         error_setg(&local_err, "Option children is not a valid array");
>         ret = -EINVAL;
>         goto exit;
>     }
>
>>> Moreover, avoid prefixes like "qemu_gluster:".  Usually, the fact that
>>> this is about Gluster is obvious.  When it isn't, providing context is
>>> the caller's job.
>>
>> I think I have removed almost all 'qemu_gluster:' in v19 by respecting
>> you comments in v18
>
> Pasto, sorry.
>
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
>>>> +    if (!ptr) {
>>>> +        error_setg(&local_err, "please provide 'volume' option ");
>>>
>>> Not an error message.
>>
>> Also same here ...
>
> The usual error message for a missing mandatory key 'volume' is
> "Parameter 'volume' is missing".  For historical reasons, that's
> commonly written as
>
>     error_setg(errp, QERR_MISSING_PARAMETER, "volume");
>
> If I didn't know that already, I'd try to find out the same way as for
> the previous one: grep for this kind of error (value of qemu_opt_get()
> null), stick to the most common way to report it.
>
> Feel free to use a message that's more tailored to this particular
> error.  My point here isn't that you should reuse existing error
> messages (that's a complete non-requirement), only that an error message
> should first and foremost tell the user what's wrong.  Telling him what
> he might have to do fix it is secondary.  It might be helpful, but in
> practice it's often misleading, because users often screw up in more
> ways than you anticipated when writing the error message.
>
>>>> +        goto out;
>>>> +    }
>>>> +    gconf->volume = g_strdup(ptr);
>>>> +
>>>> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
>>>> +    if (!ptr) {
>>>> +        error_setg(&local_err, "please provide 'path' option ");
>>>
>>> Not an error message.
>>
>> here ...
>>
>>>
>>>> +        goto out;
>>>> +    }
>>>> +    gconf->path = g_strdup(ptr);
>>>> +    qemu_opts_del(opts);
>>>> +
>>>> +    for (i = 0; i < num_servers; i++) {
>>>> +        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
>>>> +        qdict_extract_subqdict(options, &backing_options, str);
>>>> +
>>>> +        /* create opts info from runtime_type_opts list */
>>>> +        opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort);
>>>> +        qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>>> +        if (local_err) {
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>>>> +        gsconf = g_new0(GlusterServer, 1);
>>>> +        gsconf->type = qapi_enum_parse(ptr);
>>>> +        if (!ptr) {
>>>> +            error_setg(&local_err, "please provide 'type' in hostinfo.%d as "
>>>> +                                   "tcp|unix ", i);
>>>
>>> Not an error message.
>>
>> and here ...
>> How do I say whats really wrong in the command, which could be long
>> (if provides N servers in the list)
>
> The usual error message for a key 'type' having a value other than 'tcp'
> or 'unix' would be "Parameter 'type' expects 'tcp' or 'unix'".  For
> historical reasons, that's commonly written as
>
>     error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", "tcp or unix");
>
> If I didn't know that already, I'd again grep.
>
> Once again, feel free to improve on this message.
>
>>>> +            goto out;
>>>> +
>>>> +        }
>>>> +        if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
>>>> +            error_setg(&local_err, "'type' in hostinfo.%d can be tcp|unix ", i);
>>>> +            goto out;
>>>> +        }
>>>> +        qemu_opts_del(opts);
>>>> +
>>>> +        if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
>>>> +            /* create opts info from runtime_tcp_opts list */
>>>> +            opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
>>>> +            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>>> +            if (local_err) {
>>>> +                goto out;
>>>> +            }
>>>> +
>>>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
>>>> +            if (!ptr) {
>>>> +                error_setg(&local_err, "please provide 'host' in hostinfo.%d ",
>>>> +                           i);
>>>> +                goto out;
>>>> +            }
>>>> +            gsconf->u.tcp.host = g_strdup(ptr);
>>>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
>>>> +            gsconf->u.tcp.port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
>>>> +                                               GLUSTER_DEFAULT_PORT);
>>>> +            gsconf->u.tcp.has_port = true;
>>>> +            qemu_opts_del(opts);
>>>> +        } else {
>>>> +            /* create opts info from runtime_unix_opts list */
>>>> +            opts = qemu_opts_create(&runtime_unix_opts, NULL, 0, &error_abort);
>>>> +            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>>> +            if (local_err) {
>>>> +                goto out;
>>>> +            }
>>>> +
>>>> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
>>>> +            if (!ptr) {
>>>> +                error_setg(&local_err, "please provide 'socket' in hostinfo.%d ",
>>>> +                           i);
>>>> +                goto out;
>>>> +            }
>>>> +            gsconf->u.q_unix.socket = g_strdup(ptr);
>>>> +            qemu_opts_del(opts);
>>>> +        }
>>>> +
>>>> +        if (gconf->server == NULL) {
>>>> +            gconf->server = g_new0(GlusterServerList, 1);
>>>> +            gconf->server->value = gsconf;
>>>> +            curr = gconf->server;
>>>> +        } else {
>>>> +            curr->next = g_new0(GlusterServerList, 1);
>>>> +            curr->next->value = gsconf;
>>>> +            curr = curr->next;
>>>> +        }
>>>> +
>>>> +        qdict_del(backing_options, str);
>>>> +        g_free(str);
>>>> +        str = NULL;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +
>>>> +out:
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>
>>> error_propagate() does the right thing when its second argument is
>>> null.  Please drop the conditional.
>>
>> Sure
>>
>>>
>>>> +    qemu_opts_del(opts);
>>>> +    if (str) {
>>>> +        qdict_del(backing_options, str);
>>>> +        g_free(str);
>>>> +    }
>>>> +    errno = EINVAL;
>>>> +    return -errno;
>>>> +}
>>>> +
>>>> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>>> +                                      const char *filename,
>>>> +                                      QDict *options, Error **errp)
>>>> +{
>>>> +    int ret;
>>>> +    if (filename) {
>>>> +        ret = qemu_gluster_parse_uri(gconf, filename);
>>>> +        if (ret < 0) {
>>>> +            error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>>>> +                             "volume/path[?socket=...]");
>>>
>>> Not an error message.
>>
>> Can you suggest the change required here for displaying hints from a
>> leaf function, I am worried for missing your intention here, since you
>> have most of your v18 comments here.
>>
>> IIRC, the leaf function which wants to display out a hint/msg/error
>> should have an Error object, which is created by error_setg() and
>> error_append_hind() appends to it.
>
> The best you can do here is a generic error message like "invalid URI".
> To be more specific, you have to create the error in
> qemu_gluster_parse_uri(), because only there you know what exactly is
> wrong.  Requires giving it an Error **errp parameter.  I'm *not* asking
> you for that.  It's a separate improvement.
>
> "invalid URI" isn't as horrible as you might think, because the actual
> error message should come out like this:
>
>     qemu-system-x86_64: -drive file=gluster+tcp://junk: invalid URI
>
> which I find clearer than
>
>     qemu-system-x86_64: -drive file=gluster+tcp://junk: Usage: file=gluster[+transport]://[server[:port]]/volname/image[?socket=...]
>
> I reiterate my plea to *test* error messages.
>
> We generally don't print usage information hints after errors.  If we
> did, then the code would look like this:
>
>             error_setg(errp, "invalid URI");
>             error_append_hint(errp, "Usage: file=gluster[+transport]:"
>                               "//[host[:port]]/volume/path[?socket=...]\n");
>
> Should look like this:
>
>     qemu-system-x86_64: -drive file=gluster+tcp://junk: invalid URI
>     Usage: file=gluster[+transport]://[host[:port]]/volume/path[?socket=...]
>9

Make sense to me; shall split them to have 'generic msg' (first) +
'what went wrong' (hint next, if required)
first part with error_setg() + error_append_hint() for hints

> By the way, not sure what [?socket...] means.
>

[?socket] is the url query argument, which is unix domain socket file
path, on which glusterd is listening for management connection.

example:
qemu-system-x86_64
gluster+unix:///volume/fedora23.qcow2?socket=/var/run/glusterd.socket

In case, if the volfile resides on local machine, it fetches it form
there without involving the tcp loopback;

--
Prasanna

> Sorry for disappointing you Markus.
>
> We'll get there :)

Patch

diff --git a/block/gluster.c b/block/gluster.c
index ff1e783..fd2279d 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -12,8 +12,16 @@ 
 #include "block/block_int.h"
 #include "qapi/error.h"
 #include "qemu/uri.h"
+#include "qemu/error-report.h"
 
 #define GLUSTER_OPT_FILENAME        "filename"
+#define GLUSTER_OPT_VOLUME          "volume"
+#define GLUSTER_OPT_PATH            "path"
+#define GLUSTER_OPT_TYPE            "type"
+#define GLUSTER_OPT_SERVER_PATTERN  "server."
+#define GLUSTER_OPT_HOST            "host"
+#define GLUSTER_OPT_PORT            "port"
+#define GLUSTER_OPT_SOCKET          "socket"
 #define GLUSTER_OPT_DEBUG           "debug"
 #define GLUSTER_DEFAULT_PORT        24007
 #define GLUSTER_DEBUG_DEFAULT       4
@@ -82,6 +90,77 @@  static QemuOptsList runtime_opts = {
     },
 };
 
+static QemuOptsList runtime_json_opts = {
+    .name = "gluster_json",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
+    .desc = {
+        {
+            .name = GLUSTER_OPT_VOLUME,
+            .type = QEMU_OPT_STRING,
+            .help = "name of gluster volume where VM image resides",
+        },
+        {
+            .name = GLUSTER_OPT_PATH,
+            .type = QEMU_OPT_STRING,
+            .help = "absolute path to image file in gluster volume",
+        },
+        {
+            .name = GLUSTER_OPT_DEBUG,
+            .type = QEMU_OPT_NUMBER,
+            .help = "Gluster log level, valid range is 0-9",
+        },
+        { /* end of list */ }
+    },
+};
+
+static QemuOptsList runtime_type_opts = {
+    .name = "gluster_type",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_type_opts.head),
+    .desc = {
+        {
+            .name = GLUSTER_OPT_TYPE,
+            .type = QEMU_OPT_STRING,
+            .help = "tcp|unix",
+        },
+        { /* end of list */ }
+    },
+};
+
+static QemuOptsList runtime_unix_opts = {
+    .name = "gluster_unix",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_unix_opts.head),
+    .desc = {
+        {
+            .name = GLUSTER_OPT_SOCKET,
+            .type = QEMU_OPT_STRING,
+            .help = "socket file path)",
+        },
+        { /* end of list */ }
+    },
+};
+
+static QemuOptsList runtime_tcp_opts = {
+    .name = "gluster_tcp",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
+    .desc = {
+        {
+            .name = GLUSTER_OPT_TYPE,
+            .type = QEMU_OPT_STRING,
+            .help = "tcp|unix",
+        },
+        {
+            .name = GLUSTER_OPT_HOST,
+            .type = QEMU_OPT_STRING,
+            .help = "host address (hostname/ipv4/ipv6 addresses)",
+        },
+        {
+            .name = GLUSTER_OPT_PORT,
+            .type = QEMU_OPT_NUMBER,
+            .help = "port number on which glusterd is listening (default 24007)",
+        },
+        { /* end of list */ }
+    },
+};
 
 static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
 {
@@ -157,7 +236,8 @@  static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
         return -EINVAL;
     }
 
-    gconf->server = gsconf = g_new0(GlusterServer, 1);
+    gconf->server = g_new0(GlusterServerList, 1);
+    gconf->server->value = gsconf = g_new0(GlusterServer, 1);
 
     /* transport */
     if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
@@ -211,38 +291,34 @@  out:
     return ret;
 }
 
-static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
-                                      const char *filename, Error **errp)
+static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
+                                           Error **errp)
 {
-    struct glfs *glfs = NULL;
+    struct glfs *glfs;
     int ret;
     int old_errno;
-
-    ret = qemu_gluster_parse_uri(gconf, filename);
-    if (ret < 0) {
-        error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
-                         "volume/path[?socket=...]");
-        errno = -ret;
-        goto out;
-    }
+    GlusterServerList *server;
 
     glfs = glfs_new(gconf->volume);
     if (!glfs) {
         goto out;
     }
 
-    if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
-        ret = glfs_set_volfile_server(glfs,
-                                      GlusterTransport_lookup[gconf->server->type],
-                                      gconf->server->u.q_unix.socket, 0);
-    } else {
-        ret = glfs_set_volfile_server(glfs,
-                                      GlusterTransport_lookup[gconf->server->type],
-                                      gconf->server->u.tcp.host,
-                                      gconf->server->u.tcp.port);
-    }
-    if (ret < 0) {
-        goto out;
+    for (server = gconf->server; server; server = server->next) {
+        if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
+            ret = glfs_set_volfile_server(glfs,
+                                          GlusterTransport_lookup[server->value->type],
+                                          server->value->u.q_unix.socket, 0);
+        } else {
+            ret = glfs_set_volfile_server(glfs,
+                                          GlusterTransport_lookup[server->value->type],
+                                          server->value->u.tcp.host,
+                                          server->value->u.tcp.port);
+        }
+
+        if (ret < 0) {
+            goto out;
+        }
     }
 
     ret = glfs_set_logging(glfs, "-", gconf->debug_level);
@@ -252,19 +328,19 @@  static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
 
     ret = glfs_init(glfs);
     if (ret) {
-        if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
-            error_setg(errp,
-                       "Gluster connection for volume: %s, path: %s "
-                       "failed to connect on socket %s ",
-                       gconf->volume, gconf->path,
-                       gconf->server->u.q_unix.socket);
-        } else {
-            error_setg(errp,
-                       "Gluster connection for volume: %s, path: %s "
-                       "failed to connect  host  %s and port %d ",
-                       gconf->volume, gconf->path,
-                       gconf->server->u.tcp.host, gconf->server->u.tcp.port);
+        error_setg(errp, "Gluster connection for volume: %s, path: %s ",
+                   gconf->volume, gconf->path);
+        for (server = gconf->server; server; server = server->next) {
+            if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
+                error_append_hint(errp, "failed to connect on socket %s ",
+                                  server->value->u.q_unix.socket);
+            } else {
+                error_append_hint(errp, "failed to connect host %s and port %d ",
+                                  server->value->u.tcp.host,
+                                  server->value->u.tcp.port);
+            }
         }
+        error_append_hint(errp, "Please refer to gluster logs for more info ");
 
         /* glfs_init sometimes doesn't set errno although docs suggest that */
         if (errno == 0) {
@@ -284,6 +360,195 @@  out:
     return NULL;
 }
 
+static int qapi_enum_parse(const char *opt)
+{
+    int i;
+
+    if (!opt) {
+        return GLUSTER_TRANSPORT__MAX;
+    }
+
+    for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
+        if (!strcmp(opt, GlusterTransport_lookup[i])) {
+            return i;
+        }
+    }
+
+    return i;
+}
+
+/*
+ * Convert the json formatted command line into qapi.
+*/
+static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
+                                  QDict *options, Error **errp)
+{
+    QemuOpts *opts;
+    GlusterServer *gsconf;
+    GlusterServerList *curr = NULL;
+    QDict *backing_options = NULL;
+    Error *local_err = NULL;
+    char *str = NULL;
+    const char *ptr;
+    size_t num_servers;
+    int i;
+
+    /* create opts info from runtime_json_opts list */
+    opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
+    if (num_servers < 1) {
+        error_setg(&local_err, "please provide 'server' option with valid "
+                               "fields in array of hostinfo ");
+        goto out;
+    }
+
+    ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
+    if (!ptr) {
+        error_setg(&local_err, "please provide 'volume' option ");
+        goto out;
+    }
+    gconf->volume = g_strdup(ptr);
+
+    ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
+    if (!ptr) {
+        error_setg(&local_err, "please provide 'path' option ");
+        goto out;
+    }
+    gconf->path = g_strdup(ptr);
+    qemu_opts_del(opts);
+
+    for (i = 0; i < num_servers; i++) {
+        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
+        qdict_extract_subqdict(options, &backing_options, str);
+
+        /* create opts info from runtime_type_opts list */
+        opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort);
+        qemu_opts_absorb_qdict(opts, backing_options, &local_err);
+        if (local_err) {
+            goto out;
+        }
+
+        ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
+        gsconf = g_new0(GlusterServer, 1);
+        gsconf->type = qapi_enum_parse(ptr);
+        if (!ptr) {
+            error_setg(&local_err, "please provide 'type' in hostinfo.%d as "
+                                   "tcp|unix ", i);
+            goto out;
+
+        }
+        if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
+            error_setg(&local_err, "'type' in hostinfo.%d can be tcp|unix ", i);
+            goto out;
+        }
+        qemu_opts_del(opts);
+
+        if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
+            /* create opts info from runtime_tcp_opts list */
+            opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
+            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
+            if (local_err) {
+                goto out;
+            }
+
+            ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
+            if (!ptr) {
+                error_setg(&local_err, "please provide 'host' in hostinfo.%d ",
+                           i);
+                goto out;
+            }
+            gsconf->u.tcp.host = g_strdup(ptr);
+            ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
+            gsconf->u.tcp.port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
+                                               GLUSTER_DEFAULT_PORT);
+            gsconf->u.tcp.has_port = true;
+            qemu_opts_del(opts);
+        } else {
+            /* create opts info from runtime_unix_opts list */
+            opts = qemu_opts_create(&runtime_unix_opts, NULL, 0, &error_abort);
+            qemu_opts_absorb_qdict(opts, backing_options, &local_err);
+            if (local_err) {
+                goto out;
+            }
+
+            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
+            if (!ptr) {
+                error_setg(&local_err, "please provide 'socket' in hostinfo.%d ",
+                           i);
+                goto out;
+            }
+            gsconf->u.q_unix.socket = g_strdup(ptr);
+            qemu_opts_del(opts);
+        }
+
+        if (gconf->server == NULL) {
+            gconf->server = g_new0(GlusterServerList, 1);
+            gconf->server->value = gsconf;
+            curr = gconf->server;
+        } else {
+            curr->next = g_new0(GlusterServerList, 1);
+            curr->next->value = gsconf;
+            curr = curr->next;
+        }
+
+        qdict_del(backing_options, str);
+        g_free(str);
+        str = NULL;
+    }
+
+    return 0;
+
+out:
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+    qemu_opts_del(opts);
+    if (str) {
+        qdict_del(backing_options, str);
+        g_free(str);
+    }
+    errno = EINVAL;
+    return -errno;
+}
+
+static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
+                                      const char *filename,
+                                      QDict *options, Error **errp)
+{
+    int ret;
+    if (filename) {
+        ret = qemu_gluster_parse_uri(gconf, filename);
+        if (ret < 0) {
+            error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
+                             "volume/path[?socket=...]");
+            errno = -ret;
+            return NULL;
+        }
+    } else {
+        ret = qemu_gluster_parse_json(gconf, options, errp);
+        if (ret < 0) {
+            error_append_hint(errp, "Usage: "
+                             "-drive driver=qcow2,file.driver=gluster,"
+                             "file.volume=testvol,file.path=/path/a.qcow2"
+                             "[,file.debug=9],file.server.0.type=tcp,"
+                             "file.server.0.host=1.2.3.4,"
+                             "[file.server.0.port=24007,]"
+                             "file.server.1.transport=unix,"
+                             "file.server.1.socket=/var/run/glusterd.socket ...");
+            errno = -ret;
+            return NULL;
+        }
+
+    }
+
+    return qemu_gluster_glfs_init(gconf, errp);
+}
+
 static void qemu_gluster_complete_aio(void *opaque)
 {
     GlusterAIOCB *acb = (GlusterAIOCB *)opaque;
@@ -383,7 +648,7 @@  static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     gconf = g_new0(BlockdevOptionsGluster, 1);
     gconf->debug_level = s->debug_level;
     gconf->has_debug_level = true;
-    s->glfs = qemu_gluster_init(gconf, filename, errp);
+    s->glfs = qemu_gluster_init(gconf, filename, options, errp);
     if (!s->glfs) {
         ret = -errno;
         goto out;
@@ -454,7 +719,7 @@  static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
     gconf = g_new0(BlockdevOptionsGluster, 1);
     gconf->debug_level = s->debug_level;
     gconf->has_debug_level = true;
-    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
+    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp);
     if (reop_s->glfs == NULL) {
         ret = -errno;
         goto exit;
@@ -601,7 +866,7 @@  static int qemu_gluster_create(const char *filename,
     }
     gconf->has_debug_level = true;
 
-    glfs = qemu_gluster_init(gconf, filename, errp);
+    glfs = qemu_gluster_init(gconf, filename, NULL, errp);
     if (!glfs) {
         ret = -errno;
         goto out;
@@ -981,7 +1246,7 @@  static BlockDriver bdrv_gluster = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_needs_filename          = true,
+    .bdrv_needs_filename          = false,
     .bdrv_file_open               = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
@@ -1009,7 +1274,7 @@  static BlockDriver bdrv_gluster_tcp = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster+tcp",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_needs_filename          = true,
+    .bdrv_needs_filename          = false,
     .bdrv_file_open               = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d7b5c76..5557f1c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2137,7 +2137,7 @@ 
 { 'struct': 'BlockdevOptionsGluster',
   'data': { 'volume': 'str',
             'path': 'str',
-            'server': 'GlusterServer',
+            'server': ['GlusterServer'],
             '*debug_level': 'int' } }
 
 ##