Re: [PATCH] add missing checks of __copy_to_user return value ini2o_config.c

[Jesper Juhl]

On Thu, 7 Oct 2004, Markus Lidel wrote:

> Hello,
> thanks for fixing i2o_config.

You're welcome.


> In the meantime i've also made a patch, which
> tries to reduce the many return's too.
> Note: i've not touched the passthru stuff yet.
> Please let me know, which you think of it.

> --- a/drivers/message/i2o/i2o_config.c	2004-09-24 11:05:12.044972000 +0200
> +++ b/drivers/message/i2o/i2o_config.c	2004-10-07 22:49:59.786543596 +0200
> @@ -178,18 +178,17 @@
>  	struct i2o_controller *c;
>  	u8 __user *user_iop_table = (void __user *)arg;
>  	u8 tmp[MAX_I2O_CONTROLLERS];
> +	int rc = 0;
 
Purely personal preference, I think "ret" is a more intuitive name for 
that variable.


+	if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct))) {
+		rc = -EFAULT;
+		goto exit;
+	}
 
[...]

+exit:
+	return rc;
 };
 

I prefer the 

if (foo)
	goto return_fault;

[...]

return_ret:
	return ret;
return_fault:
	ret = -EFAULT;
	goto return_ret;

variant. 
Sure, your way results in just a single mov and a single jmp when 
the branch is taken, but, my way has only a single jmp at each branch, 
then a mov and an additional jmp at the end, so for a single branch your 
way wins with   
1 mov + 1 jmp vs my 2 jmp's and 1 mov.
but, when you have a lot those branches my way generates less code. At 5 
branches you have 5 mov + 5 jmp while I'll have 1 mov + 6 jmp.
Also, my way takes up less space in the source, so more code fits on 
screen.

I doubt it matters much since this doesn't look like a very hot code path, 
but I prefer the version that generates the smaller code and takes up less 
space in the source.

Apart from those nitpicks it looks good to me :)


--
Jesper Juhl

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/