Re: [Qemu-devel] [PATCH v8 16/54] Return path: Open a return path on QEMUFile for sockets

[Daniel P. Berrange]

On Fri, Oct 02, 2015 at 05:32:18PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (address@hidden) wrote:
> > On Tue, Sep 29, 2015 at 09:37:40AM +0100, Dr. David Alan Gilbert (git) 
> > wrote:
> > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > 
> > > 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 :|