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@xxxxxxx>. > > 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@xxxxxxx> > Signed-off-by: Jonathan Woithe <jwoithe@xxxxxxxxxxxxxxxxxxxxxxx> > > --- 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
