Re: [PATCH] chardev: allow specifying finer-grained reconnect timeouts
On Thu, Aug 29, 2024 at 13:56:43 +0200, Markus Armbruster wrote:
Daniil Tatianin <d-tatianin(a)yandex-team.ru> writes:
[...]
So firstly, libvirt models the timeout in the XML in seconds for now so
making use of this will require some XML design plumbing making use of
it if there'll be any users wanting it.
When libvirt would want to make use of this the amount of work for any
of the options below is almost the same (capability detection, adding
of the new plumbing, etc). The only difference is what to
do if nobody shows interest in exposing the sub-second intervals in
libvirt.
> @@ -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.
Here we'd have to do nothing.
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.
Same here.
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 one will require change to libvirt including a capability addition
even when libvirt might not want to use the new field. (Add capability
detection, switch to new interface if present. Libvirt doesn't want to
use deprecated interfaces to avoid future breakage.)
But we've done this multiple times so it's not a big deal.
Let's get additional input from management application
developers. I
cc'ed some.
From libvirt's point of view I'd say either option is viable. We're okay
with deprecating the old interface libvirt is used to do that.
I'd personally prefer if floating point numbers were avoided.