[Qemu-devel] Patch 9894dc0cdcc broke something

[Qemu-devel] Patch 9894dc0cdcc broke something

[Aleksey Kuleshov]

Hello!

I hit unexpected disconnections because of this patch:

commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
Author: Daniel P. Berrange <berrange@redhat.com>
Date:   Tue Jan 19 11:14:29 2016 +0000

    char: convert from GIOChannel to QIOChannel
    
    In preparation for introducing TLS support to the TCP chardev
    backend, convert existing chardev code from using GIOChannel
    to QIOChannel. This simplifies the chardev code by removing
    most of the OS platform conditional code for dealing with
    file descriptor passing.
    
    Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
    Message-Id: <1453202071-10289-3-git-send-email-berrange@redhat.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

breaks tcp_chr_read:

-static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
@@ -2938,9 +2801,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     if (len > s->max_size)
         len = s->max_size;
     size = tcp_chr_recv(chr, (void *)buf, len);
-    if (size == 0 ||
-        (size < 0 &&
-         socket_error() != EAGAIN && socket_error() != EWOULDBLOCK)) {
+    if (size == 0 || size == -1) {
         /* connection closed */
         tcp_chr_disconnect(chr);
     } else if (size > 0) {

since tcp_chr_recv returns -1 on blocking:

static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
{
...
    if (ret == QIO_CHANNEL_ERR_BLOCK) {
        errno = EAGAIN;
        ret = -1;
    } else if (ret == -1) {
        errno = EIO;
    }

This patch not just converts GIOChannel to QIOChannel
it also changes the logic which is not mentioned in the commit message.

Fix please :)

Re: [Qemu-devel] Patch 9894dc0cdcc broke something

[Daniel P. Berrangé]

On Thu, Feb 22, 2018 at 02:38:04PM +0300, Aleksey Kuleshov wrote:
> Hello!
> 
> I hit unexpected disconnections because of this patch:
> 
> commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
> Author: Daniel P. Berrange <berrange@redhat.com>
> Date:   Tue Jan 19 11:14:29 2016 +0000
> 
>     char: convert from GIOChannel to QIOChannel
>     
>     In preparation for introducing TLS support to the TCP chardev
>     backend, convert existing chardev code from using GIOChannel
>     to QIOChannel. This simplifies the chardev code by removing
>     most of the OS platform conditional code for dealing with
>     file descriptor passing.
>     
>     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>     Message-Id: <1453202071-10289-3-git-send-email-berrange@redhat.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> breaks tcp_chr_read:

What version of QEMU are you using ?


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: [Qemu-devel] Patch 9894dc0cdcc broke something

[Aleksey Kuleshov]

I'm using 2.11.0. 

22.02.2018, 14:45, "Daniel P. Berrangé" <berrange@redhat.com>:
> On Thu, Feb 22, 2018 at 02:38:04PM +0300, Aleksey Kuleshov wrote:
>>  Hello!
>>
>>  I hit unexpected disconnections because of this patch:
>>
>>  commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
>>  Author: Daniel P. Berrange <berrange@redhat.com>
>>  Date: Tue Jan 19 11:14:29 2016 +0000
>>
>>      char: convert from GIOChannel to QIOChannel
>>
>>      In preparation for introducing TLS support to the TCP chardev
>>      backend, convert existing chardev code from using GIOChannel
>>      to QIOChannel. This simplifies the chardev code by removing
>>      most of the OS platform conditional code for dealing with
>>      file descriptor passing.
>>
>>      Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>      Message-Id: <1453202071-10289-3-git-send-email-berrange@redhat.com>
>>      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>>  breaks tcp_chr_read:
>
> What version of QEMU are you using ?
>
> 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: [Qemu-devel] Patch 9894dc0cdcc broke something

[Daniel P. Berrangé]

On Thu, Feb 22, 2018 at 02:38:04PM +0300, Aleksey Kuleshov wrote:
> Hello!
> 
> I hit unexpected disconnections because of this patch:
> 
> commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
> Author: Daniel P. Berrange <berrange@redhat.com>
> Date:   Tue Jan 19 11:14:29 2016 +0000
> 
>     char: convert from GIOChannel to QIOChannel
>     
>     In preparation for introducing TLS support to the TCP chardev
>     backend, convert existing chardev code from using GIOChannel
>     to QIOChannel. This simplifies the chardev code by removing
>     most of the OS platform conditional code for dealing with
>     file descriptor passing.
>     
>     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>     Message-Id: <1453202071-10289-3-git-send-email-berrange@redhat.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> breaks tcp_chr_read:
> 
> -static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
> +static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>  {
>      CharDriverState *chr = opaque;
>      TCPCharDriver *s = chr->opaque;
> @@ -2938,9 +2801,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>      if (len > s->max_size)
>          len = s->max_size;
>      size = tcp_chr_recv(chr, (void *)buf, len);
> -    if (size == 0 ||
> -        (size < 0 &&
> -         socket_error() != EAGAIN && socket_error() != EWOULDBLOCK)) {
> +    if (size == 0 || size == -1) {
>          /* connection closed */
>          tcp_chr_disconnect(chr);
>      } else if (size > 0) {
> 
> since tcp_chr_recv returns -1 on blocking:

Actually it did not do that in this patch - tcp_chr_recv returns -2
on blocking when that patch was written.

> 
> static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
> {
> ...
>     if (ret == QIO_CHANNEL_ERR_BLOCK) {
>         errno = EAGAIN;
>         ret = -1;
>     } else if (ret == -1) {
>         errno = EIO;
>     }

This was caused by by a later change

  commit b6572b4f97a7b126c7b24e165893ed9fe3d72e1f
  Author: Marc-André Lureau <marcandre.lureau@redhat.com>
  Date:   Fri Mar 11 18:55:24 2016 +0100

    char: translate from QIOChannel error to errno
    

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 :|