Re: [PATCH] chardev: add nodelay option

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 02/03/21 12:39, Daniel P. Berrangé wrote:
>> On Tue, Mar 02, 2021 at 12:04:44PM +0100, Paolo Bonzini wrote:
>>> The "delay" option was introduced as a way to enable Nagle's algorithm
>>> with ",nodelay".  Since the short form for boolean options has now been
>>> deprecated, introduce a more properly named "nodelay" option.  The "delay"
>>> option remains as an undocumented option.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   chardev/char-socket.c |  9 +++++++--
>>>   gdbstub.c             |  2 +-
>>>   qemu-options.hx       | 14 +++++++-------
>>>   3 files changed, 15 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>>> index 06a37c0cc8..73a7afe5a0 100644
>>> --- a/chardev/char-socket.c
>>> +++ b/chardev/char-socket.c
>>> @@ -1472,8 +1472,13 @@ static void qemu_chr_parse_socket(QemuOpts *opts, 
>>> ChardevBackend *backend,
>>>       sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
>>>       qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
>>>   
>>> -    sock->has_nodelay = qemu_opt_get(opts, "delay");
>>> -    sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);
>>> +    sock->has_nodelay =
>>> +        qemu_opt_get(opts, "delay") ||
>>> +        qemu_opt_get(opts, "nodelay");
>>> +    sock->nodelay =
>>> +        !qemu_opt_get_bool(opts, "delay", true) ||
>>> +        qemu_opt_get_bool(opts, "nodelay", false);
>> 
>> We should raise an explicit error if both options are present,
>> otherwise you get into a debate about prioritization with nonsense
>> such as
>> 
>>     -chardev socket,.....,delay=on,nodelay=on

We then reject the phrasing

    delay=on,...,nodelay=on

while accepting the phrasings

    delay=on,...,delay=off
    nodelay=off,...,nodelay=on

The two clashing setting can be further apart, e.g.

    -readconfig vm1.cfg -set chardev.chr0.nodelay=on

where vm.cfg contains

    [chardev "chr0"]
        backend = "socket"
        delay = "on"

We may choose to declare that a feature.  Please spell it out in the
commit message then.

> Good point, we can squash this in:
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 73a7afe5a0..c8bced76b7 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1472,6 +1472,10 @@ static void qemu_chr_parse_socket(QemuOpts *opts, 
> ChardevBackend *backend,
>       sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
>       qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
>
> +    if (qemu_opt_get(opts, "delay") && qemu_opt_get(opts, "nodelay")) {
> +        error_setg(errp, "'delay' and 'nodelay' are mutually exclusive");
> +        return;
> +    }
>       sock->has_nodelay =
>           qemu_opt_get(opts, "delay") ||
>           qemu_opt_get(opts, "nodelay");
>
> Paolo

Please repost with a suitably tweaked commit message to get my R-by.