* [PATCH] return same error value from spk_set_key_info
@ 2017-02-21 11:54 Pranay Kr. Srivastava
2017-02-21 12:24 ` Samuel Thibault
0 siblings, 1 reply; 6+ messages in thread
From: Pranay Kr. Srivastava @ 2017-02-21 11:54 UTC (permalink / raw)
To: w.d.hubbs, chris, kirk, samuel.thibault, gregkh, sfr, speakup,
devel, linux-kernel
Cc: Pranay Kr. Srivastava
This patch makes spk_set_key_info return -EINVAL
in case of failure instead of returning 4 different
values for the type of error that occurred.
Retain the previous error values as debug messages
instead.
Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
---
drivers/staging/speakup/main.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index c2f70ef..396a57a 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1216,13 +1216,17 @@ int spk_set_key_info(const u_char *key_info, u_char *k_buffer)
u_char ch, version, num_keys;
version = *cp++;
- if (version != KEY_MAP_VER)
- return -1;
+ if (version != KEY_MAP_VER) {
+ pr_debug("Version mismatch %d\n", -1);
+ return -EINVAL;
+ }
num_keys = *cp;
states = (int)cp[1];
key_data_len = (states + 1) * (num_keys + 1);
- if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf))
- return -2;
+ if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf)) {
+ pr_debug("Data len and Shift table too big %d\n", -2);
+ return -EINVAL;
+ }
memset(k_buffer, 0, SHIFT_TBL_SIZE);
memset(spk_our_keys, 0, sizeof(spk_our_keys));
spk_shift_table = k_buffer;
@@ -1233,14 +1237,18 @@ int spk_set_key_info(const u_char *key_info, u_char *k_buffer)
cp1 += 2; /* now pointing at shift states */
for (i = 1; i <= states; i++) {
ch = *cp1++;
- if (ch >= SHIFT_TBL_SIZE)
- return -3;
+ if (ch >= SHIFT_TBL_SIZE) {
+ pr_debug("Key table size overflow %d\n", -3);
+ return -EINVAL;
+ }
spk_shift_table[ch] = i;
}
keymap_flags = *cp1++;
while ((ch = *cp1)) {
- if (ch >= MAX_KEY)
- return -4;
+ if (ch >= MAX_KEY) {
+ pr_debug("Max Key overflow %d\n", -4);
+ return -EINVAL;
+ }
spk_our_keys[ch] = cp1;
cp1 += states + 1;
}
--
2.10.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] return same error value from spk_set_key_info
2017-02-21 11:54 [PATCH] return same error value from spk_set_key_info Pranay Kr. Srivastava
@ 2017-02-21 12:24 ` Samuel Thibault
2017-02-21 13:44 ` [PATCH Speakup v2] " Pranay Kr. Srivastava
0 siblings, 1 reply; 6+ messages in thread
From: Samuel Thibault @ 2017-02-21 12:24 UTC (permalink / raw)
To: Pranay Kr. Srivastava
Cc: w.d.hubbs, chris, kirk, gregkh, sfr, speakup, devel, linux-kernel
Pranay Kr. Srivastava, on mar. 21 févr. 2017 17:24:42 +0530, wrote:
> Retain the previous error values as debug messages
> instead.
Well, they are not really useful because they are actually undocumented.
> + if (version != KEY_MAP_VER) {
> + pr_debug("Version mismatch %d\n", -1);
Rather print version and KEY_MAP_VER
> + if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf)) {
> + pr_debug("Data len and Shift table too big %d\n", -2);
Rather print key_data_len + SHIFT_TBL_SIZE + 4 and sizeof(spk_key_buf)
> + if (ch >= SHIFT_TBL_SIZE) {
> + pr_debug("Key table size overflow %d\n", -3);
Rather print ch and SHIFT_TBL_SIZE.
> + if (ch >= MAX_KEY) {
> + pr_debug("Max Key overflow %d\n", -4);
Rather print ch and MAX_KEY.
Apart from that, returning -EINVAL instead of different values is fine
for me.
Samuel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH Speakup v2] return same error value from spk_set_key_info
2017-02-21 12:24 ` Samuel Thibault
@ 2017-02-21 13:44 ` Pranay Kr. Srivastava
2017-02-21 13:54 ` Samuel Thibault
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Pranay Kr. Srivastava @ 2017-02-21 13:44 UTC (permalink / raw)
To: w.d.hubbs, chris, kirk, samuel.thibault, gregkh, sfr, speakup,
devel, linux-kernel
Cc: Pranay Kr. Srivastava
This patch makes spk_set_key_info return -EINVAL
in case of failure instead of returning 4 different
values for the type of error that occurred.
Print the offending values instead as debug message.
Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
---
drivers/staging/speakup/main.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index c2f70ef..ad95905 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1216,13 +1216,20 @@ int spk_set_key_info(const u_char *key_info, u_char *k_buffer)
u_char ch, version, num_keys;
version = *cp++;
- if (version != KEY_MAP_VER)
- return -1;
+ if (version != KEY_MAP_VER) {
+ pr_debug("version found %d should be %d\n",
+ version, KEY_MAP_VER);
+ return -EINVAL;
+ }
num_keys = *cp;
states = (int)cp[1];
key_data_len = (states + 1) * (num_keys + 1);
- if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf))
- return -2;
+ if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf)) {
+ pr_debug("key_data_len = %d, SHIFT_TBL_SIZE + 4 = %d,\t"
+ "sizeof(spk_key_buf) = %lu\n", key_data_len,
+ SHIFT_TBL_SIZE + 4, sizeof(spk_key_buf));
+ return -EINVAL;
+ }
memset(k_buffer, 0, SHIFT_TBL_SIZE);
memset(spk_our_keys, 0, sizeof(spk_our_keys));
spk_shift_table = k_buffer;
@@ -1233,14 +1240,19 @@ int spk_set_key_info(const u_char *key_info, u_char *k_buffer)
cp1 += 2; /* now pointing at shift states */
for (i = 1; i <= states; i++) {
ch = *cp1++;
- if (ch >= SHIFT_TBL_SIZE)
- return -3;
+ if (ch >= SHIFT_TBL_SIZE) {
+ pr_debug("ch = %d, SHIFT_TBL_SIZE = %d\n", ch,
+ SHIFT_TBL_SIZE);
+ return -EINVAL;
+ }
spk_shift_table[ch] = i;
}
keymap_flags = *cp1++;
while ((ch = *cp1)) {
- if (ch >= MAX_KEY)
- return -4;
+ if (ch >= MAX_KEY) {
+ pr_debug("ch = %d, MAX_KEY = %d\n", ch, MAX_KEY);
+ return -EINVAL;
+ }
spk_our_keys[ch] = cp1;
cp1 += states + 1;
}
--
2.10.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH Speakup v2] return same error value from spk_set_key_info
2017-02-21 13:44 ` [PATCH Speakup v2] " Pranay Kr. Srivastava
@ 2017-02-21 13:54 ` Samuel Thibault
2017-02-21 15:21 ` kbuild test robot
2017-02-21 15:36 ` kbuild test robot
2 siblings, 0 replies; 6+ messages in thread
From: Samuel Thibault @ 2017-02-21 13:54 UTC (permalink / raw)
To: Pranay Kr. Srivastava
Cc: w.d.hubbs, chris, kirk, gregkh, sfr, speakup, devel, linux-kernel
Pranay Kr. Srivastava, on mar. 21 févr. 2017 19:14:38 +0530, wrote:
> + if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf)) {
> + pr_debug("key_data_len = %d, SHIFT_TBL_SIZE + 4 = %d,\t"
> + "sizeof(spk_key_buf) = %lu\n", key_data_len,
> + SHIFT_TBL_SIZE + 4, sizeof(spk_key_buf));
Well, that's very technical, and not useful to an actual user :)
Rather print
"too many key infos (%d bytes over %d)",
key_data_len + SHIFT_TBL_SIZE + 4,
sizeof(spk_key_buf)
> + if (ch >= SHIFT_TBL_SIZE) {
> + pr_debug("ch = %d, SHIFT_TBL_SIZE = %d\n", ch,
> + SHIFT_TBL_SIZE);
similarly, rather
"ch %d is not a valid shift state (maximum %d)", ch, SHIFT_TBL_SIZE
> + if (ch >= MAX_KEY) {
> + pr_debug("ch = %d, MAX_KEY = %d\n", ch, MAX_KEY);
and
"ch %d is not a valid keycode (maximum %d)", ch, MAX_KEY
Samuel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH Speakup v2] return same error value from spk_set_key_info
2017-02-21 13:44 ` [PATCH Speakup v2] " Pranay Kr. Srivastava
2017-02-21 13:54 ` Samuel Thibault
@ 2017-02-21 15:21 ` kbuild test robot
2017-02-21 15:36 ` kbuild test robot
2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2017-02-21 15:21 UTC (permalink / raw)
To: Pranay Kr. Srivastava
Cc: kbuild-all, w.d.hubbs, chris, kirk, samuel.thibault, gregkh, sfr,
speakup, devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2751 bytes --]
Hi Pranay,
[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.10 next-20170220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Pranay-Kr-Srivastava/return-same-error-value-from-spk_set_key_info/20170221-224753
config: i386-randconfig-x018-201708 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:13:0,
from drivers/staging/speakup/main.c:21:
drivers/staging/speakup/main.c: In function 'spk_set_key_info':
>> include/linux/kern_levels.h:4:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/printk.h:136:11: note: in definition of macro 'no_printk'
printk(fmt, ##__VA_ARGS__); \
^~~
include/linux/kern_levels.h:14:20: note: in expansion of macro 'KERN_SOH'
#define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
^~~~~~~~
include/linux/printk.h:330:12: note: in expansion of macro 'KERN_DEBUG'
no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
^~~~~~~~~~
>> drivers/staging/speakup/main.c:1228:3: note: in expansion of macro 'pr_debug'
pr_debug("key_data_len = %d, SHIFT_TBL_SIZE + 4 = %d,\t"
^~~~~~~~
vim +/pr_debug +1228 drivers/staging/speakup/main.c
1212 {
1213 int i = 0, states, key_data_len;
1214 const u_char *cp = key_info;
1215 u_char *cp1 = k_buffer;
1216 u_char ch, version, num_keys;
1217
1218 version = *cp++;
1219 if (version != KEY_MAP_VER) {
1220 pr_debug("version found %d should be %d\n",
1221 version, KEY_MAP_VER);
1222 return -EINVAL;
1223 }
1224 num_keys = *cp;
1225 states = (int)cp[1];
1226 key_data_len = (states + 1) * (num_keys + 1);
1227 if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf)) {
> 1228 pr_debug("key_data_len = %d, SHIFT_TBL_SIZE + 4 = %d,\t"
1229 "sizeof(spk_key_buf) = %lu\n", key_data_len,
1230 SHIFT_TBL_SIZE + 4, sizeof(spk_key_buf));
1231 return -EINVAL;
1232 }
1233 memset(k_buffer, 0, SHIFT_TBL_SIZE);
1234 memset(spk_our_keys, 0, sizeof(spk_our_keys));
1235 spk_shift_table = k_buffer;
1236 spk_our_keys[0] = spk_shift_table;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27655 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH Speakup v2] return same error value from spk_set_key_info
2017-02-21 13:44 ` [PATCH Speakup v2] " Pranay Kr. Srivastava
2017-02-21 13:54 ` Samuel Thibault
2017-02-21 15:21 ` kbuild test robot
@ 2017-02-21 15:36 ` kbuild test robot
2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2017-02-21 15:36 UTC (permalink / raw)
To: Pranay Kr. Srivastava
Cc: kbuild-all, w.d.hubbs, chris, kirk, samuel.thibault, gregkh, sfr,
speakup, devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]
Hi Pranay,
[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.10 next-20170220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Pranay-Kr-Srivastava/return-same-error-value-from-spk_set_key_info/20170221-224753
config: i386-randconfig-i1-201708 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/staging/speakup/main.c: In function 'spk_set_key_info':
>> drivers/staging/speakup/main.c:1228:3: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
pr_debug("key_data_len = %d, SHIFT_TBL_SIZE + 4 = %d,\t"
^
vim +1228 drivers/staging/speakup/main.c
1212 {
1213 int i = 0, states, key_data_len;
1214 const u_char *cp = key_info;
1215 u_char *cp1 = k_buffer;
1216 u_char ch, version, num_keys;
1217
1218 version = *cp++;
1219 if (version != KEY_MAP_VER) {
1220 pr_debug("version found %d should be %d\n",
1221 version, KEY_MAP_VER);
1222 return -EINVAL;
1223 }
1224 num_keys = *cp;
1225 states = (int)cp[1];
1226 key_data_len = (states + 1) * (num_keys + 1);
1227 if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf)) {
> 1228 pr_debug("key_data_len = %d, SHIFT_TBL_SIZE + 4 = %d,\t"
1229 "sizeof(spk_key_buf) = %lu\n", key_data_len,
1230 SHIFT_TBL_SIZE + 4, sizeof(spk_key_buf));
1231 return -EINVAL;
1232 }
1233 memset(k_buffer, 0, SHIFT_TBL_SIZE);
1234 memset(spk_our_keys, 0, sizeof(spk_our_keys));
1235 spk_shift_table = k_buffer;
1236 spk_our_keys[0] = spk_shift_table;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28588 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-21 15:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-21 11:54 [PATCH] return same error value from spk_set_key_info Pranay Kr. Srivastava
2017-02-21 12:24 ` Samuel Thibault
2017-02-21 13:44 ` [PATCH Speakup v2] " Pranay Kr. Srivastava
2017-02-21 13:54 ` Samuel Thibault
2017-02-21 15:21 ` kbuild test robot
2017-02-21 15:36 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.