[Qemu-devel] [PATCH] char: drop data written to a disconnected pty

[Qemu-devel] [PATCH] char: drop data written to a disconnected pty

[Ed Swierk]

When a serial port writes data to a pty that's disconnected, drop the
data and return the length dropped. This avoids triggering pointless
retries in callers like the 16550A serial_xmit(), and causes
qemu_chr_fe_write() to write all data to the log file, rather than
logging only while a pty client like virsh console happens to be
connected.

Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
---
 qemu-char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 676944a..ccb6923 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1528,7 +1528,7 @@ static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
         /* guest sends data, check for (re-)connect */
         pty_chr_update_read_handler_locked(chr);
         if (!s->connected) {
-            return 0;
+            return len;
         }
     }
     return io_channel_send(s->ioc, buf, len);
-- 
1.9.1

Re: [Qemu-devel] [PATCH] char: drop data written to a disconnected pty

[Marc-André Lureau]

Hi

----- Original Message -----
> When a serial port writes data to a pty that's disconnected, drop the
> data and return the length dropped. This avoids triggering pointless
> retries in callers like the 16550A serial_xmit(), and causes
> qemu_chr_fe_write() to write all data to the log file, rather than
> logging only while a pty client like virsh console happens to be
> connected.
> 
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> ---
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 676944a..ccb6923 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1528,7 +1528,7 @@ static int pty_chr_write(CharDriverState *chr, const
> uint8_t *buf, int len)
>          /* guest sends data, check for (re-)connect */
>          pty_chr_update_read_handler_locked(chr);
>          if (!s->connected) {
> -            return 0;
> +            return len;

I think this can be confusing if some backends silently drop the data (under disconnected state), while other don't. Perhaps we should have instead a new common chardev property "hup-drop" ? (suggestions for better name welcome)

>          }
>      }
>      return io_channel_send(s->ioc, buf, len);
> --
> 1.9.1
> 
> 

Re: [Qemu-devel] [PATCH] char: drop data written to a disconnected pty

[Marc-André Lureau]

Hi

----- Original Message -----
> Hi
> 
> ----- Original Message -----
> > When a serial port writes data to a pty that's disconnected, drop the
> > data and return the length dropped. This avoids triggering pointless
> > retries in callers like the 16550A serial_xmit(), and causes
> > qemu_chr_fe_write() to write all data to the log file, rather than
> > logging only while a pty client like virsh console happens to be
> > connected.
> > 
> > Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> > ---
> >  qemu-char.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 676944a..ccb6923 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -1528,7 +1528,7 @@ static int pty_chr_write(CharDriverState *chr, const
> > uint8_t *buf, int len)
> >          /* guest sends data, check for (re-)connect */
> >          pty_chr_update_read_handler_locked(chr);
> >          if (!s->connected) {
> > -            return 0;
> > +            return len;
> 
> I think this can be confusing if some backends silently drop the data (under
> disconnected state), while other don't. Perhaps we should have instead a new
> common chardev property "hup-drop" ? (suggestions for better name welcome)
> 

actually,tcp_chr_write() already drops data on disconnected state, so they would have different default value for backward compatibility...

> >          }
> >      }
> >      return io_channel_send(s->ioc, buf, len);
> > --
> > 1.9.1
> > 
> > 
> 

Re: [Qemu-devel] [PATCH] char: drop data written to a disconnected pty

[Ed Swierk]

On Tue, Jan 31, 2017 at 6:06 AM, Marc-André Lureau <mlureau@redhat.com> wrote:
> I think this can be confusing if some backends silently drop the data (under disconnected state), while other don't. Perhaps we should have instead a new common chardev property "hup-drop" ? (suggestions for better name welcome)

Either way, when a pty is disconnected data will get dropped, whether
by the pty backend (as proposed) or by the serial port device or other
frontend (as currently).

The only difference from a user's perspective is whether the dropped
data gets written to the log file, which is the main motivation for
this change. The backward compatibility concern would be an existing
application that relies on data not being logged when the pty is
disconnected.

--Ed

Re: [Qemu-devel] [PATCH] char: drop data written to a disconnected pty

[Paolo Bonzini]

On 31/01/2017 14:19, Ed Swierk wrote:
> Either way, when a pty is disconnected data will get dropped, whether
> by the pty backend (as proposed) or by the serial port device or other
> frontend (as currently).
> 
> The only difference from a user's perspective is whether the dropped
> data gets written to the log file, which is the main motivation for
> this change. The backward compatibility concern would be an existing
> application that relies on data not being logged when the pty is
> disconnected.

Which is clearly a bug and a pointless difference between TCP and pty
backends, so the patch is okay.

Paolo

Re: [Qemu-devel] [PATCH] char: drop data written to a disconnected pty

[Paolo Bonzini]

On 31/01/2017 14:45, Ed Swierk wrote:
> When a serial port writes data to a pty that's disconnected, drop the
> data and return the length dropped. This avoids triggering pointless
> retries in callers like the 16550A serial_xmit(), and causes
> qemu_chr_fe_write() to write all data to the log file, rather than
> logging only while a pty client like virsh console happens to be
> connected.
> 
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> ---
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 676944a..ccb6923 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1528,7 +1528,7 @@ static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>          /* guest sends data, check for (re-)connect */
>          pty_chr_update_read_handler_locked(chr);
>          if (!s->connected) {
> -            return 0;
> +            return len;
>          }
>      }
>      return io_channel_send(s->ioc, buf, len);
> 

Rebased and queued, thanks.

Paolo