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

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

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

Yep.

>> +
>> +    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;
>   }
>

Could be. I'll change it.

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