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

[Prasanna Kalever]

On Thu, Jul 14, 2016 at 1:22 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>
>
> 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.

Yes, agreed
I am about do something like

+ { 'struct': 'UnixAddress',
+   'data': {
+       'socket': 'str',
+   } }
+
+ { 'struct': 'TcpAddress',
+   'data': {
+       'host': 'str',
+       'port': 'uint16',
+   } }
+
+ { 'union': 'GlusterServer',
+   'data': {
+       'unix': 'UnixAddress',
+       'Tcp': 'TcpAddress'
+   } }
No rdma!

But I need to tweak them to fit flat union patches @
git://repo.or.cz/qemu/armbru.git qapi-not-next
Currently I am reading the design changes, help in this area is appreciable

> 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.

I shall remove the rdma part because the gluster volfile fetch doesn't
support this,
Sorry for involving the rdma type in all my previous patches, I just
included that in case gluster supports that in future, but it doesn't
seems to be.

To confirm we don't need rdma type here.

>
> 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?

Sure, I shall take a look at InetSocketAddress and 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?

have a look at libglfsapi (IMO which is poorly designed)
glfs_set_volfile_server (struct glfs *fs, const char *transport, const
char *host, int port)

So the @volume and @path are parsed from the command line args and
filled in 'struct glfs' object


Thanks Markus

--
prasanna

>
>> +
>> +##
>>  # @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',