Re: [Qemu-devel] [PATCH v1 07/22] migration: introduce a new QEMUFile impl based on QIOChannel

[Daniel P. Berrange]

On Tue, Feb 02, 2016 at 05:06:24PM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (address@hidden) wrote:
> > Introduce a new QEMUFile implementation that is based on
> > the QIOChannel objects. This impl is different from existing
> > impls in that there is no file descriptor that can be made
> > available, as some channels may be based on higher level
> > protocols such as TLS.
> > 
> > Although the QIOChannel based implementation can trivially
> > provide a bi-directional stream, initially we have separate
> > functions for opening input & output directions to fit with
> > the expectation of the current QEMUFile interface.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  include/migration/qemu-file.h |   4 +
> >  migration/Makefile.objs       |   1 +
> >  migration/qemu-file-channel.c | 201 
> > ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 206 insertions(+)
> >  create mode 100644 migration/qemu-file-channel.c

> > +static ssize_t channel_writev_buffer(void *opaque,
> > +                                     struct iovec *iov,
> > +                                     int iovcnt,
> > +                                     int64_t pos)
> > +{
> > +    QIOChannel *ioc = QIO_CHANNEL(opaque);
> > +    ssize_t done = 0;
> > +    ssize_t want = iov_size(iov, iovcnt);
> > +    struct iovec oldiov = { NULL, 0 };
> > +
> > +    while (done < want) {
> > +        ssize_t len;
> > +        struct iovec *cur = iov;
> > +        int curcnt = iovcnt;
> > +
> > +        channel_skip_iov(done, &cur, &curcnt, &oldiov);
> > +
> > +        len = qio_channel_writev(ioc, cur, curcnt, NULL);
> > +        if (oldiov.iov_base) {
> > +            /* Restore original caller's info in @iov */
> > +            cur[0].iov_base = oldiov.iov_base;
> > +            cur[0].iov_len = oldiov.iov_len;
> > +            oldiov.iov_base = NULL;
> > +            oldiov.iov_len = 0;
> > +        }
> > +        if (len == QIO_CHANNEL_ERR_BLOCK) {
> > +            qio_channel_wait(ioc, G_IO_OUT);
> > +            continue;
> > +        }
> > +        if (len < 0) {
> > +            /* XXX handle Error objects */
> > +            return -EIO;
> 
> It's best to return 'len' rather than lose what little
> error information you had (similarly below).

In this case 'len' is in fact just '-1', as all error
info is in the Error ** parameter, but the QEMUFile API
contract requires an errno value. So we don't have much
choice but to return a fixed EIO - returning 'len' would
also be a fixed errno - whichever errno corresponds to
the value -1.

I'd like to switch QEMUFile over to use Error **errp
parameters for error reporting, so that we can make
detailed error info available throughout the migration
I/O code. That ought to wait until after this series
is done though, to avoid complicating it yet more.

> 
> > +        }
> > +
> > +        done += len;
> > +    }
> > +    return done;
> > +}
> > +
> > +
> > +static ssize_t channel_get_buffer(void *opaque,
> > +                                  uint8_t *buf,
> > +                                  int64_t pos,
> > +                                  size_t size)
> > +{
> > +    QIOChannel *ioc = QIO_CHANNEL(opaque);
> > +    ssize_t ret;
> > +
> > + reread:
> > +    ret = qio_channel_read(ioc, (char *)buf, size, NULL);
> > +    if (ret < 0) {
> > +        if (ret == QIO_CHANNEL_ERR_BLOCK) {
> > +            qio_channel_yield(ioc, G_IO_IN);
> > +            goto reread;
> > +        } else {
> > +            /* XXX handle Error * object */
> > +            return -EIO;
> > +        }
> > +    }
> > +    return ret;
> 
> 
> I'd prefer a loop to a goto; generally the only places we
> use goto is an error exit.
> 
>   do {
>        ret = qio_channel_read(ioc, (char *)buf, size, NULL);
>        if (ret == QIO_CHANNEL_ERR_BLOCK) {
>            qio_channel_yield(ioc, G_IO_IN);
>        }
>   } while (ret == QIO_CHANNEL_ERR_BLOCK);
> 
>   return ret;
> 
> and IMHO the loop is clearer.

Ok, will change that.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|