Re: Setting parity for remote serial

[Joel Brobecker <brobecker@adacore.com>]

> Unfortunately, now I don't have the ability to fully-test it with
> windows (but I was checking initial version one year ago, with real
> hardware). Now it has been compiled by using mingw with
> --host=i586-mingw32msvc.

No problem - the code looked about right to me, and we will let those
who do work on Windows take care of fixing any bugs if they use this
feature and encounter a bug.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ca8bbaf..02f9ddc 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,37 @@
> +2015-03-21  Yurij Grechishhev  <yurij.grechishhev@gmail.com>
> +
> +    * NEWS: Mention set/show serial parity command.
> +    * monitor.c (monitor_open): Call serial_setparity.
> +    * remote.c (remote_open_1): Likewise.
> +    * ser-base.c (ser_base_serparity): New function.
> +    * ser-base.h (ser_base_setparity): Add  declaration.
> +    * ser-go32.c (dos_ops): Add dos_noop field.

The name of the struct field is actually "setparity" (instead of
"dos_noop").

> +    * ser-mingw.c (ser_windows_raw): Remove state.fParity and
> +    state.Parity definitions.

"Do not set state.fParity and state.Parity". You remmoved assignments,
rather than definitions.

> +    (ser_windows_setparity): New function.
> +    (hardwire_ops): Add ser_windows_setparity.
> +    (tty_ops): Add NULL for setparity field.
> +    (pipe_ops): Add ser_base_setparity.
> +    (tcp_ops): Likewise.
> +    * ser-pipe.c (pipe_ops): Likewise.
> +    * ser-tcp.c (tcp_ops): Likewise.
> +    * ser-unix.c (hardwire_setparity): Add declaration.
> +    (hardwire_raw): Don't reset PARENB flag.
> +    (hardwire_setparity): New function.
> +    (hardwire_ops): Add hardwire_setparity.
> +    * serial.c (serial_setparity): New function.
> +    (serial_parity): New global.
> +    (parity_none, parity_odd, parity_even, parity_enums, parity):
> +        New static globals.

Wrong indentation of "New static globals". I suspect that this is
because you used 8 spaces instead of a tab to indent that line?

> +    (set_parity): New function.
> +    (_initialize_serial): Add set/show serial parity commands.
> +    * serial.h (GDBPARITY_NONE): Define.
> +    (GDBPARITY_ODD): Define.
> +    (GDBPARITY_EVEN): Define.
> +    (serial_setparity) Add declaration.
> +    (serial_ops): Add setparity entry.

Let's say...

        (struct serial_ops): Add setparity field

... instead.

> +    * target.h (serial_parity): Add declaration.

The rest are minor comments:

> +      internal_warning (__FILE__, __LINE__, "Incorrect parity value:
> %d", parity);

This line is too long. It shouldn't exceed 79 characters.

      internal_warning (__FILE__, __LINE__, "Incorrect parity value: %d", parity);

Use instead:

      internal_warning (__FILE__, __LINE__,
			"Incorrect parity value: %d", parity);

Also, watch out for the fact that your mailer appears to have wrapped
the line. I suggest you switch to sending the patch either using
"git send-email" (the recommended method), or else sending the patch
as an attachment ("git format-patch" + attach to email).

> @@ -409,7 +410,7 @@ hardwire_raw (struct serial *scb)
>    state.termios.c_iflag = 0;
>    state.termios.c_oflag = 0;
>    state.termios.c_lflag = 0;
> -  state.termios.c_cflag &= ~(CSIZE | PARENB);
> +  state.termios.c_cflag &= ~(CSIZE);

You don't really need the parentheses anymore -> ~CSIZE

>    state.termios.c_cflag |= CLOCAL | CS8;
>  #ifdef CRTSCTS
>    /* h/w flow control.  */
> @@ -432,7 +433,7 @@ hardwire_raw (struct serial *scb)
>    state.termio.c_iflag = 0;
>    state.termio.c_oflag = 0;
>    state.termio.c_lflag = 0;
> -  state.termio.c_cflag &= ~(CSIZE | PARENB);
> +  state.termio.c_cflag &= ~(CSIZE);

Likewise.

> @@ -893,6 +894,50 @@ hardwire_setstopbits (struct serial *scb, int num)
>    return set_tty_state (scb, &state);
>  }
> 
> +/* Implement the "setparity" serial_ops callback.  */
> +
> +static int
> +hardwire_setparity (struct serial *scb, int parity)
> +{
> +  struct hardwire_ttystate state;
> +  int newparity = 0;
> +
> +  if (get_tty_state (scb, &state))
> +    return -1;
> +
> +  switch (parity)
> +    {
> +    case GDBPARITY_NONE:
> +      newparity = 0;
> +      break;
> +    case GDBPARITY_ODD:
> +      newparity = PARENB | PARODD;
> +      break;
> +    case GDBPARITY_EVEN:
> +      newparity = PARENB;
> +      break;
> +    default:
> +      internal_warning (__FILE__, __LINE__, "Incorrect parity value:
> %d", parity);

Same as above - line is too long.

> +/* See serial.h.  */
> +
> +int
> +serial_setparity (struct serial *scb, int parity)
> +{
> +    return scb->ops->setparity (scb, parity);

The indentation is wrong (we use a 2-space indentation). Hence:

    int
    serial_setparity (struct serial *scb, int parity)
    {
      return scb->ops->setparity (scb, parity);
    }

-- 
Joel