[RFC] tty: Add get- ioctls to fetch tty status

[RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

For checkpoint/restore we need to know if tty has
exclusive or packet mode set, as well as if pty
is currently locked. Just to be able to restore
this characteristics.

For this sake the following ioctl codes are introduced

 - TIOGPKT to get packet mode state
 - TIOGPTLCK to get Pty locked state
 - TIOGEXCL to get Exclusive mode state

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
Guys, what do you think?

 drivers/tty/pty.c            |   12 ++++++++++++
 drivers/tty/tty_io.c         |    7 +++++++
 drivers/tty/tty_ioctl.c      |   16 ++++++++++++++++
 fs/compat_ioctl.c            |    3 +++
 include/asm-generic/ioctls.h |    3 +++
 5 files changed, 41 insertions(+)

Index: linux-2.6.git/drivers/tty/pty.c
===================================================================
--- linux-2.6.git.orig/drivers/tty/pty.c
+++ linux-2.6.git/drivers/tty/pty.c
@@ -173,6 +173,14 @@ static int pty_set_lock(struct tty_struc
 	return 0;
 }
 
+static int pty_get_lock(struct tty_struct *tty, int __user *arg)
+{
+	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	if (put_user(locked, arg))
+		return -EFAULT;
+	return 0;
+}
+
 /* Send a signal to the slave */
 static int pty_signal(struct tty_struct *tty, int sig)
 {
@@ -342,6 +350,8 @@ static int pty_bsd_ioctl(struct tty_stru
 	switch (cmd) {
 	case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */
 		return pty_set_lock(tty, (int __user *) arg);
+	case TIOGPTLCK:
+		return pty_get_lock(tty, (int __user *) arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
 		return pty_signal(tty, (int) arg);
 	}
@@ -449,6 +459,8 @@ static int pty_unix98_ioctl(struct tty_s
 	switch (cmd) {
 	case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */
 		return pty_set_lock(tty, (int __user *)arg);
+	case TIOGPTLCK:
+		return pty_get_lock(tty, (int __user *) arg);
 	case TIOCGPTN: /* Get PT Number */
 		return put_user(tty->index, (unsigned int __user *)arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
Index: linux-2.6.git/drivers/tty/tty_io.c
===================================================================
--- linux-2.6.git.orig/drivers/tty/tty_io.c
+++ linux-2.6.git/drivers/tty/tty_io.c
@@ -2675,6 +2675,13 @@ long tty_ioctl(struct file *file, unsign
 	case TIOCNXCL:
 		clear_bit(TTY_EXCLUSIVE, &tty->flags);
 		return 0;
+	case TIOGEXCL:
+	{
+		int excl = test_bit(TTY_EXCLUSIVE, &tty->flags);
+		if (put_user(excl, (int __user *)p))
+			return -EFAULT;
+		return 0;
+	}
 	case TIOCNOTTY:
 		if (current->signal->tty != tty)
 			return -ENOTTY;
Index: linux-2.6.git/drivers/tty/tty_ioctl.c
===================================================================
--- linux-2.6.git.orig/drivers/tty/tty_ioctl.c
+++ linux-2.6.git/drivers/tty/tty_ioctl.c
@@ -1173,6 +1173,22 @@ int n_tty_ioctl_helper(struct tty_struct
 		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 		return 0;
 	}
+	case TIOGPKT:
+	{
+		int pktmode;
+
+		if (tty->driver->type != TTY_DRIVER_TYPE_PTY ||
+		    tty->driver->subtype != PTY_TYPE_MASTER)
+			return -ENOTTY;
+
+		spin_lock_irqsave(&tty->ctrl_lock, flags);
+		pktmode = tty->packet;
+		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+		if (put_user(pktmode, (int __user *) arg))
+			return -EFAULT;
+		return 0;
+	}
 	default:
 		/* Try the mode commands */
 		return tty_mode_ioctl(tty, file, cmd, arg);
Index: linux-2.6.git/fs/compat_ioctl.c
===================================================================
--- linux-2.6.git.orig/fs/compat_ioctl.c
+++ linux-2.6.git/fs/compat_ioctl.c
@@ -842,6 +842,9 @@ COMPATIBLE_IOCTL(TIOCGDEV)
 COMPATIBLE_IOCTL(TIOCCBRK)
 COMPATIBLE_IOCTL(TIOCGSID)
 COMPATIBLE_IOCTL(TIOCGICOUNT)
+COMPATIBLE_IOCTL(TIOGPKT)
+COMPATIBLE_IOCTL(TIOGPTLCK)
+COMPATIBLE_IOCTL(TIOGEXCL)
 /* Little t */
 COMPATIBLE_IOCTL(TIOCGETD)
 COMPATIBLE_IOCTL(TIOCSETD)
Index: linux-2.6.git/include/asm-generic/ioctls.h
===================================================================
--- linux-2.6.git.orig/include/asm-generic/ioctls.h
+++ linux-2.6.git/include/asm-generic/ioctls.h
@@ -74,6 +74,9 @@
 #define TCSETXW		0x5435
 #define TIOCSIG		_IOW('T', 0x36, int)  /* pty: generate signal */
 #define TIOCVHANGUP	0x5437
+#define TIOGPKT		_IOR('T', 0x38, int) /* Get packet mode state */
+#define TIOGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
+#define TIOGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
 
 #define FIONCLEX	0x5450
 #define FIOCLEX		0x5451

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Jiri Slaby]

On 09/13/2012 11:56 AM, Cyrill Gorcunov wrote:
> For checkpoint/restore we need to know if tty has
> exclusive or packet mode set, as well as if pty
> is currently locked. Just to be able to restore
> this characteristics.
> 
> For this sake the following ioctl codes are introduced
> 
>  - TIOGPKT to get packet mode state
>  - TIOGPTLCK to get Pty locked state
>  - TIOGEXCL to get Exclusive mode state

Hi! I have only a comment regarding the naming. Why not to follow TIOC*?

thanks,
-- 
js
suse labs

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 13, 2012 at 01:46:53PM +0200, Jiri Slaby wrote:
> On 09/13/2012 11:56 AM, Cyrill Gorcunov wrote:
> > For checkpoint/restore we need to know if tty has
> > exclusive or packet mode set, as well as if pty
> > is currently locked. Just to be able to restore
> > this characteristics.
> > 
> > For this sake the following ioctl codes are introduced
> > 
> >  - TIOGPKT to get packet mode state
> >  - TIOGPTLCK to get Pty locked state
> >  - TIOGEXCL to get Exclusive mode state
> 
> Hi! I have only a comment regarding the naming. Why not to follow TIOC*?

Hi Jiri, yeah, seems to be a good idea. I'll update if there will be no
objection about idea in general. Thanks!

	Cyrill

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Alan Cox]

> +static int pty_get_lock(struct tty_struct *tty, int __user *arg)
> +{
> +	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> +	if (put_user(locked, arg))
> +		return -EFAULT;

Now explain exactly how this doesn't race with another thread chanigng
the lock setting ?

The other comment I have is that it might be better put these in now
there are sysfs patches for the tty layer bouncing about to provide the
needed infrastructure ?

Alan

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 13, 2012 at 01:51:31PM +0100, Alan Cox wrote:
> > +static int pty_get_lock(struct tty_struct *tty, int __user *arg)
> > +{
> > +	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> > +	if (put_user(locked, arg))
> > +		return -EFAULT;
> 
> Now explain exactly how this doesn't race with another thread chanigng
> the lock setting ?

It's the same as to set/clear this bit, isn't it? Please correct me
if I'm wrong.

> The other comment I have is that it might be better put these in now
> there are sysfs patches for the tty layer bouncing about to provide the
> needed infrastructure ?

Alan, could you please point me where these patches are living, so I would
take a look and check them out.

	Cyrill

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Alan Cox]

On Thu, 13 Sep 2012 16:54:01 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> On Thu, Sep 13, 2012 at 01:51:31PM +0100, Alan Cox wrote:
> > > +static int pty_get_lock(struct tty_struct *tty, int __user *arg)
> > > +{
> > > +	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> > > +	if (put_user(locked, arg))
> > > +		return -EFAULT;
> > 
> > Now explain exactly how this doesn't race with another thread chanigng
> > the lock setting ?
> 
> It's the same as to set/clear this bit, isn't it? Please correct me
> if I'm wrong.

So by the time you've finished the test bit and returned it to user space
the answer may have changed ?

> > The other comment I have is that it might be better put these in now
> > there are sysfs patches for the tty layer bouncing about to provide the
> > needed infrastructure ?
> 
> Alan, could you please point me where these patches are living, so I would
> take a look and check them out

linux-serial or check tty-next as I think Greg has now merged the test
patch.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 13, 2012 at 05:25:07PM +0100, Alan Cox wrote:
> On Thu, 13 Sep 2012 16:54:01 +0400
> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > On Thu, Sep 13, 2012 at 01:51:31PM +0100, Alan Cox wrote:
> > > > +static int pty_get_lock(struct tty_struct *tty, int __user *arg)
> > > > +{
> > > > +	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> > > > +	if (put_user(locked, arg))
> > > > +		return -EFAULT;
> > > 
> > > Now explain exactly how this doesn't race with another thread chanigng
> > > the lock setting ?
> > 
> > It's the same as to set/clear this bit, isn't it? Please correct me
> > if I'm wrong.
> 
> So by the time you've finished the test bit and returned it to user space
> the answer may have changed?

Yes. Btw, as far as I see the same applies to lock bit on its own. One
process may lock it while another process may unlock it right after that.

> > > The other comment I have is that it might be better put these in now
> > > there are sysfs patches for the tty layer bouncing about to provide the
> > > needed infrastructure ?
> > 
> > Alan, could you please point me where these patches are living, so I would
> > take a look and check them out
> 
> linux-serial or check tty-next as I think Greg has now merged the test
> patch.

Ah, thanks a lot, Alan, I'll take a look! 

	Cyrill

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 13, 2012 at 05:25:07PM +0100, Alan Cox wrote:
> On Thu, 13 Sep 2012 16:54:01 +0400
> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > On Thu, Sep 13, 2012 at 01:51:31PM +0100, Alan Cox wrote:
> > > > +static int pty_get_lock(struct tty_struct *tty, int __user *arg)
> > > > +{
> > > > +	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> > > > +	if (put_user(locked, arg))
> > > > +		return -EFAULT;
> > > 
> > > Now explain exactly how this doesn't race with another thread chanigng
> > > the lock setting ?
> > 
> > It's the same as to set/clear this bit, isn't it? Please correct me
> > if I'm wrong.
> 
> So by the time you've finished the test bit and returned it to user space
> the answer may have changed ?
> 
> > > The other comment I have is that it might be better put these in now
> > > there are sysfs patches for the tty layer bouncing about to provide the
> > > needed infrastructure ?
> > 
> > Alan, could you please point me where these patches are living, so I would
> > take a look and check them out
> 
> linux-serial or check tty-next as I think Greg has now merged the test
> patch.

Guys, you mean something like below? Look, I must admit I'm not really
sure if I've done all locking right, and there is no need for additional
kref counting on tty_struct. Could you please check if it looks more-less
sane (I've tested it but still...)

---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: tty, pty: Add pty_state attribute to fetch tty flags

For checkpoint/restore we need to know if tty has
exclusive or packet mode set, as well as if pty
is currently locked (just to be able to restore
this characteristics).

To serve this the pty_state attribute is introduced
for pty devices. A typical output looks like

 | [root@neptune ~]# cat /sys/devices/virtual/tty/ptmp0/pty_state
 | locked: 0 exclusive: 0 packet: 0

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Jiri Slaby <jslaby@suse.cz>
CC: Pavel Emelyanov <xemul@parallels.com>
---
 drivers/tty/pty.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

Index: tty.git/drivers/tty/pty.c
===================================================================
--- tty.git.orig/drivers/tty/pty.c
+++ tty.git/drivers/tty/pty.c
@@ -283,6 +283,46 @@ done:
 	return 0;
 }
 
+static ssize_t pty_show_state(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct tty_struct *tty = dev_get_drvdata(dev);
+	int locked, exclusive, packet;
+
+	tty_lock(tty);
+	locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	exclusive = test_bit(TTY_EXCLUSIVE, &tty->flags);
+	packet = tty->packet;
+	tty_unlock(tty);
+
+	return snprintf(buf, PAGE_SIZE, "locked: %d exclusive: %d packet: %d\n",
+			locked, exclusive, packet);
+}
+
+static DEVICE_ATTR(pty_state, S_IRUSR | S_IRGRP, pty_show_state, NULL);
+
+static struct attribute *pty_state_dev_attrs[] = {
+	&dev_attr_pty_state.attr,
+	NULL,
+};
+
+static const struct attribute_group pty_dev_attr_group = {
+	.attrs = pty_state_dev_attrs,
+};
+
+static const struct attribute_group *pty_dev_attr_groups[] = {
+	&pty_dev_attr_group,
+	NULL,
+};
+
+static int pty_register_attr(struct tty_driver *driver, struct tty_struct *tty)
+{
+	struct device *dev = tty_register_device_attr(driver, tty->index, tty->dev,
+						      (void *)tty, pty_dev_attr_groups);
+	return PTR_RET(dev);
+}
+
 /**
  *	pty_common_install		-	set up the pty pair
  *	@driver: the pty driver
@@ -351,7 +391,9 @@ static int pty_common_install(struct tty
 
 	tty_driver_kref_get(driver);
 	tty->count++;
-	return 0;
+
+	return pty_register_attr(driver, tty);
+
 err_free_termios:
 	if (legacy)
 		tty_free_termios(tty);
@@ -368,6 +410,7 @@ err:
 
 static void pty_cleanup(struct tty_struct *tty)
 {
+	tty_unregister_device(tty->driver, tty->index);
 	kfree(tty->port);
 }
 

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Greg Kroah-Hartman]

On Sat, Sep 22, 2012 at 10:06:39PM +0400, Cyrill Gorcunov wrote:
> On Thu, Sep 13, 2012 at 05:25:07PM +0100, Alan Cox wrote:
> > On Thu, 13 Sep 2012 16:54:01 +0400
> > Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > 
> > > On Thu, Sep 13, 2012 at 01:51:31PM +0100, Alan Cox wrote:
> > > > > +static int pty_get_lock(struct tty_struct *tty, int __user *arg)
> > > > > +{
> > > > > +	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> > > > > +	if (put_user(locked, arg))
> > > > > +		return -EFAULT;
> > > > 
> > > > Now explain exactly how this doesn't race with another thread chanigng
> > > > the lock setting ?
> > > 
> > > It's the same as to set/clear this bit, isn't it? Please correct me
> > > if I'm wrong.
> > 
> > So by the time you've finished the test bit and returned it to user space
> > the answer may have changed ?
> > 
> > > > The other comment I have is that it might be better put these in now
> > > > there are sysfs patches for the tty layer bouncing about to provide the
> > > > needed infrastructure ?
> > > 
> > > Alan, could you please point me where these patches are living, so I would
> > > take a look and check them out
> > 
> > linux-serial or check tty-next as I think Greg has now merged the test
> > patch.
> 
> Guys, you mean something like below? Look, I must admit I'm not really
> sure if I've done all locking right, and there is no need for additional
> kref counting on tty_struct. Could you please check if it looks more-less
> sane (I've tested it but still...)
> 
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: tty, pty: Add pty_state attribute to fetch tty flags
> 
> For checkpoint/restore we need to know if tty has
> exclusive or packet mode set, as well as if pty
> is currently locked (just to be able to restore
> this characteristics).
> 
> To serve this the pty_state attribute is introduced
> for pty devices. A typical output looks like
> 
>  | [root@neptune ~]# cat /sys/devices/virtual/tty/ptmp0/pty_state
>  | locked: 0 exclusive: 0 packet: 0
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Jiri Slaby <jslaby@suse.cz>
> CC: Pavel Emelyanov <xemul@parallels.com>
> ---
>  drivers/tty/pty.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> Index: tty.git/drivers/tty/pty.c
> ===================================================================
> --- tty.git.orig/drivers/tty/pty.c
> +++ tty.git/drivers/tty/pty.c
> @@ -283,6 +283,46 @@ done:
>  	return 0;
>  }
>  
> +static ssize_t pty_show_state(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct tty_struct *tty = dev_get_drvdata(dev);
> +	int locked, exclusive, packet;
> +
> +	tty_lock(tty);
> +	locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> +	exclusive = test_bit(TTY_EXCLUSIVE, &tty->flags);
> +	packet = tty->packet;
> +	tty_unlock(tty);
> +
> +	return snprintf(buf, PAGE_SIZE, "locked: %d exclusive: %d packet: %d\n",
> +			locked, exclusive, packet);
> +}

Sysfs is one value per file, you have three values here, please make 3
files.

And document them in Documentation/ABI/.

thanks,

greg k-h

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Sat, Sep 22, 2012 at 01:07:31PM -0700, Greg Kroah-Hartman wrote:
> >  drivers/tty/pty.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> > 
> > Index: tty.git/drivers/tty/pty.c
> > ===================================================================
> > --- tty.git.orig/drivers/tty/pty.c
> > +++ tty.git/drivers/tty/pty.c
> > @@ -283,6 +283,46 @@ done:
> >  	return 0;
> >  }
> >  
> > +static ssize_t pty_show_state(struct device *dev,
> > +			      struct device_attribute *attr,
> > +			      char *buf)
> > +{
> > +	struct tty_struct *tty = dev_get_drvdata(dev);
> > +	int locked, exclusive, packet;
> > +
> > +	tty_lock(tty);
> > +	locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> > +	exclusive = test_bit(TTY_EXCLUSIVE, &tty->flags);
> > +	packet = tty->packet;
> > +	tty_unlock(tty);
> > +
> > +	return snprintf(buf, PAGE_SIZE, "locked: %d exclusive: %d packet: %d\n",
> > +			locked, exclusive, packet);
> > +}
> 
> Sysfs is one value per file, you have three values here, please make 3
> files.
> 
> And document them in Documentation/ABI/.

Hmm, sure Greg, I'll update. Thanks!

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Sun, Sep 23, 2012 at 12:11:44AM +0400, Cyrill Gorcunov wrote:
>
> > Sysfs is one value per file, you have three values here, please make 3
> > files.
> > 
> > And document them in Documentation/ABI/.
> 
> Hmm, sure Greg, I'll update. Thanks!

Something like below I suppose? Look, if there will be no complains
on tech aspects on the patch (locking and tty refs) -- I'll update
Documentation. Just want be sure I've made no mistakes here.

Another question -- would not it be worth to wrap code with
CONFIG_CHECKPOINT_RESTORE?
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: tty, pty: Add sysfs attributes for pty device to fetch flags

For checkpoint/restore we need to know if pty has exclusive or packet
mode set, as well as if pty is currently locked (just to be able
to restore this characteristics).

To serve this the following attrubutes: lock, exclusive, packet,
are introduced for pty devices.

For example

 | cat /sys/devices/virtual/tty/ptmp1/lock
 | 1
 | cat /sys/devices/virtual/tty/ptmp1/exclusive
 | 0
 | cat /sys/devices/virtual/tty/ptmp1/packet
 | 0

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Jiri Slaby <jslaby@suse.cz>
CC: Pavel Emelyanov <xemul@parallels.com>
---
 drivers/tty/pty.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

Index: tty.git/drivers/tty/pty.c
===================================================================
--- tty.git.orig/drivers/tty/pty.c
+++ tty.git/drivers/tty/pty.c
@@ -283,6 +283,72 @@ done:
 	return 0;
 }
 
+static ssize_t pty_show_lock(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct tty_struct *tty = dev_get_drvdata(dev);
+	int lock;
+
+	tty_lock(tty);
+	lock = test_bit(TTY_PTY_LOCK, &tty->flags);
+	tty_unlock(tty);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", lock);
+}
+
+static ssize_t pty_show_exclusive(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct tty_struct *tty = dev_get_drvdata(dev);
+	int exclusive;
+
+	tty_lock(tty);
+	exclusive = test_bit(TTY_EXCLUSIVE, &tty->flags);
+	tty_unlock(tty);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", exclusive);
+}
+
+static ssize_t pty_show_packet(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct tty_struct *tty = dev_get_drvdata(dev);
+	int packet;
+
+	tty_lock(tty);
+	packet = tty->packet;
+	tty_unlock(tty);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", packet);
+}
+
+static DEVICE_ATTR(lock, S_IRUSR | S_IRGRP, pty_show_lock, NULL);
+static DEVICE_ATTR(exclusive, S_IRUSR | S_IRGRP, pty_show_exclusive, NULL);
+static DEVICE_ATTR(packet, S_IRUSR | S_IRGRP, pty_show_packet, NULL);
+
+static struct attribute *pty_attrs[] = {
+	&dev_attr_lock.attr,
+	&dev_attr_exclusive.attr,
+	&dev_attr_packet.attr,
+	NULL,
+};
+
+static const struct attribute_group pty_attrs_group = {
+	.attrs = pty_attrs,
+};
+
+static const struct attribute_group *pty_attr_groups[] = {
+	&pty_attrs_group,
+	NULL,
+};
+
+static int pty_register_attr(struct tty_driver *driver, struct tty_struct *tty)
+{
+	struct device *dev = tty_register_device_attr(driver, tty->index, tty->dev,
+						      (void *)tty, pty_attr_groups);
+	return PTR_RET(dev);
+}
+
 /**
  *	pty_common_install		-	set up the pty pair
  *	@driver: the pty driver
@@ -351,7 +417,9 @@ static int pty_common_install(struct tty
 
 	tty_driver_kref_get(driver);
 	tty->count++;
-	return 0;
+
+	return pty_register_attr(driver, tty);
+
 err_free_termios:
 	if (legacy)
 		tty_free_termios(tty);
@@ -368,6 +436,7 @@ err:
 
 static void pty_cleanup(struct tty_struct *tty)
 {
+	tty_unregister_device(tty->driver, tty->index);
 	kfree(tty->port);
 }
 

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Alan Cox]

> > Guys, you mean something like below? Look, I must admit I'm not really
> > sure if I've done all locking right, and there is no need for additional
> > kref counting on tty_struct. Could you please check if it looks more-less
> > sane (I've tested it but still...)

This still doesn't answer the question about 'why is this actually
useful'. It can change as or after you query it so the value is useless.

Alan

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Greg Kroah-Hartman]

On Sun, Sep 23, 2012 at 01:52:32AM +0400, Cyrill Gorcunov wrote:
> On Sun, Sep 23, 2012 at 12:11:44AM +0400, Cyrill Gorcunov wrote:
> >
> > > Sysfs is one value per file, you have three values here, please make 3
> > > files.
> > > 
> > > And document them in Documentation/ABI/.
> > 
> > Hmm, sure Greg, I'll update. Thanks!
> 
> Something like below I suppose? Look, if there will be no complains
> on tech aspects on the patch (locking and tty refs) -- I'll update
> Documentation. Just want be sure I've made no mistakes here.
> 
> Another question -- would not it be worth to wrap code with
> CONFIG_CHECKPOINT_RESTORE?

Alan's point stands, what's the use of this if it can instantly change
after you read the value?

greg k-h

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Eric W. Biederman]

I don't know where this conversation comes from but putting ptys in
sysfs in combination with the newinstance mount option is completely
broken unless the device name and device number duplication is handled,
which I don't see here.

Cyrill Gorcunov <gorcunov@openvz.org> writes:
>
> Guys, you mean something like below? Look, I must admit I'm not really
> sure if I've done all locking right, and there is no need for additional
> kref counting on tty_struct. Could you please check if it looks more-less
> sane (I've tested it but still...)
>
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: tty, pty: Add pty_state attribute to fetch tty flags
>
> For checkpoint/restore we need to know if tty has
> exclusive or packet mode set, as well as if pty
> is currently locked (just to be able to restore
> this characteristics).
>
> To serve this the pty_state attribute is introduced
> for pty devices. A typical output looks like
>
>  | [root@neptune ~]# cat /sys/devices/virtual/tty/ptmp0/pty_state
>  | locked: 0 exclusive: 0 packet: 0
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Jiri Slaby <jslaby@suse.cz>
> CC: Pavel Emelyanov <xemul@parallels.com>
> ---
>  drivers/tty/pty.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> Index: tty.git/drivers/tty/pty.c
> ===================================================================
> --- tty.git.orig/drivers/tty/pty.c
> +++ tty.git/drivers/tty/pty.c
> @@ -283,6 +283,46 @@ done:
>  	return 0;
>  }
>  
> +static ssize_t pty_show_state(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct tty_struct *tty = dev_get_drvdata(dev);
> +	int locked, exclusive, packet;
> +
> +	tty_lock(tty);
> +	locked = test_bit(TTY_PTY_LOCK, &tty->flags);
> +	exclusive = test_bit(TTY_EXCLUSIVE, &tty->flags);
> +	packet = tty->packet;
> +	tty_unlock(tty);
> +
> +	return snprintf(buf, PAGE_SIZE, "locked: %d exclusive: %d packet: %d\n",
> +			locked, exclusive, packet);
> +}
> +
> +static DEVICE_ATTR(pty_state, S_IRUSR | S_IRGRP, pty_show_state, NULL);
> +
> +static struct attribute *pty_state_dev_attrs[] = {
> +	&dev_attr_pty_state.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group pty_dev_attr_group = {
> +	.attrs = pty_state_dev_attrs,
> +};
> +
> +static const struct attribute_group *pty_dev_attr_groups[] = {
> +	&pty_dev_attr_group,
> +	NULL,
> +};
> +
> +static int pty_register_attr(struct tty_driver *driver, struct tty_struct *tty)
> +{
> +	struct device *dev = tty_register_device_attr(driver, tty->index, tty->dev,
> +						      (void *)tty, pty_dev_attr_groups);
> +	return PTR_RET(dev);
> +}
> +
>  /**
>   *	pty_common_install		-	set up the pty pair
>   *	@driver: the pty driver
> @@ -351,7 +391,9 @@ static int pty_common_install(struct tty
>  
>  	tty_driver_kref_get(driver);
>  	tty->count++;
> -	return 0;
> +
> +	return pty_register_attr(driver, tty);
> +
>  err_free_termios:
>  	if (legacy)
>  		tty_free_termios(tty);
> @@ -368,6 +410,7 @@ err:
>  
>  static void pty_cleanup(struct tty_struct *tty)
>  {
> +	tty_unregister_device(tty->driver, tty->index);
>  	kfree(tty->port);
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Sat, Sep 22, 2012 at 06:09:53PM -0700, Greg Kroah-Hartman wrote:
> On Sun, Sep 23, 2012 at 01:52:32AM +0400, Cyrill Gorcunov wrote:
> > On Sun, Sep 23, 2012 at 12:11:44AM +0400, Cyrill Gorcunov wrote:
> > >
> > > > Sysfs is one value per file, you have three values here, please make 3
> > > > files.
> > > > 
> > > > And document them in Documentation/ABI/.
> > > 
> > > Hmm, sure Greg, I'll update. Thanks!
> > 
> > Something like below I suppose? Look, if there will be no complains
> > on tech aspects on the patch (locking and tty refs) -- I'll update
> > Documentation. Just want be sure I've made no mistakes here.
> > 
> > Another question -- would not it be worth to wrap code with
> > CONFIG_CHECKPOINT_RESTORE?
> 
> Alan's point stands, what's the use of this if it can instantly change
> after you read the value?

We use it when we do a checkpoint, ie when tasks are stopped. I think it's
close to data obtained from procfs (ie valid once you read it but can be
changed right after that operation). Maybe I should put everything to
procfs, or stick back with ioctl calls?

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Sat, Sep 22, 2012 at 06:43:37PM -0700, Eric W. Biederman wrote:
> 
> I don't know where this conversation comes from but putting ptys in
> sysfs in combination with the newinstance mount option is completely
> broken unless the device name and device number duplication is handled,
> which I don't see here.

Good point, Eric, thanks! I'll take a look on this aspect (I must admit
I've missed this).

Re: [RFC] tty: Add get- ioctls to fetch tty status

[H. Peter Anvin]

Cyrill Gorcunov <gorcunov@openvz.org> wrote:

>On Sat, Sep 22, 2012 at 06:43:37PM -0700, Eric W. Biederman wrote:
>> 
>> I don't know where this conversation comes from but putting ptys in
>> sysfs in combination with the newinstance mount option is completely
>> broken unless the device name and device number duplication is
>handled,
>> which I don't see here.
>
>Good point, Eric, thanks! I'll take a look on this aspect (I must admit
>I've missed this).

Funny... I gave you that feedback the last go around.
-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[H. Peter Anvin]

Cyrill Gorcunov <gorcunov@openvz.org> wrote:

>On Sat, Sep 22, 2012 at 06:09:53PM -0700, Greg Kroah-Hartman wrote:
>> On Sun, Sep 23, 2012 at 01:52:32AM +0400, Cyrill Gorcunov wrote:
>> > On Sun, Sep 23, 2012 at 12:11:44AM +0400, Cyrill Gorcunov wrote:
>> > >
>> > > > Sysfs is one value per file, you have three values here, please
>make 3
>> > > > files.
>> > > > 
>> > > > And document them in Documentation/ABI/.
>> > > 
>> > > Hmm, sure Greg, I'll update. Thanks!
>> > 
>> > Something like below I suppose? Look, if there will be no complains
>> > on tech aspects on the patch (locking and tty refs) -- I'll update
>> > Documentation. Just want be sure I've made no mistakes here.
>> > 
>> > Another question -- would not it be worth to wrap code with
>> > CONFIG_CHECKPOINT_RESTORE?
>> 
>> Alan's point stands, what's the use of this if it can instantly
>change
>> after you read the value?
>
>We use it when we do a checkpoint, ie when tasks are stopped. I think
>it's
>close to data obtained from procfs (ie valid once you read it but can
>be
>changed right after that operation). Maybe I should put everything to
>procfs, or stick back with ioctl calls?

The problem as I see it is that you don't know if your process is the lock holder.
-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Sun, Sep 23, 2012 at 07:46:24AM -0700, H. Peter Anvin wrote:
> >changed right after that operation). Maybe I should put everything to
> >procfs, or stick back with ioctl calls?
> 
> The problem as I see it is that you don't know if your process is
> the lock holder.

Wait, Peter, this lock is set via ioctl call on the fd obtained from
open(ptmx) call, so i fail to see how one can setup such lock without
having fd on the hands. When we do checkpoint the processes we expect
the user passes us the pid of process which is the leader not some
subtree (we can deal with process subtrees too but it is not
encouraged ;)

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Sun, Sep 23, 2012 at 07:44:54AM -0700, H. Peter Anvin wrote:
> 
> Funny... I gave you that feedback the last go around.

I'm sorry, Peter, I guess I simply missed it.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Sun, Sep 23, 2012 at 07:46:24AM -0700, H. Peter Anvin wrote:
> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> >On Sat, Sep 22, 2012 at 06:09:53PM -0700, Greg Kroah-Hartman wrote:
> >> On Sun, Sep 23, 2012 at 01:52:32AM +0400, Cyrill Gorcunov wrote:
> >> > On Sun, Sep 23, 2012 at 12:11:44AM +0400, Cyrill Gorcunov wrote:
> >> > >
> >> > > > Sysfs is one value per file, you have three values here, please
> >make 3
> >> > > > files.
> >> > > > 
> >> > > > And document them in Documentation/ABI/.
> >> > > 
> >> > > Hmm, sure Greg, I'll update. Thanks!
> >> > 
> >> > Something like below I suppose? Look, if there will be no complains
> >> > on tech aspects on the patch (locking and tty refs) -- I'll update
> >> > Documentation. Just want be sure I've made no mistakes here.
> >> > 
> >> > Another question -- would not it be worth to wrap code with
> >> > CONFIG_CHECKPOINT_RESTORE?
> >> 
> >> Alan's point stands, what's the use of this if it can instantly
> >> change after you read the value?
> >
> >We use it when we do a checkpoint, ie when tasks are stopped. I think
> >it's close to data obtained from procfs (ie valid once you read it but can
> >be changed right after that operation). Maybe I should put everything to
> >procfs, or stick back with ioctl calls?
> 
> The problem as I see it is that you don't know if your process is the lock holder.

Guys, after being trying sysfs approach I think ioctls is a bit better, because
it requires only local changes not propagated to devpts, also this calls are
allowed via file descriptor owner only.

Actually I thought what if extend existing set-(lock,packet-mode,exclusive)
calls to return previous state of appropriate variable, but this will break
abi so i had to refuse this idea.

As to Alan's point on "what's the use of this if it can instantly change
after you read the value" I guess it's the same as what we have when we
simply set the value. Imagine we have two tasks fork'ed, first task do
lock the pty while another task does unlock it immediately after that,
as far as I see there is nothing preventing user space to do that, thus
first task will think it has locked the peer while in real the peer remains
unlocked. Same here with reading this value -- it's valid once read but
can be changed right after. Isn't it?

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Mon, Sep 24, 2012 at 06:14:41PM +0400, Cyrill Gorcunov wrote:
> 
> As to Alan's point on "what's the use of this if it can instantly change
> after you read the value" I guess it's the same as what we have when we
> simply set the value. Imagine we have two tasks fork'ed, first task do
> lock the pty while another task does unlock it immediately after that,
> as far as I see there is nothing preventing user space to do that, thus
> first task will think it has locked the peer while in real the peer remains
> unlocked. Same here with reading this value -- it's valid once read but
> can be changed right after. Isn't it?

Alan, Greg, what's opinion? This flags fetching is the same as say fetching
of termios settings, once fetched they can be changed immediately, and it's
up to caller what to do with termios settings. No?
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: tty: Add get- ioctls to fetch tty status

For checkpoint/restore we need to know if tty has
exclusive or packet mode set, as well as if pty
is currently locked. Just to be able to restore
this characteristics.

For this sake the following ioctl codes are introduced

 - TIOCGPKT to get packet mode state
 - TIOCGPTLCK to get Pty locked state
 - TIOCGEXCL to get Exclusive mode state

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 drivers/tty/pty.c            |   12 ++++++++++++
 drivers/tty/tty_io.c         |    7 +++++++
 drivers/tty/tty_ioctl.c      |   16 ++++++++++++++++
 fs/compat_ioctl.c            |    3 +++
 include/asm-generic/ioctls.h |    3 +++
 5 files changed, 41 insertions(+)

Index: tty.git/drivers/tty/pty.c
===================================================================
--- tty.git.orig/drivers/tty/pty.c
+++ tty.git/drivers/tty/pty.c
@@ -174,6 +174,14 @@ static int pty_set_lock(struct tty_struc
 	return 0;
 }
 
+static int pty_get_lock(struct tty_struct *tty, int __user *arg)
+{
+	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	if (put_user(locked, arg))
+		return -EFAULT;
+	return 0;
+}
+
 /* Send a signal to the slave */
 static int pty_signal(struct tty_struct *tty, int sig)
 {
@@ -393,6 +401,8 @@ static int pty_bsd_ioctl(struct tty_stru
 	switch (cmd) {
 	case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */
 		return pty_set_lock(tty, (int __user *) arg);
+	case TIOCGPTLCK:
+		return pty_get_lock(tty, (int __user *) arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
 		return pty_signal(tty, (int) arg);
 	}
@@ -507,6 +517,8 @@ static int pty_unix98_ioctl(struct tty_s
 	switch (cmd) {
 	case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */
 		return pty_set_lock(tty, (int __user *)arg);
+	case TIOCGPTLCK:
+		return pty_get_lock(tty, (int __user *) arg);
 	case TIOCGPTN: /* Get PT Number */
 		return put_user(tty->index, (unsigned int __user *)arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
Index: tty.git/drivers/tty/tty_io.c
===================================================================
--- tty.git.orig/drivers/tty/tty_io.c
+++ tty.git/drivers/tty/tty_io.c
@@ -2693,6 +2693,13 @@ long tty_ioctl(struct file *file, unsign
 	case TIOCNXCL:
 		clear_bit(TTY_EXCLUSIVE, &tty->flags);
 		return 0;
+	case TIOCGEXCL:
+	{
+		int excl = test_bit(TTY_EXCLUSIVE, &tty->flags);
+		if (put_user(excl, (int __user *)p))
+			return -EFAULT;
+		return 0;
+	}
 	case TIOCNOTTY:
 		if (current->signal->tty != tty)
 			return -ENOTTY;
Index: tty.git/drivers/tty/tty_ioctl.c
===================================================================
--- tty.git.orig/drivers/tty/tty_ioctl.c
+++ tty.git/drivers/tty/tty_ioctl.c
@@ -1173,6 +1173,22 @@ int n_tty_ioctl_helper(struct tty_struct
 		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 		return 0;
 	}
+	case TIOCGPKT:
+	{
+		int pktmode;
+
+		if (tty->driver->type != TTY_DRIVER_TYPE_PTY ||
+		    tty->driver->subtype != PTY_TYPE_MASTER)
+			return -ENOTTY;
+
+		spin_lock_irqsave(&tty->ctrl_lock, flags);
+		pktmode = tty->packet;
+		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+		if (put_user(pktmode, (int __user *) arg))
+			return -EFAULT;
+		return 0;
+	}
 	default:
 		/* Try the mode commands */
 		return tty_mode_ioctl(tty, file, cmd, arg);
Index: tty.git/fs/compat_ioctl.c
===================================================================
--- tty.git.orig/fs/compat_ioctl.c
+++ tty.git/fs/compat_ioctl.c
@@ -842,6 +842,9 @@ COMPATIBLE_IOCTL(TIOCGDEV)
 COMPATIBLE_IOCTL(TIOCCBRK)
 COMPATIBLE_IOCTL(TIOCGSID)
 COMPATIBLE_IOCTL(TIOCGICOUNT)
+COMPATIBLE_IOCTL(TIOCGPKT)
+COMPATIBLE_IOCTL(TIOCGPTLCK)
+COMPATIBLE_IOCTL(TIOCGEXCL)
 /* Little t */
 COMPATIBLE_IOCTL(TIOCGETD)
 COMPATIBLE_IOCTL(TIOCSETD)
Index: tty.git/include/asm-generic/ioctls.h
===================================================================
--- tty.git.orig/include/asm-generic/ioctls.h
+++ tty.git/include/asm-generic/ioctls.h
@@ -74,6 +74,9 @@
 #define TCSETXW		0x5435
 #define TIOCSIG		_IOW('T', 0x36, int)  /* pty: generate signal */
 #define TIOCVHANGUP	0x5437
+#define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
+#define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
+#define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
 
 #define FIONCLEX	0x5450
 #define FIOCLEX		0x5451

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 27, 2012 at 03:14:47PM +0100, Alan Cox wrote:
> > Alan, Greg, what's opinion? This flags fetching is the same as say fetching
> > of termios settings, once fetched they can be changed immediately, and it's
> > up to caller what to do with termios settings. No?
> 
> I think you need to explain what you expect to be doing with it, and why
> it is safe in that application.

OK, it seems it was unclear from changelog. We need to know this parameters
to be able to restore tty connection after checkpoint.

While we easily can fetch termios settings and such, there are few bits which
are missed to expord. So this patch provides them to user-space.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Alan Cox]

> Alan, Greg, what's opinion? This flags fetching is the same as say fetching
> of termios settings, once fetched they can be changed immediately, and it's
> up to caller what to do with termios settings. No?

I think you need to explain what you expect to be doing with it, and why
it is safe in that application.

Alan

Re: [RFC] tty: Add get- ioctls to fetch tty status

[H. Peter Anvin]

On 09/27/2012 07:14 AM, Cyrill Gorcunov wrote:
> On Thu, Sep 27, 2012 at 03:14:47PM +0100, Alan Cox wrote:
>>> Alan, Greg, what's opinion? This flags fetching is the same as say fetching
>>> of termios settings, once fetched they can be changed immediately, and it's
>>> up to caller what to do with termios settings. No?
>>
>> I think you need to explain what you expect to be doing with it, and why
>> it is safe in that application.
>
> OK, it seems it was unclear from changelog. We need to know this parameters
> to be able to restore tty connection after checkpoint.
>
> While we easily can fetch termios settings and such, there are few bits which
> are missed to expord. So this patch provides them to user-space.
>

What bothers me (and the same applies to termios) is that you have NO 
idea if your particular process is the "owner" of that tty.  tty users 
use out-of-band protocols, often implicit, to determine which process 
"owns" the tty state.

If you can't guarantee that ALL those processes are stopped and 
checkpointed/restarted, you have a huge problem.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 27, 2012 at 07:17:49AM -0700, H. Peter Anvin wrote:
> >While we easily can fetch termios settings and such, there are few bits which
> >are missed to expord. So this patch provides them to user-space.
> >
> 
> What bothers me (and the same applies to termios) is that you have
> NO idea if your particular process is the "owner" of that tty.  tty
> users use out-of-band protocols, often implicit, to determine which
> process "owns" the tty state.
> 
> If you can't guarantee that ALL those processes are stopped and
> checkpointed/restarted, you have a huge problem.

Well, sure inside our tool before doing checkpoint we stop all
tasks which are part of dumpee process tree. This unfortunatly
doesn't apply to these ioctl calls. Peter, any idea how to deal
with that?

	Cyrill

Re: [RFC] tty: Add get- ioctls to fetch tty status

[H. Peter Anvin]

On 09/27/2012 07:21 AM, Cyrill Gorcunov wrote:
> On Thu, Sep 27, 2012 at 07:17:49AM -0700, H. Peter Anvin wrote:
>>> While we easily can fetch termios settings and such, there are few bits which
>>> are missed to expord. So this patch provides them to user-space.
>>>
>>
>> What bothers me (and the same applies to termios) is that you have
>> NO idea if your particular process is the "owner" of that tty.  tty
>> users use out-of-band protocols, often implicit, to determine which
>> process "owns" the tty state.
>>
>> If you can't guarantee that ALL those processes are stopped and
>> checkpointed/restarted, you have a huge problem.
>
> Well, sure inside our tool before doing checkpoint we stop all
> tasks which are part of dumpee process tree. This unfortunatly
> doesn't apply to these ioctl calls. Peter, any idea how to deal
> with that?
>

Sorry, no.  I suspect you might be trying to do something that is 
impossible to do in a general fashion without application changes.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 27, 2012 at 07:43:44AM -0700, H. Peter Anvin wrote:
> >>If you can't guarantee that ALL those processes are stopped and
> >>checkpointed/restarted, you have a huge problem.
> >
> >Well, sure inside our tool before doing checkpoint we stop all
> >tasks which are part of dumpee process tree. This unfortunatly
> >doesn't apply to these ioctl calls. Peter, any idea how to deal
> >with that?
> 
> Sorry, no.  I suspect you might be trying to do something that is
> impossible to do in a general fashion without application changes.

I see. What if I wrap all this entries with CONFIG_CHECKPOINT_RESTORE
and say add some sysctl which would enable/disbale this ioctls. Then
it'll be up to user-space callers to make sure that data received is
valid and can be used? The problem is that I need to fetch this bits
somehow with minimal changes in kernel.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[H. Peter Anvin]

On 09/27/2012 07:48 AM, Cyrill Gorcunov wrote:
> On Thu, Sep 27, 2012 at 07:43:44AM -0700, H. Peter Anvin wrote:
>>>> If you can't guarantee that ALL those processes are stopped and
>>>> checkpointed/restarted, you have a huge problem.
>>>
>>> Well, sure inside our tool before doing checkpoint we stop all
>>> tasks which are part of dumpee process tree. This unfortunatly
>>> doesn't apply to these ioctl calls. Peter, any idea how to deal
>>> with that?
>>
>> Sorry, no.  I suspect you might be trying to do something that is
>> impossible to do in a general fashion without application changes.
>
> I see. What if I wrap all this entries with CONFIG_CHECKPOINT_RESTORE
> and say add some sysctl which would enable/disbale this ioctls. Then
> it'll be up to user-space callers to make sure that data received is
> valid and can be used? The problem is that I need to fetch this bits
> somehow with minimal changes in kernel.
>

I don't see how that solves anything.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Alan Cox]

> Well, sure inside our tool before doing checkpoint we stop all
> tasks which are part of dumpee process tree. This unfortunatly
> doesn't apply to these ioctl calls. Peter, any idea how to deal
> with that?

I suspect you can't deal with that. Which isn't to say you can't still
build something useful. The query interfaces are trivial enough and
beyond that it's simply a case of documenting that they are not intended
to be generically "safe" but provide a spot sample.

Otherwise - TICOGPKT is specific to pty devices. Please therefore put it
in the pty.c code and note that not all platforms use asm-geneic TTY
ioctls so check carefully they all build.

Alan

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 27, 2012 at 04:13:12PM +0100, Alan Cox wrote:
> > Well, sure inside our tool before doing checkpoint we stop all
> > tasks which are part of dumpee process tree. This unfortunatly
> > doesn't apply to these ioctl calls. Peter, any idea how to deal
> > with that?
> 
> I suspect you can't deal with that. Which isn't to say you can't still
> build something useful. The query interfaces are trivial enough and
> beyond that it's simply a case of documenting that they are not intended
> to be generically "safe" but provide a spot sample.

I see, thanks Alan!

> Otherwise - TICOGPKT is specific to pty devices. Please therefore put it
> in the pty.c code and note that not all platforms use asm-geneic TTY
> ioctls so check carefully they all build.

I put it into tty_ioctl.c simply because TIOCPKT was there already so
I thought it's good idea to keep them close to each other in code.

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 27, 2012 at 04:13:12PM +0100, Alan Cox wrote:
> > Well, sure inside our tool before doing checkpoint we stop all
> > tasks which are part of dumpee process tree. This unfortunatly
> > doesn't apply to these ioctl calls. Peter, any idea how to deal
> > with that?
> 
> I suspect you can't deal with that. Which isn't to say you can't still
> build something useful. The query interfaces are trivial enough and
> beyond that it's simply a case of documenting that they are not intended
> to be generically "safe" but provide a spot sample.
> 
> Otherwise - TICOGPKT is specific to pty devices. Please therefore put it
> in the pty.c code and note that not all platforms use asm-geneic TTY
> ioctls so check carefully they all build.

Alan, I've moved the TICOGPKT to pty code but where to puts information
about unsafety of the data obtained? Also I fear I can test the build
results and run-time behaviour on x86 machine only, which I own (well,
couple of them). Though I defined this ioctls the same way as existing
ioctl codes are defined so I think all should build the same.
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: tty: Add get- ioctls to fetch tty status v2

For checkpoint/restore we need to know if tty has
exclusive or packet mode set, as well as if pty
is currently locked. Just to be able to restore
this characteristics.

For this sake the following ioctl codes are introduced

 - TIOCGPKT to get packet mode state
 - TIOCGPTLCK to get Pty locked state
 - TIOCGEXCL to get Exclusive mode state

v2:
 - Use TIOC prefix for ioctl codes (by jslaby@)
 - Move TIOCGPKT to pty code (by alan@)

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 drivers/tty/pty.c            |   34 ++++++++++++++++++++++++++++++++++
 drivers/tty/tty_io.c         |    7 +++++++
 fs/compat_ioctl.c            |    3 +++
 include/asm-generic/ioctls.h |    3 +++
 4 files changed, 47 insertions(+)

Index: tty.git/drivers/tty/pty.c
===================================================================
--- tty.git.orig/drivers/tty/pty.c
+++ tty.git/drivers/tty/pty.c
@@ -174,6 +174,32 @@ static int pty_set_lock(struct tty_struc
 	return 0;
 }
 
+static int pty_get_lock(struct tty_struct *tty, int __user *arg)
+{
+	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	if (put_user(locked, arg))
+		return -EFAULT;
+	return 0;
+}
+
+static int pty_get_pktmode(struct tty_struct *tty, int __user *arg)
+{
+	unsigned long flags;
+	int pktmode;
+
+	if (tty->driver->subtype != PTY_TYPE_MASTER)
+		return -ENOTTY;
+
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	pktmode = tty->packet;
+	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+	if (put_user(pktmode, arg))
+		return -EFAULT;
+
+	return 0;
+}
+
 /* Send a signal to the slave */
 static int pty_signal(struct tty_struct *tty, int sig)
 {
@@ -393,8 +419,12 @@ static int pty_bsd_ioctl(struct tty_stru
 	switch (cmd) {
 	case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */
 		return pty_set_lock(tty, (int __user *) arg);
+	case TIOCGPTLCK: /* Get PT Lock status */
+		return pty_get_lock(tty, (int __user *)arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
 		return pty_signal(tty, (int) arg);
+	case TIOCGPKT: /* Get PT packet mode status */
+		return pty_get_pktmode(tty, (int __user *)arg);
 	}
 	return -ENOIOCTLCMD;
 }
@@ -507,10 +537,14 @@ static int pty_unix98_ioctl(struct tty_s
 	switch (cmd) {
 	case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */
 		return pty_set_lock(tty, (int __user *)arg);
+	case TIOCGPTLCK: /* Get PT Lock status */
+		return pty_get_lock(tty, (int __user *)arg);
 	case TIOCGPTN: /* Get PT Number */
 		return put_user(tty->index, (unsigned int __user *)arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
 		return pty_signal(tty, (int) arg);
+	case TIOCGPKT: /* Get PT packet mode status */
+		return pty_get_pktmode(tty, (int __user *)arg);
 	}
 
 	return -ENOIOCTLCMD;
Index: tty.git/drivers/tty/tty_io.c
===================================================================
--- tty.git.orig/drivers/tty/tty_io.c
+++ tty.git/drivers/tty/tty_io.c
@@ -2693,6 +2693,13 @@ long tty_ioctl(struct file *file, unsign
 	case TIOCNXCL:
 		clear_bit(TTY_EXCLUSIVE, &tty->flags);
 		return 0;
+	case TIOCGEXCL:
+	{
+		int excl = test_bit(TTY_EXCLUSIVE, &tty->flags);
+		if (put_user(excl, (int __user *)p))
+			return -EFAULT;
+		return 0;
+	}
 	case TIOCNOTTY:
 		if (current->signal->tty != tty)
 			return -ENOTTY;
Index: tty.git/fs/compat_ioctl.c
===================================================================
--- tty.git.orig/fs/compat_ioctl.c
+++ tty.git/fs/compat_ioctl.c
@@ -842,6 +842,9 @@ COMPATIBLE_IOCTL(TIOCGDEV)
 COMPATIBLE_IOCTL(TIOCCBRK)
 COMPATIBLE_IOCTL(TIOCGSID)
 COMPATIBLE_IOCTL(TIOCGICOUNT)
+COMPATIBLE_IOCTL(TIOCGPKT)
+COMPATIBLE_IOCTL(TIOCGPTLCK)
+COMPATIBLE_IOCTL(TIOCGEXCL)
 /* Little t */
 COMPATIBLE_IOCTL(TIOCGETD)
 COMPATIBLE_IOCTL(TIOCSETD)
Index: tty.git/include/asm-generic/ioctls.h
===================================================================
--- tty.git.orig/include/asm-generic/ioctls.h
+++ tty.git/include/asm-generic/ioctls.h
@@ -74,6 +74,9 @@
 #define TCSETXW		0x5435
 #define TIOCSIG		_IOW('T', 0x36, int)  /* pty: generate signal */
 #define TIOCVHANGUP	0x5437
+#define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
+#define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
+#define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
 
 #define FIONCLEX	0x5450
 #define FIOCLEX		0x5451

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Alan Cox]

> > Otherwise - TICOGPKT is specific to pty devices. Please therefore put it
> > in the pty.c code and note that not all platforms use asm-geneic TTY
> > ioctls so check carefully they all build.
> 
> I put it into tty_ioctl.c simply because TIOCPKT was there already so
> I thought it's good idea to keep them close to each other in code.

Perhaps we should move TIOCPKT out as well ?

Re: [RFC] tty: Add get- ioctls to fetch tty status

[Cyrill Gorcunov]

On Thu, Sep 27, 2012 at 05:00:37PM +0100, Alan Cox wrote:
> > > Otherwise - TICOGPKT is specific to pty devices. Please therefore put it
> > > in the pty.c code and note that not all platforms use asm-geneic TTY
> > > ioctls so check carefully they all build.
> > 
> > I put it into tty_ioctl.c simply because TIOCPKT was there already so
> > I thought it's good idea to keep them close to each other in code.
> 
> Perhaps we should move TIOCPKT out as well?

OK, so I'll do two patches instead of squashing everything on one patch.

Btw, as far as I see wee are zeroifying tty_struct always twice,
first in alloc_tty_struct and then in initialize_tty_struct again.
And alloc_tty_struct + initialize_tty_struct always used as a tuple
in current code, so I presume they might be squashed in one helper
instead to eliminate overhead, but it's different task.