[Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Ashley Jonathan]

I have experienced a minor difficulty using QEMU with the "-serial pty" option:

If a process opens the slave pts device, writes data to it, then immediately closes it, the data doesn't reliably get delivered to the emulated serial port. This seems to be because a read of the master pty device returns EIO on Linux if no process has the pts device open, even when data is waiting "in the pipe".

A fix seems to be for QEMU to keep the pts file descriptor open until the pty is closed, as per the below patch.

---
 qemu-char.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 2969c44..ed03ba0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1198,6 +1198,7 @@ typedef struct {
     int connected;
     guint timer_tag;
     guint open_tag;
+    int slave_fd;
 } PtyCharDriver;
 
 static void pty_chr_update_read_handler_locked(CharDriverState *chr);
@@ -1373,6 +1374,7 @@ static void pty_chr_close(struct CharDriverState *chr)
 
     qemu_mutex_lock(&chr->chr_write_lock);
     pty_chr_state(chr, 0);
+    close(s->slave_fd);
     fd = g_io_channel_unix_get_fd(s->fd);
     g_io_channel_unref(s->fd);
     close(fd);
@@ -1401,7 +1403,6 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
         return NULL;
     }
 
-    close(slave_fd);
     qemu_set_nonblock(master_fd);
 
     chr = qemu_chr_alloc();
@@ -1422,6 +1423,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
     chr->explicit_be_open = true;
 
     s->fd = io_channel_from_fd(master_fd);
+    s->slave_fd = slave_fd;
     s->timer_tag = 0;
 
     return chr;
-- 
2.1.4



---
Jonathan Ashley 
jonathan.ashley AT altran.com

Re: [Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Michael Tokarev]

11.12.2015 14:29, Ashley Jonathan wrote:
> I have experienced a minor difficulty using QEMU with the "-serial pty" option:
> 
> If a process opens the slave pts device, writes data to it, then immediately closes it, the data doesn't reliably get delivered to the emulated serial port. This seems to be because a read of the master pty device returns EIO on Linux if no process has the pts device open, even when data is waiting "in the pipe".
> 
> A fix seems to be for QEMU to keep the pts file descriptor open until the pty is closed, as per the below patch.

The patch looks fine, so

Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>

but I'd love to have an ACK from the maintainer about this one,
or for it to pick it up.

Thanks,

/mjt

> ---
>  qemu-char.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 2969c44..ed03ba0 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1198,6 +1198,7 @@ typedef struct {
>      int connected;
>      guint timer_tag;
>      guint open_tag;
> +    int slave_fd;
>  } PtyCharDriver;
>  
>  static void pty_chr_update_read_handler_locked(CharDriverState *chr);
> @@ -1373,6 +1374,7 @@ static void pty_chr_close(struct CharDriverState *chr)
>  
>      qemu_mutex_lock(&chr->chr_write_lock);
>      pty_chr_state(chr, 0);
> +    close(s->slave_fd);
>      fd = g_io_channel_unix_get_fd(s->fd);
>      g_io_channel_unref(s->fd);
>      close(fd);
> @@ -1401,7 +1403,6 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>          return NULL;
>      }
>  
> -    close(slave_fd);
>      qemu_set_nonblock(master_fd);
>  
>      chr = qemu_chr_alloc();
> @@ -1422,6 +1423,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>      chr->explicit_be_open = true;
>  
>      s->fd = io_channel_from_fd(master_fd);
> +    s->slave_fd = slave_fd;
>      s->timer_tag = 0;
>  
>      return chr;
> 

Re: [Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Paolo Bonzini]

On 11/01/2016 09:33, Michael Tokarev wrote:
> 11.12.2015 14:29, Ashley Jonathan wrote:
>> I have experienced a minor difficulty using QEMU with the "-serial pty" option:
>>
>> If a process opens the slave pts device, writes data to it, then immediately closes it, the data doesn't reliably get delivered to the emulated serial port. This seems to be because a read of the master pty device returns EIO on Linux if no process has the pts device open, even when data is waiting "in the pipe".
>>
>> A fix seems to be for QEMU to keep the pts file descriptor open until the pty is closed, as per the below patch.
> 
> The patch looks fine, so
> 
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> 
> but I'd love to have an ACK from the maintainer about this one,
> or for it to pick it up.

Ok, I'll pick it up after I've read up a bit more on PTYs.

Paolo

Re: [Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Paolo Bonzini]

On 11/12/2015 12:29, Ashley Jonathan wrote:
> I have experienced a minor difficulty using QEMU with the "-serial
> pty" option:
> 
> If a process opens the slave pts device, writes data to it, then
> immediately closes it, the data doesn't reliably get delivered to the
> emulated serial port. This seems to be because a read of the master
> pty device returns EIO on Linux if no process has the pts device
> open, even when data is waiting "in the pipe".
> 
> A fix seems to be for QEMU to keep the pts file descriptor open until
> the pty is closed, as per the below patch.

