From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53021) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqbYA-0000aE-Bp for qemu-devel@nongnu.org; Tue, 18 Nov 2014 00:42:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqbXx-0005cB-2c for qemu-devel@nongnu.org; Tue, 18 Nov 2014 00:42:02 -0500 Received: from ozlabs.org ([2401:3900:2:1::2]:34000) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqbXw-0005bM-OO for qemu-devel@nongnu.org; Tue, 18 Nov 2014 00:41:49 -0500 Date: Tue, 18 Nov 2014 14:52:00 +1100 From: David Gibson Message-ID: <20141118035200.GA2867@voom.redhat.com> References: <1412358473-31398-1-git-send-email-dgilbert@redhat.com> <1412358473-31398-17-git-send-email-dgilbert@redhat.com> <20141103034600.GL8949@voom.redhat.com> <20141103132244.GF3480@work-vm> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="zYM0uCDKw75PZbzx" Content-Disposition: inline In-Reply-To: <20141103132244.GF3480@work-vm> Subject: Re: [Qemu-devel] [PATCH v4 16/47] Return path: Source handling of return path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, lilei@linux.vnet.ibm.com, quintela@redhat.com, cristian.klein@cs.umu.se, qemu-devel@nongnu.org, amit.shah@redhat.com, yanghy@cn.fujitsu.com --zYM0uCDKw75PZbzx Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 03, 2014 at 01:22:45PM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@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" > > >=20 > > > Open a return path, and handle messages that are received upon it. > > >=20 > > > Signed-off-by: Dr. David Alan Gilbert > >=20 > > [snip] > > > @@ -414,6 +448,11 @@ static void migrate_fd_cancel(MigrationState *s) > > > int old_state ; > > > trace_migrate_fd_cancel(); > > > =20 > > > + if (s->return_path) { > > > + /* shutdown the rp socket, so causing the rp thread to shutd= own */ > > > + qemu_file_shutdown(s->return_path); > >=20 > > Terminating the rp thread via shutting down its file seems roundabout, > > and kind of dependent on the socket file implementation. >=20 > 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 =3D qemu_file_get_return_path(ms->file); > >=20 > > 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. >=20 > However, since the implementation of the get_return_path is a method > on the particular implementation, and it can be different for a=20 > 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. --=20 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 --zYM0uCDKw75PZbzx Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUasJgAAoJEGw4ysog2bOSEtUP/3GktRVWu0Y0yj9pfzkV2gsk WY5MAJTeGas2zPXclJe8rTBm3rx9lhDGGLplBc/+SO2UNMDWGoB0F2+wiIu0Briw oqB2hSG9zPTnn10DmxLCWL+n2ctS/O6lltTlVfrKcyQ6HsPYne75Ulq+Osviy7py tq71FgYxmsLd9YWheUYfr0WEnf18FCmeeRkcdOzNKsjCxUgc/pjHeR/zxlj7MJTs i1x6Q55cfnOrA4xUVIywyS7sZCkPkPAWBHlJUcebI5WSlXASZovDLxJ5EB1tr3Nm RVbosb46P4ce6DdKIUPKB9fCwRn/Mfi4aN6/0aJP5SfskQSJUP8n4OjZjKOe1gfK YlmAOJue14R8VNKRP4M+gLYCs1ntjGVTaLur7A3AKdESVEXzS+rt4am4kPUrcPe3 1WaxzOqZL3GBQKrSHYq2mbRVV2nzq/t+spZOB0LQ1CmGh+cgmQAyrfVNbVeTkpbj FjVxxUSYptO/6G52RYZZzFelA+hYfLxDFdDatHaZIZiv8tuaOzT0Bxzgt5w53pqJ gTb9sNcCy114Z7/9tIKxgwra/5yTJjNXmySVBLzm+QCo/K3PcmaqH8N0S9yQ1zz9 oGqR0BKDrwUUt9rzk7tI461xLN3guy1LmIBODdOGTa7dWNmkYY4Q0qJI8mhnN/gE wOjQHU/eDefhNZOzjo+8 =Ot8G -----END PGP SIGNATURE----- --zYM0uCDKw75PZbzx--