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

[Prasanna Kalever]

On Mon, Jul 18, 2016 at 6:41 PM, Markus Armbruster <address@hidden> wrote:
> Prasanna Kalever <address@hidden> writes:
>
>> 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
>
> I know, but the poor reader of the docs may not, so the docs better
> spell it out :)

:) Done

>
>>>> +#
>>>> +# 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:
>
> I've since been gently referred to this note in the cover letter:
>
>     patch 4/5 (i.e. 3/4 in v18) now uses union discriminator, I have
>     made a choice to use gluster with custom schema since
>     @UnixSocketAddress uses 'path' as key, which may be confusing with
>     gluster,
>
> Can you briefly explain why 'path' may be confusing?
>

As you would have noticed below in my previous reply about my ACK

>     and in @InetSocketAddress port was str again I have made a choice to
>     keep it uint16 which really make sense.
>
> A port can be given in symbolic form (service name) and in numeric form
> (port number), just like a host.  For instance, TCP service name "smtp"
> means port number 25.  See also services(5).  Naturally, a symbolic name
> only exists for sufficiently well-known ports.
>
> Interfaces should accept both service name and port.  InetSocketAddress
> does, in the same way as getaddrinfo(): it takes a string, which may
> either be a service name or a port number.  Perhaps it should take an
> alternate of int16 and string instead, but that's a separate question.

This really improved my understanding, thanks Markus
Having agreed that, I need to say about feeder api glfs_setvolfile_server()
accept only int;

look at the scaffolding here

int
pub_glfs_set_volfile_server (struct glfs *fs, const char *transport,
                             const char *host, int port)

So, I hope you stand with me, in making port as int;

>
>     Hmmm.. As Markus suggested in v18 qemu_gluster_parseuri() is
>     *parse_uri() same with *parse_json() (in 5/5)
>
> Not sure I got that.  Do you mean "I renamed qemu_gluster_parseuri() to
> qemu_gluster_parse_uri() for consistency with
> qemu_gluster_parse_json()"?

In a comment you did mentioned "parsejson isn't a word.  "parse_json"
would be two :)"

So to maintain consistency for parseuri with parsejson, I made the changes.

If I have over taken them, please excuse me for this :)

>
>>>
>>> * 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.
>
> Can't say, yet; that's why I ask you to explain why it may be confusing.
>
>>> * 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
>
> Hope I was able to explain this above.

yes!

>
>>>   - 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 ?
>
> Best explained by looking at a use of this feature.  With -display vnc,
> we start a VNC server.  By default, it listens on port 5900.  If this
> port isn't available, it fails like this:
>
>     qemu-system-x86_64:-display vnc=:0: Failed to start VNC server: Failed to 
> bind socket: Address already in use
>
> If you don't care about the port, you can use something like "-display
> vnc=:0,to=9" to bind the first free port in the range of 5900 to 5909.
>

In gluster case arriving at the port that is in use, is non trivial.
Its mostly 24007 or something user choice (say 5007) which is
something unpredictable or looping for that may not worth;

So, IMO this is not the case in gluster port pickup;

>>>   - 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?
>
> In that case, it's not a show-stopper.  When Gluster acquires IPv6
> support, we'll need them.  Until then, we can omit them.  If we don't
> omit them, the gluster driver should reject "ipv4": false.
>

So I am dropping this for now; will send a patch soon after gluster
fully supports ipv6 addressing

>>>     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.
>
> Yes, the library we use to talk to Gluster doesn't let you pass in a
> file descriptor today.
>
> My question is whether this *could* be supported.  The answer is
> probably "yes".
>
> Fd support is usually desirable for privilege separation.  There, we
> want to run QEMU with the least possible privileges.  Ideally no way to
> open TCP connections.  Instead, the management application does the
> opening, and passes the open fd to QEMU.  Makes sense because it limits
> what a malicious guest can gain by attacking QEMU.
>

I got your point here;

since we are clear that gluster doesn't support this at least for now;
Somehow I came to a opinion from all the points described above,  we
don't need 'SocketAddress' for the same reasons that gluster needs a
tweaked 'InetSocketAddress' , so lets also keep-off the fd for now.

{ 'struct': 'InetSocketAddress',
  'data': {
    'host': 'str',
    'port': 'str',
    '*to': 'uint16',
    '*ipv4': 'bool',
    '*ipv6': 'bool' } }

tweaked version of InetSocketAddress i.e GlusterInetSocketAddress
* not require 'ipv6'
* doesn't need 'to' and
* 'port' should be optional and integer, as glfs_set_volfile_server() demands

so,

{ 'struct': 'GlusterInetSocketAddress',
  'data': {
    'host': 'str',
    '*port': 'str' }}

Hence,

{ 'union': 'SocketAddress',
  'data': {
    'inet': 'InetSocketAddress',
    'unix': 'UnixSocketAddress',
    'fd': 'String' } }

after removing 'fd' part which is not supported now, this look like

{ 'union': 'GlusterSocketAddress',
  'data': {
    'inet': 'GlusterInetSocketAddress',
    'unix': 'UnixSocketAddress' } }

What do you think ?

Thanks,
--
Prasanna

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