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

[Nikolay Borisov]

On 18.10.22 г. 13:14 ч., Daniel P. Berrangé wrote:
> 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.

Right, however I think for seek we also want to rely on the feature of 
filesystem that when you seek beyond EOF you won't actually allocate/use 
up the space from (eof, CUR_SEEK), with ChannelBuffer we'd have to 
actually allocate the space or add this support on top.

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

Good point, AFAIR my train of thought was something along the lines of 
"what if the path being passed actually lies on a remote fs, we 
definitely need to test for this support", however now that I think 
about it a bit more - nothing prevents for the passed in fd to also be 
lying on a remote fs, so good point. Will fix this in next iteration.

> With regards,
> Daniel