linux-kernel - Re: [PATCH 2/2] tty: Implement lookahead to process XON/XOFF timely

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Yk1Vp+nQQBRE+MjN@smile.fi.intel.com>
Date:   Wed, 6 Apr 2022 11:56:07 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Jiri Slaby <jirislaby@...nel.org>
Cc:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        linux-serial@...r.kernel.org, Greg KH <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org,
        Gilles Buloz <gilles.buloz@...tron.com>,
        Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH 2/2] tty: Implement lookahead to process XON/XOFF timely

On Wed, Apr 06, 2022 at 11:54:05AM +0300, Andy Shevchenko wrote:
> On Wed, Apr 06, 2022 at 10:21:12AM +0200, Jiri Slaby wrote:
> > On 05. 04. 22, 18:11, Andy Shevchenko wrote:
> > > On Tue, Apr 05, 2022 at 01:24:37PM +0300, Ilpo Järvinen wrote:

> > > > +	if (c == START_CHAR(tty)) {
> > > > +		if (!lookahead_done) {
> > > > +			start_tty(tty);
> > > > +			process_echoes(tty);
> > > > +		}
> > > > +		return true;
> > > > +	}
> > > > +	if (c == STOP_CHAR(tty)) {
> > > > +		if (!lookahead_done)
> > > > +			stop_tty(tty);
> > > > +		return true;
> > > > +	}

> > > Looking into this I would first make a preparatory patch that splits out
> > > current code into something like
> > > 
> > > static bool __n_tty_receive_char_special_no_lookahead(struct tty_struct *tty, unsigned char c)
> > > {
> > > 	...current code...
> > > }
> > > 
> > > Then in the patch 1 you add
> > > 
> > > static bool __n_tty_receive_char_special_lookahead(struct tty_struct *tty, unsigned char c)
> > > {
> > > 	...
> > > }
> > > 
> > > static bool __n_tty_receive_char_special(struct tty_struct *tty, unsigned char c,
> > > 					 bool lookahead_done)
> > 
> > This should be dubbed better. Maybe n_tty_receive_char_flow_control()?
> > 
> > And I would place the if (I_IXON(tty)) to the caller. I am a bit lost in
> > this pseudo code, so maybe this doesn't make sense in your proposal. I have
> > something like in my mind:
> > 
> > if (I_IXON(tty))
> >   return n_tty_receive_char_flow_control();
> 
> My point to have three helpers which make each change cleaner:
> 
>   .-> n_tty_receive_char_flow_control_lah()
>   |
>   |  .-> n_tty_receive_char_flow_control_no_lah()
>   |  |
>   `- + -- n_tty_receive_char_flow_control()
> 
> Where no_lah variant can be split as preparatory patch prepending the current
> series.

Another possibility to keep one function, but split it first in a separate
patch and then modify.

> And yes, calling I_IXON at the caller seems better.
> 
> > Historically, these n_tty_receive* function names were a big mess. Don't
> > produce more of that by simply prepending only "__".
> > 
> > > {
> > > 	if (!I_IXON(tty))
> > > 		return false;
> > > 
> > > 	if (lookahead_done)
> > > 		return _lookahead();
> > > 
> > > 	return _no_lookahead();
> > > }

-- 
With Best Regards,
Andy Shevchenko