util-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] agetty: don't put the VC into canonical mode
@ 2018-10-16  6:41 Lubomir Rintel
  2018-10-16  7:40 ` Karel Zak
  0 siblings, 1 reply; 9+ messages in thread
From: Lubomir Rintel @ 2018-10-16  6:41 UTC (permalink / raw)
  To: util-linux; +Cc: Karel Zak, Lubomir Rintel

The wait_for_term_input()'s select() needs to be tripped when the user
starts typing. Otherwise the reloads can abort an already in-progress login.

Coupled with \4 and \6 expansions that happen to be there on Fedora Server,
this means reload on every netlink event. With a couple of IPv6 routers
announcing their networks and temporary addresses in use can make it
sometimes virtually impossible to log in.

Seems like zero lflags do the job just fine on a Linux VT. Reset it to
canonical mode before running login.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v1:
- Unreversed the logic in termio_final()/reset_vc() conditional

Tested on a vc and serial console on a stock Fedora installation:

  /sbin/agetty -o -p -- \u --noclear tty1 linux
  /sbin/agetty -o -p -- \u --keep-baud 115200,38400,9600 ttyS2 vt220

 term-utils/agetty.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/term-utils/agetty.c b/term-utils/agetty.c
index 3c87ec64e..3834813f1 100644
--- a/term-utils/agetty.c
+++ b/term-utils/agetty.c
@@ -303,7 +303,7 @@ static void parse_speeds(struct options *op, char *arg);
 static void update_utmp(struct options *op);
 static void open_tty(char *tty, struct termios *tp, struct options *op);
 static void termio_init(struct options *op, struct termios *tp);
-static void reset_vc (const struct options *op, struct termios *tp);
+static void reset_vc(const struct options *op, struct termios *tp, int canon);
 static void auto_baud(struct termios *tp);
 static void list_speeds(void);
 static void output_special_char (unsigned char c, struct options *op,
@@ -485,13 +485,14 @@ int main(int argc, char **argv)
 	if (options.timeout)
 		alarm(0);
 
-	if ((options.flags & F_VCONSOLE) == 0) {
-		/* Finalize the termios settings. */
+	/* Finalize the termios settings. */
+	if ((options.flags & F_VCONSOLE) == 0)
 		termio_final(&options, &termios, &chardata);
+	else
+		reset_vc(&options, &termios, 1);
 
-		/* Now the newline character should be properly written. */
-		write_all(STDOUT_FILENO, "\r\n", 2);
-	}
+	/* Now the newline character should be properly written. */
+	write_all(STDOUT_FILENO, "\r\n", 2);
 
 	sigaction(SIGQUIT, &sa_quit, NULL);
 	sigaction(SIGINT, &sa_int, NULL);
@@ -1234,7 +1235,7 @@ static void termio_init(struct options *op, struct termios *tp)
 		setlocale(LC_CTYPE, "POSIX");
 		op->flags &= ~F_UTF8;
 #endif
-		reset_vc(op, tp);
+		reset_vc(op, tp, 0);
 
 		if ((tp->c_cflag & (CS8|PARODD|PARENB)) == CS8)
 			op->flags |= F_EIGHTBITS;
@@ -1344,7 +1345,7 @@ static void termio_init(struct options *op, struct termios *tp)
 }
 
 /* Reset virtual console on stdin to its defaults */
-static void reset_vc(const struct options *op, struct termios *tp)
+static void reset_vc(const struct options *op, struct termios *tp, int canon)
 {
 	int fl = 0;
 
@@ -1353,6 +1354,15 @@ static void reset_vc(const struct options *op, struct termios *tp)
 
 	reset_virtual_console(tp, fl);
 
+#ifdef AGETTY_RELOAD
+	/*
+	 * Discard all the flags that makes the line go canonical with echoing.
+	 * We need to know when the user starts typing.
+	 */
+	if (canon == 0)
+		tp->c_lflag = 0;
+#endif
+
 	if (tcsetattr(STDIN_FILENO, TCSADRAIN, tp))
 		log_warn(_("setting terminal attributes failed: %m"));
 
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] agetty: don't put the VC into canonical mode
  2018-10-16  6:41 [PATCH v2] agetty: don't put the VC into canonical mode Lubomir Rintel
@ 2018-10-16  7:40 ` Karel Zak
  2018-10-18 19:25   ` Stanislav Brabec
  2018-10-19  6:09   ` Lubomir Rintel
  0 siblings, 2 replies; 9+ messages in thread
