Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753290AbZAFCCW (ORCPT ); Mon, 5 Jan 2009 21:02:22 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751544AbZAFCCM (ORCPT ); Mon, 5 Jan 2009 21:02:12 -0500 Received: from hrndva-omtalb.mail.rr.com ([71.74.56.125]:43341 "EHLO hrndva-omtalb.mail.rr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751496AbZAFCCM (ORCPT ); Mon, 5 Jan 2009 21:02:12 -0500 Date: Mon, 5 Jan 2009 21:02:07 -0500 (EST) From: Steven Rostedt X-X-Sender: rostedt@gandalf.stny.rr.com To: Rusty Russell cc: Sam Ravnborg , LKML , Steven Rostedt , Ingo Molnar , "David S. Miller" , sparclinux , Andrew Morton Subject: Re: [PATCH] module: clean up initialization of variable In-Reply-To: <200901061152.51782.rusty@rustcorp.com.au> Message-ID: References: <20090105181922.GA25622@uranus.ravnborg.org> <20090105195415.GA6204@uranus.ravnborg.org> <200901061152.51782.rusty@rustcorp.com.au> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1883 Lines: 52 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/