lkml.org 
[lkml]   [2002]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Good Idea (tm): Code Consolidation for Functions and Macros that Access the Process Address Space
On Sun, Oct 06, 2002 at 07:40:44AM -0500, Joseph D. Wagner wrote:
> >> On Sat, Oct 05, 2002 at 07:58:55PM -0500, Joseph D. Wagner wrote:
> >> SUBJECT: Good Idea (tm): Code Consolidation for Functions and Macros
> >> that Access the Process Address Space
> >>...
> >> Remember, if a function call has no place for a returned value to go,
> >> nothing bad happens; the returned value is simply ignored/discarded.
>
> > And the compiler warning?
>
> See WHY THIS SHOULD BE CHANGED #3 "Forces better coding structures and
> procedures..." Frankly, error controls should have been programmed into
> the code anyway. It's just good programming practice.

Lets address the compiler warnings point first:

1. If your build of 50,000 files produces 50 warnings per file, that's
2,500,000 warnings. You made the mistake of not initialising one
variable, which produces a different compiler warning. Are you going
to spot this? No. Can it cause serious run-time errors? Yes.
Can it cause security holes? Yes.

2. You accidentally forget the "return value;" statement at the end of
a function - same problem as (1)

3. You accidentally leave a "return value;" statement at the end of a
function - less serious, but you still want to know.

4. Cosmetically it looks bad.

5. It is _bad_ programming practice to write code that blatently violates
standards for the language you're programming it in.

So, having a build that produces needless warnings is _BAD_.

Now lets address the "forcing better programming standards". Well, we've
partially covered it above. In addition, what you're proposing is crap
like:

static int foo(int *user_arg)
{
struct bar_struct *bar;
int arg;

bar = kmalloc(...);
if (bar)
return -ENOMEM;

get_user_ret(arg, user_arg, -EFAULT);

arg = do_something(bar, arg);

put_user_ret(arg, user_arg, -EFAULT);

kfree(bar);

return 0;
}

Now count the number of errors there _because_ of the hitten function
returns. Is this good programming practise? No. Is it easy to write
the code in a way that doesn't do this? No. Is it obvious what its
doing? No.

Experience has proven that if you give enough rope to driver writers,
they _will_ hang themselves. They've done it many times in the past.
That is why it should be damned well obvious what the code is doing.
Which is why:

static int foo(int *user_arg)
{
struct bar_struct *bar;
int arg;

bar = kmalloc(...);
if (bar)
return -ENOMEM;

if (get_user(arg, user_arg))
return -EFAULT;

arg = do_something(bar, arg);

if (put_user(arg, user_arg))
return -EFAULT;

kfree(bar);

return 0;
}

is so much better. You can immediately _see_ the error. No hidden magic
functions that return underneath you.

> >> SOLUTION:
>
> > Get rid of the _ret forms. Their use is frowned on today anyway
> because
> > they hide the real meaning of what the code is trying to do, and
> hiding
> > the fact that a function can return in the middle of what looks like a
> > macro call is _REAL_ _BAD_.
>
> While I respectively disagree with you, I really don't care which set of
> functions/macros are eliminated for consolidation purposes.

Well, I've got news for you. Your requested "consolidation" has already
been done. Months ago. The xxx_ret forms no longer exist in either 2.4
nor 2.5 for the reasons I've stated above. And yes, "good programming
practice" is one of the reasons.

--
Russell King (rmk@arm.linux.org.uk) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

-
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/

\
 
 \ /
  Last update: 2005-03-22 13:29    [W:0.031 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site