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

[Markus Armbruster <armbru@redhat.com>]

Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes:

> this patch adds 'GlusterServer' related schema in qapi/block-core.json
>
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

QAPI/QMP interface review only.

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ac8f5f6..f8b38bb 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1634,13 +1634,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
> @@ -2033,6 +2034,62 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @GlusterTransport
> +#
> +# An enumeration of Gluster transport type
> +#
> +# @tcp:   TCP   - Transmission Control Protocol
> +#
> +# @unix:  UNIX  - Unix domain socket
> +#
> +# @rdma:  RDMA  - Remote direct memory access
> +#
> +# Since: 2.7
> +##
> +{ 'enum': 'GlusterTransport', 'data': [ 'tcp', 'unix', 'rdma'] }
> +
> +##
> +# @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.7
> +##
> +{ 'struct': 'GlusterServer',
> +  'data': { 'host': 'str',
> +            '*port': 'uint16',
> +            '*transport': 'GlusterTransport' } }

The meaning of @host and @port is obvious enough with @transport 'tcp',
and your documentation matches it.

Not the case for @transport 'unix'.  There is no such thing as 'host'
and 'port' with Unix domain sockets.  I'm afraid the interface makes no
sense in its current state.

I'm not familiar with RDMA, so I can't say whether your definition makes
sense there.  I can say that you shouldn't assume people are familiar
with RDMA.  Please explain what @host and @port mean there.  If they're
just like for 'tcp', say so.

For 'tcp', GlusterTransport duplicates InetSocketAddress, except it
doesn't support service names (@port is 'uint16' instead of 'str'),
doesn't support port ranges (no @to; not needed here), and doesn't
support controlling IPv4/IPv6 (no @ipv4, @ipv6).

Why is it necessary to roll your own address type?  Why can't you use
SocketAddress?

> +
> +##
> +# @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: #libgfapi log level (default '4' which is Error)
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volume': 'str',
> +            'path': 'str',
> +            'server': 'GlusterServer',
> +            '*debug_level': 'int' } }

Are @volume and @path interpreted in any way in QEMU, or are they merely
sent to the Gluster server?

> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.  Many options are available for all
> @@ -2095,7 +2152,7 @@
>        'file':       'BlockdevOptionsFile',
>        'ftp':        'BlockdevOptionsFile',
>        'ftps':       'BlockdevOptionsFile',
> -# TODO gluster: Wait for structured options
> +      'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
>        'http':       'BlockdevOptionsFile',