From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43532) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zi3kS-0006Bi-9S for qemu-devel@nongnu.org; Fri, 02 Oct 2015 13:03:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zi3kO-0005gT-V4 for qemu-devel@nongnu.org; Fri, 02 Oct 2015 13:03:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53412) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zi3kO-0005g0-81 for qemu-devel@nongnu.org; Fri, 02 Oct 2015 13:03:52 -0400 Date: Fri, 2 Oct 2015 18:03:47 +0100 From: "Daniel P. Berrange" Message-ID: <20151002170347.GJ28469@redhat.com> References: <1443515898-3594-1-git-send-email-dgilbert@redhat.com> <1443515898-3594-17-git-send-email-dgilbert@redhat.com> <20151002152925.GI28469@redhat.com> <20151002163218.GB8601@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20151002163218.GB8601@work-vm> Subject: Re: [Qemu-devel] [PATCH v8 16/54] Return path: Open a return path on QEMUFile for sockets Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: aarcange@redhat.com, quintela@redhat.com, liang.z.li@intel.com, qemu-devel@nongnu.org, luis@cs.umu.se, bharata@linux.vnet.ibm.com, amit.shah@redhat.com, pbonzini@redhat.com On Fri, Oct 02, 2015 at 05:32:18PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrange (berrange@redhat.com) wrote: > > On Tue, Sep 29, 2015 at 09:37:40AM +0100, Dr. David Alan Gilbert (git) wrote: > > > From: "Dr. David Alan Gilbert" > > > > > > Postcopy needs a method to send messages from the destination back to > > > the source, this is the 'return path'. > > > > > > Wire it up for 'socket' QEMUFile's. > > > > I find this to be a pretty wierd approach to the problem. THe underlying > > transport is bi-directional, so I would expect to have a single QEMUFile > > object that allowed bi-directional I/O on it, rather than creating a > > second QEMUFile for the back channel, which was forbidden from closing > > the shared FD. > > > > I can understand why you've done this though - since we only have a > > single buffer embedded in QEMUFile. I wonder though if we'd be better > > off changing QEMUFile to have a 'inbuf' and 'outbuf' intead of just > > 'buf' and likewise iniov & outiov. Then we can allow bi-directional > > I/O on the single QEMUFile object which is a more natural fit. > > The 'c' FILE* is one directional, and I just took it that the QEMUFile* is > like that; i.e. a buffered layer on top of an underlying one directional > transport. stdin,stdout are two separate FILE*'s. Yep, QEMUFile was really designed as a FILE* alternative, so makes sense from that POV. > Your iniov, outiov would be basically the same, so you'd end up duplicating > code for the in and out parts; where as what you really have is two of the same > thing wired up in opposite directions. I don't think it'd actually end up duplicating any code - mostly just updating which variable was accessed in each existing method, depending on whether it was a read or write related method. > Having said that, for things like RDMA, they have to do special stuff for > each direction and the QEMUFile is really a shim on top of that. Similarly when we add TLS into the mix, there is a single shared TLS session context that is used by both I/O directionals. Now this would not be visible to the QEMUFile regardless, since its hidden in the QIOChannel object I'm defining, so its not a show stopper either but I guess my general thought is that there is a mixture of state that we maintain some different for read vs write and some shared. You workaround the fact that the FD is shared by having a comment saying we should not call close() on the FD kept by the QEMUFile for the return path. All that said, I don't think it is too critical to change this right now. It would be fine to leave it to a later date, unless there's a more pressing reason. 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 :|