On Wed, Nov 19, 2014 at 05:06:50PM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (da...@gibson.dropbear.id.au) wrote:
> > On Mon, Nov 03, 2014 at 01:22:45PM +0000, Dr. David Alan Gilbert wrote:
> > > * David Gibson (da...@gibson.dropbear.id.au) wrote:
> > > > On Fri, Oct 03, 2014 at 06:47:22PM +0100, Dr. David Alan Gilbert (git)
> > > > wrote:
> > > > > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> > > > >
> > > > > Open a return path, and handle messages that are received upon it.
> > > > >
> > > > > Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> > > >
> > > > [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.
>
> It was Paolo's idea to use shutdown() and I agree - it works well;
> I'd originally thought about using pthread_cancel but it seemed to be
> generally disliked - you have to be very careful to either know exactly
> the points at which it might be killed (if you use the deferred version)
> or be prepared to deal with your thread disappearing at any time and
> ensure your data structures are always consistent. In addition there
> was some concern that there was no Windows equivalent to pthread_cancel.
Hmm, yeah all right.
> > > > [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.
>
> Yes, but hey we've got FILE* in C anyway, so it might be bad, but it's
> not inconsitent.
Uh.. not following the analogy here, sorry.
> > The return path operation is quite specific to a migration stream, and
> > doesn't really belong with a "file" abstraction.
>
> I think the bit that's specific, is as you say that I don't know whether
> I need it until later.
>
> > 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.
>
> I'm OK with some transports not supporting this; I check for it and
> error out. At a higher level I do send an 'open_return_path' command
> from src->dest early on to say I'm going to want a return path, I guess
> a pipe might be able to open that fd then and pass it back over the original
> fd? But that might be hairy.
You can pass fds over Unix sockets, but not over pipes AFAIK.
> > 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.
>
> I'd be OK with setting some flag or similar at the same time if that
> would help; but I don't think there's a safe posix'y way of killing a
> thread that might be stuck in a recv()/read() other than shutdown().
>
> Dave
>
--
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
pgpqcupq2vZUd.pgp
Description: PGP signature
