Re: [PATCH] chardev: allow specifying finer-grained reconnect timeouts
On Thu, Aug 29, 2024 at 01:56:43PM GMT, Markus Armbruster wrote:
Daniil Tatianin <d-tatianin(a)yandex-team.ru> writes:
> The "reconnect" option only allows to specify the time in seconds,
> which is way too long for certain workflows.
...
> @@ -287,7 +292,8 @@
> '*telnet': 'bool',
> '*tn3270': 'bool',
> '*websocket': 'bool',
> - '*reconnect': 'int' },
> + '*reconnect': 'int',
> + '*reconnect-is-ms': 'bool' },
> 'base': 'ChardevCommon' }
>
> ##
I don't like this interface.
PRO: compatible extension; no management application updates needed
unless they want to support sub-second delays.
CON: specifying a sub-second delay takes two parameters, which is
awkward.
CON: trap in combination with -set. Before the patch, something like
-set chardev.ID.reconnect=N means N seconds no matter what.
Afterwards, it depends on the value of reconnect-is-ms, which may be
set far away. Mitigating factor: -set is obscure.
I also dislike this interface. Having only one number plus an
optional boolean that controls its scale is not as easy to reason
about.
Alternatives:
1. Change @reconnect to 'number', specify sub-second delays as
fractions.
PRO: compatible extension; no management application updates needed
unless they want to support sub-second delays.
CON: first use of floating-point for time in seconds in QAPI, as far
as I can see.
CON: QemuOpts can't do floating-point.
Eww. I don't see this as the compelling reason to switch to floating point.
2. Deprecate @reconnect in favour of a new member with a more suitable
unit. Error if both are present.
PRO: after @reconnect is gone, the interface is what it arguably
should've been from the start.
CON: incompatible change. Management application updates needed
within the deprecation grace period.
This seems reasonable to me. We have enough places in QAPI where we
want mutual exclusion (we mark both fields optional, but expect the
user to provide exactly one or get an error), that I wonder if it is
worth making it a first-class construct in QAPI (maybe I'm spoiled by
the OneOf designation[1] in protobuf[2] used by gRPC[3] in
kubernetes). Including the scale in the name is a bonus reason to
switch the interface.
[1] https://protobuf.dev/programming-guides/proto3/#oneof
[2] https://protobuf.dev/overview/
[3] https://grpc.io/
Let's get additional input from management application developers. I
cc'ed some.
Related: NetdevSocketOptions member @reconnect.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org