[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 07/22] migration: introduce a new QEMUFile im
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v1 07/22] migration: introduce a new QEMUFile impl based on QIOChannel |
Date: |
Fri, 12 Feb 2016 17:16:53 +0000 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
* Daniel P. Berrange (address@hidden) wrote:
> 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.
Oh, I see - that's a shame that they didn't pass the
error along, but yeh if they're just returning -1 that makes
sense.
> 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.
OK, you could use a local_error I guess; I just worry
about not having any error information at all.
Dave
>
> >
> > > + }
> > > +
> > > + 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 :|
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK