Re: [PATCH 3/3] migration/qemu-file: drop incorrect const from qemu_get_buffer_at buf

Re: [PATCH 3/3] migration/qemu-file: drop incorrect const from qemu_get_buffer_at buf

[Philippe Mathieu-Daudé]

On 20/4/26 22:13, Junjie Cao wrote:
> qemu_get_buffer_at() reads data *into* buf -- it should not be const.
> Drop the qualifier and remove the now-unnecessary cast.
> 
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>   migration/qemu-file.c | 4 ++--
>   migration/qemu-file.h | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 0/3] migration: switch remaining positioned I/O to _all helpers

[Junjie Cao]

Follow-up to the pread/pwrite_all API series [1].

qemu_put_buffer_at(), qemu_get_buffer_at(), and file_write_ramblock_iov()
all use single-shot positioned I/O without retry.  They share the same
pattern as the bug fixed in multifd_file_recv_data() where a short
transfer would be mishandled.

Convert them to the _all helpers and clean up a pre-existing
const-correctness issue in qemu_get_buffer_at() along the way.

After this series no positioned I/O call site in migration/ uses raw
single-shot channel operations.

Depends-on: <20260413214549.926435-1-junjie.cao@intel.com>
("io/channel: complete pread/pwrite_all API and fix
multifd_file_recv_data")

[1] https://lore.kernel.org/qemu-devel/20260413214549.926435-1-junjie.cao@intel.com/

Junjie Cao (3):
  migration/qemu-file: switch buffer_at functions to positioned I/O _all
    helpers
  migration/file: switch file_write_ramblock_iov to pwritev_all
  migration/qemu-file: drop incorrect const from qemu_get_buffer_at buf

 migration/file.c      |  8 ++++----
 migration/qemu-file.c | 37 ++++---------------------------------
 migration/qemu-file.h |  2 +-
 3 files changed, 9 insertions(+), 38 deletions(-)

-- 
2.43.0

[PATCH 1/3] migration/qemu-file: switch buffer_at functions to positioned I/O _all helpers

[Junjie Cao]

qemu_put_buffer_at() and qemu_get_buffer_at() have the same pattern as
the bug fixed in multifd_file_recv_data(): the ssize_t return value from
the channel layer is stored in a size_t variable, and a short transfer
would be mishandled rather than retried.

Switch to qio_channel_pwrite_all() / qio_channel_pread_all() which
handle short transfers internally and make the code more robust and
consistent with the rest of the positioned I/O call sites.

Fixes: 7f5b50a401 ("migration/qemu-file: add utility methods for working with seekable channels")
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 migration/qemu-file.c | 35 +++--------------------------------
 1 file changed, 3 insertions(+), 32 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 9cf7dc3bd5..9dfb202203 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -535,28 +535,13 @@ void qemu_put_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
                         off_t pos)
 {
     Error *err = NULL;
-    size_t ret;
 
     if (f->last_error) {
         return;
     }
 
     qemu_fflush(f);
-    ret = qio_channel_pwrite(f->ioc, (char *)buf, buflen, pos, &err);
-
-    if (err) {
-        qemu_file_set_error_obj(f, -EIO, err);
-        return;
-    }
-
-    if ((ssize_t)ret == QIO_CHANNEL_ERR_BLOCK) {
-        qemu_file_set_error_obj(f, -EAGAIN, NULL);
-        return;
-    }
-
-    if (ret != buflen) {
-        error_setg(&err, "Partial write of size %zu, expected %zu", ret,
-                   buflen);
+    if (qio_channel_pwrite_all(f->ioc, buf, buflen, pos, &err) < 0) {
         qemu_file_set_error_obj(f, -EIO, err);
         return;
     }
@@ -569,31 +554,17 @@ size_t qemu_get_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
                           off_t pos)
 {
     Error *err = NULL;
-    size_t ret;
 
     if (f->last_error) {
         return 0;
     }
 
-    ret = qio_channel_pread(f->ioc, (char *)buf, buflen, pos, &err);
-
-    if ((ssize_t)ret == -1 || err) {
+    if (qio_channel_pread_all(f->ioc, (char *)buf, buflen, pos, &err) < 0) {
         qemu_file_set_error_obj(f, -EIO, err);
         return 0;
     }
 
-    if ((ssize_t)ret == QIO_CHANNEL_ERR_BLOCK) {
-        qemu_file_set_error_obj(f, -EAGAIN, NULL);
-        return 0;
-    }
-
-    if (ret != buflen) {
-        error_setg(&err, "Partial read of size %zu, expected %zu", ret, buflen);
-        qemu_file_set_error_obj(f, -EIO, err);
-        return 0;
-    }
-
-    return ret;
+    return buflen;
 }
 
 void qemu_set_offset(QEMUFile *f, off_t off, int whence)
-- 
2.43.0

[PATCH 2/3] migration/file: switch file_write_ramblock_iov to pwritev_all

[Junjie Cao]

file_write_ramblock_iov() uses single-shot qio_channel_pwritev() and
only checks for ret < 0.  A short write (0 <= ret < requested) would be
treated as success.

Switch to qio_channel_pwritev_all() which retries until all bytes are
written or an error occurs.

Fixes: f427d90b98 ("migration/multifd: Support outgoing mapped-ram stream format")
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 migration/file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/file.c b/migration/file.c
index 0853188601..3729728dfe 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -200,7 +200,7 @@ void file_connect_incoming(FileMigrationArgs *file_args, Error **errp)
 int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
                             int niov, MultiFDPages_t *pages, Error **errp)
 {
-    ssize_t ret = 0;
+    int ret = 0;
     int i, slice_idx, slice_num;
     uintptr_t base, next, offset;
     size_t len;
@@ -239,8 +239,8 @@ int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
             break;
         }
 
-        ret = qio_channel_pwritev(ioc, &iov[slice_idx], slice_num,
-                                  block->pages_offset + offset, errp);
+        ret = qio_channel_pwritev_all(ioc, &iov[slice_idx], slice_num,
+                                      block->pages_offset + offset, errp);
         if (ret < 0) {
             break;
         }
@@ -249,7 +249,7 @@ int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
         slice_num = 0;
     }
 
