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

[Markus Armbruster]

Prasanna Kumar Kalever <address@hidden> writes:

> this patch adds 'GlusterServer' related schema in qapi/block-core.json
>
> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> ---
>  block/gluster.c      | 111 
> +++++++++++++++++++++++++++++----------------------
>  qapi/block-core.json |  94 ++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 153 insertions(+), 52 deletions(-)
[Skipping ahead to QAPI schema...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index a7fdb28..d7b5c76 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1658,13 +1658,14 @@
>  # @host_device, @host_cdrom: Since 2.1
>  #
>  # Since: 2.0
> +# @gluster: Since 2.7
>  ##
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> -            'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -            'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels',
> -            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
> -            'vmdk', 'vpc', 'vvfat' ] }
> +            'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> +            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> +            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp',
> +            'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>  
>  ##
>  # @BlockdevOptionsFile
> @@ -2057,6 +2058,89 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @GlusterTransport
> +#
> +# An enumeration of Gluster transport type
> +#
> +# @tcp:   TCP   - Transmission Control Protocol
> +#
> +# @unix:  UNIX  - Unix domain socket
> +#
> +# Since: 2.7
> +##
> +{ 'enum': 'GlusterTransport',
> +  'data': [ 'unix', 'tcp' ] }
> +
> +##
> +# @GlusterUnixSocketAddress
> +#
> +# Captures a socket address in the local ("Unix socket") namespace.
> +#
> +# @socket:   absolute path to socket file
> +#
> +# Since 2.7
> +##
> +{ 'struct': 'GlusterUnixSocketAddress',
> +  'data': { 'socket': 'str' } }

Compare:

   ##
   # @UnixSocketAddress
   #
   # Captures a socket address in the local ("Unix socket") namespace.
   #
   # @path: filesystem path to use
   #
   # Since 1.3
   ##
   { 'struct': 'UnixSocketAddress',
     'data': {
       'path': 'str' } }

> +
> +##
> +# @GlusterInetSocketAddress
> +#
> +# Captures a socket address or address range in the Internet namespace.
> +#
> +# @host:  host part of the address
> +#
> +# @port:  #optional port part of the address, or lowest port if @to is 
> present

There is no @to.

What's the default port?

> +#
> +# Since 2.7
> +##
> +{ 'struct': 'GlusterInetSocketAddress',
> +  'data': { 'host': 'str',
> +            '*port': 'uint16' } }

Compare:

   ##
   # @InetSocketAddress
   #
   # Captures a socket address or address range in the Internet namespace.
   #
   # @host: host part of the address
   #
   # @port: port part of the address, or lowest port if @to is present
   #
   # @to: highest port to try
   #
   # @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
   #        #optional
   #
   # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
   #        #optional
   #
   # Since 1.3
   ##
   { 'struct': 'InetSocketAddress',
     'data': {
       'host': 'str',
       'port': 'str',
       '*to': 'uint16',
       '*ipv4': 'bool',
       '*ipv6': 'bool' } }

> +
> +##
> +# @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': 'GlusterUnixSocketAddress',
> +            'tcp': 'GlusterInetSocketAddress' } }
> +

Compare:

   ##
   # @SocketAddress
   #
   # Captures the address of a socket, which could also be a named file 
descriptor
   #
   # Since 1.3
   ##
   { 'union': 'SocketAddress',
     'data': {
       'inet': 'InetSocketAddress',
       'unix': 'UnixSocketAddress',
       'fd': 'String' } }

You cleaned up the confusing abuse of @host as Unix domain socket file
name.  Good.

You're still defining your own QAPI types instead of using the existing
ones.  To see whether that's a good idea, let's compare your definitions
to the existing ones:

* GlusterUnixSocketAddress vs. UnixSocketAddress

  Identical but for the member name.  Why can't we use
  UnixSocketAddress?

* GlusterInetSocketAddress vs. InetSocketAddress

  Changes in GlusterInetSocketAddress over InetSocketAddress:

  - @port is optional

    Convenience feature.  We can discuss making it optional in
    InetSocketAddress, too.

  - @port has type 'uint16' instead of 'str'

    No support for service names.  Why is that a good idea?

  - Lacks @to

    No support for trying a range of ports.  I guess that simply makes
    no sense for a Gluster client.  I guess makes no sense in some other
    uses of InetSocketAddress, too.  Eric has proposed to split off
    InetSocketAddressRange off InetSocketAddress.

  - Lacks @ipv4, @ipv6

    No control over IPv4 vs. IPv6 use.  Immediate show-stopper.

  Can we use InetSocketAddress?

* GlusterServer vs. SocketAddress

  GlusterServer lacks case 'fd'.  Do file descriptors make no sense
  here, or is it merely an implementation restriction?

  GlusterServer is a flat union, SocketAddress is a simple union.  The flat
  unions is nicer on the wire:
      { "type": "tcp", "host": "gluster.example.com", ... }
  vs.
      { "type": "tcp", "data": { "host": "gluster.example.com", ... }

  Perhaps we should use a flat union for new interfaces.

> +##
> +# @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)
> +#
> +# 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 +2203,7 @@
>        'file':       'BlockdevOptionsFile',
>        'ftp':        'BlockdevOptionsFile',
>        'ftps':       'BlockdevOptionsFile',
> -# TODO gluster: Wait for structured options
> +      'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
>        'http':       'BlockdevOptionsFile',