diff mbox series

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

Message ID 20220524110235.145079-15-berrange@redhat.com (mailing list archive)
State New, archived
Headers show
Series migration: remove QEMUFileOps concept and assume use of QIOChannel | expand

Commit Message

Daniel P. Berrangé May 24, 2022, 11:02 a.m. UTC 
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(-)

Comments

Dr. David Alan Gilbert June 9, 2022, 4:12 p.m. UTC | #1 
* 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?


>      if (!f->last_error) {
>          qemu_file_set_error(f, -EIO);
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 674c2c409b..2049dfe7e4 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -89,22 +89,12 @@ typedef size_t (QEMURamSaveFunc)(QEMUFile *f,
>   */
>  typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
>  
> -/*
> - * Stop any read or write (depending on flags) on the underlying
> - * transport on the QEMUFile.
> - * Existing blocking reads/writes must be woken
> - * Returns 0 on success, -err on error
> - */
> -typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr,
> -                                   Error **errp);
> -
>  typedef struct QEMUFileOps {
>      QEMUFileGetBufferFunc *get_buffer;
>      QEMUFileCloseFunc *close;
>      QEMUFileSetBlocking *set_blocking;
>      QEMUFileWritevBufferFunc *writev_buffer;
>      QEMURetPathFunc *get_return_path;
> -    QEMUFileShutdownFunc *shut_down;
>  } QEMUFileOps;
>  
>  typedef struct QEMUFileHooks {
> -- 
> 2.36.1
>
Daniel P. Berrangé June 9, 2022, 4:14 p.m. UTC | #2 
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
Dr. David Alan Gilbert June 9, 2022, 4:17 p.m. UTC | #3 
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> 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.

Ah OK.

Dave

> 
> 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 :|
>
diff mbox series

Patch

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;
+    }
 
     if (!f->last_error) {
         qemu_file_set_error(f, -EIO);
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 674c2c409b..2049dfe7e4 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -89,22 +89,12 @@  typedef size_t (QEMURamSaveFunc)(QEMUFile *f,
  */
 typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
 
-/*
- * Stop any read or write (depending on flags) on the underlying
- * transport on the QEMUFile.
- * Existing blocking reads/writes must be woken
- * Returns 0 on success, -err on error
- */
-typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr,
-                                   Error **errp);
-
 typedef struct QEMUFileOps {
     QEMUFileGetBufferFunc *get_buffer;
     QEMUFileCloseFunc *close;
     QEMUFileSetBlocking *set_blocking;
     QEMUFileWritevBufferFunc *writev_buffer;
     QEMURetPathFunc *get_return_path;
-    QEMUFileShutdownFunc *shut_down;
 } QEMUFileOps;
 
 typedef struct QEMUFileHooks {