[RFC][PATCH 2/3] c/r: [pty 1/2] allow allocation of desired pty slave
orenl at librato.com
Tue Aug 25 11:28:12 PDT 2009
Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl at librato.com):
>> 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?
The problem is that the way pty works: the index allocation takes
place inside ptmx_open(). As it is, I can't find of a way to
directly transfer a piece of data to it, because all we do is
For this, we need add new functionality for pty code to
a) Allocate a master PTY without the corresponding file; this
function will be called from ptmx_open() and from restart code,
and will accept the "desired index" variable.
b) Attach an existing (master) PTY to a file pointer. This
will also be called from ptmx_open() and from restart code.
c) For restart only, manually do the work of filp_open() to
create a file, dentry, inode etc. This is what I really wanted
> Then, if we ever want to support a tty type other than unix98,
> maybe we can more generally split up more like:
Other tty types have index that is indicated by the device minor
number (e.g. old style PTYs) or no index considerations :)
> 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.
More information about the Containers