Re: [PATCH 05/50] tty_ioctl(): start taking TIOC[SG]SERIAL into separate methods

From: Al Viro <viro@ZenIV.linux.org.uk>
To: Johan Hovold <johan@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 05/50] tty_ioctl(): start taking TIOC[SG]SERIAL into separate methods
Date: Fri, 14 Sep 2018 16:18:54 +0100	[thread overview]
Message-ID: <20180914151854.GA19965@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20180914132216.GC1362@localhost>

On Fri, Sep 14, 2018 at 03:22:16PM +0200, Johan Hovold wrote:
> On Thu, Sep 13, 2018 at 03:40:04AM +0100, Al Viro wrote:
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > ->set_serial() and ->get_serial() resp., both taking tty and
> > a kernel pointer to serial_struct.
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> >  drivers/tty/tty_io.c       | 36 ++++++++++++++++++++++++++++++------
> >  include/linux/tty_driver.h |  3 +++
> >  2 files changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 2b34ccf269e0..91d73732e11c 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -97,6 +97,7 @@
> >  #include <linux/seq_file.h>
> >  #include <linux/serial.h>
> >  #include <linux/ratelimit.h>
> > +#include <linux/compat.h>
> 
> Looks like you don't need this (just yet anyway).

Actually, that should've been added several commits earlier ;-)

> >  #include <linux/uaccess.h>
> >  
> > @@ -2455,22 +2456,40 @@ static int tty_tiocgicount(struct tty_struct *tty, void __user *arg)
> >  	return 0;
> >  }
> 
> [...]
> 
> > +static int tty_tiocgserial(struct tty_struct *tty, struct serial_struct __user *ss)
> > +{
> > +	struct serial_struct v;
> > +	int err;
> 
> Missing newline.

Umm...  Matter of taste, but since you work in that area more often than
I do...  Note, BTW, that tty_tiocgicount() just a bit above that place also
doesn't follow that style.  Anyway, I don't care either way - if you want
an empty line in that, no problem.

> > +	memset(&v, 0, sizeof(struct serial_struct));
> > +
> > +	if (!tty->ops->set_serial)
> 
> You want get_serial here.

Nice catch.  Fixed.

> > +		return -ENOIOCTLCMD;
> > +	err = tty->ops->get_serial(tty, &v);
> > +	if (!err && copy_to_user(ss, &v, sizeof(struct serial_struct)))
> > +		err = -EFAULT;
> > +	return err;
> >  }
> >  
> >  /*
> > @@ -2602,8 +2621,13 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >  		}
> >  		break;
> >  	case TIOCSSERIAL:
> > -		tty_warn_deprecated_flags(p);
> > -		break;
> > +		retval = tty_tiocsserial(tty, p);
> > +		if (retval != -ENOIOCTLCMD)
> > +			return retval;
> 
> Missing break.

*blink*
Right you are... that's a bisect hazard (in the end of the series returns
become unconditional anyway).  Fixed.