kernel/itimer.c: beautify code, not need check 'value', so save one instruction, simpler and easier for readers.

PATCH In review Chen Gang 2 participants 12 messages Jun 20, 2013 – Jul 22, 2013

Share & tools

Chen Gang

author

2013-06-20 11:27 UTC #0 lore raw

PATCH v2 1/1 kernel/itimer.c: beautify code, not need check 'value', so save one instruction, simpler and easier for readers.

Since copy_to_user() will check 'value', we do not need check it outside again, so can save one comparing instruction at least.

Also can let code simpler and easier for readers: if checking parameter 'value', it will easily lead readers to think about why not return -EINVAL instead of -EFAULT, when checking parameter failed.

Signed-off-by Chen Gang <[email protected]>

---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

kernel/itimer.c +6 -7

index 8d262b4..3b12271 100644
@@ -102,15 +102,14 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error;
 	struct itimerval get_buffer;
 
-	if (value) {
-		error = do_getitimer(which, &get_buffer);
-		if (!error &&
-		    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
-			error = -EFAULT;
-	}
+	error = do_getitimer(which, &get_buffer);
+	if (!error &&
+	    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
+		error = -EFAULT;
+
 	return error;
 }

Signature · 1 line

1.7.7.6

Chen Gang

author

2013-06-20 12:03 UTC #1 lore raw

Oh, sorry, maybe it is a trivial patch, also need send to [email protected].

Quoted 40 lines · Chen Gang

Since copy_to_user() will check 'value', we do not need check it outside again, so can save one comparing instruction at least.
Also can let code simpler and easier for readers: if checking parameter 'value', it will easily lead readers to think about why not return -EINVAL instead of -EFAULT, when checking parameter failed.
Signed-off-by Chen Gang <[email protected]>
---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
kernel/itimer.c +6 -7
index 8d262b4..3b12271 100644
@@ -102,15 +102,14 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error;
 	struct itimerval get_buffer;
 
-	if (value) {
-		error = do_getitimer(which, &get_buffer);
-		if (!error &&
-		    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
-			error = -EFAULT;
-	}
+	error = do_getitimer(which, &get_buffer);
+	if (!error &&
+	    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
+		error = -EFAULT;
+
 	return error;
 }
 

Signature · 3 lines

Chen Gang

Asianux Corporation

Thomas Gleixner

2013-06-20 12:56 UTC #2 lore raw

Quoted 3 lines · Chen Gang

Oh, sorry, maybe it is a trivial patch, also need send to [email protected].

No. This is not a trivial patch, it's changing the code flow.

Quoted 49 lines · Chen Gang

Quoted 40 lines
Since copy_to_user() will check 'value', we do not need check it outside again, so can save one comparing instruction at least.
Also can let code simpler and easier for readers: if checking parameter 'value', it will easily lead readers to think about why not return -EINVAL instead of -EFAULT, when checking parameter failed.
Signed-off-by Chen Gang <[email protected]>
---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
kernel/itimer.c +6 -7
index 8d262b4..3b12271 100644
@@ -102,15 +102,14 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error;
 	struct itimerval get_buffer;
 
-	if (value) {
-		error = do_getitimer(which, &get_buffer);
-		if (!error &&
-		    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
-			error = -EFAULT;
-	}
+	error = do_getitimer(which, &get_buffer);
+	if (!error &&
+	    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
+		error = -EFAULT;
+
 	return error;
 }
 
Signature · 3 lines
Chen Gang

Asianux Corporation

Chen Gang

author

2013-06-21 01:24 UTC #3 lore raw

Quoted 7 lines · Thomas Gleixner

Quoted 3 lines · Chen Gang
Quoted 3 lines
Oh, sorry, maybe it is a trivial patch, also need send to [email protected].
No. This is not a trivial patch, it's changing the code flow.

OK, that is not a trivial patch, but a minor patch, is it correct ??

;-)

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation

Thomas Gleixner

2013-06-20 13:42 UTC #4 lore raw

In reply to Chen Gang

On Thu, 20 Jun 2013, Chen Gang wrote:

kernel/itimer.c: beautify code, not need check 'value', so
save one instruction, simpler and easier for readers.

That's an essay and not a proper subject line for a patch.

See Documentation/SubmittingPatches and look at the other patch subject lines in git log.

Quoted 2 lines

Since copy_to_user() will check 'value', we do not need check it outside again, so can save one comparing instruction at least.

copy_to_user() does not check value, it will fault due to a NULL pointer dereference and execute the exception fixup.

That's a massive difference which wants to be documented and argued why it's ok to do so.

Aside of that, please line break the changelog lines around 78 characters.

Quoted 3 lines

Also can let code simpler and easier for readers: if checking parameter 'value', it will easily lead readers to think about why not return -EINVAL instead of -EFAULT, when checking parameter failed.

So you are seriously claiming, that the check for !value makes people think that the return value should be -EINVAL?

That's hillarious.

Can you please start to think about, why YOU thought that returning -EINVAL is the proper return value for that case?

Simply because in your rush to submit patches according to your self defined contribution plan, you fail to sit down and carefully study the code and the according documentation (man page). Instead of that you see some random snippet of code which looks wrong to you and you send out patches without care. After someone points out your failure you claim that the code is misleading to readers.

The code is not misleading to careful readers, it's only misleading to sloppy readers.

And I'm neither accepting sloppy patches nor am I accepting sloppy changelogs which make false claims.

Thanks,

tglx

Chen Gang

author

2013-06-21 02:05 UTC #5 lore raw

Firstly, I guess:

since you can spend your time resource to reply, that means "you also
think this patch is valuable, but the comments need improving"

Is it correct ?

Quoted 10 lines · Thomas Gleixner

On Thu, 20 Jun 2013, Chen Gang wrote:
kernel/itimer.c: beautify code, not need check 'value', so
save one instruction, simpler and easier for readers.
That's an essay and not a proper subject line for a patch.
See Documentation/SubmittingPatches and look at the other patch subject lines in git log.

How about "kernel/itimer.c: remove the checking 'value' statement".

Or please provide your samples for the subject.

Quoted 5 lines

Quoted 2 lines
Quoted 2 lines
Since copy_to_user() will check 'value', we do not need check it outside again, so can save one comparing instruction at least.
copy_to_user() does not check value, it will fault due to a NULL pointer dereference and execute the exception fixup.

Change it to:

Since copy_to_user() will process "bad address" internally, we need not check 'value' again, then can save one comparing instruction at least.

Quoted 6 lines

That's a massive difference which wants to be documented and argued why it's ok to do so.
Aside of that, please line break the changelog lines around 78 characters.

I use Thunderbird mail client, enable world wrap, is it OK ?

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Also can let code simpler and easier for readers: if checking parameter 'value', it will easily lead readers to think about why not return -EINVAL instead of -EFAULT, when checking parameter failed.
So you are seriously claiming, that the check for !value makes people think that the return value should be -EINVAL?
That's hillarious.

That seems not a quite polite word, is it ? ;-)

Quoted 3 lines

Can you please start to think about, why YOU thought that returning -EINVAL is the proper return value for that case?

If you check the parameter, and find it invalid, and want to return with failure, every one can assume you want to return -EINVAL.

Hmm... in some of embedded system which NOMMU, 'NULL' does not means "bad address" (at least can write).

Quoted 10 lines

Simply because in your rush to submit patches according to your self defined contribution plan, you fail to sit down and carefully study the code and the according documentation (man page). Instead of that you see some random snippet of code which looks wrong to you and you send out patches without care. After someone points out your failure you claim that the code is misleading to readers.
The code is not misleading to careful readers, it's only misleading to sloppy readers.

Do you mean this patch can not make the code simpler and clearer ?

I guess, that is not your meaning, so how about this improving:

"after remove the code, also can let it simpler and clearer for readers"

Is it OK ?

Quoted 3 lines

And I'm neither accepting sloppy patches nor am I accepting sloppy changelogs which make false claims.

That is one of the reason for why we need reviewer, the work flow need be "providing patch --> review --> apply".

BTW: Can you guess how many my patches have been applied by upstream, since this year ? That seems most of appliers are very polite, I wish that will include you. ;-)

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation

Chen Gang

author

2013-06-21 10:32 UTC #6 lore raw

PATCH v3 1/1 kernel/itimer.c: remove the checking 'value' statement

Since copy_to_user() will process "bad address" internally, we need not check 'value' again, then can save one comparing instruction at least.

For some embedded system with NOMMU, 'NULL' may not mean "bad address" (EFAULT), so recommend to let copy_to_user() to check 'EFAULT' in this case, completely.

Removing the checking code, also can let it simpler and clearer to readers.

Signed-off-by Chen Gang <[email protected]>

---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

kernel/itimer.c +6 -7

index 8d262b4..3b12271 100644
@@ -102,15 +102,14 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error;
 	struct itimerval get_buffer;
 
-	if (value) {
-		error = do_getitimer(which, &get_buffer);
-		if (!error &&
-		    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
-			error = -EFAULT;
-	}
+	error = do_getitimer(which, &get_buffer);
+	if (!error &&
+	    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
+		error = -EFAULT;
+
 	return error;
 }

Signature · 1 line

1.7.7.6

Thomas Gleixner

2013-06-24 23:28 UTC #7 lore raw

Re: [PATCH v2] kernel/itimer.c: beautify code, not need check 'value', so save one instruction, simpler and easier for readers.t

In reply to Chen Gang

Quoted 9 lines · Chen Gang

Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Also can let code simpler and easier for readers: if checking parameter 'value', it will easily lead readers to think about why not return -EINVAL instead of -EFAULT, when checking parameter failed.
So you are seriously claiming, that the check for !value makes people think that the return value should be -EINVAL?
That's hillarious.
That seems not a quite polite word, is it ? ;-)

My apologies for being so impolite. Let me rephrase it. Here is a "sample" changelog for your patch:

Subject: itimers: Remove bogus NULL pointer check in sys_getitimer()

People might be tricked into assuming that the return value for a failed NULL pointer check should be -EINVAL instead of -EFAULT.

Remove the misleading NULL pointer check to fix this nuisance.

Aside of that this patch fixes the problem of NOMMU kernels, where a NULL pointer dereference is a valid operation. This allows to boot NOMMU kernels without working around the shortcomings of the getitimer() system call, which have been ignored since this NULL pointer check was introduced in Linux 0.96a.

Please resubmit.

Thanks,

tglx

Chen Gang

author

2013-06-25 00:59 UTC #8 lore raw

Re: [PATCH v2] kernel/itimer.c: beautify code, not need check 'value', so save one instruction, simpler and easier for readers.t

Quoted 4 lines · Thomas Gleixner

Quoted 3 lines · Chen Gang
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Also can let code simpler and easier for readers: if checking parameter 'value', it will easily lead readers to think about why not return -EINVAL instead of -EFAULT, when checking parameter failed.

Quoted 9 lines

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
So you are seriously claiming, that the check for !value makes people think that the return value should be -EINVAL?
That's hillarious.
That seems not a quite polite word, is it ? ;-)
My apologies for being so impolite. Let me rephrase it. Here is a "sample" changelog for your patch:

