From: Namhyung Kim <namhyung@gmail.com>
To: Theodore Tso <tytso@MIT.EDU>
Cc: Jan Kara <jack@suse.cz>,
Andrew Morton <akpm@linux-foundation.org>,
Andreas Dilger <adilger.kernel@dilger.ca>,
linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ext3: Coding style fix in namei.c
Date: Fri, 19 Nov 2010 23:45:28 +0900 [thread overview]
Message-ID: <1290177928.1678.51.camel@leonhard> (raw)
In-Reply-To: <60024FF1-3903-49AD-9A68-B04C6D678A8E@mit.edu>
2010-11-19 (금), 07:57 -0500, Theodore Tso:
> On Nov 19, 2010, at 5:13 AM, Namhyung Kim wrote:
>
> > * break long lines (using temp variables if needed)
> > * merge short lines
> > * put open brace on the same line
> > * use C89-style comments
> > * remove a space between function name and parenthesis
> > * remove a space between '*' and pointer name
> > * add a space after ','
> > * other random whitespace fixes
> >
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
>
> What's the benefit of such massive cleanup patches, really? Does it
> really enhance readability _that_ much?
>
> I believe in cleaning up code as I make substantive, useful change, but
> code churn for code churn's sake has a number of downsides:
>
> *) It breaks other people's patches that might be pending (probably not
> as much of an issue for ext3)
> *) It makes it really easy to introduce
> security holes in code (although it looks like --- I haven't checked
> to make sure --- this shouldn't change the compiled code any so we can
> at least audit this by applying the patch, and then checking to make
> sure the .o hasn't changed. What really makes my skin crawl is a
> massive change like that which doesn't have zero impact on the
> compiled object code. If I get a patch like that, I reject it out of
> hand for ext4.)
>
> Bottom line is I really don't think cleanup code helps a lot. It
> wastes your time --- why not find some way of improving the kernel
> that has more impact --- and it wastes the time of the responsible
> maintainer (who has to go through the code with a fined-toothed comb
> to make sure there's nothing bad hidden in a massive patch like this).
>
> Best regards,
>
> -- Ted
>
Hi Ted,
I wrote this patch because checkpatch complains about the code when I
tried to write another. Since I saw many codes in namei.c doesn't
conform the kernel coding style so I decided to write this coding style
patch first and others on top of it. But if you think it is totally
useless, I'm fine with dropping it.
BTW, I just checked that compiled code itself has no change on x86_64,
but there was a change in .rodata section.
Thanks.
--
Regards,
Namhyung Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Namhyung Kim <namhyung@gmail.com>
To: Theodore Tso <tytso@MIT.EDU>
Cc: Jan Kara <jack@suse.cz>,
Andrew Morton <akpm@linux-foundation.org>,
Andreas Dilger <adilger.kernel@dilger.ca>,
linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ext3: Coding style fix in namei.c
Date: Fri, 19 Nov 2010 23:45:28 +0900 [thread overview]
Message-ID: <1290177928.1678.51.camel@leonhard> (raw)
In-Reply-To: <60024FF1-3903-49AD-9A68-B04C6D678A8E@mit.edu>
2010-11-19 (금), 07:57 -0500, Theodore Tso:
> On Nov 19, 2010, at 5:13 AM, Namhyung Kim wrote:
>
> > * break long lines (using temp variables if needed)
> > * merge short lines
> > * put open brace on the same line
> > * use C89-style comments
> > * remove a space between function name and parenthesis
> > * remove a space between '*' and pointer name
> > * add a space after ','
> > * other random whitespace fixes
> >
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
>
> What's the benefit of such massive cleanup patches, really? Does it
> really enhance readability _that_ much?
>
> I believe in cleaning up code as I make substantive, useful change, but
> code churn for code churn's sake has a number of downsides:
>
> *) It breaks other people's patches that might be pending (probably not
> as much of an issue for ext3)
> *) It makes it really easy to introduce
> security holes in code (although it looks like --- I haven't checked
> to make sure --- this shouldn't change the compiled code any so we can
> at least audit this by applying the patch, and then checking to make
> sure the .o hasn't changed. What really makes my skin crawl is a
> massive change like that which doesn't have zero impact on the
> compiled object code. If I get a patch like that, I reject it out of
> hand for ext4.)
>
> Bottom line is I really don't think cleanup code helps a lot. It
> wastes your time --- why not find some way of improving the kernel
> that has more impact --- and it wastes the time of the responsible
> maintainer (who has to go through the code with a fined-toothed comb
> to make sure there's nothing bad hidden in a massive patch like this).
>
> Best regards,
>
> -- Ted
>
Hi Ted,
I wrote this patch because checkpatch complains about the code when I
tried to write another. Since I saw many codes in namei.c doesn't
conform the kernel coding style so I decided to write this coding style
patch first and others on top of it. But if you think it is totally
useless, I'm fine with dropping it.
BTW, I just checked that compiled code itself has no change on x86_64,
but there was a change in .rodata section.
Thanks.
--
Regards,
Namhyung Kim
next prev parent reply other threads:[~2010-11-19 14:45 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-19 10:13 [PATCH] ext3: Coding style fix in namei.c Namhyung Kim
2010-11-19 12:57 ` Theodore Tso
2010-11-19 13:05 ` Theodore Tso
2010-11-19 14:47 ` Namhyung Kim
2010-11-19 14:47 ` Namhyung Kim
2010-11-22 17:34 ` Harvey Harrison
2010-11-19 14:45 ` Namhyung Kim [this message]
2010-11-19 14:45 ` Namhyung Kim
2010-11-22 17:03 ` Jan Kara
2011-01-02 9:11 ` Pavel Machek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1290177928.1678.51.camel@leonhard \
--to=namhyung@gmail.com \
--cc=adilger.kernel@dilger.ca \
--cc=akpm@linux-foundation.org \
--cc=jack@suse.cz \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tytso@MIT.EDU \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.