linux-kernel - Re: [PATCH v3] drivers/staging: refactor dgnc tty registration.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9efb4f29-5097-91bd-8aba-7fd247f65a5c@gmail.com>
Date:   Mon, 15 May 2017 16:49:16 +0300
From:   Haim Daniel <haimdaniel@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     lidza.louina@...il.com, markh@...pro.net,
        gregkh@...uxfoundation.org, driverdev-devel@...uxdriverproject.org,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] drivers/staging: refactor dgnc tty registration.

me@...m-toshiba1 ~ $ dpkg -l sparse
Desired=Unknown/Install/Remove/Purge/Hold
| 
Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name              Version       Architecture  Description
+++-=================-=============-=============-========================================
ii  sparse            0.4.5~rc1-1   amd64         semantic parser of 
source files

me@...m-toshiba1 linux (next-20170512) $ make clean M=drivers/staging 
/dgnc/; make C=1 M=drivers/staging/dgnc/

drivers/staging/dgnc/dgnc_tty.c:66:25: warning: too long 
initializer-string for array of char

On 05/15/2017 03:52 PM, Dan Carpenter wrote:
> On Mon, May 15, 2017 at 03:30:50PM +0300, Haim Daniel wrote:
>> -remove duplicate tty allocation code for serial and printer drivers.
>> -fix sparse warning: too long initializer-string for array of char.
>
> I think my version of Sparse is too old.  I don't get a warning.  Please
> cut and paste the exact warning.
>
>>
>> Signed-off-by: Haim Daniel <haimdaniel@...il.com>
>> ---
>>  drivers/staging/dgnc/dgnc_driver.h |  13 ----
>>  drivers/staging/dgnc/dgnc_tty.c    | 150 +++++++++++++++----------------------
>>  2 files changed, 59 insertions(+), 104 deletions(-)
>>
>> diff --git a/drivers/staging/dgnc/dgnc_driver.h b/drivers/staging/dgnc/dgnc_driver.h
>> index 980410f..764d6fe 100644
>> --- a/drivers/staging/dgnc/dgnc_driver.h
>> +++ b/drivers/staging/dgnc/dgnc_driver.h
>> @@ -52,19 +52,6 @@
>>
>>  #define dgnc_jiffies_from_ms(a) (((a) * HZ) / 1000)
>>
>> -/*
>> - * Define a local default termios struct. All ports will be created
>> - * with this termios initially.  This is the same structure that is defined
>> - * as the default in tty_io.c with the same settings overridden as in serial.c
>> - *
>> - * In short, this should match the internal serial ports' defaults.
>> - */
>> -#define	DEFAULT_IFLAGS	(ICRNL | IXON)
>> -#define	DEFAULT_OFLAGS	(OPOST | ONLCR)
>> -#define	DEFAULT_CFLAGS	(B9600 | CS8 | CREAD | HUPCL | CLOCAL)
>> -#define	DEFAULT_LFLAGS	(ISIG | ICANON | ECHO | ECHOE | ECHOK | \
>> -			ECHOCTL | ECHOKE | IEXTEN)
>> -
>>  #ifndef _POSIX_VDISABLE
>>  #define   _POSIX_VDISABLE '\0'
>>  #endif
>> diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
>> index 9e98781..d3736da 100644
>> --- a/drivers/staging/dgnc/dgnc_tty.c
>> +++ b/drivers/staging/dgnc/dgnc_tty.c
>> @@ -51,22 +51,6 @@
>>  	.digi_term =	"ansi"		/* default terminal type */
>>  };
>>
>> -/*
>> - * Define a local default termios struct. All ports will be created
>> - * with this termios initially.
>> - *
>> - * This defines a raw port at 9600 baud, 8 data bits, no parity,
>> - * 1 stop bit.
>> - */
>> -static const struct ktermios default_termios = {
>> -	.c_iflag =	(DEFAULT_IFLAGS),
>> -	.c_oflag =	(DEFAULT_OFLAGS),
>> -	.c_cflag =	(DEFAULT_CFLAGS),
>> -	.c_lflag =	(DEFAULT_LFLAGS),
>> -	.c_cc =		INIT_C_CC,
>
> We don't need INIT_C_CC?
No, the idea was reusing the tty_std_termios, and setting only the 
relevant params.

>
>> -	.c_line =	0,
>> -};
>> -
>>  static int dgnc_tty_open(struct tty_struct *tty, struct file *file);
>>  static void dgnc_tty_close(struct tty_struct *tty, struct file *file);
>>  static int dgnc_block_til_ready(struct tty_struct *tty, struct file *file,
>> @@ -129,6 +113,49 @@ static void dgnc_tty_set_termios(struct tty_struct *tty,
>>
>>  /* TTY Initialization/Cleanup Functions */
>>
>> +static struct tty_driver *dgnc_tty_create(char *serial_name, uint maxports,
>> +					  int major, int minor)
>> +{
>> +	int rc;
>> +	struct tty_driver *drv;
>> +
>> +	drv = tty_alloc_driver(maxports,
>> +			       TTY_DRIVER_REAL_RAW |
>> +			       TTY_DRIVER_DYNAMIC_DEV |
>> +			       TTY_DRIVER_HARDWARE_BREAK);
>> +	if (IS_ERR(drv))
>> +		return drv;
>> +
>> +	drv->name = serial_name;
>> +	drv->name_base = 0;
>> +	drv->major = major;
>> +	drv->minor_start = minor;
>> +	drv->type = TTY_DRIVER_TYPE_SERIAL;
>> +	drv->subtype = SERIAL_TYPE_NORMAL;
>> +	drv->init_termios = tty_std_termios;
>> +	drv->init_termios.c_cflag = (B9600 | CS8 | CREAD | HUPCL | CLOCAL);
>> +	drv->init_termios.c_ispeed = 9600;
>> +	drv->init_termios.c_ospeed = 9600;
>
> Setting c_ispeed and c_ospeed is almost certainly correct, but they're
> not mentioned in the changelog.
Ack, will do.
> Btw, the overwhelming vast majority of staging drivers are sent without
> testing.  But it looks like maybe you are testing yours?  Please mention
> that in the changelog because it makes us feel warm inside.
I only unit tested a fraction of the code, will mention it in the 
changelog, for the sake of that fuzzy feeling :)

> Otherwise it looks fine to me.  Sorry for making you redo it so many
> times instead of reviewing thouroughly all at once.  I shouldn't have
> been so lazy.
np.

> I still feel a bit bad about my review that I didn't spot bug Sparse
> saw...
>
> regards,
> dan carpenter
>
>