From: Karel Zak @ 2018-10-16  7:40 UTC (permalink / raw)
  To: Lubomir Rintel; +Cc: util-linux, Stanislav Brabec

On Tue, Oct 16, 2018 at 08:41:49AM +0200, Lubomir Rintel wrote:
> The wait_for_term_input()'s select() needs to be tripped when the user
> starts typing. Otherwise the reloads can abort an already in-progress login.
> 
> Coupled with \4 and \6 expansions that happen to be there on Fedora Server,
> this means reload on every netlink event. With a couple of IPv6 routers
> announcing their networks and temporary addresses in use can make it
> sometimes virtually impossible to log in.

It's too late for v2.33. It also seems we need to wait for Stanislav's
work on this issue -- his idea is to complete disable the reload
notification when user start typing, and it also requires to switch
to non-canonical mode.

This is definitely something we need to fix in v2.34 and v2.33.1.

> Seems like zero lflags do the job just fine on a Linux VT. Reset it to
> canonical mode before running login.

What about Del/Backspace keys when user is typing login name? :-)

    Karel

> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> ---
> Changes since v1:
> - Unreversed the logic in termio_final()/reset_vc() conditional
> 
> Tested on a vc and serial console on a stock Fedora installation:
> 
>   /sbin/agetty -o -p -- \u --noclear tty1 linux
>   /sbin/agetty -o -p -- \u --keep-baud 115200,38400,9600 ttyS2 vt220
> 
>  term-utils/agetty.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/term-utils/agetty.c b/term-utils/agetty.c
> index 3c87ec64e..3834813f1 100644
> --- a/term-utils/agetty.c
> +++ b/term-utils/agetty.c
> @@ -303,7 +303,7 @@ static void parse_speeds(struct options *op, char *arg);
>  static void update_utmp(struct options *op);
>  static void open_tty(char *tty, struct termios *tp, struct options *op);
>  static void termio_init(struct options *op, struct termios *tp);
> -static void reset_vc (const struct options *op, struct termios *tp);
> +static void reset_vc(const struct options *op, struct termios *tp, int canon);
>  static void auto_baud(struct termios *tp);
>  static void list_speeds(void);
>  static void output_special_char (unsigned char c, struct options *op,
> @@ -485,13 +485,14 @@ int main(int argc, char **argv)
>  	if (options.timeout)
>  		alarm(0);
>  
> -	if ((options.flags & F_VCONSOLE) == 0) {
> -		/* Finalize the termios settings. */
> +	/* Finalize the termios settings. */
> +	if ((options.flags & F_VCONSOLE) == 0)
>  		termio_final(&options, &termios, &chardata);
> +	else
> +		reset_vc(&options, &termios, 1);
>  
> -		/* Now the newline character should be properly written. */
> -		write_all(STDOUT_FILENO, "\r\n", 2);
> -	}
> +	/* Now the newline character should be properly written. */
> +	write_all(STDOUT_FILENO, "\r\n", 2);
>  
>  	sigaction(SIGQUIT, &sa_quit, NULL);
>  	sigaction(SIGINT, &sa_int, NULL);
> @@ -1234,7 +1235,7 @@ static void termio_init(struct options *op, struct termios *tp)
>  		setlocale(LC_CTYPE, "POSIX");
>  		op->flags &= ~F_UTF8;
>  #endif
> -		reset_vc(op, tp);
> +		reset_vc(op, tp, 0);
>  
>  		if ((tp->c_cflag & (CS8|PARODD|PARENB)) == CS8)
>  			op->flags |= F_EIGHTBITS;
> @@ -1344,7 +1345,7 @@ static void termio_init(struct options *op, struct termios *tp)
>  }
>  
>  /* Reset virtual console on stdin to its defaults */
> -static void reset_vc(const struct options *op, struct termios *tp)
> +static void reset_vc(const struct options *op, struct termios *tp, int canon)
>  {
>  	int fl = 0;
>  
> @@ -1353,6 +1354,15 @@ static void reset_vc(const struct options *op, struct termios *tp)
>  
>  	reset_virtual_console(tp, fl);
>  
> +#ifdef AGETTY_RELOAD
> +	/*
> +	 * Discard all the flags that makes the line go canonical with echoing.
> +	 * We need to know when the user starts typing.
> +	 */
> +	if (canon == 0)
> +		tp->c_lflag = 0;
> +#endif
> +
>  	if (tcsetattr(STDIN_FILENO, TCSADRAIN, tp))
>  		log_warn(_("setting terminal attributes failed: %m"));
>  
> -- 
> 2.19.1
> 

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] agetty: don't put the VC into canonical mode
  2018-10-16  7:40 ` Karel Zak
@ 2018-10-18 19:25   ` Stanislav Brabec
  2018-10-19  7:53     ` Karel Zak
  2018-10-19  6:09   ` Lubomir Rintel
  1 sibling, 1 reply; 9+ messages in thread
From: Stanislav Brabec @ 2018-10-18 19:25 UTC (permalink / raw)
  To: Karel Zak, Lubomir Rintel; +Cc: util-linux

On Oct 16, 2018 at 9:40 AM Karel Zak wrote:
> On Tue, Oct 16, 2018 at 08:41:49AM +0200, Lubomir Rintel wrote:
>> Seems like zero lflags do the job just fine on a Linux VT. Reset it to
>> canonical mode before running login.
> 
> What about Del/Backspace keys when user is typing login name? :-)

It could be easy without Backspace support. Just read the characters.
But we have to implement Backspace.

When you leave canonical mode, then agetty code has to handle Backspace.

Hopefully, agetty doesn't support arrows and Delete (forward delete),
so we don't have to implement advanced parts of line editing.

My experiments show two possibilities. I am not sure, what is better,
but 1 seems to be more straightforward.


1) c_lflag &= ~(ICANON | ECHO)

This falls back to a no echo mode. Program has to handle echo and
buffer parsing. Program has to handle backspace.

The implementation seems to be straightforward.

Downside: Remote logins can experience delays when getting echo.


2) c_lflag &= ~(ICANON)

Non-canonical mode with echo seems to be usable as well. The terminal
itself does the echo, program does the input parsing. Program has to
handle Backspace. Echo will be immediate even with slow remote logins,
but Backspace has to be handled remotely.

The second implementation has a problem: The Linux console input and
output are out of sync in the time of agetty. Input returns 0x7f after
pressing backspace, but output does back step (not back space) after
sending 0x08. I don't know the fix yet.

Even with a proper configuration of erase character, terminal
in a non-canonical mode cannot do a back space. At the best, it will
do back step.

So, in the echo mode, program has to handle Backspace not only for
the input, but also for the output: Print a space to erase the deleted
character and then one back step again.

This approach is vulnerable to race: New letter is typed before
processing of backspace is not finished yet. But if I am correct, this
race will not affect the result on the screen.


-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] agetty: don't put the VC into canonical mode
  2018-10-16  7:40 ` Karel Zak
  2018-10-18 19:25   ` Stanislav Brabec
@ 2018-10-19  6:09   ` Lubomir Rintel
  2018-10-19 13:08     ` Stanislav Brabec
  1 sibling, 1 reply; 9+ messages in thread
From: Lubomir Rintel @ 2018-10-19  6:09 UTC (permalink / raw)
  To: Karel Zak; +Cc: util-linux, Stanislav Brabec

On Tue, 2018-10-16 at 09:40 +0200, Karel Zak wrote:
> On Tue, Oct 16, 2018 at 08:41:49AM +0200, Lubomir Rintel wrote:
> > The wait_for_term_input()'s select() needs to be tripped when the
> > user
> > starts typing. Otherwise the reloads can abort an already in-
> > progress login.
> > 
> > Coupled with \4 and \6 expansions that happen to be there on Fedora
> > Server,
> > this means reload on every netlink event. With a couple of IPv6
> > routers
> > announcing their networks and temporary addresses in use can make
> > it
> > sometimes virtually impossible to log in.
> 
> It's too late for v2.33. It also seems we need to wait for
> Stanislav's
> work on this issue -- his idea is to complete disable the reload
> notification when user start typing, and it also requires to switch
> to non-canonical mode.
> 
> This is definitely something we need to fix in v2.34 and v2.33.1.
> 
> > Seems like zero lflags do the job just fine on a Linux VT. Reset it
> > to
> > canonical mode before running login.
> 
> What about Del/Backspace keys when user is typing login name? :-)

