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

[Prasanna Kalever]

On Mon, Jul 18, 2016 at 2:59 PM, Markus Armbruster <address@hidden> wrote:
> 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?

#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