-    return (ret < 0) ? ret : 0;
+    return ret;
 }
 
 int multifd_file_recv_data(MultiFDRecvParams *p, Error **errp)
-- 
2.43.0

[PATCH 3/3] migration/qemu-file: drop incorrect const from qemu_get_buffer_at buf

[Junjie Cao]

qemu_get_buffer_at() reads data *into* buf -- it should not be const.
Drop the qualifier and remove the now-unnecessary cast.

Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 migration/qemu-file.c | 4 ++--
 migration/qemu-file.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 9dfb202203..d5a48115bd 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -550,7 +550,7 @@ void qemu_put_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
 }
 
 
-size_t qemu_get_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
+size_t qemu_get_buffer_at(QEMUFile *f, uint8_t *buf, size_t buflen,
                           off_t pos)
 {
     Error *err = NULL;
@@ -559,7 +559,7 @@ size_t qemu_get_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
         return 0;
     }
 
-    if (qio_channel_pread_all(f->ioc, (char *)buf, buflen, pos, &err) < 0) {
+    if (qio_channel_pread_all(f->ioc, buf, buflen, pos, &err) < 0) {
         qemu_file_set_error_obj(f, -EIO, err);
         return 0;
     }
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index a390554208..8f824c124d 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -76,7 +76,7 @@ void qemu_set_offset(QEMUFile *f, off_t off, int whence);
 off_t qemu_get_offset(QEMUFile *f);
 void qemu_put_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
                         off_t pos);
-size_t qemu_get_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
+size_t qemu_get_buffer_at(QEMUFile *f, uint8_t *buf, size_t buflen,
                           off_t pos);
 
 QIOChannel *qemu_file_get_ioc(QEMUFile *file);
-- 
2.43.0

Re: [PATCH 0/3] migration: switch remaining positioned I/O to _all helpers

[Peter Xu]

