[patch] staging: panel: pass correct lengths to keypad_send_key()

[patch] staging: panel: pass correct lengths to keypad_send_key()

[Dan Carpenter]

We changed the sizeof() statements in 429ccf058b "staging:panel: Fixed
coding conventions." so that they could fit inside the 80 character
line limit.  Unfortunately, the new sizeof() statements are a smaller
size.  This reverts it.

There isn't a nice way to stay within the 80 character limit without
a re-work so I've gone over.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Untested.  The ->u.kbd.press_str and friends are funny data types.  I'm
not sure how this code, works but the stuff we have is wrong and I've
reverted it back to the original.

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index d9fec5b..dca42ce 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -1758,7 +1758,7 @@ static inline int input_state_high(struct logical_input *input)
 				char *press_str = input->u.kbd.press_str;
 				if (press_str[0])
 					keypad_send_key(press_str,
-							sizeof(press_str));
+							sizeof(input->u.kbd.press_str));
 			}
 
 			if (input->u.kbd.repeat_str[0]) {
@@ -1766,7 +1766,7 @@ static inline int input_state_high(struct logical_input *input)
 				if (input->high_timer >= KEYPAD_REP_START) {
 					input->high_timer -= KEYPAD_REP_DELAY;
 					keypad_send_key(repeat_str,
-							sizeof(repeat_str));
+							sizeof(input->u.kbd.repeat_str));
 				}
 				/* we will need to come back here soon */
 				inputs_stable = 0;
@@ -1805,7 +1805,7 @@ static inline void input_state_falling(struct logical_input *input)
 				if (input->high_timer >= KEYPAD_REP_START)
 					input->high_timer -= KEYPAD_REP_DELAY;
 					keypad_send_key(repeat_str,
-							sizeof(repeat_str));
+							sizeof(input->u.kbd.repeat_str));
 				/* we will need to come back here soon */
 				inputs_stable = 0;
 			}
@@ -1824,7 +1824,7 @@ static inline void input_state_falling(struct logical_input *input)
 			char *release_str = input->u.kbd.release_str;
 			if (release_str[0])
 				keypad_send_key(release_str,
-						sizeof(release_str));
+						sizeof(input->u.kbd.release_str));
 		}
 
 		input->state = INPUT_ST_LOW;

Re: [patch] staging: panel: pass correct lengths to keypad_send_key()

[walter harms]

Am 29.11.2012 15:38, schrieb Dan Carpenter:
> We changed the sizeof() statements in 429ccf058b "staging:panel: Fixed
> coding conventions." so that they could fit inside the 80 character
> line limit.  Unfortunately, the new sizeof() statements are a smaller
> size.  This reverts it.
> 
> There isn't a nice way to stay within the 80 character limit without
> a re-work so I've gone over.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Untested.  The ->u.kbd.press_str and friends are funny data types.  I'm
> not sure how this code, works but the stuff we have is wrong and I've
> reverted it back to the original.
> 
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index d9fec5b..dca42ce 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -1758,7 +1758,7 @@ static inline int input_state_high(struct logical_input *input)
>  				char *press_str = input->u.kbd.press_str;
>  				if (press_str[0])
>  					keypad_send_key(press_str,
> -							sizeof(press_str));
> +							sizeof(input->u.kbd.press_str));
>  			}
yep, this looks like the original intention.
NTL i would like to question the use of  press_str and friends.
Moving the if (press_str[0]) into keypad_send_key() seems
more sensible.

just my two cents,

re,
 wh


>  
>  			if (input->u.kbd.repeat_str[0]) {
> @@ -1766,7 +1766,7 @@ static inline int input_state_high(struct logical_input *input)
>  				if (input->high_timer >= KEYPAD_REP_START) {
>  					input->high_timer -= KEYPAD_REP_DELAY;
>  					keypad_send_key(repeat_str,
> -							sizeof(repeat_str));
> +							sizeof(input->u.kbd.repeat_str));
>  				}
>  				/* we will need to come back here soon */
>  				inputs_stable = 0;
> @@ -1805,7 +1805,7 @@ static inline void input_state_falling(struct logical_input *input)
>  				if (input->high_timer >= KEYPAD_REP_START)
>  					input->high_timer -= KEYPAD_REP_DELAY;
>  					keypad_send_key(repeat_str,
> -							sizeof(repeat_str));
> +							sizeof(input->u.kbd.repeat_str));
>  				/* we will need to come back here soon */
>  				inputs_stable = 0;
>  			}
> @@ -1824,7 +1824,7 @@ static inline void input_state_falling(struct logical_input *input)
>  			char *release_str = input->u.kbd.release_str;
>  			if (release_str[0])
>  				keypad_send_key(release_str,
> -						sizeof(release_str));
> +						sizeof(input->u.kbd.release_str));
>  		}
>  
>  		input->state = INPUT_ST_LOW;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [patch] staging: panel: pass correct lengths to keypad_send_key()

[Willy Tarreau]

Hi Dan,

On Thu, Nov 29, 2012 at 05:38:35PM +0300, Dan Carpenter wrote:
> We changed the sizeof() statements in 429ccf058b "staging:panel: Fixed
> coding conventions." so that they could fit inside the 80 character
> line limit.  Unfortunately, the new sizeof() statements are a smaller
> size.  This reverts it.
> 
> There isn't a nice way to stay within the 80 character limit without
> a re-work so I've gone over.

Don't bother too much about the limit anyway, it's more of a guideline,
and the first time the code was adapted to fit, it became totally unreadable.

And BTW, I think that the only reason for the bug you found precisely is
the introduction of these repeat_str/release_str local variables that seem
to be there just to reduce line length...

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Untested.  The ->u.kbd.press_str and friends are funny data types.

I agree, it looks like the initial intent was to have the strings
fit well within the union, but this is all but obvious.

> I'm
> not sure how this code, works but the stuff we have is wrong and I've
> reverted it back to the original.

Acked-by: Willy Tarreau <w@1wt.eu>

Willy

Re: [patch] staging: panel: pass correct lengths to keypad_send_key()

[Willy Tarreau]

On Fri, Nov 30, 2012 at 10:19:06AM +0100, walter harms wrote:
> 
> 
> Am 29.11.2012 15:38, schrieb Dan Carpenter:
> > We changed the sizeof() statements in 429ccf058b "staging:panel: Fixed
> > coding conventions." so that they could fit inside the 80 character
> > line limit.  Unfortunately, the new sizeof() statements are a smaller
> > size.  This reverts it.
> > 
> > There isn't a nice way to stay within the 80 character limit without
> > a re-work so I've gone over.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Untested.  The ->u.kbd.press_str and friends are funny data types.  I'm
> > not sure how this code, works but the stuff we have is wrong and I've
> > reverted it back to the original.
> > 
> > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > index d9fec5b..dca42ce 100644
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -1758,7 +1758,7 @@ static inline int input_state_high(struct logical_input *input)
> >  				char *press_str = input->u.kbd.press_str;
> >  				if (press_str[0])
> >  					keypad_send_key(press_str,
> > -							sizeof(press_str));
> > +							sizeof(input->u.kbd.press_str));
> >  			}
> yep, this looks like the original intention.
> NTL i would like to question the use of  press_str and friends.
> Moving the if (press_str[0]) into keypad_send_key() seems
> more sensible.

Good point, I agree with you.

Willy

Re: [patch] staging: panel: pass correct lengths to keypad_send_key()

[Dan Carpenter]

On Fri, Nov 30, 2012 at 10:19:06AM +0100, walter harms wrote:
> >  				if (press_str[0])
> >  					keypad_send_key(press_str,
> > -							sizeof(press_str));
> > +							sizeof(input->u.kbd.press_str));
> >  			}
> yep, this looks like the original intention.
> NTL i would like to question the use of  press_str and friends.
> Moving the if (press_str[0]) into keypad_send_key() seems
> more sensible.
> 
> just my two cents,
> 

