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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5007 bytes --]

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 <prasanna.kalever@redhat.com>
> ---
>  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 <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]