Re: [PATCH 14/20] migration: remove the QEMUFileOps 'shut_down' callback

[Daniel P . Berrangé]

On Thu, Jun 09, 2022 at 05:12:41PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > This directly implements the shutdown logic using QIOChannel APIs.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  migration/qemu-file-channel.c | 27 ---------------------------
> >  migration/qemu-file.c         | 10 +++++++---
> >  migration/qemu-file.h         | 10 ----------
> >  3 files changed, 7 insertions(+), 40 deletions(-)
> > 
> > diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> > index 5cb8ac93c0..80f05dc371 100644
> > --- a/migration/qemu-file-channel.c
> > +++ b/migration/qemu-file-channel.c
> > @@ -112,31 +112,6 @@ static int channel_close(void *opaque, Error **errp)
> >  }
> >  
> >  
> > -static int channel_shutdown(void *opaque,
> > -                            bool rd,
> > -                            bool wr,
> > -                            Error **errp)
> > -{
> > -    QIOChannel *ioc = QIO_CHANNEL(opaque);
> > -
> > -    if (qio_channel_has_feature(ioc,
> > -                                QIO_CHANNEL_FEATURE_SHUTDOWN)) {
> > -        QIOChannelShutdown mode;
> > -        if (rd && wr) {
> > -            mode = QIO_CHANNEL_SHUTDOWN_BOTH;
> > -        } else if (rd) {
> > -            mode = QIO_CHANNEL_SHUTDOWN_READ;
> > -        } else {
> > -            mode = QIO_CHANNEL_SHUTDOWN_WRITE;
> > -        }
> > -        if (qio_channel_shutdown(ioc, mode, errp) < 0) {
> > -            return -EIO;
> > -        }
> > -    }
> > -    return 0;
> > -}
> > -
> > -
> >  static int channel_set_blocking(void *opaque,
> >                                  bool enabled,
> >                                  Error **errp)
> > @@ -166,7 +141,6 @@ static QEMUFile *channel_get_output_return_path(void 
> > *opaque)
> >  static const QEMUFileOps channel_input_ops = {
> >      .get_buffer = channel_get_buffer,
> >      .close = channel_close,
> > -    .shut_down = channel_shutdown,
> >      .set_blocking = channel_set_blocking,
> >      .get_return_path = channel_get_input_return_path,
> >  };
> > @@ -175,7 +149,6 @@ static const QEMUFileOps channel_input_ops = {
> >  static const QEMUFileOps channel_output_ops = {
> >      .writev_buffer = channel_writev_buffer,
> >      .close = channel_close,
> > -    .shut_down = channel_shutdown,
> >      .set_blocking = channel_set_blocking,
> >      .get_return_path = channel_get_output_return_path,
> >  };
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > index 5548e1abf3..fd9f060c02 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -74,13 +74,17 @@ struct QEMUFile {
> >   */
> >  int qemu_file_shutdown(QEMUFile *f)
> >  {
> > -    int ret;
> > +    int ret = 0;
> >  
> >      f->shutdown = true;
> > -    if (!f->ops->shut_down) {
> > +    if (!qio_channel_has_feature(f->ioc,
> > +                                 QIO_CHANNEL_FEATURE_SHUTDOWN)) {
> >          return -ENOSYS;
> >      }
> > -    ret = f->ops->shut_down(f->ioc, true, true, NULL);
> > +
> > +    if (qio_channel_shutdown(f->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL) < 0) 
> > {
> > +        ret = -EIO;
> > +    }
> 
> OK, so this is following the code you're flattening; so:
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
>  I wonder if there's any reason it doesn't just pass the return value through 
> to ret rather
> than flattening it to -EIO?

qio methods never return errno values just positive integer or -1.

Since qemu_file_shutdown seems to want an errno, I picked EIO

Better would be for qemu_file_shutdown to have an Error **errp
param instead but that could come later.


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