Re: [PATCH 0/2] remove deprecated 'reconnect' options

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 10.10.25 10:52, Michael Tokarev wrote:
>> On 10/9/25 17:17, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 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.

> 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

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.