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

[Eric Blake]

On 11/12/2015 03:22 AM, Prasanna Kumar Kalever wrote:
> this patch adds GlusterConf to qapi/block-core.json

Missing a vNN in the subject line.  I think we're up to v14?  But it
doesn't affect what 'git am' will do.

> 
> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> ---
>  block/gluster.c      | 104 
> +++++++++++++++++++++++++--------------------------
>  qapi/block-core.json |  60 +++++++++++++++++++++++++++--
>  2 files changed, 109 insertions(+), 55 deletions(-)
> 

Modulo Jeff's findings,

> diff --git a/block/gluster.c b/block/gluster.c
> index ededda2..615f28b 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c

> -typedef struct GlusterConf {
> -    char *host;
> -    int port;
> -    char *volume;
> -    char *path;
> -    char *transport;
> -} GlusterConf;
> -
> -

So this is the struct being replaced by qapi BlockdevOptionsGluster.
/me jumps ahead to [1] in my review, before continuing here...

I'm back. Looks like your qapi struct matches this nicely, with the
possible exception of what happens if we try to avoid churn by
using/enforcing a 1-element array now rather than converting to array in
patch 4.

> @@ -143,8 +127,10 @@ static int parse_volume_options(GlusterConf *gconf, char 
> *path)
>   * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
>   * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
>   */
> -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> +static int qemu_gluster_parseuri(BlockdevOptionsGluster **pgconf,
> +                                 const char *filename)

I'm not sure from looking at just the signature why you changed from
*gconf to **pgconf; maybe that sort of conversion would have been worth
mentioning in the commit message (a good rule of thumb - if the change
isn't blatantly obvious, then calling it out in the commit message will
prepare reviewers for it).

> @@ -190,13 +180,23 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
>              ret = -EINVAL;
>              goto out;
>          }
> -        gconf->host = g_strdup(qp->p[0].value);
> +        gconf->server->host = g_strdup(qp->p[0].value);
>      } else {
> -        gconf->host = g_strdup(uri->server ? uri->server : "localhost");
> -        gconf->port = uri->port;
> +        gconf->server->host = g_strdup(uri->server ? uri->server : 
> "localhost");
> +        if (uri->port) {
> +            gconf->server->port = uri->port;
> +        } else {
> +            gconf->server->port = GLUSTER_DEFAULT_PORT;
> +        }
> +        gconf->server->has_port = true;
>      }
>  
> +    *pgconf = gconf;

Okay, now I see where the change in signature comes into play - you want
to return a new allocation to the user, but only on success.  But I'm
still not necessarily convinced that you need it.  See more at [3] below.

> +
>  out:
> +    if (ret < 0) {
> +        qapi_free_BlockdevOptionsGluster(gconf);
> +    }
>      if (qp) {
>          query_params_free(qp);
>      }
> @@ -204,14 +204,15 @@ out:
>      return ret;
>  }

Okay, this parseuri conversion is sane.  It will need tweaking in patch
4 to deal with gconf->server becoming a list rather than a single
server, but as long as both patches go in, we should be okay.

>  
> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char 
> *filename,
> -                                      Error **errp)
> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **pgconf,
> +                                      const char *filename, Error **errp)
>  {
> -    struct glfs *glfs = NULL;
> +    struct glfs *glfs;

Jeff already spotted that the change here is spurious.

>      int ret;
>      int old_errno;
> +    BlockdevOptionsGluster *gconf;
>  
> -    ret = qemu_gluster_parseuri(gconf, filename);
> +    ret = qemu_gluster_parseuri(&gconf, filename);
>      if (ret < 0) {
>          error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>                           "volume/path[?socket=...]");
> @@ -224,8 +225,9 @@ 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);
> +    ret = glfs_set_volfile_server(glfs,
> +                                  
> GlusterTransport_lookup[gconf->server->transport],

Line longer than 80 characters; I might have used an intermediate const
char * variable to cut down on the length. But as long as it gets past
scripts/checkpatch.pl, I won't insist on a reformat.

> +                                  gconf->server->host, gconf->server->port);

Ouch - since you aren't validating that gconf->server->port fits in 16
bits, you may be passing something so large that it silently wraps around.

>      if (ret < 0) {
>          goto out;
>      }
> @@ -242,10 +244,10 @@ 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);
> +                         "Gluster connection failed for host=%s port=%ld "

Change to %ld is due to your choice of 'int' for port; had we gone with
'uint16', you could keep %d, and would be slightly better off (the
parser would validate that things fit in 16 bits, rather than you having
to worry about wraparound).

> +                         "volume=%s path=%s transport=%s", 
> gconf->server->host,
> +                         gconf->server->port, gconf->volume, gconf->path,
> +                         GlusterTransport_lookup[gconf->server->transport]);
>  
>          /* glfs_init sometimes doesn't set errno although docs suggest that 
> */
>          if (errno == 0)
> @@ -253,6 +255,7 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, 
> const char *filename,
>  
>          goto out;
>      }
> +    *pgconf = gconf;
>      return glfs;
>  
>  out:

If I'm reading this right, you leaks gconf on failure.  The old code
took gconf as a parameter and modified it in place (so it won't be
freeing anything); the new code creates a local gconf, and then passes
its address to the parseuri helper which allocates, so you must have a
matching free on all code paths that do not pass gconf on to *pgconf.

But if you hadn't changed the signature, then this would be no different
to what it was pre-patch.

> @@ -315,7 +318,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;

[3] The old code allocates storage, then lets the helpers populate it.
The new code relies on the helpers to allocate storage.  But what was
wrong with the old style?  Why can't we just
g_new0(BlockdevOptionsGluster, 1), and pass that gconf to all the
helpers to be modified in place?

See, because you didn't mention it in the commit message, I have to
guess at things that aren't obvious.

>      QemuOpts *opts;
>      Error *local_err = NULL;
>      const char *filename;
> @@ -329,8 +332,7 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict 
> *options,
>      }
>  
>      filename = qemu_opt_get(opts, "filename");
> -
> -    s->glfs = qemu_gluster_init(gconf, filename, errp);
> +    s->glfs = qemu_gluster_init(&gconf, filename, errp);

Why the loss of the blank line?

>      if (!s->glfs) {
>          ret = -errno;
>          goto out;
> @@ -345,7 +347,7 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict 
> *options,
>  
>  out:
>      qemu_opts_del(opts);
> -    qemu_gluster_gconf_free(gconf);
> +    qapi_free_BlockdevOptionsGluster(gconf);
>      if (!ret) {
>          return ret;
>      }
> @@ -363,7 +365,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState 
> *state,
>  {
>      int ret = 0;
>      BDRVGlusterReopenState *reop_s;
> -    GlusterConf *gconf = NULL;
> +    BlockdevOptionsGluster *gconf = NULL;
>      int open_flags = 0;
>  
>      assert(state != NULL);
> @@ -374,9 +376,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState 
> *state,
>  
>      qemu_gluster_parse_flags(state->flags, &open_flags);
>  
> -    gconf = g_new0(GlusterConf, 1);
> -
> -    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
> +    reop_s->glfs = qemu_gluster_init(&gconf, state->bs->filename, errp);

Here, the changed signature merely changes who is allocating gconf.

>      if (reop_s->glfs == NULL) {
>          ret = -errno;
>          goto exit;
> @@ -391,7 +391,7 @@ 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);
> +    qapi_free_BlockdevOptionsGluster(gconf);
>      return ret;
>  }
>  
> @@ -500,15 +500,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 = NULL;
>      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);

I might have made the insertion and deletion at the same line, rather
than floating it up the declarations.  Minor style.

>  
> -    glfs = qemu_gluster_init(gconf, filename, errp);
> +    glfs = qemu_gluster_init(&gconf, filename, errp);
>      if (!glfs) {
>          ret = -errno;
>          goto out;
> @@ -548,7 +548,7 @@ static int qemu_gluster_create(const char *filename,
>      }
>  out:
>      g_free(tmp);
> -    qemu_gluster_gconf_free(gconf);
> +    qapi_free_BlockdevOptionsGluster(gconf);
>      if (glfs) {
>          glfs_fini(glfs);
>      }

Okay, we're at the end. Down to [2] for the conclusion

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 425fdab..bbefe43 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json

[1] I'm reviewing the interface first, then the implementation

> @@ -1375,13 +1375,14 @@
>  # Drivers that are supported in block device operations.
>  #
>  # @host_device, @host_cdrom: Since 2.1
> +# @gluster: Since 2.5
>  #

Not your fault, but this list, and the list for BlockDeviceInfo, are
similar to one another yet not identical.  Doesn't hold up this patch,
but it would be nice to have a unified story some day.

> +##
> +# @GlusterServer
> +#
> +# Details for connecting to a gluster server
> +#
> +# @host:       host address (hostname/ipv4/ipv6 addresses)
> +#
> +# @port:       #optional port number on which glusterd is listening
> +#               (default 24007)
> +#
> +# @transport:  #optional transport type used to connect to gluster management
> +#               daemon (default 'tcp')
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'GlusterServer',
> +  'data': { 'host': 'str',
> +            '*port': 'int',
> +            '*transport': 'GlusterTransport' } }

Looks reasonable.  It seems like we may need similar types for other
networked devices, but hopefully we aren't painting ourselves into a
corner.  'int' is rather large for port, if you wanted to go with
'uint16' instead.

> +
> +##
> +# @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
> +#
> +# @servers:  gluster server description
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volume': 'str',
> +            'path': 'str',
> +            'server': 'GlusterServer' } }

If this patch goes in to 2.5, but 4/4 does not, then you have painted
yourself into a corner.  That's because 4/4 changes this to
'server':['GlusterServer'], which is not a compatible change.  We can
get away with it as long as both patches go into the same release (and
therefore I won't complain too loudly), but I really would have liked to
see this patch use/enforce a one-element array rather than having to
retouch things to convert from single element to array in patch 4.
(That is, your split wasn't quite done along the lines I had envisioned
- but it's not necessarily a show-stopper if patch 4 fixes things.)

[2] Overall, I think the remaining items might be fixable by a
maintainer, rather than forcing a delay due to requiring a respin.

Jeff, if you are comfortable making changes to fix at least the memleak,
or more invasively to undo the change from *gconf to **pgconf in the
various signatures, you can add:
Reviewed-by: Eric Blake <address@hidden>

Or, we can wait for a v15, but that puts getting this into 2.5 at more risk.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature