kernel/sysctl_binary.c: improve the usage of return value 'result'

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

Quoted 2 lines · On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> wrote:

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

It used to be pervasive kernel style do to

	ret = -ENOMEM;
	foo = alloc(...);
	if (!foo)
		goto out;

whereas nowadays people usually do the more straightforward

	foo = alloc(...);
	if (!foo) {
		ret = -ENOMEM;
		goto out;
	}

The thinking was that the old style generated better code, but for the life of me I can't remember why :(

Quoted 20 lines · Andrew Morton

Quoted 2 lines · On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> wrote:

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

It used to be pervasive kernel style do to

	ret = -ENOMEM;
	foo = alloc(...);
	if (!foo)
		goto out;

whereas nowadays people usually do the more straightforward

	foo = alloc(...);
	if (!foo) {
		ret = -ENOMEM;
		goto out;
	}

Quoted 2 lines

The thinking was that the old style generated better code, but for the life of me I can't remember why :(

https://lkml.org/lkml/2008/12/16/383

Quoted 2 lines · On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> wrote:

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

It used to be pervasive kernel style do to

	ret = -ENOMEM;
	foo = alloc(...);
	if (!foo)
		goto out;

whereas nowadays people usually do the more straightforward

	foo = alloc(...);
	if (!foo) {
		ret = -ENOMEM;
		goto out;
	}

The thinking was that the old style generated better code, but for the life of me I can't remember why :(

Your patch switches from old-style to new-style. And it appears to have increased the text size. I did this, to switch three sites back to old-style:

kernel/sysctl_binary.c renamed from kernel/sysctl_binary.c~kernel-sysctl_binaryc-improve-the-usage-of-return-value-result-fix +6 -9

@@ -941,17 +941,15 @@ static ssize_t bin_string(struct file *f
 		copied = result;
 		lastp = oldval + copied - 1;
 
-		if (get_user(ch, lastp)) {
-			result = -EFAULT;
+		result = -EFAULT;
+		if (get_user(ch, lastp))
 			goto out;
-		}
 
 		/* Trim off the trailing newline */
 		if (ch == '\n') {
-			if (put_user('\0', lastp)) {
-				result = -EFAULT;
+			result = -EFAULT;
+			if (put_user('\0', lastp))
 				goto out;
-			}
 			copied -= 1;
 		}
 	}

