Messages in this thread | | | Date | Sun, 6 Oct 2002 14:29:22 +0100 | From | Russell King <> | Subject | Re: 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/
| |