They seem to work the same as they used to?

From Stanislav's message I notice that there's supposed to be some
problem with them -- I'm not seeing that. Either the patch does
something different or I'm not looking at the right thing.

(I haven't looked at Stanislav's patch -- but I surely intend to give
it a try.)

Lubo

> 
>     Karel
> 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > 
> > ---
> > Changes since v1:
> > - Unreversed the logic in termio_final()/reset_vc() conditional
> > 
> > Tested on a vc and serial console on a stock Fedora installation:
> > 
> >   /sbin/agetty -o -p -- \u --noclear tty1 linux
> >   /sbin/agetty -o -p -- \u --keep-baud 115200,38400,9600 ttyS2
> > vt220
> > 
> >  term-utils/agetty.c | 26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> > 
> > diff --git a/term-utils/agetty.c b/term-utils/agetty.c
> > index 3c87ec64e..3834813f1 100644
> > --- a/term-utils/agetty.c
> > +++ b/term-utils/agetty.c
> > @@ -303,7 +303,7 @@ static void parse_speeds(struct options *op,
> > char *arg);
> >  static void update_utmp(struct options *op);
> >  static void open_tty(char *tty, struct termios *tp, struct options
> > *op);
> >  static void termio_init(struct options *op, struct termios *tp);
> > -static void reset_vc (const struct options *op, struct termios
> > *tp);
> > +static void reset_vc(const struct options *op, struct termios *tp,
> > int canon);
> >  static void auto_baud(struct termios *tp);
> >  static void list_speeds(void);
> >  static void output_special_char (unsigned char c, struct options
> > *op,
> > @@ -485,13 +485,14 @@ int main(int argc, char **argv)
> >  	if (options.timeout)
> >  		alarm(0);
> >  
> > -	if ((options.flags & F_VCONSOLE) == 0) {
> > -		/* Finalize the termios settings. */
> > +	/* Finalize the termios settings. */
> > +	if ((options.flags & F_VCONSOLE) == 0)
> >  		termio_final(&options, &termios, &chardata);
> > +	else
> > +		reset_vc(&options, &termios, 1);
> >  
> > -		/* Now the newline character should be properly
> > written. */
> > -		write_all(STDOUT_FILENO, "\r\n", 2);
> > -	}
> > +	/* Now the newline character should be properly written. */
> > +	write_all(STDOUT_FILENO, "\r\n", 2);
> >  
> >  	sigaction(SIGQUIT, &sa_quit, NULL);
> >  	sigaction(SIGINT, &sa_int, NULL);
> > @@ -1234,7 +1235,7 @@ static void termio_init(struct options *op,
> > struct termios *tp)
> >  		setlocale(LC_CTYPE, "POSIX");
> >  		op->flags &= ~F_UTF8;
> >  #endif
> > -		reset_vc(op, tp);
> > +		reset_vc(op, tp, 0);
> >  
> >  		if ((tp->c_cflag & (CS8|PARODD|PARENB)) == CS8)
> >  			op->flags |= F_EIGHTBITS;
> > @@ -1344,7 +1345,7 @@ static void termio_init(struct options *op,
> > struct termios *tp)
> >  }
> >  
> >  /* Reset virtual console on stdin to its defaults */
> > -static void reset_vc(const struct options *op, struct termios *tp)
> > +static void reset_vc(const struct options *op, struct termios *tp,
> > int canon)
> >  {
> >  	int fl = 0;
> >  
> > @@ -1353,6 +1354,15 @@ static void reset_vc(const struct options
> > *op, struct termios *tp)
> >  
> >  	reset_virtual_console(tp, fl);
> >  
> > +#ifdef AGETTY_RELOAD
> > +	/*
> > +	 * Discard all the flags that makes the line go canonical with
> > echoing.
> > +	 * We need to know when the user starts typing.
> > +	 */
> > +	if (canon == 0)
> > +		tp->c_lflag = 0;
> > +#endif
> > +
> >  	if (tcsetattr(STDIN_FILENO, TCSADRAIN, tp))
> >  		log_warn(_("setting terminal attributes failed: %m"));
> >  
> > -- 
> > 2.19.1
> > 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] agetty: don't put the VC into canonical mode
  2018-10-18 19:25   ` Stanislav Brabec
@ 2018-10-19  7:53     ` Karel Zak
  0 siblings, 0 replies; 9+ messages in thread