@@ -976,11 +974,10 @@ static ssize_t bin_intvec(struct file *f
 	char *buffer;
 	ssize_t result;
 
+	result = -ENOMEM;
 	buffer = kmalloc(BUFSZ, GFP_KERNEL);
-	if (!buffer) {
-		result = -ENOMEM;
+	if (!buffer)
 		goto out;
-	}
 
 	if (oldval && oldlen) {
 		unsigned __user *vec = oldval;

_

and kernel/sysctl_binary.o's .text got six bytes smaller.

Now, smaller text doesn't mean faster code. But it probably means larger cache footprint, which can mean slower code.

IOW, it isn't obvious that this was an improvement.

Quoted 14 lines · Andrew Morton

Quoted 2 lines · On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> wrote:

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

It used to be pervasive kernel style do to

	ret = -ENOMEM;
	foo = alloc(...);
	if (!foo)
		goto out;

whereas nowadays people usually do the more straightforward

Quoted 8 lines

	foo = alloc(...);
	if (!foo) {
		ret = -ENOMEM;
		goto out;
	}

The thinking was that the old style generated better code, but for the life of me I can't remember why :(

Because doing the assignment outside of the if() goto . Allows the compiler to emit the if() goto as a single branch.

While a smart compiler may perform the code motion across the branch, it is much easier for the compiler to branch to somewhere else perform the assignment and then branch out.

Eric

Quoted 44 lines

Your patch switches from old-style to new-style. And it appears to have increased the text size. I did this, to switch three sites back to old-style:

kernel/sysctl_binary.c renamed from kernel/sysctl_binary.c~kernel-sysctl_binaryc-improve-the-usage-of-return-value-result-fix +6 -9

@@ -941,17 +941,15 @@ static ssize_t bin_string(struct file *f
 		copied = result;
 		lastp = oldval + copied - 1;
 
-		if (get_user(ch, lastp)) {
-			result = -EFAULT;
+		result = -EFAULT;
+		if (get_user(ch, lastp))
 			goto out;
-		}
 
 		/* Trim off the trailing newline */
 		if (ch == '\n') {
-			if (put_user('\0', lastp)) {
-				result = -EFAULT;
+			result = -EFAULT;
+			if (put_user('\0', lastp))
 				goto out;
-			}
 			copied -= 1;
 		}
 	}

@@ -976,11 +974,10 @@ static ssize_t bin_intvec(struct file *f
 	char *buffer;
 	ssize_t result;
 
+	result = -ENOMEM;
 	buffer = kmalloc(BUFSZ, GFP_KERNEL);
-	if (!buffer) {
-		result = -ENOMEM;
+	if (!buffer)
 		goto out;
-	}
 
 	if (oldval && oldlen) {
 		unsigned __user *vec = oldval;

_

Quoted 6 lines

and kernel/sysctl_binary.o's .text got six bytes smaller.

Now, smaller text doesn't mean faster code. But it probably means larger cache footprint, which can mean slower code.

IOW, it isn't obvious that this was an improvement.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 2 lines · On Tue, 06 Aug 2013 15:29:42 +0800 Chen Gang <[email protected]> wrote:

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

It used to be pervasive kernel style do to

	ret = -ENOMEM;
	foo = alloc(...);
	if (!foo)
		goto out;

whereas nowadays people usually do the more straightforward

	foo = alloc(...);
	if (!foo) {
		ret = -ENOMEM;
		goto out;
	}

The thinking was that the old style generated better code, but for the life of me I can't remember why :(

Your patch switches from old-style to new-style. And it appears to have increased the text size. I did this, to switch three sites back to old-style:

kernel/sysctl_binary.c renamed from kernel/sysctl_binary.c~kernel-sysctl_binaryc-improve-the-usage-of-return-value-result-fix +6 -9

@@ -941,17 +941,15 @@ static ssize_t bin_string(struct file *f
 		copied = result;
 		lastp = oldval + copied - 1;
 
-		if (get_user(ch, lastp)) {
-			result = -EFAULT;
+		result = -EFAULT;
+		if (get_user(ch, lastp))
 			goto out;
-		}
 
 		/* Trim off the trailing newline */
 		if (ch == '\n') {
-			if (put_user('\0', lastp)) {
-				result = -EFAULT;
+			result = -EFAULT;
+			if (put_user('\0', lastp))
 				goto out;
-			}
 			copied -= 1;
 		}
 	}

@@ -976,11 +974,10 @@ static ssize_t bin_intvec(struct file *f
 	char *buffer;
 	ssize_t result;
 
+	result = -ENOMEM;
 	buffer = kmalloc(BUFSZ, GFP_KERNEL);
-	if (!buffer) {
-		result = -ENOMEM;
+	if (!buffer)
 		goto out;
-	}
 
 	if (oldval && oldlen) {
 		unsigned __user *vec = oldval;

_

and kernel/sysctl_binary.o's .text got six bytes smaller.

Now, smaller text doesn't mean faster code. But it probably means larger cache footprint, which can mean slower code.

IOW, it isn't obvious that this was an improvement.

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

Assign the return error code only when error occurs, so can save some instructions (some of them are inside looping).

Rewrite the sysctl_getname() to make code clearer to readers, and also can save some instructions (it is mainly related with the usage of the variable 'result').

Remove useless variable 'result' for sysctl() and compat sysctl(), when do_sysctl() fails, it is meaningless to assign 'oldlenp ' with original same value.

Also modify the related code to pass "./scripts/checkpatch.pl" checking.

Chen Gang <[email protected]> writes:

Have you tested this code? Do you have anything that actually the uses sysctl binary interface?

If you do have code that actually uses this interface please fix it not to use it. This code is fundamentally a stop gap measure and will bit-rot in time and then we will remove it.

Quoted 5 lines

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

Assign the return error code only when error occurs, so can save some instructions (some of them are inside looping).

That actually is a code pessimization, not an optimization. As are most of your proposed changes. Only assigning the error code simply can not generate better assembly code.

Quoted 3 lines

Rewrite the sysctl_getname() to make code clearer to readers, and also can save some instructions (it is mainly related with the usage of the variable 'result').

Again you are introducing branches and pessimizing the code in the name of optimization.

Quoted 5 lines

Remove useless variable 'result' for sysctl() and compat sysctl(), when do_sysctl() fails, it is meaningless to assign 'oldlenp ' with original same value.

Also modify the related code to pass "./scripts/checkpatch.pl" checking.

I really don't care about checpatch.pl at this point.

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

The sysctl binary path has never been properly maintained and I don't
intend to start. But I will spend 5 minutes to say this patch seems to
make the code worse not better.

Eric

Quoted 3 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

Pardon?

Excuse me, my English is not quite well, I don't quite understand your meaning, could you please repeat again in details or say more clearly ?

Quoted 4 lines

The sysctl binary path has never been properly maintained and I don't
intend to start. But I will spend 5 minutes to say this patch seems to
make the code worse not better.

I guess no one ever invited you to maintain this file (for just as you said, this file will be removed), so don't worry about it.

Hmm... do you mean you spend 5 minutes to get a conclusion ? if so, better not use word 'seems' which is not a suitable word appeared in the results, proofs or conclusions.

At least, one conclusion is: this patch switches from old-style to new-style, not for optimization.

But for sysctl_getname() and "checkpatch.pl" of this patch, better to get more discussion.

Is it OK ?

Quoted 2 lines

Quoted 2 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

Quoted 3 lines

Hmm... do you mean you spend 5 minutes to get a conclusion ? if so, better not use word 'seems' which is not a suitable word appeared in the results, proofs or conclusions.

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

Quoted 2 lines · Li Zefan

Quoted 2 lines

Quoted 2 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

[]

Quoted 3 lines

Quoted 3 lines

Hmm... do you mean you spend 5 minutes to get a conclusion ? if so, better not use word 'seems' which is not a suitable word appeared in the results, proofs or conclusions.

[]

Quoted 2 lines

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

I don't think that's true.

For instance, pessimization is a pretty obscure word. Most native English speakers could probably guess what it meant, but I couldn't hold any low opinion of non-native speaker that asked "what does that mean".

The code communicates just fine.

Chen Gang could be a bit more careful when making significant changes to core kernel facilities.

(said the guy that broke just printk)

I think the fluent English speakers can be tolerant reasonably well when the code is correct and compelling (and correct).

cheers, Joe

Quoted 2 lines · Li Zefan

Quoted 2 lines

Quoted 2 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

[]

Quoted 3 lines

Quoted 3 lines

Hmm... do you mean you spend 5 minutes to get a conclusion ? if so, better not use word 'seems' which is not a suitable word appeared in the results, proofs or conclusions.

[]

Quoted 2 lines

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

I don't think that's true.

For instance, pessimization is a pretty obscure word. Most native English speakers could probably guess what it meant, but I couldn't hold any low opinion of non-native speaker that asked "what does that mean".

Quoted 13 lines · Joe Perches

Quoted 2 lines · Li Zefan

Quoted 2 lines

Quoted 2 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

[]

Quoted 3 lines

Quoted 3 lines

Hmm... do you mean you spend 5 minutes to get a conclusion ? if so, better not use word 'seems' which is not a suitable word appeared in the results, proofs or conclusions.

[]

Quoted 2 lines

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

I don't think that's true.

Quoted 6 lines

For instance, pessimization is a pretty obscure word. Most native English speakers could probably guess what it meant, but I couldn't hold any low opinion of non-native speaker that asked "what does that mean".

I know, and that's why I didn't quote that part.

Quoted 2 lines · Li Zefan

Quoted 2 lines

Quoted 2 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

[]

Quoted 3 lines

Quoted 3 lines

Hmm... do you mean you spend 5 minutes to get a conclusion ? if so, better not use word 'seems' which is not a suitable word appeared in the results, proofs or conclusions.

[]

Quoted 2 lines

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

I don't think that's true.

For instance, pessimization is a pretty obscure word. Most native English speakers could probably guess what it meant, but I couldn't hold any low opinion of non-native speaker that asked "what does that mean".

Quoted 15 lines

Quoted 3 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

Pardon?

Excuse me, my English is not quite well, I don't quite understand your meaning, could you please repeat again in details or say more clearly ?

Quoted 4 lines

The sysctl binary path has never been properly maintained and I don't
intend to start. But I will spend 5 minutes to say this patch seems to
make the code worse not better.

Quoted 7 lines

I guess no one ever invited you to maintain this file (for just as you said, this file will be removed), so don't worry about it.

Hmm... do you mean you spend 5 minutes to get a conclusion ? if so, better not use word 'seems' which is not a suitable word appeared in the results, proofs or conclusions.

To be honest...

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

And people easily get frustrated or even pissed off when discussing with you. That's why tglx descided to put your emails into /dev/null...

Quoted 7 lines

At least, one conclusion is: this patch switches from old-style to new-style, not for optimization.

But for sysctl_getname() and "checkpatch.pl" of this patch, better to get more discussion.

Is it OK ?

Quoted 5 lines

To be honest...

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

So I should improve my English, and now I am just trying improving.

At least, it is not an excuse to leave upstream kernel development, is it right ? or do you have additional ideas or suggestions ?

Quoted 8 lines · On Wed, 07 Aug 2013 14:24:05 +0800 Chen Gang <[email protected]> wrote:

Quoted 5 lines

To be honest...

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

So I should improve my English, and now I am just trying improving.

Quoted 2 lines

At least, it is not an excuse to leave upstream kernel development, is it right ? or do you have additional ideas or suggestions ?

I find your English to be adequate. There are worse ;)

Don't be afraid of writing too much text - trust me, I've never seen a changelog which was too long!

Quoted 5 lines

To be honest...

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

So I should improve my English, and now I am just trying improving.

At least, it is not an excuse to leave upstream kernel development, is it right ? or do you have additional ideas or suggestions ?

Quoted 6 lines

Quoted 5 lines

To be honest...

You are too bad in english to do kernel development. You don't seem to know how to communicate in english...

Quoted 5 lines

So I should improve my English, and now I am just trying improving.

At least, it is not an excuse to leave upstream kernel development, is it right ? or do you have additional ideas or suggestions ?

Two sugguestions.

The first one is, if you get a reply from a maintainer (especially a top maintainer), try harder to understand/learn from that reply, but don't keep asking why and don't keep arguing without much thinking. I think what's why sometimes people are annoyed in the discussion with you.

The second one is, better focus on a specific subsystem and try to do some real work. It's not bad to start making patches like this one, but it won't do you any good in the long term. Don't get addicted in increasing patch contribution.

Quoted 5 lines

The first one is, if you get a reply from a maintainer (especially a top maintainer), try harder to understand/learn from that reply, but don't keep asking why and don't keep arguing without much thinking. I think what's why sometimes people are annoyed in the discussion with you.

In my opinion, "understand/learn" means:

learn the proof which the author supplied;
understand the author's opinion;
know about what the author wants to do now (especially why he intents to send/reply mail to you).

But "understand/learn" does not mean:

familiar about the 'professional' details.
if each related member knows about the 'professional' details, it only need a work flow, not need discussing.

Do you think so too ?

Hmm... for each reply, I think it has 3 requirements:

1. match the original contents which we want to reply.
2. say opinion clearly.
3. provide proof.

I guess your suggestion is for 1st: if we can not understand/learn from the original contents, of cause, our reply can not match it.

Since discussing is thinking process, and we may get more understanding during thinking, so it permits to continue reply multiple times (if for each reply is qualified with the 3 requirements above).

Have you ever seen some of my reply which misunderstand(or not learn enough) from original contents ?

Maybe you often saw that I continue reply multiple times for a thread, but I think, each reply matches the 3 requirements above.

Quoted 34 lines

Quoted 5 lines

The first one is, if you get a reply from a maintainer (especially a top maintainer), try harder to understand/learn from that reply, but don't keep asking why and don't keep arguing without much thinking. I think what's why sometimes people are annoyed in the discussion with you.

In my opinion, "understand/learn" means:

learn the proof which the author supplied;
understand the author's opinion;
know about what the author wants to do now (especially why he intents to send/reply mail to you).

But "understand/learn" does not mean:

familiar about the 'professional' details.
if each related member knows about the 'professional' details, it only need a work flow, not need discussing.

Do you think so too ?

Hmm... for each reply, I think it has 3 requirements:

1. match the original contents which we want to reply.
2. say opinion clearly.
3. provide proof.

I guess your suggestion is for 1st: if we can not understand/learn from the original contents, of cause, our reply can not match it.

Since discussing is thinking process, and we may get more understanding during thinking, so it permits to continue reply multiple times (if for each reply is qualified with the 3 requirements above).

Quoted 6 lines

Have you ever seen some of my reply which misunderstand(or not learn enough) from original contents ?

Maybe you often saw that I continue reply multiple times for a thread, but I think, each reply matches the 3 requirements above.

You fail to see there's a problem in you and how you frustrate people and waste their time...

For example in this thread:

https://lkml.org/lkml/2013/7/4/405

and this therad:

https://lkml.org/lkml/2013/6/20/228

Please don't argue anymore...

Back to coding and won't reply to this thread...

Quoted 5 lines · Eric W. Biederman

Chen Gang <[email protected]> writes:

Have you tested this code? Do you have anything that actually the uses sysctl binary interface?

No, I only compile about it, not give a test. It is really better to give a test, but it seems not quite necessary to must give a test (since it is a simple change).

Quoted 4 lines

If you do have code that actually uses this interface please fix it not to use it. This code is fundamentally a stop gap measure and will bit-rot in time and then we will remove it.

OK, since these code will be removed, and this patch is not for bug fixing, so at least, this patch is useless.

Quoted 6 lines

Quoted 5 lines

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

Assign the return error code only when error occurs, so can save some instructions (some of them are inside looping).

Quoted 4 lines

That actually is a code pessimization, not an optimization. As are most of your proposed changes. Only assigning the error code simply can not generate better assembly code.

Excuse me, my English is not quite well, I don't know about the meaning of 'pessimization' (I can not find it in my English-Chinese dictionary, in Thunderbird Spell Check, it is not a correct word either).

But it seems it doesn't matter, what you want to say is only "it is not an optimization", is it correct ?

Quoted 7 lines

Quoted 3 lines

Rewrite the sysctl_getname() to make code clearer to readers, and also can save some instructions (it is mainly related with the usage of the variable 'result').

Again you are introducing branches and pessimizing the code in the name of optimization.

Hmm... it is useless for optimization.

But in my opinion, at least, it can make code more clearer for C code readers, although it may make performance a litter lower.

Please see the 2 implementations below, I feel 2nd is clearer than 1st, how about your feeling ?

Do you mean: you don't care about "checkpatch.pl", because some of another reasons which in fact not related with "checkpatch.pl" though ?

Quoted 3 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

Pardon?

Excuse me, my English is not quite well, I don't quite understand your meaning, could you please repeat again in details or say more clearly?

Quoted 4 lines

The sysctl binary path has never been properly maintained and I don't
intend to start. But I will spend 5 minutes to say this patch seems to
make the code worse not better.

I guess no one ever invited you to maintain this file (for just as you said, this file will be removed), so don't worry about it.

Quoted 7 lines · Chen Gang

Quoted 5 lines · Eric W. Biederman

Chen Gang <[email protected]> writes:

Have you tested this code? Do you have anything that actually the uses sysctl binary interface?

Quoted 3 lines

No, I only compile about it, not give a test. It is really better to give a test, but it seems not quite necessary to must give a test (since it is a simple change).

Many programs have been broken by programmers not caring enough to test their changes. In this case it is doubly important because if you don't test this code it is likely no one will run this code for months.

Quoted 5 lines

Quoted 4 lines

If you do have code that actually uses this interface please fix it not to use it. This code is fundamentally a stop gap measure and will bit-rot in time and then we will remove it.

Quoted 2 lines

OK, since these code will be removed, and this patch is not for bug fixing, so at least, this patch is useless.

The point of all of this is that it really helps if you stop and learn the context of the code you are working on. It also helps if you care about the changes you are making.

In this case the history of sysctl(2) is that someone copied sysctl(2) from bsd. Then someone quickly created /proc/sys/ to reflect the same data in a friendlier format. sysctl(2) was quickly deprecated and was effectively never used in practice. With one notable exception an old use in glibc.

I was very nice a few years ago and instead of just deleting the code I wrote a wrapper layer so that the binary interface would translate into read/write operations on /proc/sys. Mostly that was about not letting the maintenance of sysctl(2) be a drag on the maintenance of /proc/sys/. People do care about /proc/sys and do test it and fix bugs with it.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Improve the usage of return value 'result', so not only can make code clearer to readers, but also can improve the performance.

Assign the return error code only when error occurs, so can save some instructions (some of them are inside looping).

Quoted 8 lines

Quoted 4 lines

That actually is a code pessimization, not an optimization. As are most of your proposed changes. Only assigning the error code simply can not generate better assembly code.

Excuse me, my English is not quite well, I don't know about the meaning of 'pessimization' (I can not find it in my English-Chinese dictionary, in Thunderbird Spell Check, it is not a correct word either).

Pessimization is the opposite of optimization. Both words are literally ridiculous as optimization means to make something more optimum, and pessimization means to make things more pessimal, and optimum and pessimal refer to the best and the worst possible situtations. But whoever let a silly dictionary meaning get in the way of a good use of a word.

Quoted 2 lines

But it seems it doesn't matter, what you want to say is only "it is not an optimization", is it correct ?

Correct.

Quoted 13 lines

Quoted 7 lines

Quoted 3 lines

Rewrite the sysctl_getname() to make code clearer to readers, and also can save some instructions (it is mainly related with the usage of the variable 'result').

Again you are introducing branches and pessimizing the code in the name of optimization.

Hmm... it is useless for optimization.

But in my opinion, at least, it can make code more clearer for C code readers, although it may make performance a litter lower.

Quoted 2 lines

Please see the 2 implementations below, I feel 2nd is clearer than 1st, how about your feeling ?

My feeling is if you don't care about sysctl(2) enough to figure out the history or compile test it, or even actually use it, it is highly unlikely you actually care about reading the code.

I care just enough about the code to keep people who would not even notice if it the code breaks from touching it.

Quoted 3 lines

Do you mean: you don't care about "checkpatch.pl", because some of another reasons which in fact not related with "checkpatch.pl" though ?

checkpatch.pl is not the definition of good code, or good style but meerly a tool to help point out problems before a human has to be bother to look at it. If a style problem is real it can be explained without reference to an arbitrary tool.

Quoted 6 lines

Quoted 3 lines

The right answer to the code is to config it out and then you don't have to worry about it one way or another.

Pardon?

Quoted 2 lines

Excuse me, my English is not quite well, I don't quite understand your meaning, could you please repeat again in details or say more clearly?

You can enable or disable this code with "make menuconfig". Everyone
should just turn binary sysctl support off and ignore this code. The
someday when the code has bit-rotted because no one actually cares we can delete this code.

Quoted 5 lines

Quoted 4 lines

The sysctl binary path has never been properly maintained and I don't
intend to start. But I will spend 5 minutes to say this patch seems to
make the code worse not better.

Quoted 2 lines

I guess no one ever invited you to maintain this file (for just as you said, this file will be removed), so don't worry about it.

We may get around to removing it. There are just enough silly people who think it is more important to keep open the possibility of keeping strange old binaries running than to avoid code bugs that the code has stayed this long. But no one has ever cared enough to in practice to maintain this code.

I use people sending patches to sysctl_binary.c as an opportunity to educate people that their program is doing something silly and they should change their ways. sysctl_binary.c really is effectively a honey pot for developers and organizations who are not paying attention.

Eric

Firstly, sorry for replying late, and also thank you for your detail patient reply.

Quoted 9 lines · Eric W. Biederman

Quoted 7 lines · Chen Gang

Quoted 5 lines · Eric W. Biederman

Chen Gang <[email protected]> writes:

Have you tested this code? Do you have anything that actually the uses sysctl binary interface?

Quoted 8 lines

Quoted 3 lines

No, I only compile about it, not give a test. It is really better to give a test, but it seems not quite necessary to must give a test (since it is a simple change).

Many programs have been broken by programmers not caring enough to test their changes. In this case it is doubly important because if you don't test this code it is likely no one will run this code for months.

It is only for code reconstruction, not for modifying real contents (especially not touch much code).

So in my option, do 3 checking is enough:

when edit the code, try to get selftest (only copy/past/delete, high light selections for checking, auto sentence checking ...). review the whole diff 2-3 times by oneself, review the whole source code 2-3 times by oneself.
let it pass compiling without error/warnings.

So I think, for an experienced programmer, it is better to give a test for our case, but not quite necessary to must do it (especially no related environments).

Quoted 13 lines

Quoted 13 lines

Quoted 7 lines

Quoted 3 lines

Rewrite the sysctl_getname() to make code clearer to readers, and also can save some instructions (it is mainly related with the usage of the variable 'result').

Again you are introducing branches and pessimizing the code in the name of optimization.

Hmm... it is useless for optimization.

But in my opinion, at least, it can make code more clearer for C code readers, although it may make performance a litter lower.

Quoted 7 lines

Quoted 2 lines

Please see the 2 implementations below, I feel 2nd is clearer than 1st, how about your feeling ?

My feeling is if you don't care about sysctl(2) enough to figure out the history or compile test it, or even actually use it, it is highly unlikely you actually care about reading the code.

Hmm... e.g. for coredump analysers:

they don't care about each sub-systems,
they can not figure out most of their history,
and never compile test most of them,
or even never actually use most of them,
but at least, they have to care about reading the code which may related with various sub-systems
(also need analyze the related binary data).
(also need read some of the related dis-assembly code).
...
especially, some of root cause often exist in the 'almost waste' code.

Quoted 5 lines

I use people sending patches to sysctl_binary.c as an opportunity to educate people that their program is doing something silly and they should change their ways. sysctl_binary.c really is effectively a honey pot for developers and organizations who are not paying attention.

Hmm... how about for coredump analyzers, they are 'crazy' but not 'silly' programmers. ;-)

Quoted 31 lines · Chen Gang

Firstly, sorry for replying late, and also thank you for your detail patient reply.

Quoted 9 lines · Eric W. Biederman

Quoted 7 lines · Chen Gang

Quoted 5 lines · Eric W. Biederman

Chen Gang <[email protected]> writes:

Have you tested this code? Do you have anything that actually the uses sysctl binary interface?

Quoted 8 lines

Quoted 3 lines

No, I only compile about it, not give a test. It is really better to give a test, but it seems not quite necessary to must give a test (since it is a simple change).

Many programs have been broken by programmers not caring enough to test their changes. In this case it is doubly important because if you don't test this code it is likely no one will run this code for months.

It is only for code reconstruction, not for modifying real contents (especially not touch much code).

So in my option, do 3 checking is enough:

when edit the code, try to get selftest (only copy/past/delete, high light selections for checking, auto sentence checking ...). review the whole diff 2-3 times by oneself, review the whole source code 2-3 times by oneself.
let it pass compiling without error/warnings.

Quoted 3 lines

So I think, for an experienced programmer, it is better to give a test for our case, but not quite necessary to must do it (especially no related environments).

Sometimes it is not possible to test the code, when dealing with strange architectures or hardware we don't have, and a best effort must be made.

However in the case of a single system call that is trivially callable it is just good practice to test the code.

But seriously there is a human factor at play. Our internal algorthims for doing things as human beings are not exact but best effor hueristics and sometimes they go wrong. So practical double checks are important.

Being an experienced programmer actually means there is more reason to stop and test your changes because it is terribly easy to get overconfident.

And frankly I don't want to see patches from people who in general don't care enough about what they are changing that they are not motivated to test their code.

Quoted 23 lines

Quoted 13 lines

Quoted 13 lines

Quoted 7 lines

Quoted 3 lines

Rewrite the sysctl_getname() to make code clearer to readers, and also can save some instructions (it is mainly related with the usage of the variable 'result').

Again you are introducing branches and pessimizing the code in the name of optimization.

Hmm... it is useless for optimization.

But in my opinion, at least, it can make code more clearer for C code readers, although it may make performance a litter lower.

Quoted 7 lines

Quoted 2 lines

Please see the 2 implementations below, I feel 2nd is clearer than 1st, how about your feeling ?

My feeling is if you don't care about sysctl(2) enough to figure out the history or compile test it, or even actually use it, it is highly unlikely you actually care about reading the code.

Hmm... e.g. for coredump analysers:

Quoted 9 lines

they don't care about each sub-systems,
they can not figure out most of their history,
and never compile test most of them,
or even never actually use most of them,
but at least, they have to care about reading the code which may related with various sub-systems
(also need analyze the related binary data).
(also need read some of the related dis-assembly code).
...
especially, some of root cause often exist in the 'almost waste' code.

Then I would love to hear about which program was using the sysctl(2) system call in a core dump because that program needs to be fixed. Even if it was not the cause of the core dump.

But seriously if you are reading a lot of kernel code don't expect to be able to convert it to your exact preferred style. Within limits of Documentation/CodingStyle kernel code is kept in the authors preferred style.

Quoted 6 lines

Quoted 5 lines

I use people sending patches to sysctl_binary.c as an opportunity to educate people that their program is doing something silly and they should change their ways. sysctl_binary.c really is effectively a honey pot for developers and organizations who are not paying attention.

Quoted 2 lines

Hmm... how about for coredump analyzers, they are 'crazy' but not 'silly' programmers. ;-)

Well if your coredump lead you into sysctl_binary.c either you have a broken trace, there is a binary using sysctl(2) that really should be changed to use /proc/sys, or your following of the trace took you very far off the mark.

So please find the owner of that binary using sysctl(2) and politely inform them that it is a bad idea and regardless of whatever else is going on the binary should be updated to do something better.

Eric