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

[Andy Shevchenko]

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