RFA: [ser-unix.c] Fix handling of baud rates

RFA: [ser-unix.c] Fix handling of baud rates

[Fernando Nasser]

If the user requests an invalid baud rate (with -b or set remotebaud),
GDB will try to set the serial port speed to -1 and eventually dump
core.

The following patch will cause GDB to use only legal values for the
serial line speed.  If the speed requested is not one of the valid
speeds, the best speed that is less than the value requested will be
used.  However, if the requested speed is less than 50, 50 bauds will be
used.

OK to commit?


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9
Index: ser-unix.c
===================================================================
RCS file: /cvs/src/src/gdb/ser-unix.c,v
retrieving revision 1.12
diff -c -p -r1.12 ser-unix.c
*** ser-unix.c	2001/03/06 08:21:16	1.12
--- ser-unix.c	2001/04/25 21:54:59
*************** rate_to_code (int rate)
*** 741,750 ****
    int i;
  
    for (i = 0; baudtab[i].rate != -1; i++)
!     if (rate == baudtab[i].rate)
!       return baudtab[i].code;
! 
!   return -1;
  }
  
  static int
--- 741,768 ----
    int i;
  
    for (i = 0; baudtab[i].rate != -1; i++)
!     {
!       /* test for perfect macth. */
!       if (rate == baudtab[i].rate)
!         return baudtab[i].code;
!       else
!         {
! 	  /* check if it is in between valid values. */
!           if (rate < baudtab[i].rate)
! 	    {
! 	      /* set to the valid speed immediately below
! 	         or to B50 if the value requested was less
! 		 than 50 bauds. */
! 	      if (i)
! 	        return baudtab[i - 1].code;
! 	      else
! 	        return baudtab[0].code;
! 	    }
!         }
!     }
!  
!   /* If the requested speed was too large, set to the best we can do. */
!   return baudtab[i - 1].code;
  }
  
  static int
From jtc@redback.com Wed Apr 25 16:44:00 2001
From: jtc@redback.com (J.T. Conklin)
To: Fernando Nasser <fnasser@redhat.com>
Cc: gdb-patches@sources.redhat.com
Subject: Re: RFA: [ser-unix.c] Fix handling of baud rates
Date: Wed, 25 Apr 2001 16:44:00 -0000
Message-id: <5mu23c4kao.fsf@jtc.redback.com>
References: <3AE751A5.AE7633E4@redhat.com>
X-SW-Source: 2001-04/msg00242.html
Content-length: 1261

>>>>> "Fernando" == Fernando Nasser <fnasser@redhat.com> writes:
Fernando> If the user requests an invalid baud rate (with -b or set
Fernando> remotebaud), GDB will try to set the serial port speed to -1
Fernando> and eventually dump core.

Fernando> The following patch will cause GDB to use only legal values
Fernando> for the serial line speed.  If the speed requested is not
Fernando> one of the valid speeds, the best speed that is less than
Fernando> the value requested will be used.  However, if the requested
Fernando> speed is less than 50, 50 bauds will be used.

Fernando> OK to commit?

I'm not sure I like the behavior of selecting a rate different than
what the user selected is a good idea.

It appears that the higher layers of the code handle the case when
SERIAL_SETBAUDRATE() returns non-zero, so all that needs to be done is
to return -1 in hardwire_setbaudrate() when rate_to_code returns -1
instead of trying to using that value with cfset{i,o}speed, or masking
it in to termio.c_flag, or setting sgttyb.sg_{i,o}speed.  The only
additional thing we might want to do is to set errno to an appropriate
value so the perror_with_name() calls in the upper layers output a
reasonable message.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: RFA: [ser-unix.c] Fix handling of baud rates

[Eli Zaretskii]

> Date: Wed, 25 Apr 2001 18:37:25 -0400
> From: Fernando Nasser <fnasser@redhat.com>
> 
> The following patch will cause GDB to use only legal values for the
> serial line speed.  If the speed requested is not one of the valid
> speeds, the best speed that is less than the value requested will be
> used.  However, if the requested speed is less than 50, 50 bauds will be
> used.

I'd say it is better to print an error message than silently use the
wrong baudrate.  What if the user simply mistyped the value?

Re: RFA: [ser-unix.c] Fix handling of baud rates

