Prasanna Kalever <pkale...@redhat.com> writes:
> On Thu, Jul 14, 2016 at 1:22 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>
>>
>> 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!
Even closer to SocketAddress now.
> 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
If you have questions, ask Eric or me in e-mail or on IRC.
>> 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.
If you want to prepare for future features, don't guess how they'll look
like (we all guess wrong more often than not). Make your interface
extensible instead. QAPI unions are a good tool for that.
>> 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
After discussing this on IRC, I understand that:
* @server specifies a Gluster server
* @volume is effectively the name of a file name space on that server
* @path is a name in that name space
* Together, they specify an image file stored on Gluster.
If that's correct, the design makes sense for QMP.
Is the legacy syntax involving a gluster URI accessible via QMP?
> Thanks Markus
You're welcome!