It doesn't matter, I really don't (shouldn't) care about it.

Next time, I should try to send patch carefully, so may save the maintainers' timer resource.

And excuse me for my poor English and either not familiar with kernel, I am trying to improve them, and keep improving them.

Quoted 5 lines

Subject: itimers: Remove bogus NULL pointer check in sys_getitimer()
People might be tricked into assuming that the return value for a failed NULL pointer check should be -EINVAL instead of -EFAULT.

Quoted 8 lines

Remove the misleading NULL pointer check to fix this nuisance.
Aside of that this patch fixes the problem of NOMMU kernels, where a NULL pointer dereference is a valid operation. This allows to boot NOMMU kernels without working around the shortcomings of the getitimer() system call, which have been ignored since this NULL pointer check was introduced in Linux 0.96a.

Really very good comments, at least for me now, I really can not write a comment like that.

Quoted 2 lines

Please resubmit.

I will send patch v4 (patch v3 has sent, and should be obsoleted)

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation

Chen Gang

author

2013-06-25 01:17 UTC #9 lore raw

PATCH v4 1/1 itimers: Remove bogus NULL pointer check in sys_getitimer()

People might be tricked into assuming that the return value for a failed NULL pointer check should be -EINVAL instead of -EFAULT.

Remove the misleading NULL pointer check to fix this nuisance.

Signed-off-by Chen Gang <[email protected]>

---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

kernel/itimer.c +6 -7

index 8d262b4..3b12271 100644
@@ -102,15 +102,14 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error;
 	struct itimerval get_buffer;
 
-	if (value) {
-		error = do_getitimer(which, &get_buffer);
-		if (!error &&
-		    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
-			error = -EFAULT;
-	}
+	error = do_getitimer(which, &get_buffer);
+	if (!error &&
+	    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
+		error = -EFAULT;
+
 	return error;
 }

Signature · 1 line

1.7.7.6

Chen Gang

author

2013-07-05 01:29 UTC #10 lore raw

Re: [PATH v4] itimers: Remove bogus NULL pointer check in sys_getitimer()

Hello Maintainers:

Is this patch under the normal work flow (or I should need a little more patience) ?

Thanks.

Quoted 43 lines · Chen Gang

People might be tricked into assuming that the return value for a failed NULL pointer check should be -EINVAL instead of -EFAULT.
Remove the misleading NULL pointer check to fix this nuisance.
Aside of that this patch fixes the problem of NOMMU kernels, where a NULL pointer dereference is a valid operation. This allows to boot NOMMU kernels without working around the shortcomings of the getitimer() system call, which have been ignored since this NULL pointer check was introduced in Linux 0.96a.
Signed-off-by Chen Gang <[email protected]>
---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
kernel/itimer.c +6 -7
index 8d262b4..3b12271 100644
@@ -102,15 +102,14 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error;
 	struct itimerval get_buffer;
 
-	if (value) {
-		error = do_getitimer(which, &get_buffer);
-		if (!error &&
-		    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
-			error = -EFAULT;
-	}
+	error = do_getitimer(which, &get_buffer);
+	if (!error &&
+	    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
+		error = -EFAULT;
+
 	return error;
 }
 

Signature · 1 line

Chen Gang

Chen Gang

author

2013-07-22 02:46 UTC #11 lore raw

Re: [PATH v4] itimers: Remove bogus NULL pointer check in sys_getitimer()

Hello Maintainers:

Please help check this patch, when you have time.

Thanks.

Quoted 53 lines · Chen Gang

Hello Maintainers:
Is this patch under the normal work flow (or I should need a little more patience) ?
Thanks.
Quoted 43 lines · Chen Gang
People might be tricked into assuming that the return value for a failed NULL pointer check should be -EINVAL instead of -EFAULT.
Remove the misleading NULL pointer check to fix this nuisance.
Aside of that this patch fixes the problem of NOMMU kernels, where a NULL pointer dereference is a valid operation. This allows to boot NOMMU kernels without working around the shortcomings of the getitimer() system call, which have been ignored since this NULL pointer check was introduced in Linux 0.96a.
Signed-off-by Chen Gang <[email protected]>
---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
kernel/itimer.c +6 -7
index 8d262b4..3b12271 100644
@@ -102,15 +102,14 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error;
 	struct itimerval get_buffer;
 
-	if (value) {
-		error = do_getitimer(which, &get_buffer);
-		if (!error &&
-		    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
-			error = -EFAULT;
-	}
+	error = do_getitimer(which, &get_buffer);
+	if (!error &&
+	    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
+		error = -EFAULT;
+
 	return error;
 }
 

Signature · 1 line

Chen Gang