Re: [Qemu-devel] [PATCH v20 4/5] block/gluster: using new qapi schema

[Markus Armbruster <armbru@redhat.com>]

Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes:

> this patch adds 'GlusterServer' related schema in qapi/block-core.json
>
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---
>  block/gluster.c      | 115 +++++++++++++++++++++++++++++----------------------
>  qapi/block-core.json |  68 +++++++++++++++++++++++++++---
>  2 files changed, 128 insertions(+), 55 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index 8a54ad4..c4ca59e 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -16,6 +16,7 @@
>  
>  #define GLUSTER_OPT_FILENAME        "filename"
>  #define GLUSTER_OPT_DEBUG           "debug"
> +#define GLUSTER_DEFAULT_PORT        24007
>  #define GLUSTER_DEBUG_DEFAULT       4
>  #define GLUSTER_DEBUG_MAX           9
>  
> @@ -40,15 +41,6 @@ typedef struct BDRVGlusterReopenState {
>      struct glfs_fd *fd;
>  } BDRVGlusterReopenState;
>  
> -typedef struct GlusterConf {
> -    char *host;
> -    int port;
> -    char *volume;
> -    char *path;
> -    char *transport;
> -    int debug_level;
> -} GlusterConf;
> -
>  
>  static QemuOptsList qemu_gluster_create_opts = {
>      .name = "qemu-gluster-create-opts",
> @@ -92,18 +84,7 @@ static QemuOptsList runtime_opts = {
>  };
>  
>  
> -static void qemu_gluster_gconf_free(GlusterConf *gconf)
> -{
> -    if (gconf) {
> -        g_free(gconf->host);
> -        g_free(gconf->volume);
> -        g_free(gconf->path);
> -        g_free(gconf->transport);
> -        g_free(gconf);
> -    }
> -}
> -
> -static int parse_volume_options(GlusterConf *gconf, char *path)
> +static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
>  {
>      char *p, *q;
>  
> @@ -160,8 +141,10 @@ static int parse_volume_options(GlusterConf *gconf, char *path)
>   * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img
>   * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
>   */
> -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> +static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
> +                                  const char *filename)
>  {
> +    GlusterServer *gsconf;
>      URI *uri;
>      QueryParams *qp = NULL;
>      bool is_unix = false;
> @@ -172,16 +155,18 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
>          return -EINVAL;
>      }
>  
> +    gconf->server = gsconf = g_new0(GlusterServer, 1);
> +
>      /* transport */
>      if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> -        gconf->transport = g_strdup("tcp");
> +        gsconf->type = GLUSTER_TRANSPORT_TCP;
>      } else if (!strcmp(uri->scheme, "gluster+tcp")) {
> -        gconf->transport = g_strdup("tcp");
> +        gsconf->type = GLUSTER_TRANSPORT_TCP;
>      } else if (!strcmp(uri->scheme, "gluster+unix")) {
> -        gconf->transport = g_strdup("unix");
> +        gsconf->type = GLUSTER_TRANSPORT_UNIX;
>          is_unix = true;
>      } else if (!strcmp(uri->scheme, "gluster+rdma")) {
> -        gconf->transport = g_strdup("tcp");
> +        gsconf->type = GLUSTER_TRANSPORT_TCP;
>          error_report("Warning: rdma feature is not supported falling "
>                       "back to tcp");
>      } else {
> @@ -209,10 +194,14 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
>              ret = -EINVAL;
>              goto out;
>          }
> -        gconf->host = g_strdup(qp->p[0].value);
> +        gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
>      } else {
> -        gconf->host = g_strdup(uri->server ? uri->server : "localhost");
> -        gconf->port = uri->port;
> +        gsconf->u.tcp.host = g_strdup(uri->server ? uri->server : "localhost");
> +        if (uri->port) {
> +            gsconf->u.tcp.port = g_strdup_printf("%d", uri->port);
> +        } else {
> +            gsconf->u.tcp.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
> +        }

I'd prefer something like

            gsconf->u.tcp.port = g_strdup_printf("%d",
                                                 uri->has_port ? uri->port
                                                 : GLUSTER_DEFAULT_PORT);

or with a new variable port:

            port = uri->has_port ? uri->port : GLUSTER_DEFAULT_PORT;
            gsconf->u.tcp.port = g_strdup_printf("%d", port);

Not worth a respin now, we can take it as is.

>      }
>  
>  out:
> @@ -223,17 +212,18 @@ out:
>      return ret;
>  }
>  
> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
> -                                      Error **errp)
> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
> +                                      const char *filename, Error **errp)
>  {
>      struct glfs *glfs = NULL;
>      int ret;
>      int old_errno;
>  
> -    ret = qemu_gluster_parseuri(gconf, filename);
> +    ret = qemu_gluster_parse_uri(gconf, filename);
>      if (ret < 0) {
> -        error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
> -                         "volume/path[?socket=...]");
> +        error_setg(errp, "Invalid URI");
> +        error_append_hint(errp, "Usage: file=gluster[+transport]://"
> +                                "[host[:port]]/volume/path[?socket=...]\n");
>          errno = -ret;
>          goto out;
>      }
> @@ -243,8 +233,16 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
>          goto out;
>      }
>  
> -    ret = glfs_set_volfile_server(glfs, gconf->transport, gconf->host,
> -            gconf->port);
> +    if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
> +        ret = glfs_set_volfile_server(glfs,
> +                                      GlusterTransport_lookup[gconf->server->type],
> +                                      gconf->server->u.q_unix.path, 0);
> +    } else {
> +        ret = glfs_set_volfile_server(glfs,
> +                                      GlusterTransport_lookup[gconf->server->type],
> +                                      gconf->server->u.tcp.host,
> +                                      atoi(gconf->server->u.tcp.port));

atoi() is okay, because its argument can only come from
qemu_gluster_parse_uri(), so atoi() can't fail.  It'll become wrong when
the argument comes from a QMP client.

> +    }
>      if (ret < 0) {
>          goto out;
>      }
> @@ -256,15 +254,22 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
>  
>      ret = glfs_init(glfs);
>      if (ret) {
> -        error_setg_errno(errp, errno,
> -                         "Gluster connection failed for host=%s port=%d "
> -                         "volume=%s path=%s transport=%s", gconf->host,
> -                         gconf->port, gconf->volume, gconf->path,
> -                         gconf->transport);
> +        if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
> +            error_setg(errp,
> +                       "Gluster connection for volume %s, path %s failed on "
> +                       "socket %s ", gconf->volume, gconf->path,
> +                       gconf->server->u.q_unix.path);
> +        } else {
> +            error_setg(errp,
> +                       "Gluster connection for volume %s, path %s failed on "
> +                       "host  %s and port %s ", gconf->volume, gconf->path,
> +                       gconf->server->u.tcp.host, gconf->server->u.tcp.port);
> +        }
>  
>          /* glfs_init sometimes doesn't set errno although docs suggest that */
> -        if (errno == 0)
> +        if (errno == 0) {
>              errno = EINVAL;
> +        }
>  
>          goto out;
>      }
> @@ -352,7 +357,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>      BDRVGlusterState *s = bs->opaque;
>      int open_flags = 0;
>      int ret = 0;
> -    GlusterConf *gconf = g_new0(GlusterConf, 1);
> +    BlockdevOptionsGluster *gconf = NULL;
>      QemuOpts *opts;
>      Error *local_err = NULL;
>      const char *filename;
> @@ -375,7 +380,9 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>          s->debug_level = GLUSTER_DEBUG_MAX;
>      }
>  
> +    gconf = g_new0(BlockdevOptionsGluster, 1);
>      gconf->debug_level = s->debug_level;
> +    gconf->has_debug_level = true;
>      s->glfs = qemu_gluster_init(gconf, filename, errp);
>      if (!s->glfs) {
>          ret = -errno;
> @@ -410,7 +417,9 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>  
>  out:
>      qemu_opts_del(opts);
> -    qemu_gluster_gconf_free(gconf);
> +    if (gconf) {
> +        qapi_free_BlockdevOptionsGluster(gconf);
> +    }
>      if (!ret) {
>          return ret;
>      }
> @@ -429,7 +438,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>      int ret = 0;
>      BDRVGlusterState *s;
>      BDRVGlusterReopenState *reop_s;
> -    GlusterConf *gconf = NULL;
> +    BlockdevOptionsGluster *gconf;
>      int open_flags = 0;
>  
>      assert(state != NULL);
> @@ -442,9 +451,9 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>  
>      qemu_gluster_parse_flags(state->flags, &open_flags);
>  
> -    gconf = g_new0(GlusterConf, 1);
> -
> +    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);
>      if (reop_s->glfs == NULL) {
>          ret = -errno;
> @@ -470,7 +479,9 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>  
>  exit:
>      /* state->opaque will be freed in either the _abort or _commit */
> -    qemu_gluster_gconf_free(gconf);
> +    if (gconf) {
> +        qapi_free_BlockdevOptionsGluster(gconf);
> +    }
>      return ret;
>  }
>  
> @@ -572,14 +583,15 @@ static inline int qemu_gluster_zerofill(struct glfs_fd *fd, int64_t offset,
>  static int qemu_gluster_create(const char *filename,
>                                 QemuOpts *opts, Error **errp)
>  {
> +    BlockdevOptionsGluster *gconf;
>      struct glfs *glfs;
>      struct glfs_fd *fd;
>      int ret = 0;
>      int prealloc = 0;
>      int64_t total_size = 0;
>      char *tmp = NULL;
> -    GlusterConf *gconf = g_new0(GlusterConf, 1);
>  
> +    gconf = g_new0(BlockdevOptionsGluster, 1);
>      gconf->debug_level = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG,
>                                                   GLUSTER_DEBUG_DEFAULT);
>      if (gconf->debug_level < 0) {
> @@ -587,6 +599,7 @@ static int qemu_gluster_create(const char *filename,
>      } else if (gconf->debug_level > GLUSTER_DEBUG_MAX) {
>          gconf->debug_level = GLUSTER_DEBUG_MAX;
>      }
> +    gconf->has_debug_level = true;
>  
>      glfs = qemu_gluster_init(gconf, filename, errp);
>      if (!glfs) {
> @@ -628,7 +641,9 @@ static int qemu_gluster_create(const char *filename,
>      }
>  out:
>      g_free(tmp);
> -    qemu_gluster_gconf_free(gconf);
> +    if (gconf) {
> +        qapi_free_BlockdevOptionsGluster(gconf);
> +    }
>      if (glfs) {
>          glfs_fini(glfs);
>      }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index a7fdb28..1fa0674 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1658,13 +1658,14 @@
>  # @host_device, @host_cdrom: Since 2.1
>  #
>  # Since: 2.0
> +# @gluster: Since 2.7
>  ##
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> -            'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -            'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels',
> -            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
> -            'vmdk', 'vpc', 'vvfat' ] }
> +            'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> +            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> +            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp',
> +            'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>  
>  ##
>  # @BlockdevOptionsFile
> @@ -2057,6 +2058,63 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @GlusterTransport
> +#
> +# An enumeration of Gluster transport type
> +#
> +# @tcp:   TCP   - Transmission Control Protocol
> +#
> +# @unix:  UNIX  - Unix domain socket
> +#
> +# Since: 2.7
> +##
> +{ 'enum': 'GlusterTransport',
> +  'data': [ 'unix', 'tcp' ] }
> +
> +
> +##
> +# @GlusterServer
> +#
> +# Captures the address of a socket
> +#
> +# Details for connecting to a gluster server
> +#
> +# @type:       Transport type used for gluster connection
> +#
> +# @unix:       socket file
> +#
> +# @tcp:        host address and port number
> +#
> +# Since: 2.7
> +##
> +{ 'union': 'GlusterServer',
> +  'base': { 'type': 'GlusterTransport' },
> +  'discriminator': 'type',
> +  'data': { 'unix': 'UnixSocketAddress',
> +            'tcp': 'InetSocketAddress' } }

We might want to add comments describing the relationship to
SocketAddress, but that can be done on top.

> +
> +##
> +# @BlockdevOptionsGluster
> +#
> +# Driver specific block device options for Gluster
> +#
> +# @volume:      name of gluster volume where VM image resides
> +#
> +# @path:        absolute path to image file in gluster volume
> +#
> +# @server:      gluster server description
> +#
> +# @debug_level: #optional libgfapi log level (default '4' which is Error)
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volume': 'str',
> +            'path': 'str',
> +            'server': 'GlusterServer',
> +            '*debug_level': 'int' } }
> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.  Many options are available for all
> @@ -2119,7 +2177,7 @@
>        'file':       'BlockdevOptionsFile',
>        'ftp':        'BlockdevOptionsFile',
>        'ftps':       'BlockdevOptionsFile',
> -# TODO gluster: Wait for structured options
> +      'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
>        'http':       'BlockdevOptionsFile',

Reviewed-by: Markus Armbruster <armbru@redhat.com>