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

[Jeff Cody]

On Tue, Jul 19, 2016 at 04:39:39PM -0400, Jeff Cody wrote:
> On Tue, Jul 19, 2016 at 10:27:32PM +0530, Prasanna Kumar Kalever wrote:
> > this patch adds 'GlusterServer' related schema in qapi/block-core.json
> > 
> > Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> > ---
> >  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);
> > +        }
> >      }
> >  
> >  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],
> 
> Formatting issue found by checkpatch; exceeds 80 char.  Will fix on commit,
> no need for respin.
> 
> > +                                      gconf->server->u.q_unix.path, 0);
> > +    } else {
> > +        ret = glfs_set_volfile_server(glfs,
> > +                                      
> > GlusterTransport_lookup[gconf->server->type],
> 
> Formatting issue found by checkpatch; exceeds 80 char.  Will fix on commit,
> no need for respin.
> 
> 
> > +                                      gconf->server->u.tcp.host,
> > +                                      atoi(gconf->server->u.tcp.port));
> > +    }
> >      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;
> > +        }
> 
> Eric pointed out the errno risk here; as this is pre-existing, nothing to
> hold up this patch (can be fixed in later patch).
> 
> >  
> >          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);
> > +    }
> 
> Per Markus's suggestion, will remove conditional on commit.
> 
> >      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);
> > +    }
> 
> Here too.
> 
> >      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);
> > +    }

I almost missed this one; will remove this conditional as well.

> >      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' } }
> > +
> > +##
> > +# @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)
> > +#
> 
> Per Eric's suggestion, will change this to debug-level on commit.
> 
> > +# 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',
> > -- 
> > 2.7.4
> >
> 
> After fix-ups:
> 
> Reviewed-by: Jeff Cody <address@hidden>