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

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

[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 <gang.chen@asianux.com>
---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/itimer.c b/kernel/itimer.c
index 8d262b4..3b12271 100644
--- a/kernel/itimer.c
+++ b/kernel/itimer.c
@@ -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;
 }
 
-- 
1.7.7.6

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

[Chen Gang]

Oh, sorry, maybe it is a trivial patch, also need send to
trivial@kernel.org.


On 06/20/2013 07:26 PM, Chen Gang wrote:
> 
> 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 <gang.chen@asianux.com>
> ---
>  kernel/itimer.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/itimer.c b/kernel/itimer.c
> index 8d262b4..3b12271 100644
> --- a/kernel/itimer.c
> +++ b/kernel/itimer.c
> @@ -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;
>  }
>  
> 


-- 
Chen Gang

Asianux Corporation

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

[Thomas Gleixner]

On Thu, 20 Jun 2013, Chen Gang wrote:

> 
> Oh, sorry, maybe it is a trivial patch, also need send to
> trivial@kernel.org.

No. This is not a trivial patch, it's changing the code flow.
 
> 
> On 06/20/2013 07:26 PM, Chen Gang wrote:
> > 
> > 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 <gang.chen@asianux.com>
> > ---
> >  kernel/itimer.c |   13 ++++++-------
> >  1 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/itimer.c b/kernel/itimer.c
> > index 8d262b4..3b12271 100644
> > --- a/kernel/itimer.c
> > +++ b/kernel/itimer.c
> > @@ -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;
> >  }
> >  
> > 
> 
> 
> -- 
> Chen Gang
> 
> Asianux Corporation
> 

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

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

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

> 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

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

[Chen Gang]

On 06/20/2013 08:55 PM, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, Chen Gang wrote:
> 
>> > 
>> > Oh, sorry, maybe it is a trivial patch, also need send to
>> > trivial@kernel.org.
> 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.
-- 
Chen Gang

Asianux Corporation

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

[Chen Gang]

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 ?


On 06/20/2013 09:42 PM, Thomas Gleixner wrote:
> 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.


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


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


>> > 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 ?  ;-)


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


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

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

Asianux Corporation

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

[Chen Gang]

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 <gang.chen@asianux.com>
---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/itimer.c b/kernel/itimer.c
index 8d262b4..3b12271 100644
--- a/kernel/itimer.c
+++ b/kernel/itimer.c
@@ -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;
 }
 
-- 
1.7.7.6

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

[Thomas Gleixner]

On Fri, 21 Jun 2013, Chen Gang wrote:
> >> > 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

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

[Chen Gang]

On 06/25/2013 07:28 AM, Thomas Gleixner wrote:
> On Fri, 21 Jun 2013, Chen Gang wrote:
>>>>> > >> > 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:
> 

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.


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

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

> 
> Please resubmit.

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

Thanks.
-- 
Chen Gang

Asianux Corporation

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

[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 <gang.chen@asianux.com>
---
 kernel/itimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/itimer.c b/kernel/itimer.c
index 8d262b4..3b12271 100644
--- a/kernel/itimer.c
+++ b/kernel/itimer.c
@@ -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;
 }
 
-- 
1.7.7.6

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

[Chen Gang]

Hello Maintainers:

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

Thanks.

On 06/25/2013 09:16 AM, Chen Gang wrote:
> 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 <gang.chen@asianux.com>
> ---
>  kernel/itimer.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/itimer.c b/kernel/itimer.c
> index 8d262b4..3b12271 100644
> --- a/kernel/itimer.c
> +++ b/kernel/itimer.c
> @@ -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;
>  }
>  
> 


-- 
Chen Gang

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

[Chen Gang]

Hello Maintainers:

Please help check this patch, when you have time.

Thanks.

On 07/05/2013 09:28 AM, Chen Gang wrote:
> Hello Maintainers:
> 
> Is this patch under the normal work flow (or I should need a little more
> patience) ?
> 
> Thanks.
> 
> On 06/25/2013 09:16 AM, Chen Gang wrote:
>> 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 <gang.chen@asianux.com>
>> ---
>>  kernel/itimer.c |   13 ++++++-------
>>  1 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/kernel/itimer.c b/kernel/itimer.c
>> index 8d262b4..3b12271 100644
>> --- a/kernel/itimer.c
>> +++ b/kernel/itimer.c
>> @@ -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;
>>  }
>>  
>>
> 
> 


-- 
Chen Gang