[Qemu-devel] [PATCH] char: translate from QIOChannel error to errno

[Qemu-devel] [PATCH] char: translate from QIOChannel error to errno

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Caller of CharDriverState.chr* callback assume errno error conventions.
Translate QIOChannel error to errno (this fixes potential EAGAIN
regression, for ex if a vhost-user backend block, qemu_chr_fe_read_all()
could get error -2 and not wait)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index ad11b75..4317388 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2727,6 +2727,13 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
                                      NULL);
     }
 
+    if (ret == QIO_CHANNEL_ERR_BLOCK) {
+        errno = EAGAIN;
+        ret = -1;
+    } else if (ret == -1) {
+        errno = EIO;
+    }
+
     if (msgfds_num) {
         /* close and clean read_msgfds */
         for (i = 0; i < s->read_msgfds_num; i++) {
-- 
2.5.0

Re: [Qemu-devel] [PATCH] char: translate from QIOChannel error to errno

[Daniel P. Berrange]

On Fri, Mar 11, 2016 at 06:55:24PM +0100, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Caller of CharDriverState.chr* callback assume errno error conventions.
> Translate QIOChannel error to errno (this fixes potential EAGAIN
> regression, for ex if a vhost-user backend block, qemu_chr_fe_read_all()
> could get error -2 and not wait)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

> ---
>  qemu-char.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index ad11b75..4317388 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2727,6 +2727,13 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>                                       NULL);
>      }
>  
> +    if (ret == QIO_CHANNEL_ERR_BLOCK) {
> +        errno = EAGAIN;
> +        ret = -1;
> +    } else if (ret == -1) {
> +        errno = EIO;
> +    }
> +
>      if (msgfds_num) {
>          /* close and clean read_msgfds */
>          for (i = 0; i < s->read_msgfds_num; i++) {

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH] char: translate from QIOChannel error to errno

[Paolo Bonzini]

On 11/03/2016 18:55, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Caller of CharDriverState.chr* callback assume errno error conventions.
> Translate QIOChannel error to errno (this fixes potential EAGAIN
> regression, for ex if a vhost-user backend block, qemu_chr_fe_read_all()
> could get error -2 and not wait)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qemu-char.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index ad11b75..4317388 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2727,6 +2727,13 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>                                       NULL);
>      }
>  
> +    if (ret == QIO_CHANNEL_ERR_BLOCK) {
> +        errno = EAGAIN;
> +        ret = -1;
> +    } else if (ret == -1) {
> +        errno = EIO;
> +    }
> +
>      if (msgfds_num) {
>          /* close and clean read_msgfds */
>          for (i = 0; i < s->read_msgfds_num; i++) {
> 

I can take this patch as it fixes a real regression, but could
QIOChannel just return negative errno?

Paolo

Re: [Qemu-devel] [PATCH] char: translate from QIOChannel error to errno

[Daniel P. Berrange]

On Fri, Mar 18, 2016 at 12:55:34PM +0100, Paolo Bonzini wrote:
> 
> 
> On 11/03/2016 18:55, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Caller of CharDriverState.chr* callback assume errno error conventions.
> > Translate QIOChannel error to errno (this fixes potential EAGAIN
> > regression, for ex if a vhost-user backend block, qemu_chr_fe_read_all()
> > could get error -2 and not wait)
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  qemu-char.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/qemu-char.c b/qemu-char.c
> > index ad11b75..4317388 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -2727,6 +2727,13 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
> >                                       NULL);
> >      }
> >  
> > +    if (ret == QIO_CHANNEL_ERR_BLOCK) {
> > +        errno = EAGAIN;
> > +        ret = -1;
> > +    } else if (ret == -1) {
> > +        errno = EIO;
> > +    }
> > +
> >      if (msgfds_num) {
> >          /* close and clean read_msgfds */
> >          for (i = 0; i < s->read_msgfds_num; i++) {
> > 
> 
> I can take this patch as it fixes a real regression, but could
> QIOChannel just return negative errno?

No, QIOChannel explicitly does not ever use errno's because that only
works if all the functions QIOChannel uses internally set errnos. This
is not the case for the TLS impl of QIOChannel. For this reason, the
QIOChannel APIs all use 'Error **errp' for error reporting.

Ultimately as callers of the QIOChannel code in QEMU are converted to
propagate 'Error **errp' we'll be ok, but in the immediate some callers
like chardev still have a built-in assumption that they'll have errno's
available, so we have to do this compat hack.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|