Prasanna Kumar Kalever <prasanna.kale...@redhat.com> writes:
> this patch adds 'GlusterServer' related schema in qapi/block-core.json
>
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kale...@redhat.com>
> ---
> 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',
