The code handling the return value of qio_channel_read proceses
len == 0 (EOF) separately from len < 1  (error), but in both
cases ends up calling qemu_file_set_error_obj() with -EIO as the
errno. This logic can be merged into one codepath to simplify it.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 migration/qemu-file.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index b6ac190034..8ee44c5ac9 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -348,17 +348,13 @@ static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
             } else {
                 qio_channel_wait(f->ioc, G_IO_IN);
             }
-        } else if (len < 0) {
-            len = -EIO;
         }
     } while (len == QIO_CHANNEL_ERR_BLOCK);
 
     if (len > 0) {
         f->buf_size += len;
-    } else if (len == 0) {
-        qemu_file_set_error_obj(f, -EIO, local_error);
     } else {
-        qemu_file_set_error_obj(f, len, local_error);
+        qemu_file_set_error_obj(f, -EIO, local_error);
     }
 
     for (int i = 0; i < nfd; i++) {
-- 
2.50.1

On Fri, Aug 01, 2025 at 06:02:11PM +0100, Daniel P. Berrangé wrote:
> The code handling the return value of qio_channel_read proceses
> len == 0 (EOF) separately from len < 1  (error), but in both
> cases ends up calling qemu_file_set_error_obj() with -EIO as the
> errno. This logic can be merged into one codepath to simplify it.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

On Sat, 2 Aug 2025 at 00:06, Daniel P. Berrangé <berrange@redhat.com> wrote:
> The code handling the return value of qio_channel_read proceses
> len == 0 (EOF) separately from len < 1  (error), but in both
> cases ends up calling qemu_file_set_error_obj() with -EIO as the
> errno. This logic can be merged into one codepath to simplify it.
>
>              } else {
>                  qio_channel_wait(f->ioc, G_IO_IN);
>              }
> -        } else if (len < 0) {
> -            len = -EIO;
>          }
>      } while (len == QIO_CHANNEL_ERR_BLOCK);
>
>      if (len > 0) {
>          f->buf_size += len;
> -    } else if (len == 0) {
> -        qemu_file_set_error_obj(f, -EIO, local_error);
>      } else {
> -        qemu_file_set_error_obj(f, len, local_error);
> +        qemu_file_set_error_obj(f, -EIO, local_error);
>      }

* But should  _file_set_error_obj(...  -EIO) be called for len == 0
(EOF) case? ie. function is trying to read from a file, at some point
it is bound to reach EOF.  '-EIO' indicates an I/O error, reaching EOF
could not be an error. Maybe we could just return zero(0) ?  (just
checking)

Thank you.
---
  - Prasad

On Mon, Aug 04, 2025 at 03:48:59PM +0530, Prasad Pandit wrote:
> On Sat, 2 Aug 2025 at 00:06, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > The code handling the return value of qio_channel_read proceses
> > len == 0 (EOF) separately from len < 1  (error), but in both
> > cases ends up calling qemu_file_set_error_obj() with -EIO as the
> > errno. This logic can be merged into one codepath to simplify it.
> >
> >              } else {
> >                  qio_channel_wait(f->ioc, G_IO_IN);
> >              }
> > -        } else if (len < 0) {
> > -            len = -EIO;
> >          }
> >      } while (len == QIO_CHANNEL_ERR_BLOCK);
> >
> >      if (len > 0) {
> >          f->buf_size += len;
> > -    } else if (len == 0) {
> > -        qemu_file_set_error_obj(f, -EIO, local_error);
> >      } else {
> > -        qemu_file_set_error_obj(f, len, local_error);
> > +        qemu_file_set_error_obj(f, -EIO, local_error);
> >      }
> 
> * But should  _file_set_error_obj(...  -EIO) be called for len == 0
> (EOF) case? ie. function is trying to read from a file, at some point
> it is bound to reach EOF.  '-EIO' indicates an I/O error, reaching EOF
> could not be an error. Maybe we could just return zero(0) ?  (just
> checking)

The migration protocol knows whether it is expecting more data or not.

If we want more data, then a call to qemu_fill_buffer must successfully
read at least 1 byte.

If we don't want more data, then we would not have triggered any call
to qemu_fill_buffer.

Thus, a call to qemu_fill_buffer which gets EOF is an error scenario.

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

On Mon, 4 Aug 2025 at 15:52, Daniel P. Berrangé <berrange@redhat.com> wrote:
> The migration protocol knows whether it is expecting more data or not.
> If we want more data, then a call to qemu_fill_buffer must successfully
> read at least 1 byte.
> If we don't want more data, then we would not have triggered any call
> to qemu_fill_buffer.
> Thus, a call to qemu_fill_buffer which gets EOF is an error scenario.

* I see. In that case the change looks fine.

Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thank you.
---
  - Prasad