Re: termios constants should be unsigned

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jun 12, 2024 at 02:16:12PM +0200, Alejandro Colomar wrote:
> Hi!
> 
> While compiling an example program in ioctl_tty(2) examples (now moving
> to TCSETS(2const)), I saw several warnings:
> 
> tcgets.c:53:24: error: implicit conversion changes signedness: 'int' to 'tcflag_t' (aka 'unsigned int') [clang-diagnostic-sign-conversion,-warnings-as-errors]
>         tio.c_cflag &= ~CBAUD;
>                     ~~ ^~~~~~
> tcgets.c:53:24: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise]
>         tio.c_cflag &= ~CBAUD;
>                     ~~ ^~~~~~
> tcgets.c:58:24: error: implicit conversion changes signedness: 'int' to 'tcflag_t' (aka 'unsigned int') [clang-diagnostic-sign-conversion,-warnings-as-errors]
>         tio.c_cflag &= ~(CBAUD << IBSHIFT);
>                     ~~ ^~~~~~~~~~~~~~~~~~~
> tcgets.c:58:24: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise]
>         tio.c_cflag &= ~(CBAUD << IBSHIFT);
>                     ~~ ^~~~~~~~~~~~~~~~~~~
> tcgets.c:58:25: warning: use of a signed integer operand with a unary bitwise operator [hicpp-signed-bitwise]
>         tio.c_cflag &= ~(CBAUD << IBSHIFT);
>                        ~^~~~~~~~~~~~~~~~~~
> 
> Which seem reasonable warnings.  Bitwise operations on signed integers
> is bad for ye health.  Let's see the definitions of struct termios:
> 
> 	alx@debian:/usr/include$ grepc termios x86_64-linux-gnu/ asm-generic/
> 	x86_64-linux-gnu/bits/termios-struct.h:struct termios
> 	  {
> 	    tcflag_t c_iflag;		/* input mode flags */
> 	    tcflag_t c_oflag;		/* output mode flags */
> 	    tcflag_t c_cflag;		/* control mode flags */
> 	    tcflag_t c_lflag;		/* local mode flags */
> 	    cc_t c_line;			/* line discipline */
> 	    cc_t c_cc[NCCS];		/* control characters */
> 	    speed_t c_ispeed;		/* input speed */
> 	    speed_t c_ospeed;		/* output speed */
> 	#define _HAVE_STRUCT_TERMIOS_C_ISPEED 1
> 	#define _HAVE_STRUCT_TERMIOS_C_OSPEED 1
> 	  };
> 	asm-generic/termbits.h:struct termios {
> 		tcflag_t c_iflag;		/* input mode flags */
> 		tcflag_t c_oflag;		/* output mode flags */
> 		tcflag_t c_cflag;		/* control mode flags */
> 		tcflag_t c_lflag;		/* local mode flags */
> 		cc_t c_line;			/* line discipline */
> 		cc_t c_cc[NCCS];		/* control characters */
> 	};
> 
> Except for the speed members, they seem the same.  The types of the
> members are correctly unsigned:
> 
> 	alx@debian:/usr/include$ grepc tcflag_t x86_64-linux-gnu/ asm-generic/
> 	x86_64-linux-gnu/bits/termios.h:typedef unsigned int	tcflag_t;
> 	asm-generic/termbits.h:typedef unsigned int	tcflag_t;
> 
> So, all of the constants that are to be used in these members, which
> will undergo bitwise operations, should use the U suffix.  Here's the
> list for the UAPI:
> 
> 	$ sed -n '/flag bits/,/^$/p' asm-generic/termbits.h
> 	/* c_iflag bits */
> 	#define IUCLC	0x0200
> 	#define IXON	0x0400
> 	#define IXOFF	0x1000
> 	#define IMAXBEL	0x2000
> 	#define IUTF8	0x4000
> 
> 	/* c_oflag bits */
> 	#define OLCUC	0x00002
> 	#define ONLCR	0x00004
> 	#define NLDLY	0x00100
> 	#define   NL0	0x00000
> 	#define   NL1	0x00100
> 	#define CRDLY	0x00600
> 	#define   CR0	0x00000
> 	#define   CR1	0x00200
> 	#define   CR2	0x00400
> 	#define   CR3	0x00600
> 	#define TABDLY	0x01800
> 	#define   TAB0	0x00000
> 	#define   TAB1	0x00800
> 	#define   TAB2	0x01000
> 	#define   TAB3	0x01800
> 	#define   XTABS	0x01800
> 	#define BSDLY	0x02000
> 	#define   BS0	0x00000
> 	#define   BS1	0x02000
> 	#define VTDLY	0x04000
> 	#define   VT0	0x00000
> 	#define   VT1	0x04000
> 	#define FFDLY	0x08000
> 	#define   FF0	0x00000
> 	#define   FF1	0x08000
> 
> 	/* c_lflag bits */
> 	#define ISIG	0x00001
> 	#define ICANON	0x00002
> 	#define XCASE	0x00004
> 	#define ECHO	0x00008
> 	#define ECHOE	0x00010
> 	#define ECHOK	0x00020
> 	#define ECHONL	0x00040
> 	#define NOFLSH	0x00080
> 	#define TOSTOP	0x00100
> 	#define ECHOCTL	0x00200
> 	#define ECHOPRT	0x00400
> 	#define ECHOKE	0x00800
> 	#define FLUSHO	0x01000
> 	#define PENDIN	0x04000
> 	#define IEXTEN	0x08000
> 	#define EXTPROC	0x10000
> 
> And glibc also (re)defines some of them, so those should also have the
> U suffix.
> 
> Any opinions?

Have a proposed patch that you feel would resolve this?

thanks,

greg k-h