* Re: [PATCH 15/16] drivers/platform/x86: Use available error codes
@ 2010-08-16 17:11 ` Jonathan Woithe
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Woithe @ 2010-08-16 17:23 UTC (permalink / raw)
To: Julia Lawall
Cc: Jonathan Woithe, Matthew Garrett, platform-driver-x86,
linux-kernel, kernel-janitors
I've had a quick look at this and the idea behind it is sound. While it
does change the symantics as noted in the original post, the change is only
in an error path and to my eye the error code should be returned in that
case - as the code currently stands a 0 is returned even if ENOMEM has
occurred.
It seems to me that error and result have been (wrongly) used interchangedly
within these two functions (possibly be different authors). I agree that
one of them should be eliminated, and error seems the sensible choice. So
something like the following is probably in order.
=
An error code is stored in the variable error, but it is the variable result
that is returned instead. So store the error code in result and eliminate
the unnecessary variable error. Initial report and patch from Julia Lawall
<julia@diku.dk>.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r@
local idexpression x;
constant C;
@@
if (...) { ...
x = -C
... when != x
(
return <+...x...+>;
|
return NULL;
|
return;
|
* return ...;
)
}
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Jonathan Woithe <jwoithe@physics.adelaide.edu.au>
--- a/drivers/platform/x86/fujitsu-laptop.c 2010-03-16 02:39:39.000000000 +1030
+++ b/drivers/platform/x86/fujitsu-laptop.c 2010-08-17 02:37:18.556779664 +0930
@@ -655,7 +655,6 @@
int result = 0;
int state = 0;
struct input_dev *input;
- int error;
if (!device)
return -EINVAL;
@@ -667,7 +666,7 @@
fujitsu->input = input = input_allocate_device();
if (!input) {
- error = -ENOMEM;
+ result = -ENOMEM;
goto err_stop;
}
@@ -684,8 +683,8 @@
set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
set_bit(KEY_UNKNOWN, input->keybit);
- error = input_register_device(input);
- if (error)
+ result = input_register_device(input);
+ if (result)
goto err_free_input_dev;
result = acpi_bus_get_power(fujitsu->acpi_handle, &state);
@@ -810,7 +809,6 @@
int result = 0;
int state = 0;
struct input_dev *input;
- int error;
int i;
if (!device)
@@ -824,16 +822,16 @@
/* kfifo */
spin_lock_init(&fujitsu_hotkey->fifo_lock);
- error = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
+ result = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
GFP_KERNEL);
- if (error) {
+ if (result) {
printk(KERN_ERR "kfifo_alloc failed\n");
goto err_stop;
}
fujitsu_hotkey->input = input = input_allocate_device();
if (!input) {
- error = -ENOMEM;
+ result = -ENOMEM;
goto err_free_fifo;
}
@@ -853,8 +851,8 @@
set_bit(fujitsu->keycode4, input->keybit);
set_bit(KEY_UNKNOWN, input->keybit);
- error = input_register_device(input);
- if (error)
+ result = input_register_device(input);
+ if (result)
goto err_free_input_dev;
result = acpi_bus_get_power(fujitsu_hotkey->acpi_handle, &state);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 15/16] drivers/platform/x86: Use available error codes
@ 2010-08-16 17:11 ` Jonathan Woithe
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Woithe @ 2010-08-16 17:11 UTC (permalink / raw)
To: Julia Lawall
Cc: Jonathan Woithe, Matthew Garrett, platform-driver-x86,
linux-kernel, kernel-janitors
I've had a quick look at this and the idea behind it is sound. While it
does change the symantics as noted in the original post, the change is only
in an error path and to my eye the error code should be returned in that
case - as the code currently stands a 0 is returned even if ENOMEM has
occurred.
It seems to me that error and result have been (wrongly) used interchangedly
within these two functions (possibly be different authors). I agree that
one of them should be eliminated, and error seems the sensible choice. So
something like the following is probably in order.
===
An error code is stored in the variable error, but it is the variable result
that is returned instead. So store the error code in result and eliminate
the unnecessary variable error. Initial report and patch from Julia Lawall
<julia@diku.dk>.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r@
local idexpression x;
constant C;
@@
if (...) { ...
x = -C
... when != x
(
return <+...x...+>;
|
return NULL;
|
return;
|
* return ...;
)
}
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Jonathan Woithe <jwoithe@physics.adelaide.edu.au>
--- a/drivers/platform/x86/fujitsu-laptop.c 2010-03-16 02:39:39.000000000 +1030
+++ b/drivers/platform/x86/fujitsu-laptop.c 2010-08-17 02:37:18.556779664 +0930
@@ -655,7 +655,6 @@
int result = 0;
int state = 0;
struct input_dev *input;
- int error;
if (!device)
return -EINVAL;
@@ -667,7 +666,7 @@
fujitsu->input = input = input_allocate_device();
if (!input) {
- error = -ENOMEM;
+ result = -ENOMEM;
goto err_stop;
}
@@ -684,8 +683,8 @@
set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
set_bit(KEY_UNKNOWN, input->keybit);
- error = input_register_device(input);
- if (error)
+ result = input_register_device(input);
+ if (result)
goto err_free_input_dev;
result = acpi_bus_get_power(fujitsu->acpi_handle, &state);
@@ -810,7 +809,6 @@
int result = 0;
int state = 0;
struct input_dev *input;
- int error;
int i;
if (!device)
@@ -824,16 +822,16 @@
/* kfifo */
spin_lock_init(&fujitsu_hotkey->fifo_lock);
- error = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
+ result = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
GFP_KERNEL);
- if (error) {
+ if (result) {
printk(KERN_ERR "kfifo_alloc failed\n");
goto err_stop;
}
fujitsu_hotkey->input = input = input_allocate_device();
if (!input) {
- error = -ENOMEM;
+ result = -ENOMEM;
goto err_free_fifo;
}
@@ -853,8 +851,8 @@
set_bit(fujitsu->keycode4, input->keybit);
set_bit(KEY_UNKNOWN, input->keybit);
- error = input_register_device(input);
- if (error)
+ result = input_register_device(input);
+ if (result)
goto err_free_input_dev;
result = acpi_bus_get_power(fujitsu_hotkey->acpi_handle, &state);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 15/16] drivers/platform/x86: Use available error codes
2010-08-16 17:11 ` Jonathan Woithe
@ 2010-08-16 20:43 ` Julia Lawall
-1 siblings, 0 replies; 9+ messages in thread
From: Julia Lawall @ 2010-08-16 20:43 UTC (permalink / raw)
To: Jonathan Woithe
Cc: Matthew Garrett, platform-driver-x86, linux-kernel,
kernel-janitors
Looks ok to me. Thanks.
julia
On Tue, 17 Aug 2010, Jonathan Woithe wrote:
> I've had a quick look at this and the idea behind it is sound. While it
> does change the symantics as noted in the original post, the change is only
> in an error path and to my eye the error code should be returned in that
> case - as the code currently stands a 0 is returned even if ENOMEM has
> occurred.
>
> It seems to me that error and result have been (wrongly) used interchangedly
> within these two functions (possibly be different authors). I agree that
> one of them should be eliminated, and error seems the sensible choice. So
> something like the following is probably in order.
>
> =>
> An error code is stored in the variable error, but it is the variable result
> that is returned instead. So store the error code in result and eliminate
> the unnecessary variable error. Initial report and patch from Julia Lawall
> <julia@diku.dk>.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> local idexpression x;
> constant C;
> @@
>
> if (...) { ...
> x = -C
> ... when != x
> (
> return <+...x...+>;
> |
> return NULL;
> |
> return;
> |
> * return ...;
> )
> }
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
> Signed-off-by: Jonathan Woithe <jwoithe@physics.adelaide.edu.au>
>
> --- a/drivers/platform/x86/fujitsu-laptop.c 2010-03-16 02:39:39.000000000 +1030
> +++ b/drivers/platform/x86/fujitsu-laptop.c 2010-08-17 02:37:18.556779664 +0930
> @@ -655,7 +655,6 @@
> int result = 0;
> int state = 0;
> struct input_dev *input;
> - int error;
>
> if (!device)
> return -EINVAL;
> @@ -667,7 +666,7 @@
>
> fujitsu->input = input = input_allocate_device();
> if (!input) {
> - error = -ENOMEM;
> + result = -ENOMEM;
> goto err_stop;
> }
>
> @@ -684,8 +683,8 @@
> set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
> set_bit(KEY_UNKNOWN, input->keybit);
>
> - error = input_register_device(input);
> - if (error)
> + result = input_register_device(input);
> + if (result)
> goto err_free_input_dev;
>
> result = acpi_bus_get_power(fujitsu->acpi_handle, &state);
> @@ -810,7 +809,6 @@
> int result = 0;
> int state = 0;
> struct input_dev *input;
> - int error;
> int i;
>
> if (!device)
> @@ -824,16 +822,16 @@
>
> /* kfifo */
> spin_lock_init(&fujitsu_hotkey->fifo_lock);
> - error = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
> + result = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
> GFP_KERNEL);
> - if (error) {
> + if (result) {
> printk(KERN_ERR "kfifo_alloc failed\n");
> goto err_stop;
> }
>
> fujitsu_hotkey->input = input = input_allocate_device();
> if (!input) {
> - error = -ENOMEM;
> + result = -ENOMEM;
> goto err_free_fifo;
> }
>
> @@ -853,8 +851,8 @@
> set_bit(fujitsu->keycode4, input->keybit);
> set_bit(KEY_UNKNOWN, input->keybit);
>
> - error = input_register_device(input);
> - if (error)
> + result = input_register_device(input);
> + if (result)
> goto err_free_input_dev;
>
> result = acpi_bus_get_power(fujitsu_hotkey->acpi_handle, &state);
> --
> 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 15/16] drivers/platform/x86: Use available error codes
@ 2010-08-16 20:43 ` Julia Lawall
0 siblings, 0 replies; 9+ messages in thread
From: Julia Lawall @ 2010-08-16 20:43 UTC (permalink / raw)
To: Jonathan Woithe
Cc: Matthew Garrett, platform-driver-x86, linux-kernel,
kernel-janitors
Looks ok to me. Thanks.
julia
On Tue, 17 Aug 2010, Jonathan Woithe wrote:
> I've had a quick look at this and the idea behind it is sound. While it
> does change the symantics as noted in the original post, the change is only
> in an error path and to my eye the error code should be returned in that
> case - as the code currently stands a 0 is returned even if ENOMEM has
> occurred.
>
> It seems to me that error and result have been (wrongly) used interchangedly
> within these two functions (possibly be different authors). I agree that
> one of them should be eliminated, and error seems the sensible choice. So
> something like the following is probably in order.
>
> ===
>
> An error code is stored in the variable error, but it is the variable result
> that is returned instead. So store the error code in result and eliminate
> the unnecessary variable error. Initial report and patch from Julia Lawall
> <julia@diku.dk>.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> local idexpression x;
> constant C;
> @@
>
> if (...) { ...
> x = -C
> ... when != x
> (
> return <+...x...+>;
> |
> return NULL;
> |
> return;
> |
> * return ...;
> )
> }
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
> Signed-off-by: Jonathan Woithe <jwoithe@physics.adelaide.edu.au>
>
> --- a/drivers/platform/x86/fujitsu-laptop.c 2010-03-16 02:39:39.000000000 +1030
> +++ b/drivers/platform/x86/fujitsu-laptop.c 2010-08-17 02:37:18.556779664 +0930
> @@ -655,7 +655,6 @@
> int result = 0;
> int state = 0;
> struct input_dev *input;
> - int error;
>
> if (!device)
> return -EINVAL;
> @@ -667,7 +666,7 @@
>
> fujitsu->input = input = input_allocate_device();
> if (!input) {
> - error = -ENOMEM;
> + result = -ENOMEM;
> goto err_stop;
> }
>
> @@ -684,8 +683,8 @@
> set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
> set_bit(KEY_UNKNOWN, input->keybit);
>
> - error = input_register_device(input);
> - if (error)
> + result = input_register_device(input);
> + if (result)
> goto err_free_input_dev;
>
> result = acpi_bus_get_power(fujitsu->acpi_handle, &state);
> @@ -810,7 +809,6 @@
> int result = 0;
> int state = 0;
> struct input_dev *input;
> - int error;
> int i;
>
> if (!device)
> @@ -824,16 +822,16 @@
>
> /* kfifo */
> spin_lock_init(&fujitsu_hotkey->fifo_lock);
> - error = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
> + result = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
> GFP_KERNEL);
> - if (error) {
> + if (result) {
> printk(KERN_ERR "kfifo_alloc failed\n");
> goto err_stop;
> }
>
> fujitsu_hotkey->input = input = input_allocate_device();
> if (!input) {
> - error = -ENOMEM;
> + result = -ENOMEM;
> goto err_free_fifo;
> }
>
> @@ -853,8 +851,8 @@
> set_bit(fujitsu->keycode4, input->keybit);
> set_bit(KEY_UNKNOWN, input->keybit);
>
> - error = input_register_device(input);
> - if (error)
> + result = input_register_device(input);
> + if (result)
> goto err_free_input_dev;
>
> result = acpi_bus_get_power(fujitsu_hotkey->acpi_handle, &state);
> --
> 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread