[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 14/20] migration: remove the QEMUFileOps 'shut_down' callback
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH 14/20] migration: remove the QEMUFileOps 'shut_down' callback |
Date: |
Thu, 9 Jun 2022 17:17:49 +0100 |
User-agent: |
Mutt/2.2.1 (2022-02-19) |
* 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 :|
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK