On 10.10.25 14:24, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <[email protected]> writes:
On 10.10.25 10:52, Michael Tokarev wrote:
On 10/9/25 17:17, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <[email protected]> writes:
They were deprecated in 9.2, now we can remove them.
New options to use are reconnect-ms.
Speaking of the option itself.. I'd not remove it, instead,
I'd de-deprecate it, and allow units to be specified for it,
like reconnect=10ms (defaults to s). Or reconnect=0.1 (in
fractions of second). But it's just me, it looks like :)
QAPI is not for humans) So simple milliseconds integer is better,
then parse the variety of suffixes. And it better fit into json
(number is number, not a string)
Also, `has_reconnect_ms` becomes redundant after applying this
patch, - it should be enough to use just reconnect_ms, which
defaults to 0. But this can be done in a subsequent cleanup.
You mean just use sock->reconnect_ms instead of explicit
int64_t reconnect_ms = sock->has_reconnect_ms ? sock->reconnect_ms : 0;
?
We routinely exploit that QAPI initializes absent members to zero.
What I'm afraid of: generated code is not the only source of QAPI structures,
sometimes they are initialized by hand. And looking at code like
bool is_telnet = sock->has_telnet ? sock->telnet : false;
I can't say, does the structure comes from generated code and this check
is redundant, or the structure may come from other place, and we chose
be explicitly safe here..
I believe that QMP will zero-initialize everything. But I'm not sure
that we do zero initialize this structure on all paths.. Keeping also in mind
handling other fields here like
bool is_telnet = sock->has_telnet ? sock->telnet : false;
bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false;
bool is_waitconnect = sock->has_wait ? sock->wait : false;
bool is_websock = sock->has_websocket ? sock->websocket : false;
To drop this, we should check for all paths, that incoming structure is
zero-initialized. And no guarantee that it does not break in future.
Probably, we can implement a new QAPI feature "value with default to zero",
so that we can avoid existence of .has_foo field at all for such fields.
No field - no problem.. But not in this series)
The simple way to do "optional" is to have the machinery supply a
default value.
The less simple way is to add a distinct extra value that means
"absent". This permits "absent" to means something else than any value.
QAPI does the latter. Not my choice; I inherited it :)
For pointers, generated C uses null as distinct extra value. Works,
because "present" implies non-null.
For other types, generated C uses a pair of variables (has_FOO, FOO),
where
(true, V) means present with value V
(false, zero) means absent
(false, non-zero) is invalid
Existing of invalid combinations is always a headache
This results in slightly more complicated code.
Most of the time, code maps "absent" to a default value. This default
value is not visible in the schema, it's buried in the C code. When a
type gets used in multiple places, each place can pick its own default.
Bothersome to document, and the system cannot ensure the code matches
its documentation. Strong dislike.
Special case: when the default value is zero, we can ignore has_FOO and
just use FOO. See "routinely exploit" above.
We could extend the QAPI schema language to let us specify the default
value. The generator could then elide has_FOO. Code would become
simpler.
Yes, I meant something like this. May be in some future day... Some AI agent
will come with patches, after reading our discussion)
--
Best regards,
Vladimir
