This is about rejecting the inet options that -netdev dgram currently
accepts and silently ignores. (The subject is still the stream-docs
patch this came up under.)
> We should not accept options and then silently ignore them. Can we
> reject them instead?
Agreed. I prototyped two ways to reject them and confirmed both work
as intended. Of the two I lean towards B (reject in the schema); I
would like your opinion, mainly on the ABI change and on how far to
take it.
Background: -netdev dgram only ever uses the host and port of its
addresses. It applies none of the other optional InetSocketAddress
members (numeric, to, ipv4, ipv6, keep-alive and friends, mptcp, and
now user-timeout) -- they are currently accepted and silently ignored.
(This is inet-specific; the unix abstract/tight members and the vsock
type are a separate matter, see the open point at the end.)
With either approach, valid configurations keep working as before;
only settings that used to be accepted but silently ignored (say
remote.keep-alive=on) now error out. The approaches differ in how and
where the rejection happens, and in the schema reported by
query-qmp-schema.
Approach A -- reject in a dedicated check function
Keep NetdevDgramOptions on SocketAddress and check in
net_init_dgram() whether the unusable InetSocketAddress members are
present on the 'inet' variant, erroring out with e.g.
"'remote.keep-alive' is not supported by dgram".
+ No QMP schema/ABI change; the change stays within net/dgram.c.
- The check has to be extended whenever a new InetSocketAddress
member is added (user-timeout already needed one). Forgetting to
do so silently reintroduces exactly the bug we are fixing.
- query-qmp-schema keeps advertising the members for dgram even
though they are rejected at run time, so introspection disagrees
with reality.
- The members stay in dgram's schema although they do nothing there.
The friendly "... is not supported by dgram" message only exists to
paper over that; for an option that is not part of dgram (and not
in its documentation), a plain "unexpected" would be the honest
result.
Approach B -- reject in the schema
Change dgram's 'inet' variant to InetSocketAddressBase (host/port
only) instead of InetSocketAddress.
+ The schema tells the truth: dgram's inet address is just host and
port, and query-qmp-schema reflects that.
+ An unsupported option is then rejected like any other unknown
parameter ("Parameter 'remote.keep-alive' is unexpected"),
identically for CLI and QMP -- the natural result for something
that is not part of dgram.
+ Any future InetSocketAddress member is handled automatically, with
no per-option code.
- It is a QMP schema/ABI change (though it only removes members that
never had any effect).
- It needs a new union type: SocketAddress and InetSocketAddress are
shared with stream, chardev, migration, etc., so they cannot be
narrowed just for dgram. Hence a parallel SocketAddressDgram whose
'inet' variant is InetSocketAddressBase.
So I would go with B: it makes the schema and introspection honest and
needs no per-option maintenance. A avoids the ABI change and stays
within net/dgram.c, but it keeps advertising options it then rejects
and has to be kept in sync by hand.
One open point for B is how far to take it. dgram also does not support
the vsock type (rejected in net_init_dgram() today) and ignores the unix
abstract/tight members. My current prototype only narrows the inet
variant, so in that minimal form B still has the same
introspection-vs-reality mismatch as A for vsock and for the unix
abstract/tight members. Making the schema fully honest would
additionally need a dgram variant of UnixSocketAddress and a
dgram-specific type enum (without vsock).
So: does B sound like the right direction to you? If so, how far should
the "only what dgram supports" cleanup go -- just the inet options, or
also a UnixSocketAddress base type and a vsock-less enum? Or would you
rather keep it minimal with A, to avoid touching the ABI?
Thanks,
Mitsuru
