Re: [PATCH 17/20] migration: remove the QEMUFileOps 'get_buffer' callback

[Daniel P . Berrangé]

On Thu, Jun 09, 2022 at 05:46:29PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > This directly implements the get_buffer logic using QIOChannel APIs.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  migration/qemu-file-channel.c | 29 -----------------------------
> >  migration/qemu-file.c         | 18 ++++++++++++++++--
> >  migration/qemu-file.h         |  9 ---------
> >  3 files changed, 16 insertions(+), 40 deletions(-)
> > 
> > diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> > index 8ff58e81f9..7b32831752 100644
> > --- a/migration/qemu-file-channel.c
> > +++ b/migration/qemu-file-channel.c
> > @@ -74,34 +74,6 @@ static ssize_t channel_writev_buffer(void *opaque,
> >  }
> >  
> >  
> > -static ssize_t channel_get_buffer(void *opaque,
> > -                                  uint8_t *buf,
> > -                                  int64_t pos,
> > -                                  size_t size,
> > -                                  Error **errp)
> > -{
> > -    QIOChannel *ioc = QIO_CHANNEL(opaque);
> > -    ssize_t ret;
> > -
> > -    do {
> > -        ret = qio_channel_read(ioc, (char *)buf, size, errp);
> > -        if (ret < 0) {
> > -            if (ret == QIO_CHANNEL_ERR_BLOCK) {
> > -                if (qemu_in_coroutine()) {
> > -                    qio_channel_yield(ioc, G_IO_IN);
> > -                } else {
> > -                    qio_channel_wait(ioc, G_IO_IN);
> > -                }
> > -            } else {
> > -                return -EIO;
> > -            }
> > -        }
> > -    } while (ret == QIO_CHANNEL_ERR_BLOCK);
> > -
> > -    return ret;
> > -}
> > -
> > -
> >  static QEMUFile *channel_get_input_return_path(void *opaque)
> >  {
> >      QIOChannel *ioc = QIO_CHANNEL(opaque);
> > @@ -117,7 +89,6 @@ static QEMUFile *channel_get_output_return_path(void 
> > *opaque)
> >  }
> >  
> >  static const QEMUFileOps channel_input_ops = {
> > -    .get_buffer = channel_get_buffer,
> >      .get_return_path = channel_get_input_return_path,
> >  };
> >  
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > index a855ce33dc..e024b43851 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -374,8 +374,22 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> >          return 0;
> >      }
> >  
> > -    len = f->ops->get_buffer(f->ioc, f->buf + pending, 
> > f->total_transferred,
> > -                             IO_BUF_SIZE - pending, &local_error);
> > +    do {
> > +        len = qio_channel_read(f->ioc,
> 
> Yes, I think that's OK - not that 'len' is an int where 'ret'
> was a ssize_t; but I think our buffers are guranteed to be 'small'.

There are a few places in qemu-file.c where we're fast & loose
with int rather than size_t, that are probably worth cleaning.

> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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