staging: panel: pass correct lengths to keypad_send_key()

PATCH In review Dan Carpenter 3 participants 7 messages Nov 29, 2012 – Dec 2, 2012

Subsystems (2)

Staging Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2012-11-29 14:38 UTC #0 lore raw

PATCH 1/1 staging: panel: pass correct lengths to keypad_send_key()

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 <[email protected]>

---
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.

drivers/staging/panel/panel.c +4 -4

index d9fec5b..dca42ce 100644
@@ -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;

walter harms

2012-11-30 09:19 UTC #1 lore raw

Am 29.11.2012 15:38, schrieb Dan Carpenter:

Quoted 19 lines

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 <[email protected]>
---
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.
drivers/staging/panel/panel.c +0 -0
index d9fec5b..dca42ce 100644

drivers/staging/panel/panel.c static inline int input_state_high(struct logical_input *input) · line 1762

Quoted 6 lines

drivers/staging/panel/panel.c +1 -1

@@ -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

Quoted 35 lines

drivers/staging/panel/panel.c +3 -4

 			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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Willy Tarreau

Acked-by on v1 1/1

2012-12-01 08:08 UTC #2 lore raw

In reply to Dan Carpenter

Hi Dan,

Quoted 7 lines · 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.

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...

Quoted 3 lines

Signed-off-by Dan Carpenter <[email protected]>
---
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.

Quoted 3 lines

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 <[email protected]>

Willy

Willy Tarreau

2012-12-01 08:13 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 17 lines · walter harms

Am 29.11.2012 15:38, schrieb Dan Carpenter:
Quoted 14 lines
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 <[email protected]>
---
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.

Quoted 15 lines

drivers/staging/panel/panel.c static inline int input_state_high(struct logical_input *input) · line 1762
Quoted 11 lines
drivers/staging/panel/panel.c +1 -1
index d9fec5b..dca42ce 100644
@@ -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

Dan Carpenter

author

2012-12-02 11:22 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 10 lines · walter harms

drivers/staging/panel/panel.c static inline int input_state_high(struct logical_input *input) · line 1762
Quoted 5 lines
drivers/staging/panel/panel.c +1 -1
 				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.

Quoted 2 lines

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

Willy Tarreau

2012-12-02 11:53 UTC #5 lore raw

In reply to Dan Carpenter

Hi Dan,

Quoted 20 lines · Dan Carpenter

Quoted 10 lines · walter harms
drivers/staging/panel/panel.c static inline int input_state_high(struct logical_input *input) · line 1762
Quoted 5 lines
drivers/staging/panel/panel.c +1 -1
 				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.
Quoted 2 lines
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.

Quoted 5 lines

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

walter harms

2012-12-02 15:44 UTC #6 lore raw

In reply to Dan Carpenter

Am 02.12.2012 12:53, schrieb Willy Tarreau:

Quoted 29 lines

Hi Dan,
Quoted 20 lines · Dan Carpenter
Quoted 10 lines · walter harms
drivers/staging/panel/panel.c static inline int input_state_high(struct logical_input *input) · line 1762
Quoted 5 lines
drivers/staging/panel/panel.c +1 -1
 				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.
Quoted 2 lines
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.
Quoted 5 lines
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)];"?

Quoted 6 lines

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