On Tue, Apr 21, 2026 at 04:13:14AM +0800, Junjie Cao wrote:
> Follow-up to the pread/pwrite_all API series [1].
> 
> qemu_put_buffer_at(), qemu_get_buffer_at(), and file_write_ramblock_iov()
> all use single-shot positioned I/O without retry.  They share the same
> pattern as the bug fixed in multifd_file_recv_data() where a short
> transfer would be mishandled.
> 
> Convert them to the _all helpers and clean up a pre-existing
> const-correctness issue in qemu_get_buffer_at() along the way.
> 
> After this series no positioned I/O call site in migration/ uses raw
> single-shot channel operations.
> 
> Depends-on: <20260413214549.926435-1-junjie.cao@intel.com>
> ("io/channel: complete pread/pwrite_all API and fix
> multifd_file_recv_data")
> 
> [1] https://lore.kernel.org/qemu-devel/20260413214549.926435-1-junjie.cao@intel.com/
> 
> Junjie Cao (3):
>   migration/qemu-file: switch buffer_at functions to positioned I/O _all
>     helpers
>   migration/file: switch file_write_ramblock_iov to pwritev_all
>   migration/qemu-file: drop incorrect const from qemu_get_buffer_at buf

Thanks a lot, Junjie.  All look sane.

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

-- 
Peter Xu

Re: [PATCH 0/3] migration: switch remaining positioned I/O to _all helpers

[Michael Tokarev]

On 20.04.2026 23:13, Junjie Cao wrote:
> Follow-up to the pread/pwrite_all API series [1].
> 
> qemu_put_buffer_at(), qemu_get_buffer_at(), and file_write_ramblock_iov()
> all use single-shot positioned I/O without retry.  They share the same
> pattern as the bug fixed in multifd_file_recv_data() where a short
> transfer would be mishandled.
> 
> Convert them to the _all helpers and clean up a pre-existing
> const-correctness issue in qemu_get_buffer_at() along the way.
> 
> After this series no positioned I/O call site in migration/ uses raw
> single-shot channel operations.
> 
> Depends-on: <20260413214549.926435-1-junjie.cao@intel.com>
> ("io/channel: complete pread/pwrite_all API and fix
> multifd_file_recv_data")
> 
> [1] https://lore.kernel.org/qemu-devel/20260413214549.926435-1-junjie.cao@intel.com/
> 
> Junjie Cao (3):
>    migration/qemu-file: switch buffer_at functions to positioned I/O _all
>      helpers
>    migration/file: switch file_write_ramblock_iov to pwritev_all
>    migration/qemu-file: drop incorrect const from qemu_get_buffer_at buf
> 
>   migration/file.c      |  8 ++++----
>   migration/qemu-file.c | 37 ++++---------------------------------
>   migration/qemu-file.h |  2 +-
>   3 files changed, 9 insertions(+), 38 deletions(-)

And just like the previous changes in this area, - should I pick these 3
to the current qemu-stable series, or is it not needed?

Applies to 10.0.x series and passes tests, but I haven't done any more
testing here.

Thanks,

/mjt

Re: [PATCH 0/3] migration: switch remaining positioned I/O to _all helpers

[Peter Xu]

On Sun, Apr 26, 2026 at 11:27:08AM +0300, Michael Tokarev wrote:
> On 20.04.2026 23:13, Junjie Cao wrote:
> > Follow-up to the pread/pwrite_all API series [1].
> > 
> > qemu_put_buffer_at(), qemu_get_buffer_at(), and file_write_ramblock_iov()
> > all use single-shot positioned I/O without retry.  They share the same
> > pattern as the bug fixed in multifd_file_recv_data() where a short
> > transfer would be mishandled.
> > 
> > Convert them to the _all helpers and clean up a pre-existing
> > const-correctness issue in qemu_get_buffer_at() along the way.
> > 
> > After this series no positioned I/O call site in migration/ uses raw
> > single-shot channel operations.
> > 
> > Depends-on: <20260413214549.926435-1-junjie.cao@intel.com>
> > ("io/channel: complete pread/pwrite_all API and fix
> > multifd_file_recv_data")
> > 
> > [1] https://lore.kernel.org/qemu-devel/20260413214549.926435-1-junjie.cao@intel.com/
> > 
> > Junjie Cao (3):
> >    migration/qemu-file: switch buffer_at functions to positioned I/O _all
> >      helpers
> >    migration/file: switch file_write_ramblock_iov to pwritev_all
> >    migration/qemu-file: drop incorrect const from qemu_get_buffer_at buf
> > 
> >   migration/file.c      |  8 ++++----
> >   migration/qemu-file.c | 37 ++++---------------------------------
> >   migration/qemu-file.h |  2 +-
> >   3 files changed, 9 insertions(+), 38 deletions(-)
> 
> And just like the previous changes in this area, - should I pick these 3
> to the current qemu-stable series, or is it not needed?
> 
> Applies to 10.0.x series and passes tests, but I haven't done any more
> testing here.

My understanding is we don't yet have a real report / reproducer for the
partial-io issue fixed here. Considering the size of the patchsets
(especially the dependency series), maybe we can skip these two sets of
patches until we know a solid report from anyone.

Junjie, did you find & fix this problem due to a real bug, or it was
theoretical while you were reading the code?

Thanks,

-- 
Peter Xu

Re: [PATCH 0/3] migration: switch remaining positioned I/O to _all helpers

[Junjie Cao]

Hi Peter and Michael,

No, I don't have a real reproducer.  Triggering partial I/O on regular
files in practice requires unusual conditions, the window is narrow.
Agree with skipping stable for both sets.

Many thanks,
Junjie