Ben Chaney <[email protected]> writes: > From: Steve Sistare <[email protected]> > > 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 <[email protected]> > Signed-off-by: Ben Chaney <[email protected]> [...] > 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:
