From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUHL3-00018A-Ph for qemu-devel@nongnu.org; Fri, 12 Feb 2016 12:17:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aUHKz-0005mq-Vh for qemu-devel@nongnu.org; Fri, 12 Feb 2016 12:17:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36898) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUHKz-0005mU-P3 for qemu-devel@nongnu.org; Fri, 12 Feb 2016 12:16:57 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 55763C003587 for ; Fri, 12 Feb 2016 17:16:57 +0000 (UTC) Date: Fri, 12 Feb 2016 17:16:53 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20160212171652.GH2411@work-vm> References: <1452599056-27357-1-git-send-email-berrange@redhat.com> <1452599056-27357-8-git-send-email-berrange@redhat.com> <20160202170623.GA4498@work-vm> <20160203133725.GM30222@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160203133725.GM30222@redhat.com> Subject: Re: [Qemu-devel] [PATCH v1 07/22] migration: introduce a new QEMUFile impl based on QIOChannel List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Amit Shah , qemu-devel@nongnu.org, Juan Quintela * Daniel P. Berrange (berrange@redhat.com) wrote: > On Tue, Feb 02, 2016 at 05:06:24PM +0000, Dr. David Alan Gilbert wrote: > > * Daniel P. Berrange (berrange@redhat.com) 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 > > > --- > > > 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 / dgilbert@redhat.com / Manchester, UK