Re: [Qemu-devel] [PATCH 2/2] nbd-client: enable TCP keepalive

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3006 bytes --]

On 6/5/19 12:36 PM, Daniel P. Berrangé wrote:

>>
>> Ok.
>>
>> One more thing to discuss then. Should I add keepalive directly to BlockdevOptionsNbd?
>>
>> Seems more useful to put it into SocketAddress, to be reused by other socket users..
>> But "SocketAddress" sounds like address, not like address+connection-options. On
>> the other hand, structure names are not part of API. So, finally, is InetSocketAddress
>> a good place for such thing?
> 
> That's an interesting idea. Using InetSocketAddress would mean that we could
> get support for this enabled "for free" everywhere in QEMU that uses an
> InetSocketAddress as its master config format.

I like the idea as well.

> 
> Of course there's plenty of places not using InetSocketAddress that would
> still require some glue to wire up the code which converts the custom
> format into InetSocketAddress

Hmm - how many places are we using InetSocketAddress (which allows an
optional 'to' port value) when we really meant InetSocketAddressBase?
There may be some interesting hierarchy decisions to consider on where
we stick a keepalive option.

This also made me wonder if we should start a deprecation clock to
improve the nbd-server-start command to use SocketAddress instead of
SocketAddressLegacy.  If we revive Max's work on implementing a default
branch for a union discriminator
(https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01682.html),
we could have something like:

{ 'enum': 'NbdSocketAddressHack',
  'data': [ 'legacy', 'inet', 'unix' ] }
{ 'struct': 'NbdServerAddrLegacy',
  'data': { 'addr', 'SocketAddressLegacy' } }
{ 'union': 'NbdServerAddr',
  'base': { 'type': 'NbdSocketAddressHack',
            '*tls-creds': 'str',
            '*tls-authz': 'str' },
  'discriminator': 'type',
  'default-variant': 'legacy',
  'data': { 'legacy': 'NbdServerAddrLegacy',
            'inet', 'InetSocketAddress',
            'unix', 'UnixSocketAddress' } }
{ 'command', 'nbd-server-start', 'data': 'NbdServerAddr' }

which should be backwards compatible with the existing:

{ "execute": "nbd-server-start", "arguments": {
    "tls-authz": "authz0",
    "addr": { "type": "inet", "data": {
      "host": "localhost", "port": "10809" } } } }

by relying on the discriminator's default expansion to:

{ "execute": "nbd-server-start", "arguments": {
    "tls-authz": "authz0",
    "type": "legacy",
    "addr": { "type": "inet", "data": {
      "host": "localhost", "port": "10809" } } } }

but also permit the flatter:

{ "execute": "nbd-server-start", "arguments": {
    "tls-authz": "authz0",
    "type": "inet", "host": "localhost", "port": "10809" } }

and let us start a deprecation clock to get rid of the "legacy" branch
(especially if coupled with Kevin's work on adding introspectable
deprecation annotations in QAPI).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]