Re: [RFC PATCH v3 05/30] migration/qemu-file: add utility methods for working with seekable channels

[Peter Xu]

On Mon, Nov 27, 2023 at 05:25:47PM -0300, Fabiano Rosas wrote:
> From: Nikolay Borisov <nborisov@suse.com>
> 
> Add utility methods that will be needed when implementing 'fixed-ram'
> migration capability.
> 
> qemu_file_is_seekable
> qemu_put_buffer_at
> qemu_get_buffer_at
> qemu_set_offset
> qemu_get_offset
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/migration/qemu-file-types.h |  2 +
>  migration/qemu-file.c               | 82 +++++++++++++++++++++++++++++
>  migration/qemu-file.h               |  6 +++
>  3 files changed, 90 insertions(+)
> 
> diff --git a/include/migration/qemu-file-types.h 
> b/include/migration/qemu-file-types.h
> index 9ba163f333..adec5abc07 100644
> --- a/include/migration/qemu-file-types.h
> +++ b/include/migration/qemu-file-types.h
> @@ -50,6 +50,8 @@ unsigned int qemu_get_be16(QEMUFile *f);
>  unsigned int qemu_get_be32(QEMUFile *f);
>  uint64_t qemu_get_be64(QEMUFile *f);
>  
> +bool qemu_file_is_seekable(QEMUFile *f);
> +
>  static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
>  {
>      qemu_put_be64(f, *pv);
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 94231ff295..faf6427b91 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -33,6 +33,7 @@
>  #include "options.h"
>  #include "qapi/error.h"
>  #include "rdma.h"
> +#include "io/channel-file.h"
>  
>  #define IO_BUF_SIZE 32768
>  #define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)
> @@ -255,6 +256,10 @@ static void qemu_iovec_release_ram(QEMUFile *f)
>      memset(f->may_free, 0, sizeof(f->may_free));
>  }
>  
> +bool qemu_file_is_seekable(QEMUFile *f)
> +{
> +    return qio_channel_has_feature(f->ioc, QIO_CHANNEL_FEATURE_SEEKABLE);
> +}
>  
>  /**
>   * Flushes QEMUFile buffer
> @@ -447,6 +452,83 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, 
> size_t size)
>      }
>  }
>  
> +void qemu_put_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
> +                        off_t pos)
> +{
> +    Error *err = NULL;
> +
> +    if (f->last_error) {
> +        return;
> +    }
> +
> +    qemu_fflush(f);
> +    qio_channel_pwrite(f->ioc, (char *)buf, buflen, pos, &err);

Partial writes won't set err.  Do we want to check the retval here too and
fail properly if detected partial writes?

> +
> +    if (err) {
> +        qemu_file_set_error_obj(f, -EIO, err);
> +    } else {
> +        stat64_add(&mig_stats.qemu_file_transferred, buflen);

buflen is only accurate if with above, iiuc.

> +    }
> +
> +    return;
> +}
> +
> +
> +size_t qemu_get_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
> +                          off_t pos)
> +{
> +    Error *err = NULL;
> +    ssize_t ret;
> +
> +    if (f->last_error) {
> +        return 0;
> +    }
> +
> +    ret = qio_channel_pread(f->ioc, (char *)buf, buflen, pos, &err);

Same question here.

> +    if (ret == -1 || err) {
> +        goto error;
> +    }
> +
> +    return (size_t)ret;
> +
> + error:
> +    qemu_file_set_error_obj(f, -EIO, err);
> +    return 0;
> +}
> +
> +void qemu_set_offset(QEMUFile *f, off_t off, int whence)
> +{
> +    Error *err = NULL;
> +    off_t ret;
> +
> +    qemu_fflush(f);
> +
> +    if (!qemu_file_is_writable(f)) {
> +        f->buf_index = 0;
> +        f->buf_size = 0;
> +    }

There's the qemu_file_is_writable() check after all, then put qemu_fflush()
into condition too?

  if (qemu_file_is_writable(f)) {
      qemu_fflush(f);
  } else {
     /* Drop all the cached buffers if existed; will trigger a re-fill later */
     f->buf_index = 0;
     f->buf_size = 0;
  }

> +
> +    ret = qio_channel_io_seek(f->ioc, off, whence, &err);
> +    if (ret == (off_t)-1) {
> +        qemu_file_set_error_obj(f, -EIO, err);
> +    }
> +}
> +
> +off_t qemu_get_offset(QEMUFile *f)
> +{
> +    Error *err = NULL;
> +    off_t ret;
> +
> +    qemu_fflush(f);
> +
> +    ret = qio_channel_io_seek(f->ioc, 0, SEEK_CUR, &err);
> +    if (ret == (off_t)-1) {
> +        qemu_file_set_error_obj(f, -EIO, err);
> +    }
> +    return ret;
> +}
> +
> +
>  void qemu_put_byte(QEMUFile *f, int v)
>  {
>      if (f->last_error) {
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 8aec9fabf7..32fd4a34fd 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -75,6 +75,12 @@ QEMUFile *qemu_file_get_return_path(QEMUFile *f);
>  int qemu_fflush(QEMUFile *f);
>  void qemu_file_set_blocking(QEMUFile *f, bool block);
>  int qemu_file_get_to_fd(QEMUFile *f, int fd, size_t size);
> +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,
> +                          off_t pos);
>  
>  QIOChannel *qemu_file_get_ioc(QEMUFile *file);
>  
> -- 
> 2.35.3
> 

-- 
Peter Xu