[Fernando Nasser]

Eli Zaretskii wrote:
> 
> > Date: Wed, 25 Apr 2001 18:37:25 -0400
> > From: Fernando Nasser <fnasser@redhat.com>
> >
> > The following patch will cause GDB to use only legal values for the
> > serial line speed.  If the speed requested is not one of the valid
> > speeds, the best speed that is less than the value requested will be
> > used.  However, if the requested speed is less than 50, 50 bauds will be
> > used.
> 
> I'd say it is better to print an error message than silently use the
> wrong baudrate.  What if the user simply mistyped the value?

I don't really mind, I just want to get rid of the core dump.

The problem with the error message is that:
1) there is nothing very nice to set the errno to (suggestions?)
2) it comes too late; the user set this value either in the command 
   line or through a "set baudrate" and it was accepted.  I cannot add
   a test in there because I don't know what the target will be.

To solve the criptic error message, I could add a warning() to the
rate_to_mode() function, so that would preceed the error message (with
the best possible errno):
  "Illegal baud rate %d."

But as we are adding a warning(), why not just say:
  "Illegal baud rate %d; using %d instead."
and gracefully fall back?


Just let me know of the preferences.  This bug is in 5.0, GNUPro and
everywhere and I just want to get rid of the user's complaints.


P.S.: ARM RDI does fall back on the speed if the board (with the Angel
monitor) cannot speak at the speed requested by the user.  The only
indication is in the welcome string returned by the board, which has the
Angel version, copyright and the speed being used (Insight users don't
see that this happened yet).





-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: RFA: [ser-unix.c] Fix handling of baud rates

[Fernando Nasser]

Eli Zaretskii wrote:
> 
> > Date: Thu, 26 Apr 2001 10:15:15 -0400
> > From: Fernando Nasser <fnasser@redhat.com>
> >
> > The problem with the error message is that:
> > 1) there is nothing very nice to set the errno to (suggestions?)
> 
> EINVAL comes to mind, although it is admittedly not very specific.
> 

Yes, but it is good enough if we show the warning first.  Thank you.


> > But as we are adding a warning(), why not just say:
> >   "Illegal baud rate %d; using %d instead."
> 
> Can you prompt for another value (after making sure this is an
> interactive session)?
> 

We already prompt for many things, but I believe we want to get rid of
these prompts outside the interpreters.  It complicates the
interpreter<->libgdb protocol, I've heard.  The idea is that idempotent
libgdb operations fail and the interpreter (after asking the user what
to do) retries them with the proper values.


> (Btw, saying ``illegal'' is discouraged by the GNU standards, unless
> you are talking about actions that break the law.  ``Invalid'' is the
> suggested alternative.)

Thanks, I didn't know that.  Please read is as "invalid".




I just though of another problem with the fall back:  most of the boards
do no accept connection at different speeds and GDB current has a very
annoying behavior of keep trying these impossible connections for ages. 
If it would quickly realize that the communication is not good and give
up this would be a good thing.  Actually, I have many requests from
users that want GDB to stop being so stubborn.  But until that is fixed,
we may end up getting into that trap.  So, we would have to go with the
error solution until that is fixed.

Is that right?






-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: RFA: [ser-unix.c] Fix handling of baud rates

[Eli Zaretskii]

> Date: Thu, 26 Apr 2001 10:45:36 -0400
> From: Fernando Nasser <fnasser@redhat.com>
> 
> > > But as we are adding a warning(), why not just say:
> > >   "Illegal baud rate %d; using %d instead."
> > 
> > Can you prompt for another value (after making sure this is an
> > interactive session)?
> 
> We already prompt for many things, but I believe we want to get rid of
> these prompts outside the interpreters.  It complicates the
> interpreter<->libgdb protocol, I've heard.  The idea is that idempotent
> libgdb operations fail and the interpreter (after asking the user what
> to do) retries them with the proper values.

Yes, that makes sense.  Is there any way that libgdb can give the
interpreter an indication that it wants to prompt the user?

> So, we would have to go with the error solution until that is fixed.
> 
> Is that right?

I'm not very experienced with debugging such boards, but it sounds
that a failure would indeed be the best way out.

