Re: [Qemu-devel] [PATCH v4 16/47] Return path: Source handling of return path

[David Gibson]

On Mon, Nov 03, 2014 at 01:22:45PM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (address@hidden) wrote:
> > On Fri, Oct 03, 2014 at 06:47:22PM +0100, Dr. David Alan Gilbert (git) 
> > wrote:
> > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > 
> > > Open a return path, and handle messages that are received upon it.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > 
> > [snip]
> > > @@ -414,6 +448,11 @@ static void migrate_fd_cancel(MigrationState *s)
> > >      int old_state ;
> > >      trace_migrate_fd_cancel();
> > >  
> > > +    if (s->return_path) {
> > > +        /* shutdown the rp socket, so causing the rp thread to shutdown 
> > > */
> > > +        qemu_file_shutdown(s->return_path);
> > 
> > Terminating the rp thread via shutting down its file seems roundabout,
> > and kind of dependent on the socket file implementation.
> 
> The rp thread might be in the middle of a blocking read()/recv()
> so I'm doing a shutdown() to cause those to exit; once I have to do that
> anyway it didn't seem necessary to add anything etra.

Hm.  I don't recall, does the rp thread need to do some cleanup at
this point?  Otherwise pthread_cancel() should kill a thread, even if
it's blocked at the moment.

> > [snip]
> > > +__attribute__ (( unused )) /* Until later in patch series */
> > > +static int open_outgoing_return_path(MigrationState *ms)
> > > +{
> > > +
> > > +    ms->return_path = qemu_file_get_return_path(ms->file);
> > 
> > So, another reason this get_return_path abstraction doesn't seem right
> > to me, is that it's not obvious that for non-socket file types, the
> > source and destination side "get return path" operations would
> > necessarily be the same.
> 
> However, since the implementation of the get_return_path is a method
> on the particular implementation, and it can be different for a 
> qemu_file opened for read or write, then that non-socket file type
> could implement it how it likes including something like shutdown).

So, I'm a little less bothered by this since I realised that QemuFile
is basically only used for migration streams, not for other file type
operations.  The fact that that makes QemuFile a really bad name is a
different matter.

The return path operation is quite specific to a migration stream, and
doesn't really belong with a "file" abstraction.

The case I've been considering where it's not easy to see how to
abstract this is that of a pipe - in that case it will be necessary to
open a second pipe from destination to source, which probably needs
some preliminary work when first opening the connection, and therefore
can't easily be encapsulated into a "get return path" callback.

The abstraction of the shutdown is another question again - I can't
think of any other file type which has an operation similar in effect
to shutdown(), so it seems really socket specific. Which is another
reason I'm not convinced telling the rp thread to die via its stream
is a good idea.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpScQr1qXevN.pgp
Description: PGP signature