Good Idea (tm): Code Consolidation for Functions and Macros that Access the Process Address Space

Good Idea (tm): Code Consolidation for Functions and Macros that Access the Process Address Space

[Joseph D. Wagner]

SUBJECT: Good Idea (tm): Code Consolidation for Functions and Macros
that Access the Process Address Space

PROBLEM: Eight (8) functions/macros are near duplicates of, and
equivalent to, other functions/macros that access the process address
space.  Specifically:

	get_user		duplicates get_user_ret
	__get_user		duplicates __get_user_ret
	put_user		duplicates put_user_ret
	__put_user		duplicates __put_user_ret
	copy_from_user	duplicates copy_from_user_ret
	__copy_from_user	duplicates copy_from_user_ret
	copy_to_user	duplicates copy_to_user_ret
	__copy_to_user	duplicates copy_to_user_ret

EXPLAINATION: One functional difference exists between the
functions/macros with "_ret" and those without: functions/macros with
"_ret" return an error code; those without "_ret" return void.

Since the only difference is whether or not an error code is returned,
the functions/macros can be used interchangeably.

Remember, if a function call has no place for a returned value to go,
nothing bad happens; the returned value is simply ignored/discarded.

WHY THIS SHOULD BE CHANGED:
1) Easier to maintain code (because there's less of it to maintain).
2) Less code means smaller kernel.
3) Forces better coding structures and procedures (because no matter
which function the user will choose under the new system, an error code
will always be returned).
4) Is backward compatible with all existing code.
5) The solution can be seamlessly integrated.
6) The overhead for returning an error code is nominal.

SOLUTION:

Use the #define Preprocessor Directive for Symbolic Constants.  Here's
some sample code:

	#define get_user get_user_ret
	#define __get_user __get_user_ret
	#define put_user put_user_ret
	#define __put_user __put_user_ret
	#define copy_from_user copy_from_user_ret
	#define __copy_from_user copy_from_user_ret
	#define copy_to_user copy_to_user_ret
	#define __copy_to_user copy_to_user_ret

By placing that code in the appropriate header file(s), the #define
statements will trickle down to the appropriate source files.

Hence the functions/macros without "_ret" can be eliminated, resulting
in code consolidation.

NOTE: The validity of using a #define Preprocessor Directive as a
Symbolic Constant on a function has been tested, and proven viable, in
the following sample program:

#include <iostream>
using std::cout;
using std::cin;
using std::endl;

void Hello();
void Hello_Again();

#define Hi Hello
#define Hi_Again Hello_Again

/*
void Hi();
void Hi_Again();
*/

int main() {
	Hello();
	Hello_Again();
	Hi();
	Hi_Again();
}

void Hello () {
	cout << "Hello." << endl;
}

void Hello_Again() {
	cout << "Hello, again." << endl;
}

/*
void Hi() {
	cout << "Hi." << endl;
}

void Hi_Again() {
	cout << "Hi, again." << endl;
}
*/

Re: Good Idea (tm): Code Consolidation for Functions and Macros that Access the Process Address Space

[Russell King]

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?

> 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_.

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

RE: Good Idea (tm): Code Consolidation for Functions and Macros that Access the Process Address Space

[Joseph D. Wagner]

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

>> 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.  My original
point still stands that maintaining duplicate functions -- well, near
duplicate with the exception of the returned error code -- is a waste of
time, resources, and coding, and for the purpose of simplified
maintenance, one of the sets of duplicate functions/macros should be
eliminated.

Joseph Wagner

Re: Good Idea (tm): Code Consolidation for Functions and Macros that Access the Process Address Space

[Russell King]

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