Re: [PATCH2] Basic braille screen reader support

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

Andrew Morton, le Sat 23 Feb 2008 00:04:10 -0800, a écrit :
> > +++ linux-2.6.24.1-perso/kernel/printk.c	2008-02-21 12:09:06.000000000 +0100
> > @@ -105,6 +105,7 @@
> 
> Please use `diff -p'

Argl, that's editdiff's fault.  We need to fix it to keep the -p
comments.

> >  	char	name[8];			/* Name of the driver	    */
> >  	int	index;				/* Minor dev. to use	    */
> >  	char	*options;			/* Options for the driver   */
> > +	char	*brl_options;			/* Options for braille driver */
> >  };
> 
> Should this depend on CONFIG_A11Y_BRAILLE_CONSOLE or whatever?

It can, indeed.  It just makes the code a little more clumsy to save 32
or 64 bytes.

> > +	for (i = 0; i < MAX_CMDLINECONSOLES && console_cmdline[i].name[0]; i++)
> > +		if (strcmp(console_cmdline[i].name, name) == 0 &&
> > +			  console_cmdline[i].index == idx) {
> > +				if (!brl_options)
> > +					selected_console = i;
> > +				return 0;
> > +		}
> > +	if (i == MAX_CMDLINECONSOLES)
> > +		return -E2BIG;
> 
> Does the array of consles have any locking here?

Well, that's actually just the original code that I moved a bit earlier,
so the code path is not changed.

> > +	if (!brl_options)
> > +		selected_console = i;
> > +	c = &console_cmdline[i];
> > +	memcpy(c->name, name, sizeof(c->name));
> > +	c->name[sizeof(c->name) - 1] = 0;
> 
> strlcpy()?

Again, that's the original code, unmodified, but we can put an strlcpy
here yes.

> > +static void beep(unsigned int freq)
> > +{
> > +	if (sound)
> > +		kd_mksound(freq, HZ/10);
> > +}
> 
> hm, do we have enough Kconfig dependencies here to ensure that kd_mksound()
> is always available?

Ah, CONFIG_VT is needed indeed.

I'll address those in replies to this mail.

Samuel