You need to include a "Signed-off-by: Ashley Jonathan <jonathan.ashley@altran.com>"
line in the commit message, meaning that you have read and understood the
"Developer Certificate of Origin":

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297

Just reply to this message with the above line.

Paolo

Re: [Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Ashley Jonathan]

Apologies; I overlooked that detail:

Signed-off-by: Ashley Jonathan <jonathan.ashley@altran.com>

Regards,
--
Jon Ashley

-----Original Message-----
From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
Sent: 11 January 2016 09:16
To: Ashley Jonathan; qemu-devel@nongnu.org
Cc: qemu-trivial@nongnu.org
Subject: Re: [PATCH] Keep pty slave file descriptor open until the master is closed



On 11/12/2015 12:29, Ashley Jonathan wrote:
> I have experienced a minor difficulty using QEMU with the "-serial 
> pty" option:
> 
> If a process opens the slave pts device, writes data to it, then 
> immediately closes it, the data doesn't reliably get delivered to the 
> emulated serial port. This seems to be because a read of the master 
> pty device returns EIO on Linux if no process has the pts device open, 
> even when data is waiting "in the pipe".
> 
> A fix seems to be for QEMU to keep the pts file descriptor open until 
> the pty is closed, as per the below patch.

You need to include a "Signed-off-by: Ashley Jonathan <jonathan.ashley@altran.com>"
line in the commit message, meaning that you have read and understood the "Developer Certificate of Origin":

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297

Just reply to this message with the above line.

Paolo

Re: [Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Marc-André Lureau]

Hi

On Mon, Jan 11, 2016 at 10:13 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 11/01/2016 09:33, Michael Tokarev wrote:
>> 11.12.2015 14:29, Ashley Jonathan wrote:
>>> I have experienced a minor difficulty using QEMU with the "-serial pty" option:
>>>
>>> If a process opens the slave pts device, writes data to it, then immediately closes it, the data doesn't reliably get delivered to the emulated serial port. This seems to be because a read of the master pty device returns EIO on Linux if no process has the pts device open, even when data is waiting "in the pipe".
>>>
>>> A fix seems to be for QEMU to keep the pts file descriptor open until the pty is closed, as per the below patch.
>>
>> The patch looks fine, so
>>
>> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
>>
>> but I'd love to have an ACK from the maintainer about this one,
>> or for it to pick it up.
>
> Ok, I'll pick it up after I've read up a bit more on PTYs.

That patch slows down qemu a lot when using -chardev
pty,id=charserial0 -device isa-serial,chardev=charserial0. I forgot a
lot about how pty/pts work, and reading some man pages didn't help me
much to understand the issue, I would suggest to revert it until a
better solution is found.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] Keep pty slave file descriptor open until the master is closed

[Marc-André Lureau]

Hi

On Fri, Feb 12, 2016 at 3:29 AM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Mon, Jan 11, 2016 at 10:13 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 11/01/2016 09:33, Michael Tokarev wrote:
>>> 11.12.2015 14:29, Ashley Jonathan wrote:
>>>> I have experienced a minor difficulty using QEMU with the "-serial pty" option:
>>>>
>>>> If a process opens the slave pts device, writes data to it, then immediately closes it, the data doesn't reliably get delivered to the emulated serial port. This seems to be because a read of the master pty device returns EIO on Linux if no process has the pts device open, even when data is waiting "in the pipe".
>>>>
>>>> A fix seems to be for QEMU to keep the pts file descriptor open until the pty is closed, as per the below patch.
>>>
>>> The patch looks fine, so
>>>
>>> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
>>>
>>> but I'd love to have an ACK from the maintainer about this one,
>>> or for it to pick it up.
>>
>> Ok, I'll pick it up after I've read up a bit more on PTYs.
>
> That patch slows down qemu a lot when using -chardev
> pty,id=charserial0 -device isa-serial,chardev=charserial0. I forgot a
> lot about how pty/pts work, and reading some man pages didn't help me
> much to understand the issue, I would suggest to revert it until a
> better solution is found.

It looks like if a the slave is opened, then Linux will buffer the
master writes, up to a few kb and then throttle, so it's not entirely
blocked but eventually the guest VM dies. However, not having any
slave open it will simply let the write go and discard the data. At
least, virt-install configures a pty for the serial but viewers like
virt-manager do not necessarily open it. And, if there are no viewers,
it will just hang. If qemu starts reading all the data from the slave,
I don't think interactions with other slaves will work. I don't see
much options but to close the slave, thus reverting this patch.

Regarding the original bug, I wonder if the slave writing process
couldn't simply fsync/fdatasync (I guess you tried that already)
Probably better to interact with the guest with something else than a
terminal though..


-- 
Marc-André Lureau