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

[Markus Armbruster]

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

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

    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.

    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()"?

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

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

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

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

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