On 9/17/20 8:22 AM, Richard W.M. Jones wrote:
On Fri, Sep 11, 2020 at 09:31:11AM -0500, Eric Blake wrote:
As mentioned in commits 176fc4ea and 609c25f0, our original plan in adding a flags argument to nbd_shutdown was to let us specify different behaviors at the libnbd level, rather than NBD protocol flags (for that, the user has nbd_aio_disconnect). But when we later parameterized OFlags to accept various bitmasks (commit f891340b), we failed to mark nbd_shutdown as using a different bitmask than NBD_CMD_FLAG.
I've finally found a use for such a flag. By itself, nbd_aio_disconnect merely queues itself at the back of all pending commands. But if the server is processing responses slowly, it can be desirable to elevate a disconnect request to the front of the queue, intentionally abandoning all subsequent commands that have not already started on their way to the server.
Fortunately, we have been rejecting all flag values, so changing the type of the OFlags argument now has no impact to either the C API or the ABI. Other language bindings that are more strongly-typed will see a different enum, but we don't promise compatibility there, and even then, languages that permit omitting the flags argument in favor of a default don't see any difference for client that were omitting the argument in favor of the default.
I think for the reasons you've outlined here it's fine to change this from cmd_flags to shutdown_flags. And the new flag looks useful.
Patch looks good, so ACK.
I'm leaning towards having the flag value be 0x10000 (that is, outside the range of cmd flags). Once we release with a value, it becomes locked into the API; but by intentionally picking something other than 0x1, we still leave room for ABI to take command flags at a later date, with a little less hassle than having to translate from LIBNBD_SHUTDOWN_FOO to LIBNBD_CMD_FLAG_FOO. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org