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

Pointing it out here for completeness, even though I first stumbled on
it when reviewing 4/4:

> @@ -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);

This is abusing the 'host' field of GlusterServer to track a socket
path, and ignores the fact that port is meaningless for a
gluster+unix:// connection.

> @@ -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],
> +                                  gconf->server->host, gconf->server->port);

At least gluster itself has the same overloaded abuse of terminology;
I'm hoping that a port of 0 is okay when requesting a "unix"
volfile_server.  [I don't know, because I didn't read the docs for
glfs_set_volfile_server()]

> +##
> +# @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' } }

And my idea on patch 4/4 was that converting this from simple struct to
flat union might be a more realistic view of things (if transport is
'unix', there can't be a port; and rather than abusing the name 'host'
we could use the name 'socket'; similarly for 'rdma') - but without
additional qapi support, I don't know that we can have an optional
'transport' and still have a discriminated union in time for 2.5.

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

signature.asc
Description: OpenPGP digital signature