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

[Markus Armbruster]

Cc'ing Eric, because I'd like his advice on a couple of points.

Prasanna Kalever <address@hidden> writes:

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

If we consider just interfacing with (the current version of)
glusterfs-devel, int is the natural type of port.

However, we already have a related abstraction: InetSocketAddress.  If
it fits, we should probably reuse it.  If we decide not to reuse it now,
we may still want to minimize differences to keep the door open for
future reuse.

InetSocketAddress's port is str.  If we make GlusterInetSocketAddress's
port an integer, then a future unification with InetSocketAddress will
have to make port an alternate.  Not impossible, but why tie our hands
that way now?  But let's not get bogged down in this detail now.  We
first have to decide whether to reuse InetSocketAddress now (more on
that below).  If yes, the question is moot.

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

Makes sense.

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

Nah, I just didn't understand your note, and wanted to make sure I
didn't miss anything.

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

Okay, looks like GlusterUnixSocketAddress is not really necessary.

>>>> * GlusterInetSocketAddress vs. InetSocketAddress
>>>>
>>>>   Changes in GlusterInetSocketAddress over InetSocketAddress:
>>>>
>>>>   - @port is optional
>>>>
>>>>     Convenience feature.  We can discuss making it optional in
>>>>     InetSocketAddress, too.
>>>
>>> sure.

Of course, it's too late in the development cycle for making it optional
*now*.

If we reuse InetSocketAddress now, as is, then the QMP client has to
specify the Gluster port, even though it's usually 24007.  For command
line and HMP, we can still make it optional.  I think that's tolerable.
We can investigate making it optional later.

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

Port ranges make sense for some users of InetSocketAddress, but not for
others.  Gluster is of the latter kind.

So far, the latter kind simply uses InetSocketAddress, and either
rejects @to or ignores it.  The latter would be a bug.

We could do the same for Gluster: if has_to, error out.

Alternatively, we could split InetSocketAddressRange (with @to) off
InetSocketAddress (delete @to).  Makes range support visible in
introspection.

If we use InetSocketAddress as is now, and have gluster.c reject @to, we
can still split off InetSocketAddressRange later.  The external
interface doesn't care whether @to is rejected as unknown parameter by
QAPI or as unsupported parameter by gluster.c.

Remember, the purpose of reviewing differences between
GlusterInetSocketAddress and InetSocketAddressRange is to figure out
whether we can use InetSocketAddress for Gluster, and if we can, whether
we should.  The result here is that @to doesn't preclude reuse of
InetSocketAddress.  We've already seen that @port doesn't preclude it,
either.

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

Like @to, @ipv4 and @ipv6 don't preclude reuse of InetSocketAddress: C
code can simply reject the ones it can't obey.

Now let me summarize.  We could do without GlusterInetSocketAddress,
because:

* SocketAddress's non-optional @port is a minor inconvenience which
  we can address in the future without breaking compatibility.

* SocketAddress's string @port is a minor inconvenience for the C code
  using it.  Keeping the external interface consistent (same type for
  TCP ports everywhere) is worth some inconvenience in the C code.

* SocketAddress's @to complicates the external interface, but the
  complication exists elsewhere already.  gluster.c can reject @to.  We
  can clean up the external interface in the future without breaking
  compatibility.

* SocketAddress's @ipv4, @ipv6 cannot be fully implemented by gluster.c,
  yet.  gluster.c can simply reject the settings it doesn't implement,
  until they get implemented.

Reasons for having a separate GlusterInetSocketAddress:

* Slightly less code in gluster.c.  I reject that reason.  Keeping the
  interface lean is worth some extra code.

  Note that extra schema definitions to avoid code complications may be
  fine as long as they don't affect external interfaces.

* Makes non-support of port ranges and IPv6 visible in introspection.
  That's a valid argument, but it's an argument for having
  Inet4SockAddress, not for GlusterInetSocketAddress.

  Eric, do you think there's a need for introspecting IPv6 support?

Any other reasons?

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

Similar design question as for @ipv4, @ipv6: reuse of SocketAddress is
possible, but the C code has to reject options it doesn't implement,
i.e. "type": "fd".  Non-support of fd isn't visible in introspection
then.

Eric, do you think there's a need for introspecting fd support?

Additional design question: do we want to move away from SocketAddress's
pointless nesting on the wire for new interfaces?  I.e. move from
something like

    { "type": "tcp", "data": { "host": "gluster.example.com", ... }

to

    { "type": "tcp", "host": "gluster.example.com", ... }

This isn't a Gluster question, it's a general question.  Eric, what do
you think?

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

This is fine if

* we decide we want a new GlusterInetSocketAddress instead of reusing
  InetSocketAddress, and

* we decide we don't want to avoid nesting on the wire.

But we haven't settled these questions.  I'd like to settle them today,
taking Eric's advice into account.