Re: [RFC][PATCH 2/3] c/r: [pty 1/2] allow allocation of desired pty slave

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> During restart, we need to allocate pty slaves with the same
> identifiers as recorded during checkpoint. Modify the allocation code
> to allow an in-kernel caller to request a specific slave identifier.
> 
> For this, add a new field to task_struct - 'required_id'. It will
> hold the desired identifier when restoring a (master) pty.

This is really unfortunate.

I assume you left the rather generic name 'required_id' because
you intend to use it for other calls as well?  Which is even
more unfortunate...

I know I'm generally the one who pushes for re-using existing
helpers as much as possible, both to avoid missing existing
security checks, and to prevent future code changes which forget
to update the c/r code.

But in this case I don't think re-using fopen to ask the kernel
to create a new device with more info than fopen usually gets is
right.  Unfortunately, actually coming up with 'the right way' is
escaping me at the moment :)

Though... then again...  your code is hardcoded for devpts and
opening /dev/ptmx anyway, so surely we can create a new helper
which takes a devpts_ns and index and returns the new devpts
tty_struct without using the current->required_id?

Then, if we ever want to support a tty type other than unix98,
maybe we can more generally split up more like:

	1. add 'struct tty_struct *make_tty(struct tty_driver *driver,
			void *data) to tty_operations.  It is
			undefined for all but devpts.
	2. for devpts, the void *data includes an int devpts_ns_id
			and index
	3. the container-level checkpoint stores a relation between
		pathnames and devpts_ns_id.  devpts_ns_id is valid
		only for one checkpoint image.  The pathname is
		where that devpts_ns is mounted.  If we ever start
		checkpointing and restoring fs mounts from kernel,
		then we can tweak this treatment.  For now it is
		assumed (and verified) that at restart, pathname is
		in fact where a devpts instance is mounted.
	4. the tty_struct itself is checkpointed and restored.  It
		is restored using make_tty() in tty_operations,
		and for devpts the void *data  is a struct storing
		the devpts_ns_id and index.
	5. when a struct file for a devpts is restored, the tty_struct
		will already have been re-created.  The  tty_struct's objref
		is stored in the file struct checkpoint data.

It requires more finagling of the file/inode/tty_struct relationship
though...  But I don't see required_id being acceptable upstream.

Of course I've been wrong about such things before.

-serge