From: Karel Zak @ 2018-10-19  7:53 UTC (permalink / raw)
  To: Stanislav Brabec; +Cc: Lubomir Rintel, util-linux

On Thu, Oct 18, 2018 at 09:25:33PM +0200, Stanislav Brabec wrote:
> On Oct 16, 2018 at 9:40 AM Karel Zak wrote:
> > On Tue, Oct 16, 2018 at 08:41:49AM +0200, Lubomir Rintel wrote:
> >> Seems like zero lflags do the job just fine on a Linux VT. Reset it to
> >> canonical mode before running login.
> > 
> > What about Del/Backspace keys when user is typing login name? :-)
> 
> It could be easy without Backspace support. Just read the characters.
> But we have to implement Backspace.
> 
> When you leave canonical mode, then agetty code has to handle Backspace.
> 
> Hopefully, agetty doesn't support arrows and Delete (forward delete),
> so we don't have to implement advanced parts of line editing.
> 
> My experiments show two possibilities. I am not sure, what is better,
> but 1 seems to be more straightforward.
> 
> 
> 1) c_lflag &= ~(ICANON | ECHO)
> 
> This falls back to a no echo mode. Program has to handle echo and
> buffer parsing. Program has to handle backspace.
> 
> The implementation seems to be straightforward.
> 
> Downside: Remote logins can experience delays when getting echo.

This seems like a small downside as data from remote machine is pretty
common thing for remote access :-)

> 2) c_lflag &= ~(ICANON)
> 
> Non-canonical mode with echo seems to be usable as well. The terminal
> itself does the echo, program does the input parsing. Program has to
> handle Backspace. Echo will be immediate even with slow remote logins,
> but Backspace has to be handled remotely.
> 
> The second implementation has a problem: The Linux console input and
> output are out of sync in the time of agetty. Input returns 0x7f after
> pressing backspace, but output does back step (not back space) after
> sending 0x08. I don't know the fix yet.

This seems fragile, especially on environment with obscure serial
lines (or serial over ssh, etc.) where nobody has clue about proper
setting. It seems better to be slow than fragile.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] agetty: don't put the VC into canonical mode
  2018-10-19  6:09   ` Lubomir Rintel
@ 2018-10-19 13:08     ` Stanislav Brabec
  2018-10-19 13:48       ` Lubomir Rintel
  0 siblings, 1 reply; 9+ messages in thread
From: Stanislav Brabec @ 2018-10-19 13:08 UTC (permalink / raw)
  To: Lubomir Rintel, Karel Zak; +Cc: util-linux

Dne 19. 10. 18 v 8:09 Lubomir Rintel napsal(a):
> On Tue, 2018-10-16 at 09:40 +0200, Karel Zak wrote:
>> On Tue, Oct 16, 2018 at 08:41:49AM +0200, Lubomir Rintel wrote:
>>> Seems like zero lflags do the job just fine on a Linux VT. Reset it
>>> to
>>> canonical mode before running login.
>>
>> What about Del/Backspace keys when user is typing login name? :-)
> 
> They seem to work the same as they used to?
> 
> From Stanislav's message I notice that there's supposed to be some
> problem with them -- I'm not seeing that. Either the patch does
> something different or I'm not looking at the right thing.
> 
> (I haven't looked at Stanislav's patch -- but I surely intend to give
> it a try.)
I am writing about non-canonical mode. It was partially used in past.

Now agetty uses canonical mode. It handles Backspace by itself, but it
makes impossible to implement "stop reloads when at least one character
is entered". So we will have to switch to a non-canonical mode.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] agetty: don't put the VC into canonical mode
  2018-10-19 13:08     ` Stanislav Brabec
@ 2018-10-19 13:48       ` Lubomir Rintel
  2018-10-19 20:08         ` Stanislav Brabec
  0 siblings, 1 reply; 9+ messages in thread
From: Lubomir Rintel @ 2018-10-19 13:48 UTC (permalink / raw)
  To: Stanislav Brabec, Karel Zak; +Cc: util-linux

On Fri, 2018-10-19 at 15:08 +0200, Stanislav Brabec wrote:
> Dne 19. 10. 18 v 8:09 Lubomir Rintel napsal(a):
> > On Tue, 2018-10-16 at 09:40 +0200, Karel Zak wrote:
> > > On Tue, Oct 16, 2018 at 08:41:49AM +0200, Lubomir Rintel wrote:
> > > > Seems like zero lflags do the job just fine on a Linux VT. Reset it
> > > > to
> > > > canonical mode before running login.
> > > 
> > > What about Del/Backspace keys when user is typing login name? :-)
> > 
> > They seem to work the same as they used to?
> > 
> > From Stanislav's message I notice that there's supposed to be some
> > problem with them -- I'm not seeing that. Either the patch does
> > something different or I'm not looking at the right thing.
> > 
> > (I haven't looked at Stanislav's patch -- but I surely intend to give
> > it a try.)
> I am writing about non-canonical mode.

Yes, so am I.

> It was partially used in past.
> 
> Now agetty uses canonical mode. It handles Backspace by itself, but it
> makes impossible to implement "stop reloads when at least one character
> is entered". So we will have to switch to a non-canonical mode.

I've submitted a patch that does that, and my understanding is that so
did you (I didn't yet take a look -- sorry. I definitely intend to).

I just remarked that I haven't noticed a problem with character erase.
I have no idea why.

Lubo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] agetty: don't put the VC into canonical mode
  2018-10-19 13:48       ` Lubomir Rintel
@ 2018-10-19 20:08         ` Stanislav Brabec
  2018-10-22  9:23           ` Karel Zak
  0 siblings, 1 reply; 9+ messages in thread
From: Stanislav Brabec @ 2018-10-19 20:08 UTC (permalink / raw)
  To: Lubomir Rintel, Karel Zak; +Cc: util-linux

I just ported your patch to the current head and did some experiments.

I verified that input indeed works in the non-canonical mode, and
Backspace is handled. I didn't understand why it works and why my
implementation did not work.

So I started to debug it in deep. We all overseen that the non-canonical +
non-echo mode processing is already implemented in agetty.c: get_logname()
in a previously dead part of the code that was never called:

2180					if ((tp->c_lflag & ECHO) == 0)

This code implements both echo and Backspace processing!

There is one small piece for improvement:
When you press letter and then Backspace, reloads remain blocked.
I guess it is acceptable and fixable later.

Many thanks for your patch!

Note that the patch works even with less aggressive
+		tp->c_lflag &= ~(ICANON | ECHO | ECHOE | ECHOK | ECHOKE);
I am not sure whether setting c_lflag to 0 is completely safe.

Subject: [PATCH] agetty: don't put the VC into canonical mode

The wait_for_term_input()'s select() needs to be tripped when the user
starts typing. Otherwise the reloads can abort an already in-progress login.

Coupled with \4 and \6 expansions that happen to be there on Fedora Server,
this means reload on every netlink event. With a couple of IPv6 routers
announcing their networks and temporary addresses in use can make it
sometimes virtually impossible to log in.

Seems like zero lflags do the job just fine on a Linux VT. Reset it to
canonical mode before running login.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>

---
Changes since v1:
- Unreversed the logic in termio_final()/reset_vc() conditional

Tested on a vc and serial console on a stock Fedora installation:

  /sbin/agetty -o -p -- \u --noclear tty1 linux
  /sbin/agetty -o -p -- \u --keep-baud 115200,38400,9600 ttyS2 vt220
---
 term-utils/agetty.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/term-utils/agetty.c b/term-utils/agetty.c
index 59122ca45..cfcdffe93 100644
--- a/term-utils/agetty.c
+++ b/term-utils/agetty.c
@@ -316,7 +316,7 @@ static void parse_speeds(struct options *op, char *arg);
 static void update_utmp(struct options *op);
 static void open_tty(char *tty, struct termios *tp, struct options *op);
 static void termio_init(struct options *op, struct termios *tp);
-static void reset_vc (const struct options *op, struct termios *tp);
+static void reset_vc(const struct options *op, struct termios *tp, int canon);
 static void auto_baud(struct termios *tp);
 static void list_speeds(void);
 static void output_special_char (struct issue *ie, unsigned char c, struct options *op,
@@ -501,13 +501,14 @@ int main(int argc, char **argv)
 	if (options.timeout)
 		alarm(0);
 
-	if ((options.flags & F_VCONSOLE) == 0) {
-		/* Finalize the termios settings. */
+	/* Finalize the termios settings. */
+	if ((options.flags & F_VCONSOLE) == 0)
 		termio_final(&options, &termios, &chardata);
+	else
+		reset_vc(&options, &termios, 1);
 
-		/* Now the newline character should be properly written. */
-		write_all(STDOUT_FILENO, "\r\n", 2);
-	}
+	/* Now the newline character should be properly written. */
+	write_all(STDOUT_FILENO, "\r\n", 2);
 
 	sigaction(SIGQUIT, &sa_quit, NULL);
 	sigaction(SIGINT, &sa_int, NULL);
@@ -1250,7 +1251,7 @@ static void termio_init(struct options *op, struct termios *tp)
 		setlocale(LC_CTYPE, "POSIX");
 		op->flags &= ~F_UTF8;
 #endif
-		reset_vc(op, tp);
+		reset_vc(op, tp, 0);
 
 		if ((tp->c_cflag & (CS8|PARODD|PARENB)) == CS8)
 			op->flags |= F_EIGHTBITS;
@@ -1360,7 +1361,7 @@ static void termio_init(struct options *op, struct termios *tp)
 }
 
 /* Reset virtual console on stdin to its defaults */
-static void reset_vc(const struct options *op, struct termios *tp)
+static void reset_vc(const struct options *op, struct termios *tp, int canon)
 {
 	int fl = 0;
 
@@ -1369,6 +1370,15 @@ static void reset_vc(const struct options *op, struct termios *tp)
 
 	reset_virtual_console(tp, fl);
 
+#ifdef AGETTY_RELOAD
+	/*
+	 * Discard all the flags that makes the line go canonical with echoing.
+	 * We need to know when the user starts typing.
+	 */
+	if (canon == 0)
+		tp->c_lflag = 0;
+#endif
+
 	if (tcsetattr(STDIN_FILENO, TCSADRAIN, tp))
 		log_warn(_("setting terminal attributes failed: %m"));
 
-- 
2.19.1

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] agetty: don't put the VC into canonical mode
  2018-10-19 20:08         ` Stanislav Brabec
