On Mon, Jul 18, 2016 at 2:59 PM, Markus Armbruster <arm...@redhat.com> wrote:
> 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?
#define GLUSTER_DEFAULT_PORT 24007
>
>> +#
>> +# 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?
May be you are right, it may not worth just for a member name.
>
> * GlusterInetSocketAddress vs. InetSocketAddress
>
> Changes in GlusterInetSocketAddress over InetSocketAddress:
>
> - @port is optional
>
> Convenience feature. We can discuss making it optional in
> InetSocketAddress, too.
sure.
>
> - @port has type 'uint16' instead of 'str'
>
> No support for service names. Why is that a good idea?
I honestly do not understand tie service names to port.
As far all I understand at least from gluster use-case wise its just
an integer ranging from 0 - 65535 (mostly 24007)
I am happy to learn this
>
> - 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.
I still don't understand the essence, why we need to loop through the ports ?
>
> - Lacks @ipv4, @ipv6
Gluster don't support ipv6 addresses (there is some work needed in it rpc code)
Do you think its worth to have a control over ipv4/ipv6 just to
restrict from ipv6 addresses?
>
> No control over IPv4 vs. IPv6 use. Immediate show-stopper.
>
> Can we use InetSocketAddress?
sorry, I don't have an answer, since I was unclear in my comments above.
>
> * GlusterServer vs. SocketAddress
>
> GlusterServer lacks case 'fd'. Do file descriptors make no sense
> here, or is it merely an implementation restriction?
Again, Gluster doesn't support.
>
> 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',
Thanks Markus.
--
Prasanna
