diff mbox

module: clean up initialization of variable

Message ID alpine.DEB.1.10.0901051523370.25066@gandalf.stny.rr.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Steven Rostedt Jan. 5, 2009, 8:30 p.m. UTC 
Impact: clean up

On Mon, 5 Jan 2009, Sam Ravnborg wrote:
> 
> Fully ageed on the readability.
> I happen to trigger this as an error in the sparc code.
> But I see the same warning also in generic code.
> 
> >From kernel/module.c:
>         /* Suck in entire file: we'll want most of it. */
>         /* vmalloc barfs on "unusual" numbers.  Check here */
>         if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
>                 return ERR_PTR(-ENOMEM);
> 
> 
> This gives following warning:
> kernel/module.c: In function `load_module':
> kernel/module.c:1842: warning: 'hdr' might be used uninitialized in this function

The above is caused by having the branch tracer on and the following type 
of initialization:

	if (x || !(y = init_me())
		return;

	use_me(y);

This is sloppy initialization because it initializes, not only in an
if condition, but also as the second part of a complex conditional.

This patch makes the code a bit easier to read.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
Reported-by: Sam Ravnborg <sam@ravnborg.org>

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Harvey Harrison Jan. 5, 2009, 10:59 p.m. UTC | #1 
On Mon, 2009-01-05 at 15:30 -0500, Steven Rostedt wrote:
> +	hdr = vmalloc(len);
> +	if (hdr == NULL)

A bit pedantic but,

if (!hdr) perhaps?

Cheers,

Harvey

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rusty Russell Jan. 6, 2009, 1:22 a.m. UTC | #2 
On Tuesday 06 January 2009 07:00:25 Steven Rostedt wrote:
> This is sloppy initialization because it initializes, not only in an
> if condition, but also as the second part of a complex conditional.
> 
> This patch makes the code a bit easier to read.
...
>  	/* Suck in entire file: we'll want most of it. */
>  	/* vmalloc barfs on "unusual" numbers.  Check here */
> -	if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
> +	if (len > 64 * 1024 * 1024)
> +		return ERR_PTR(-ENOMEM);
> +	hdr = vmalloc(len);
> +	if (hdr == NULL)
>  		return ERR_PTR(-ENOMEM);
>  	if (copy_from_user(hdr, umod, len) != 0) {
>  		err = -EFAULT;

This line is not accidental nor casually written: the two statements are deliberately entwined.  It is a succint complaint against the vagaries of vmalloc.

So this patch is a messup, not a cleanup.

But it's really upset me because it is lazy and timid: and too much kernel code is becoming mired in such scars.  Instead of "how do I kill this warning and get it in the merge window" you should be thinking "how do I make the kernel better", and "I wonder if vmalloc still has this problem"...

And I so look forward to the warm fuzzies I get when applying a real cleanup patch.

Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Jan. 6, 2009, 2:02 a.m. UTC | #3 
On Tue, 6 Jan 2009, Rusty Russell wrote:

> On Tuesday 06 January 2009 07:00:25 Steven Rostedt wrote:
> > This is sloppy initialization because it initializes, not only in an
> > if condition, but also as the second part of a complex conditional.
> > 
> > This patch makes the code a bit easier to read.
> ...
> >  	/* Suck in entire file: we'll want most of it. */
> >  	/* vmalloc barfs on "unusual" numbers.  Check here */
> > -	if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
> > +	if (len > 64 * 1024 * 1024)
> > +		return ERR_PTR(-ENOMEM);
> > +	hdr = vmalloc(len);
> > +	if (hdr == NULL)
> >  		return ERR_PTR(-ENOMEM);
> >  	if (copy_from_user(hdr, umod, len) != 0) {
> >  		err = -EFAULT;
> 
> This line is not accidental nor casually written: the two statements 
> are deliberately entwined.  It is a succint complaint against the 
> vagaries of vmalloc.
> 
> So this patch is a messup, not a cleanup.

It is not that much of a messup. I did not realize that the code was
a political protest against the horrors of vmalloc ;-)

> 
> But it's really upset me because it is lazy and timid: and too much 
> kernel code is becoming mired in such scars.  Instead of "how do I kill 
> this warning and get it in the merge window" you should be thinking "how 
> do I make the kernel better", and "I wonder if vmalloc still has this 
> problem"...
> 
> And I so look forward to the warm fuzzies I get when applying a real 
> cleanup patch.

Well, I'm not about to go solve the vmalloc issues (not today anyway). But 
I'll go and see if I can get the branch tracer macro to work on all 
versions of gcc.

Thanks,

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/kernel/module.c b/kernel/module.c
index 9712c52..112d6cd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1864,7 +1864,10 @@  static noinline struct module *load_module(void __user *umod,
 
 	/* Suck in entire file: we'll want most of it. */
 	/* vmalloc barfs on "unusual" numbers.  Check here */
-	if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
+	if (len > 64 * 1024 * 1024)
+		return ERR_PTR(-ENOMEM);
+	hdr = vmalloc(len);
+	if (hdr == NULL)
 		return ERR_PTR(-ENOMEM);
 	if (copy_from_user(hdr, umod, len) != 0) {
 		err = -EFAULT;