@ 2018-10-22  9:23           ` Karel Zak
  0 siblings, 0 replies; 9+ messages in thread
From: Karel Zak @ 2018-10-22  9:23 UTC (permalink / raw)
  To: Stanislav Brabec; +Cc: Lubomir Rintel, util-linux

On Fri, Oct 19, 2018 at 10:08:17PM +0200, Stanislav Brabec wrote:
> This code implements both echo and Backspace processing!

 :-) I guess it's the original serial line code...

> Note that the patch works even with less aggressive
> +		tp->c_lflag &= ~(ICANON | ECHO | ECHOE | ECHOK | ECHOKE);
> I am not sure whether setting c_lflag to 0 is completely safe.

"It's aggressive" was my first feeling from the patch -- but it seems
there is nothing else in lflags relevant for Linux and agetty and for
serial line we do the same.

> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>

Thanks guys! I have applied the patch to master branch -- it's not
invasive patch ant it seems important enough for v2.33.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-10-22  9:23 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-16  6:41 [PATCH v2] agetty: don't put the VC into canonical mode Lubomir Rintel
2018-10-16  7:40 ` Karel Zak
2018-10-18 19:25   ` Stanislav Brabec
2018-10-19  7:53     ` Karel Zak
2018-10-19  6:09   ` Lubomir Rintel
2018-10-19 13:08     ` Stanislav Brabec
2018-10-19 13:48       ` Lubomir Rintel
2018-10-19 20:08         ` Stanislav Brabec
2018-10-22  9:23           ` Karel Zak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).