Re: [PATCH v2 05/11] io: Add support for seekable channels

[Daniel P . Berrangé]

On Mon, Oct 10, 2022 at 04:34:02PM +0300, Nikolay Borisov wrote:
>  Add a bunch of auxiliarry methods and a feature flag to work with
>  SEEKABLE channels. Currently the only channel considered seekable is
>  QIOChannelFile. Also add a bunch of helper functions to QEMUFile that
>  can make use of this channel feature. All of this is in prepration for
>  supporting 'fixed-ram' migration stream feature.

QIOChannelBuffer/Null are also seekable.

> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  include/io/channel.h                |  1 +
>  include/migration/qemu-file-types.h |  2 +
>  io/channel-file.c                   |  5 +++
>  migration/qemu-file.c               | 59 +++++++++++++++++++++++++++++
>  migration/qemu-file.h               |  3 ++
>  5 files changed, 70 insertions(+)

Can you separate the migration/ tree bits into a second patch
that follows the io/ bits.

> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index c680ee748021..4fc37c78e68c 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -41,6 +41,7 @@ enum QIOChannelFeature {
>      QIO_CHANNEL_FEATURE_SHUTDOWN,
>      QIO_CHANNEL_FEATURE_LISTEN,
>      QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY,
> +    QIO_CHANNEL_FEATURE_SEEKABLE,
>  };
>  
>  
> diff --git a/include/migration/qemu-file-types.h 
> b/include/migration/qemu-file-types.h
> index 2867e3da84ab..eb0325ee8687 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/io/channel-file.c b/io/channel-file.c
> index da17d0a11ba7..d84a6737f2f7 100644
> --- a/io/channel-file.c
> +++ b/io/channel-file.c
> @@ -35,6 +35,7 @@ qio_channel_file_new_fd(int fd)
>  
>      ioc->fd = fd;
>  
> +    qio_channel_set_feature(QIO_CHANNEL(ioc), QIO_CHANNEL_FEATURE_SEEKABLE);
>      trace_qio_channel_file_new_fd(ioc, fd);
>  
>      return ioc;
> @@ -59,6 +60,10 @@ qio_channel_file_new_path(const char *path,
>          return NULL;
>      }
>  
> +    if (lseek(ioc->fd, 0, SEEK_CUR) != (off_t)-1) {
> +        qio_channel_set_feature(QIO_CHANNEL(ioc), 
> QIO_CHANNEL_FEATURE_SEEKABLE);
> +    }
> +
>      trace_qio_channel_file_new_path(ioc, path, flags, mode, ioc->fd);

Wondering why you do the lseek() sanitytest for only one of the
two constructors ? Shouldn't we do it for both ?




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