Re: RFA: [ser-unix.c] Fix handling of baud rates [REPOST]

[Fernando Nasser]

This patch implements what was discussed with JT and Eli.  An error is
now being issued when the set baud rate is invalid.  Here is the output:

 ./gdb -nw -b 30

(gdb) target remote /dev/ttyS0
warning: Invalid baud rate 30.  Minimum value is 50.
/dev/ttyS0: Invalid argument.
(gdb)


 ./gdb -nw -b 200000

(gdb) target remote /dev/ttyS0
warning: Invalid baud rate 200000.  Closest values are 115200 and
230400.
/dev/ttyS0: Invalid argument.
(gdb)


 ./gdb -nw -b 500000

(gdb) target remote /dev/ttyS0
warning: Invalid baud rate 500000.  Maximum value is 460800.
/dev/ttyS0: Invalid argument.
(gdb)


The patch is attached.  With it, I will fix the calls to
SERIAL_SETBAUDRATE in the few targets that "forgot" to check for the
return code:
   nindy-share/nindy.c
   remote-e7000.c
   remote-st.c


OK to commit now?



ChangeLog:

	* ser-unix.c (rate_to_code): Issue warning if baud rate is invalid.
	(hardwire_setbaudrate): Set errno to EINVAl and return with error
	if the conversion of the baud rate to code fails.




-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9
Index: ser-unix.c
===================================================================
RCS file: /cvs/src/src/gdb/ser-unix.c,v
retrieving revision 1.12
diff -c -p -r1.12 ser-unix.c
*** ser-unix.c	2001/03/06 08:21:16	1.12
--- ser-unix.c	2001/05/10 18:54:36
*************** rate_to_code (int rate)
*** 741,749 ****
    int i;
  
    for (i = 0; baudtab[i].rate != -1; i++)
!     if (rate == baudtab[i].rate)
!       return baudtab[i].code;
! 
    return -1;
  }
  
--- 741,773 ----
    int i;
  
    for (i = 0; baudtab[i].rate != -1; i++)
!     {
!       /* test for perfect macth. */
!       if (rate == baudtab[i].rate)
!         return baudtab[i].code;
!       else
!         {
! 	  /* check if it is in between valid values. */
!           if (rate < baudtab[i].rate)
! 	    {
! 	      if (i)
! 	        {
! 	          warning ("Invalid baud rate %d.  Closest values are %d and %d.",
! 	                    rate, baudtab[i - 1].rate, baudtab[i].rate);
! 		}
! 	      else
! 	        {
! 	          warning ("Invalid baud rate %d.  Minimum value is %d.",
! 	                    rate, baudtab[0].rate);
! 		}
! 	      return -1;
! 	    }
!         }
!     }
!  
!   /* The requested speed was too large. */
!   warning ("Invalid baud rate %d.  Maximum value is %d.",
!             rate, baudtab[i - 1].rate);
    return -1;
  }
  
*************** static int
*** 751,763 ****
  hardwire_setbaudrate (serial_t scb, int rate)
  {
    struct hardwire_ttystate state;
  
    if (get_tty_state (scb, &state))
      return -1;
  
  #ifdef HAVE_TERMIOS
!   cfsetospeed (&state.termios, rate_to_code (rate));
!   cfsetispeed (&state.termios, rate_to_code (rate));
  #endif
  
  #ifdef HAVE_TERMIO
--- 775,796 ----
  hardwire_setbaudrate (serial_t scb, int rate)
  {
    struct hardwire_ttystate state;
+   int baud_code = rate_to_code (rate);
+   
+   if (baud_code < 0)
+     {
+       /* The baud rate was not valid.
+          A warning has already been issued. */
+       errno = EINVAL;
+       return -1;
+     }
  
    if (get_tty_state (scb, &state))
      return -1;
  
  #ifdef HAVE_TERMIOS
!   cfsetospeed (&state.termios, baud_code);
!   cfsetispeed (&state.termios, baud_code);
  #endif
  
  #ifdef HAVE_TERMIO
*************** hardwire_setbaudrate (serial_t scb, int 
*** 766,777 ****
  #endif
  
    state.termio.c_cflag &= ~(CBAUD | CIBAUD);
!   state.termio.c_cflag |= rate_to_code (rate);
  #endif
  
  #ifdef HAVE_SGTTY
!   state.sgttyb.sg_ispeed = rate_to_code (rate);
!   state.sgttyb.sg_ospeed = rate_to_code (rate);
  #endif
  
    return set_tty_state (scb, &state);
--- 799,810 ----
  #endif
  
    state.termio.c_cflag &= ~(CBAUD | CIBAUD);
!   state.termio.c_cflag |= baud_code;
  #endif
  
  #ifdef HAVE_SGTTY
!   state.sgttyb.sg_ispeed = baud_code;
!   state.sgttyb.sg_ospeed = baud_code;
  #endif
  
    return set_tty_state (scb, &state);
From ac131313@cygnus.com Thu May 10 12:23:00 2001
From: Andrew Cagney <ac131313@cygnus.com>
To: Michael Snyder <msnyder@cygnus.com>
Cc: gdb-patches@sources.redhat.com, cagney@cygnus.com
Subject: Re: [PATCH] factor out some functions in remote.c
Date: Thu, 10 May 2001 12:23:00 -0000
Message-id: <3AFAEA9C.2070609@cygnus.com>
References: <3AFAE59D.71F434B4@cygnus.com>
X-SW-Source: 2001-05/msg00176.html
Content-length: 199

> +   /* May use a length, or a nul-terminated string as input. */
> +   if (count == 0)
> +     count = strlen (hex) / 2;
> + 
Michael,  you've still not addressed my concern about this.

	Andrew

Re: RFA: [ser-unix.c] Fix handling of baud rates [REPOST]

[Eli Zaretskii]

> Date: Thu, 10 May 2001 15:06:23 -0400
> From: Fernando Nasser <fnasser@redhat.com>
> 
> This patch implements what was discussed with JT and Eli.  An error is
> now being issued when the set baud rate is invalid.  Here is the output:
> 
>  ./gdb -nw -b 30
> 
> (gdb) target remote /dev/ttyS0
> warning: Invalid baud rate 30.  Minimum value is 50.
> /dev/ttyS0: Invalid argument.
> (gdb)
> 
> 
>  ./gdb -nw -b 200000
> 
> (gdb) target remote /dev/ttyS0
> warning: Invalid baud rate 200000.  Closest values are 115200 and
> 230400.
> /dev/ttyS0: Invalid argument.
> (gdb)
> 
> 
>  ./gdb -nw -b 500000
> 
> (gdb) target remote /dev/ttyS0
> warning: Invalid baud rate 500000.  Maximum value is 460800.
> /dev/ttyS0: Invalid argument.
> (gdb)

Looks good to me, thanks.

Re: RFA: [ser-unix.c] Fix handling of baud rates [REPOST]

[Fernando Nasser]

Well, I have already addressed JT and Eli's concerns.  Eli has reviewed
this last version and said it is OK.

I could not find a specific maintainer for ser-unix.c to get a formal
approval. This has been around for a while and no one besides JT and Eli
had any objection, so I guess it is time to check it in.  We can't leave
GDB dumping core for such a simple user input error.

I will commit the change.

Fernando 

Fernando Nasser wrote:
> 
> This patch implements what was discussed with JT and Eli.  An error is
> now being issued when the set baud rate is invalid.  Here is the output:
> 
>  ./gdb -nw -b 30
> 
> (gdb) target remote /dev/ttyS0
> warning: Invalid baud rate 30.  Minimum value is 50.
> /dev/ttyS0: Invalid argument.
> (gdb)
> 
>  ./gdb -nw -b 200000
> 
> (gdb) target remote /dev/ttyS0
> warning: Invalid baud rate 200000.  Closest values are 115200 and
> 230400.
> /dev/ttyS0: Invalid argument.
> (gdb)
> 
>  ./gdb -nw -b 500000
> 
> (gdb) target remote /dev/ttyS0
> warning: Invalid baud rate 500000.  Maximum value is 460800.
> /dev/ttyS0: Invalid argument.
> (gdb)
> 
> The patch is attached.  With it, I will fix the calls to
> SERIAL_SETBAUDRATE in the few targets that "forgot" to check for the
> return code:
>    nindy-share/nindy.c
>    remote-e7000.c
>    remote-st.c
> 
> OK to commit now?
> 
> ChangeLog:
> 
>         * ser-unix.c (rate_to_code): Issue warning if baud rate is invalid.
>         (hardwire_setbaudrate): Set errno to EINVAl and return with error
>         if the conversion of the baud rate to code fails.
> 
> --
> Fernando Nasser
> Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
> 2323 Yonge Street, Suite #300
> Toronto, Ontario   M4P 2C9
> 
>   ------------------------------------------------------------------------
> Index: ser-unix.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/ser-unix.c,v
> retrieving revision 1.12
> diff -c -p -r1.12 ser-unix.c
> *** ser-unix.c  2001/03/06 08:21:16     1.12
> --- ser-unix.c  2001/05/10 18:54:36
> *************** rate_to_code (int rate)
> *** 741,749 ****
>     int i;
> 
>     for (i = 0; baudtab[i].rate != -1; i++)
> !     if (rate == baudtab[i].rate)
> !       return baudtab[i].code;
> !
>     return -1;
>   }
> 
> --- 741,773 ----
>     int i;
> 
>     for (i = 0; baudtab[i].rate != -1; i++)
> !     {
> !       /* test for perfect macth. */
> !       if (rate == baudtab[i].rate)
> !         return baudtab[i].code;
> !       else
> !         {
> !         /* check if it is in between valid values. */
> !           if (rate < baudtab[i].rate)
> !           {
> !             if (i)
> !               {
> !                 warning ("Invalid baud rate %d.  Closest values are %d and %d.",
> !                           rate, baudtab[i - 1].rate, baudtab[i].rate);
> !               }
> !             else
> !               {
> !                 warning ("Invalid baud rate %d.  Minimum value is %d.",
> !                           rate, baudtab[0].rate);
> !               }
> !             return -1;
> !           }
> !         }
> !     }
> !
> !   /* The requested speed was too large. */
> !   warning ("Invalid baud rate %d.  Maximum value is %d.",
> !             rate, baudtab[i - 1].rate);
>     return -1;
>   }
> 
> *************** static int
> *** 751,763 ****
>   hardwire_setbaudrate (serial_t scb, int rate)
>   {
>     struct hardwire_ttystate state;
> 
>     if (get_tty_state (scb, &state))
>       return -1;
> 
>   #ifdef HAVE_TERMIOS
> !   cfsetospeed (&state.termios, rate_to_code (rate));
> !   cfsetispeed (&state.termios, rate_to_code (rate));
>   #endif
> 
>   #ifdef HAVE_TERMIO
> --- 775,796 ----
>   hardwire_setbaudrate (serial_t scb, int rate)
>   {
>     struct hardwire_ttystate state;
> +   int baud_code = rate_to_code (rate);
> +
> +   if (baud_code < 0)
> +     {
> +       /* The baud rate was not valid.
> +          A warning has already been issued. */
> +       errno = EINVAL;
> +       return -1;
> +     }
> 
>     if (get_tty_state (scb, &state))
>       return -1;
> 
>   #ifdef HAVE_TERMIOS
> !   cfsetospeed (&state.termios, baud_code);
> !   cfsetispeed (&state.termios, baud_code);
>   #endif
> 
>   #ifdef HAVE_TERMIO
> *************** hardwire_setbaudrate (serial_t scb, int
> *** 766,777 ****
>   #endif
> 
>     state.termio.c_cflag &= ~(CBAUD | CIBAUD);
> !   state.termio.c_cflag |= rate_to_code (rate);
>   #endif
> 
>   #ifdef HAVE_SGTTY
> !   state.sgttyb.sg_ispeed = rate_to_code (rate);
> !   state.sgttyb.sg_ospeed = rate_to_code (rate);
>   #endif
> 
>     return set_tty_state (scb, &state);
> --- 799,810 ----
>   #endif
> 
>     state.termio.c_cflag &= ~(CBAUD | CIBAUD);
> !   state.termio.c_cflag |= baud_code;
>   #endif
> 
>   #ifdef HAVE_SGTTY
> !   state.sgttyb.sg_ispeed = baud_code;
> !   state.sgttyb.sg_ospeed = baud_code;
>   #endif
> 
>     return set_tty_state (scb, &state);

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9