[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v20 4/5] block/gluster: using new qapi schema
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v20 4/5] block/gluster: using new qapi schema |
Date: |
Tue, 19 Jul 2016 12:38:30 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 07/19/2016 10:57 AM, 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(-)
>
> @@ -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);
> + }
You are throwing away the errno information that used to be reported
here. Is that intentional?
>
> /* glfs_init sometimes doesn't set errno although docs suggest that
> */
> - if (errno == 0)
> + if (errno == 0) {
> errno = EINVAL;
> + }
Pre-existing, but this is too late for messing with errno. You are not
guaranteed that errno is unchanged by error_setg_errno(). You aren't
even guaranteed that errno == 0 after glfs_init() if it wasn't 0 before
entry. You probably want a separate patch that reorders this to:
errno = 0;
ret = glfs_init(glfs);
if (ret) {
if (!errno) {
errno = EINVAL;
}
error_setg...
> +++ b/qapi/block-core.json
> @@ -1658,13 +1658,14 @@
> # @host_device, @host_cdrom: Since 2.1
> #
> # Since: 2.0
> +# @gluster: Since 2.7
I would stick this line a bit earlier:
# @host_device, @host_cdrom: Since 2.1
# @gluster: Since 2.7
#
# Since: 2.0
> @@ -2057,6 +2058,63 @@
> '*read-pattern': 'QuorumReadPattern' } }
>
> ##
> +# @GlusterTransport
> +#
> +# An enumeration of Gluster transport type
Maybe s/type/types/
> +#
> +# @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)
s/debug_level/debug-level/ - all new QAPI interfaces should prefer '-'
over '_' (the generated C code is the same, though)
> +#
> +# 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',
>
Between my findings and Markus', it's up to the maintainer whether to
take this as-is and then fix up the problems with a followup patch, or
whether we need a v21.
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v20 0/5] block/gluster: add support for multiple gluster servers, Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 1/5] block/gluster: rename [server, volname, image] -> [host, volume, path], Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 2/5] block/gluster: code cleanup, Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 3/5] block/gluster: deprecate rdma support, Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 4/5] block/gluster: using new qapi schema, Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 5/5] block/gluster: add support for multiple gluster servers, Prasanna Kumar Kalever, 2016/07/19
- Re: [Qemu-devel] [PATCH v20 0/5] block/gluster: add support for multiple gluster servers, Jeff Cody, 2016/07/19