[PATCH] chardev: add nodelay option

[PATCH] chardev: add nodelay option

[Paolo Bonzini]

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 have different default to QMP for 'server', hence
      * we can't just check for existence of 'server'
diff --git a/gdbstub.c b/gdbstub.c
index 3ee40479b6..16d7c8f534 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -3505,7 +3505,7 @@ int gdbserver_start(const char *device)
         if (strstart(device, "tcp:", NULL)) {
             /* enforce required TCP attributes */
             snprintf(gdbstub_device_name, sizeof(gdbstub_device_name),
-                     "%s,wait=off,delay=off,server=on", device);
+                     "%s,wait=off,nodelay=on,server=on", device);
             device = gdbstub_device_name;
         }
 #ifndef _WIN32
diff --git a/qemu-options.hx b/qemu-options.hx
index 252db9357c..90801286c6 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3033,7 +3033,7 @@ DEFHEADING(Character device options:)
 DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
     "-chardev help\n"
     "-chardev null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
-    "-chardev socket,id=id[,host=host],port=port[,to=to][,ipv4=on|off][,ipv6=on|off][,delay=on|off][,reconnect=seconds]\n"
+    "-chardev socket,id=id[,host=host],port=port[,to=to][,ipv4=on|off][,ipv6=on|off][,nodelay=on|off][,reconnect=seconds]\n"
     "         [,server=on|off][,wait=on|off][,telnet=on|off][,websocket=on|off][,reconnect=seconds][,mux=on|off]\n"
     "         [,logfile=PATH][,logappend=on|off][,tls-creds=ID][,tls-authz=ID] (tcp)\n"
     "-chardev socket,id=id,path=path[,server=on|off][,wait=on|off][,telnet=on|off][,websocket=on|off][,reconnect=seconds]\n"
@@ -3184,7 +3184,7 @@ The available backends are:
 
     TCP and unix socket options are given below:
 
-    ``TCP options: port=port[,host=host][,to=to][,ipv4=on|off][,ipv6=on|off][,delay=on|off]``
+    ``TCP options: port=port[,host=host][,to=to][,ipv4=on|off][,ipv6=on|off][,nodelay=on|off]``
         ``host`` for a listening socket specifies the local address to
         be bound. For a connecting socket species the remote host to
         connect to. ``host`` is optional for listening sockets. If not
@@ -3204,7 +3204,7 @@ The available backends are:
         or IPv6 must be used. If neither is specified the socket may
         use either protocol.
 
-        ``delay=on|off`` disables the Nagle algorithm.
+        ``nodelay=on|off`` disables the Nagle algorithm.
 
     ``unix options: path=path[,abstract=on|off][,tight=on|off]``
         ``path`` specifies the local path of the unix socket. ``path``
@@ -3593,13 +3593,13 @@ SRST
         ``telnet options:``
             localhost 5555
 
-    ``tcp:[host]:port[,server=on|off][,wait=on|off][,delay=on|off][,reconnect=seconds]``
+    ``tcp:[host]:port[,server=on|off][,wait=on|off][,nodelay=on|off][,reconnect=seconds]``
         The TCP Net Console has two modes of operation. It can send the
         serial I/O to a location or wait for a connection from a
         location. By default the TCP Net Console is sent to host at the
         port. If you use the ``server=on`` option QEMU will wait for a client
         socket application to connect to the port before continuing,
-        unless the ``wait=on|off`` option was specified. The ``delay=on|off``
+        unless the ``wait=on|off`` option was specified. The ``nodelay=on|off``
         option disables the Nagle buffering algorithm. The ``reconnect=on``
         option only applies if ``server=no`` is set, if the connection goes
         down it will attempt to reconnect at the given interval. If host
@@ -3616,7 +3616,7 @@ SRST
         ``Example to not wait and listen on ip 192.168.0.100 port 4444``
             -serial tcp:192.168.0.100:4444,server=on,wait=off
 
-    ``telnet:host:port[,server=on|off][,wait=on|off][,delay=on|off]``
+    ``telnet:host:port[,server=on|off][,wait=on|off][,nodelay=on|off]``
         The telnet protocol is used instead of raw tcp sockets. The
         options work the same as if you had specified ``-serial tcp``.
         The difference is that the port acts like a telnet server or
@@ -3626,7 +3626,7 @@ SRST
         you do it with Control-] and then type "send break" followed by
         pressing the enter key.
 
-    ``websocket:host:port,server=on[,wait=on|off][,delay=on|off]``
+    ``websocket:host:port,server=on[,wait=on|off][,nodelay=on|off]``
         The WebSocket protocol is used instead of raw tcp socket. The
         port acts as a WebSocket server. Client mode is not supported.
 
-- 
2.29.2

Re: [PATCH] chardev: add nodelay option

[Philippe Mathieu-Daudé]

On 3/2/21 12:04 PM, 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);

Should we add a deprecation note to remember to remove this later,
or do we want to keep it infinitely? Then a comment here would be
useful.

Re: [PATCH] chardev: add nodelay option

[Daniel P. Berrangé]

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


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] chardev: add nodelay option

[Paolo Bonzini]

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

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

Re: [PATCH] chardev: add nodelay option

[Paolo Bonzini]

On 02/03/21 12:33, Philippe Mathieu-Daudé wrote:
> On 3/2/21 12:04 PM, 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);
> 
> Should we add a deprecation note to remember to remove this later,
> or do we want to keep it infinitely? Then a comment here would be
> useful.

We cannot deprecate it only when options like -serial start warning for 
short-form boolean options (which I was going to propose for 6.1).  We 
probably also don't want a warning like "Please use delay=off instead"; 
suggesting "nodelay=on" would be an improvement (though an ugly one).

So the plan could be:

6.0: add "nodelay", deprecate -chardev short-form boolean options
6.1: deprecate "delay" and -serial short-form boolean options
6.2: some options (-M/-accel) start rejecting short-form boolean options
6.3: remove "delay"

Paolo

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.