Re: [PATCH v3 6/8] tap: cpr support

[Markus Armbruster <armbru@redhat.com>]

Ben Chaney <bchaney@akamai.com> writes:

> From: Steve Sistare <steven.sistare@oracle.com>
>
> Provide the cpr=on option to preserve TAP and vhost descriptors during
> cpr-transfer, so the management layer does not need to create a new
> device for the target.
>
> Save all tap fd's in canonical order, leveraging the index argument of
> cpr_save_fd.  For the i'th queue, the tap device fd is saved at index 2*i,
> and the vhostfd (if any) at index 2*i+1.
>
> tap and vhost fd's are passed by name to the monitor when a NIC is hot
> plugged, but the name is not known to qemu after cpr.  Allow the manager
> to pass -1 for the fd "name" in the new qemu args to indicate that QEMU
> should search for a saved value.  Example:
>
>   -netdev tap,id=hostnet2,fds=-1:-1,vhostfds=-1:-1,cpr=on

Hmm.  See below.

>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> Signed-off-by: Ben Chaney <bchaney@akamai.com>

[...]

> diff --git a/qapi/net.json b/qapi/net.json
> index 118bd34965..264213b5d9 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -355,6 +355,8 @@
   ##
   # @NetdevTapOptions:
   #
   # Used to configure a host TAP network interface backend.
   #
   # @ifname: interface name
   #
   # @fd: file descriptor of an already opened tap
   #
   # @fds: multiple file descriptors of already opened multiqueue capable
   #     tap

Not this patch's fault: the interface is misguided, and its
documentation inadequate.

@fds is a string of file descriptor names or numbers separated by ':'.
Not documented.  I found out by reading the code.

This violates QAPI design principle "no string parsing".  It should be
an array of strings.

Aside: get_fds() should use g_strsplit().

Your patch extends the syntax to "file descriptor names or numbers or
"-1" separated by ":".  This is problematic.

Before the patch, a file descriptor name or number is interpreted as a
file descriptor number if it starts with a digit.  "-1" doesn't, so it's
interpreted as a file descriptor name.  Yes, "-1" works as file
descriptor name.  I just verified that

    {"execute": "getfd", "arguments": {"fdname": "-1"}}

works by changing 'fdname': 'fdname' to 'fdname': '-1' in
tests/qtest/libqtest.c, and running tests/qtest/dbus-display-test with
QTEST_LOG=/dev/stdout.  The test passes using file descriptor name "-1".

Aside: not restricting the syntax of identifiers to something sensible
like "begin with a letter, and contain only ASCII letters, digits, and
hyphen" is a mistake we've make again and again.

Your patch changes the interpretation of "-1" from "file descriptor
name" to "saved file descriptor".

If it does so regardless of the value of @cpr, then this is an
incompatible change.

We normally require such changes to go through the deprecation process.
We waive that when we're *confident* not doing so will not inconvenience
any users.  Are we here?

If it does so only when @cpr is true, the semantics of "-1" depends on
@cpr.  Yuck!

We can accept "yuck!" when the alternatives are no better.  Have we
considered any?

Regardless, we clearly need to document syntax and semantics of @fds.
Please fix the doc string before this patch, then have this patch update
it.

   #
   # @script: script to initialize the interface
   #
   # @downscript: script to shut down the interface
   #
   # @br: bridge name (since 2.8)
   #
   # @helper: command to execute to configure bridge
   #
   # @sndbuf: send buffer limit.  Understands [TGMKkb] suffixes.
   #
   # @vnet_hdr: enable the IFF_VNET_HDR flag on the tap interface
   #
   # @vhost: enable vhost-net network accelerator
   #
   # @vhostfd: file descriptor of an already opened vhost net device
   #
   # @vhostfds: file descriptors of multiple already opened vhost net
   #     devices

Likewise.

   #
   # @vhostforce: vhost on for non-MSIX virtio guests
   #
   # @queues: number of queues to be created for multiqueue capable tap
   #
>  # @poll-us: maximum number of microseconds that could be spent on busy
>  #     polling for tap (since 2.7)
>  #
> +# @cpr: preserve fds and vhostfds during cpr-transfer.

The commit message explains things in a lot more detail.  Users may not
need to know all that detail.  But this feels too terse.

Please don't abbreviate "file descriptors" to "fds" in documentation
prose.

> +#
>  # Since: 1.2
>  ##
>  { 'struct': 'NetdevTapOptions',
> @@ -373,7 +375,8 @@
     'data': {
       '*ifname':     'str',
       '*fd':         'str',
       '*fds':        'str',
       '*script':     'str',
       '*downscript': 'str',
       '*br':         'str',
       '*helper':     'str',
       '*sndbuf':     'size',
       '*vnet_hdr':   'bool',
       '*vhost':      'bool',
       '*vhostfd':    'str',
>      '*vhostfds':   'str',
>      '*vhostforce': 'bool',
>      '*queues':     'uint32',
> -    '*poll-us':    'uint32'} }
> +    '*poll-us':    'uint32',
> +    '*cpr':        'bool'} }
>  
>  ##
>  # @NetdevSocketOptions: