Parallel device don't register be->chr_can_read function, but remote disconnect event is handled in chr_read.
So connected parallel device can not detect remote disconnect event.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
Reviewed-by: Wang Yechao <wang.yechao255@zte.com.cn>
---
 chardev/char-socket.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index ccc499c..59509d4 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -131,6 +131,14 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
     }
 }
 
+static gboolean is_parallel_device(Chardev *chr)
+{
+    if (chr && chr->label && strstr(chr->label, "charparallel")) {
+        return TRUE;
+    }
+    return FALSE;
+}
+
 static int tcp_chr_read_poll(void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
@@ -138,6 +146,8 @@ static int tcp_chr_read_poll(void *opaque)
     if (!s->connected) {
         return 0;
     }
+    if (is_parallel_device(chr)) {
+        return 1;
+    }
     s->max_size = qemu_chr_be_can_write(chr);
     return s->max_size;
 }
@@ -422,6 +432,15 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     uint8_t buf[CHR_READ_BUF_LEN];
     int len, size;
 
+    /* for parallel-device handle the socket close event here*/
+    if (!s->max_size && is_parallel_device(chr)) {
+        size = tcp_chr_recv(chr, (void *)buf, CHR_READ_BUF_LEN);
+        if (size == 0 || size == -1) {
+            tcp_chr_disconnect(chr);
+        }
+        return TRUE;
+    }
+
     if (!s->connected || s->max_size <= 0) {
         return TRUE;
     }
-- 
1.8.3.1

Hi

On Mon, Jul 10, 2017 at 10:36 AM Peng Hao <peng.hao2@zte.com.cn> wrote:

> Parallel device don't register be->chr_can_read function, but remote
> disconnect event is handled in chr_read.
> So connected parallel device can not detect remote disconnect event.
>
>
What is it that you call a parallel device? you are modifying the socket
code here. Could you describe a test setup? even better would be to write a
test in test-char.c.


> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> Reviewed-by: Wang Yechao <wang.yechao255@zte.com.cn>
> ---
>  chardev/char-socket.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index ccc499c..59509d4 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -131,6 +131,14 @@ static int tcp_chr_write(Chardev *chr, const uint8_t
> *buf, int len)
>      }
>  }
>
> +static gboolean is_parallel_device(Chardev *chr)
> +{
> +    if (chr && chr->label && strstr(chr->label, "charparallel")) {
> +        return TRUE;
>

What guarantees that a parallel device will have "charparallel" in its
label?


> +    }
> +    return FALSE;
> +}
> +
>  static int tcp_chr_read_poll(void *opaque)
>  {
>      Chardev *chr = CHARDEV(opaque);
> @@ -138,6 +146,8 @@ static int tcp_chr_read_poll(void *opaque)
>      if (!s->connected) {
>          return 0;
>      }
> +    if (is_parallel_device(chr)) {
> +        return 1;
>

That means 1 byte can be read, but s->max_size isn't updated, this will
confuse tcp_chr_read().


> +    }
>      s->max_size = qemu_chr_be_can_write(chr);
>      return s->max_size;
>  }
> @@ -422,6 +432,15 @@ static gboolean tcp_chr_read(QIOChannel *chan,
> GIOCondition cond, void *opaque)
>      uint8_t buf[CHR_READ_BUF_LEN];
>      int len, size;
>
> +    /* for parallel-device handle the socket close event here*/
> +    if (!s->max_size && is_parallel_device(chr)) {
> +        size = tcp_chr_recv(chr, (void *)buf, CHR_READ_BUF_LEN);
> +        if (size == 0 || size == -1) {
> +            tcp_chr_disconnect(chr);
>

I don't understand why that condition wouldn't be reached by the following
tcp_chr_recv() handling. Furthermore, you may read more that s->max_size,
which will likely break qemu_chr_be_write() in various cases.



> +        }
> +        return TRUE;
> +    }
> +
>      if (!s->connected || s->max_size <= 0) {
>          return TRUE;
>      }
> --
> 1.8.3.1
>
>
>
> --
Marc-André Lureau

On 10/07/2017 11:17, Marc-André Lureau wrote:
>     Parallel device don't register be->chr_can_read function, but remote
>     disconnect event is handled in chr_read.
>     So connected parallel device can not detect remote disconnect event.
> 
> What is it that you call a parallel device? you are modifying the socket
> code here. Could you describe a test setup? even better would be to
> write a test in test-char.c.

If I understand the patch correctly, the parallel device is the parallel
port emulation.

It's still not clear to me what scenario the patch is fixing, and the
patch is wrong anyway:

1) because the backend is not supposed to know the details of the
frontend that is connected to it;

2) because the character device's label is not a reliable indicator for
the frontend type (there is no such indicator, because of point 1).

Paolo