Greg had already applied my patch by the you sent this email.

What you're saying sounds like it should be a good cleanup, but when
I actually look at it, the callers still have to test repeat_str[0]
so it's not as useful as one would hope.

Really this stuff is really confusing.  I'm not sure what is
actually stored in press_str.  Is it a pointer or just chars?  If
it's a pointer then why are we only checking the first bit instead
of checking the pointer?  I think it's characters but why is it
declared as "char press_str[sizeof(void *) + sizeof(int)];"?

regards,
dan carpenter

Re: [patch] staging: panel: pass correct lengths to keypad_send_key()

[Willy Tarreau]

Hi Dan,

On Sun, Dec 02, 2012 at 02:22:13PM +0300, Dan Carpenter wrote:
> On Fri, Nov 30, 2012 at 10:19:06AM +0100, walter harms wrote:
> > >  				if (press_str[0])
> > >  					keypad_send_key(press_str,
> > > -							sizeof(press_str));
> > > +							sizeof(input->u.kbd.press_str));
> > >  			}
> > yep, this looks like the original intention.
> > NTL i would like to question the use of  press_str and friends.
> > Moving the if (press_str[0]) into keypad_send_key() seems
> > more sensible.
> > 
> > just my two cents,
> > 
> 
> Greg had already applied my patch by the you sent this email.
> 
> What you're saying sounds like it should be a good cleanup, but when
> I actually look at it, the callers still have to test repeat_str[0]
> so it's not as useful as one would hope.
> 
> Really this stuff is really confusing.  I'm not sure what is
> actually stored in press_str.  Is it a pointer or just chars?  If
> it's a pointer then why are we only checking the first bit instead
> of checking the pointer?  I think it's characters but why is it
> declared as "char press_str[sizeof(void *) + sizeof(int)];"?

It's just chars. It was declared this way so that it did not
inflate the union it's declared in. This is totally ugly and
should be cleaned. But it dates 13 years ago when we were trying
to shrink everything to have full-featured kernels in less than
500kB to boot from floppies...

Regards,
Willy

Re: [patch] staging: panel: pass correct lengths to keypad_send_key()

[walter harms]

Am 02.12.2012 12:53, schrieb Willy Tarreau:
> Hi Dan,
> 
> On Sun, Dec 02, 2012 at 02:22:13PM +0300, Dan Carpenter wrote:
>> On Fri, Nov 30, 2012 at 10:19:06AM +0100, walter harms wrote:
>>>>  				if (press_str[0])
>>>>  					keypad_send_key(press_str,
>>>> -							sizeof(press_str));
>>>> +							sizeof(input->u.kbd.press_str));
>>>>  			}
>>> yep, this looks like the original intention.
>>> NTL i would like to question the use of  press_str and friends.
>>> Moving the if (press_str[0]) into keypad_send_key() seems
>>> more sensible.
>>>
>>> just my two cents,
>>>
>>
>> Greg had already applied my patch by the you sent this email.
>>
>> What you're saying sounds like it should be a good cleanup, but when
>> I actually look at it, the callers still have to test repeat_str[0]
>> so it's not as useful as one would hope.
>>
>> Really this stuff is really confusing.  I'm not sure what is
>> actually stored in press_str.  Is it a pointer or just chars?  If
>> it's a pointer then why are we only checking the first bit instead
>> of checking the pointer?  I think it's characters but why is it
>> declared as "char press_str[sizeof(void *) + sizeof(int)];"?
> 
> It's just chars. It was declared this way so that it did not
> inflate the union it's declared in. This is totally ugly and
> should be cleaned. But it dates 13 years ago when we were trying
> to shrink everything to have full-featured kernels in less than
> 500kB to boot from floppies...
> 

It is not a bad think to save space.
The question is about readability.

re,
 wh