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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK