[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema |
Date: |
Thu, 12 Nov 2015 15:44:44 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
- [Qemu-devel] [PATCH 0/4] block/gluster: add support for multiple gluster servers, Prasanna Kumar Kalever, 2015/11/12
- [Qemu-devel] [PATCH 1/4] block/gluster: rename [server, volname, image] -> [host, volume, path], Prasanna Kumar Kalever, 2015/11/12
- [Qemu-devel] [PATCH 2/4] block/gluster: code cleanup, Prasanna Kumar Kalever, 2015/11/12
- [Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema, Prasanna Kumar Kalever, 2015/11/12
- [Qemu-devel] [PATCH 4/4] block/gluster: add support for multiple gluster servers, Prasanna Kumar Kalever, 2015/11/12
- Re: [Qemu-devel] [PATCH 0/4] block/gluster: add support for multiple gluster servers, Eric Blake, 2015/11/12