Paolo Bonzini <pbonz...@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 <pbonz...@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.
