[PATCH 0/3] net-PPP: Deletion of a few unnecessary checks

[PATCH 0/3] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 30 Nov 2014 17:25:40 +0100

Further update suggestions were taken into account after a patch was applied
from static source code analysis.

Markus Elfring (3):
  Deletion of unnecessary checks before the function call "kfree"
  Less function calls in mppe_alloc() after error detection
  Delete an unnecessary assignment

 drivers/net/ppp/ppp_mppe.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

-- 
2.1.3

[PATCH 1/3] net-PPP: Deletion of unnecessary checks before the function call "kfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 30 Nov 2014 17:02:07 +0100

The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 911b216..7e44212 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	return (void *)state;
 
 	out_free:
-	    if (state->sha1_digest)
-		kfree(state->sha1_digest);
+	kfree(state->sha1_digest);
 	    if (state->sha1)
 		crypto_free_hash(state->sha1);
 	    if (state->arc4)
@@ -256,13 +255,12 @@ static void mppe_free(void *arg)
 {
 	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
 	if (state) {
-	    if (state->sha1_digest)
 		kfree(state->sha1_digest);
-	    if (state->sha1)
-		crypto_free_hash(state->sha1);
-	    if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
+		if (state->sha1)
+			crypto_free_hash(state->sha1);
+		if (state->arc4)
+			crypto_free_blkcipher(state->arc4);
+		kfree(state);
 	}
 }
 
-- 
2.1.3

Re: [PATCH 1/3] net-PPP: Deletion of unnecessary checks before the function call "kfree"

[Sergei Shtylyov]

Hello.

On 11/30/2014 7:44 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 30 Nov 2014 17:02:07 +0100

> The kfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.

> This issue was detected by using the Coccinelle software.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/net/ppp/ppp_mppe.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)

> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> index 911b216..7e44212 100644
> --- a/drivers/net/ppp/ppp_mppe.c
> +++ b/drivers/net/ppp/ppp_mppe.c
> @@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
>   	return (void *)state;
>
>   	out_free:
> -	    if (state->sha1_digest)
> -		kfree(state->sha1_digest);
> +	kfree(state->sha1_digest);

    Please keep this line aligned to the others.

>   	    if (state->sha1)
>   		crypto_free_hash(state->sha1);
>   	    if (state->arc4)

[...]

WBR, Sergei

Re: [PATCH 1/3] net-PPP: Deletion of unnecessary checks before the function call "kfree"

[SF Markus Elfring]

>> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
>> index 911b216..7e44212 100644
>> --- a/drivers/net/ppp/ppp_mppe.c
>> +++ b/drivers/net/ppp/ppp_mppe.c
>> @@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
>>       return (void *)state;
>>
>>       out_free:
>> -        if (state->sha1_digest)
>> -        kfree(state->sha1_digest);
>> +    kfree(state->sha1_digest);
> 
>    Please keep this line aligned to the others.

Can it be that the previous source code contained unwanted space
characters at this place?
Do you want indentation fixes as a separate update step?

Regards,
Markus

Re: [PATCH 1/3] net-PPP: Deletion of unnecessary checks before the function call "kfree"

[Sergei Shtylyov]

On 12/01/2014 06:00 PM, SF Markus Elfring wrote:

>>> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
>>> index 911b216..7e44212 100644
>>> --- a/drivers/net/ppp/ppp_mppe.c
>>> +++ b/drivers/net/ppp/ppp_mppe.c
>>> @@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
>>>        return (void *)state;
>>>
>>>        out_free:
>>> -        if (state->sha1_digest)
>>> -        kfree(state->sha1_digest);
>>> +    kfree(state->sha1_digest);

>>     Please keep this line aligned to the others.

> Can it be that the previous source code contained unwanted space
> characters at this place?

    Yes, it seems so.

> Do you want indentation fixes as a separate update step?

    Yes, that would be better to keep it separate.

> Regards,
> Markus

WBR, Sergei

[PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:50:28 +0100

Further update suggestions were taken into account before and after a patch
was applied from static source code analysis.

Markus Elfring (6):
  Replacement of a printk() call by pr_warn() in mppe_rekey()
  Fix indentation
  Deletion of unnecessary checks before the function call "kfree"
  Less function calls in mppe_alloc() after error detection
  Delete an unnecessary assignment in mppe_alloc()
  Delete another unnecessary assignment in mppe_alloc()

 drivers/net/ppp/ppp_mppe.c | 49 +++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

-- 
2.1.3

[PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 18:28:52 +0100

The mppe_rekey() function contained a few update candidates.
* Curly brackets were still used around a single function call "printk".
* Unwanted space characters

Let us improve these implementation details according to the current Linux
coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 911b216..84b7bce 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
 		setup_sg(sg_in, state->sha1_digest, state->keylen);
 		setup_sg(sg_out, state->session_key, state->keylen);
 		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
-					     state->keylen) != 0) {
-    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
-		}
+					     state->keylen) != 0)
+			pr_warn("mppe_rekey: cipher_encrypt failed\n");
 	} else {
 		memcpy(state->session_key, state->sha1_digest, state->keylen);
 	}
-- 
2.1.3

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Joe Perches]

On Thu, 2014-12-04 at 23:10 +0100, SF Markus Elfring wrote:
> The mppe_rekey() function contained a few update candidates.
> * Curly brackets were still used around a single function call "printk".
> * Unwanted space characters
> 
> Let us improve these implementation details according to the current Linux
> coding style convention.

trivia:

> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
[]
> @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
>  		setup_sg(sg_in, state->sha1_digest, state->keylen);
>  		setup_sg(sg_out, state->session_key, state->keylen);
>  		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
> -					     state->keylen) != 0) {
> -    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
> -		}
> +					     state->keylen) != 0)
> +			pr_warn("mppe_rekey: cipher_encrypt failed\n");

It's generally nicer to replace embedded function names
with "%s: ", __func__

			pr_warn("%s: cipher_encrypt failed\n", __func__);

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[SF Markus Elfring]

>> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> []
>> @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
>>  		setup_sg(sg_in, state->sha1_digest, state->keylen);
>>  		setup_sg(sg_out, state->session_key, state->keylen);
>>  		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
>> -					     state->keylen) != 0) {
>> -    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
>> -		}
>> +					     state->keylen) != 0)
>> +			pr_warn("mppe_rekey: cipher_encrypt failed\n");
> 
> It's generally nicer to replace embedded function names
> with "%s: ", __func__
> 
> 			pr_warn("%s: cipher_encrypt failed\n", __func__);

Do you want that I send a third patch series for the fine-tuning of these parameters?

Regards,
Martkus

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Joe Perches]

On Thu, 2014-12-04 at 23:27 +0100, SF Markus Elfring wrote:
> >> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> > []
> >> @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
> >>  		setup_sg(sg_in, state->sha1_digest, state->keylen);
> >>  		setup_sg(sg_out, state->session_key, state->keylen);
> >>  		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
> >> -					     state->keylen) != 0) {
> >> -    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
> >> -		}
> >> +					     state->keylen) != 0)
> >> +			pr_warn("mppe_rekey: cipher_encrypt failed\n");
> > 
> > It's generally nicer to replace embedded function names
> > with "%s: ", __func__
> > 
> > 			pr_warn("%s: cipher_encrypt failed\n", __func__);
> 
> Do you want that I send a third patch series for the fine-tuning of these parameters?

If you want.

I just wanted you to be aware of it for future patches.

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Julia Lawall]

On Thu, 4 Dec 2014, Joe Perches wrote:

> On Thu, 2014-12-04 at 23:27 +0100, SF Markus Elfring wrote:
> > >> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> > > []
> > >> @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
> > >>  		setup_sg(sg_in, state->sha1_digest, state->keylen);
> > >>  		setup_sg(sg_out, state->session_key, state->keylen);
> > >>  		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
> > >> -					     state->keylen) != 0) {
> > >> -    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
> > >> -		}
> > >> +					     state->keylen) != 0)
> > >> +			pr_warn("mppe_rekey: cipher_encrypt failed\n");
> > > 
> > > It's generally nicer to replace embedded function names
> > > with "%s: ", __func__
> > > 
> > > 			pr_warn("%s: cipher_encrypt failed\n", __func__);
> > 
> > Do you want that I send a third patch series for the fine-tuning of these parameters?
> 
> If you want.
> 
> I just wanted you to be aware of it for future patches.

Markus, are you sure that you cannot use netdev_warn in this case?

julia

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[SF Markus Elfring]

> Markus, are you sure that you cannot use netdev_warn in this case?

No. - I did not become familiar enough with the software infrastructure around
the mppe_rekey() function.

I do not see so far how I could determine a pointer for the first parameter there.
The data structure "ppp_mppe_state" (which is referenced by the input variable
"state") does not contain corresponding context information, does it?

Regards,
Markus

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Julia Lawall]

On Fri, 5 Dec 2014, SF Markus Elfring wrote:

> > Markus, are you sure that you cannot use netdev_warn in this case?
>
> No. - I did not become familiar enough with the software infrastructure around
> the mppe_rekey() function.
>
> I do not see so far how I could determine a pointer for the first parameter there.
> The data structure "ppp_mppe_state" (which is referenced by the input variable
> "state") does not contain corresponding context information, does it?

Indeed, I also don't see anything promising.

julia

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[SF Markus Elfring]

>>> It's generally nicer to replace embedded function names
>>> with "%s: ", __func__
>>>
>>> 			pr_warn("%s: cipher_encrypt failed\n", __func__);
>>
>> Do you want that I send a third patch series for the fine-tuning of these parameters?
> 
> If you want.

Would "a committer" fix such a small source code adjustment also without a resend of
a patch series?


> I just wanted you to be aware of it for future patches.

Thanks for your tip.

Does it make sense to express such implementation details in the Linux coding
style documentation more explicitly (besides the fact that this update suggestion
was also triggered by a warning from the script "checkpatch.pl").

Regards,
Markus

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Joe Perches]

On Fri, 2014-12-05 at 08:18 +0100, SF Markus Elfring wrote:
> >>> It's generally nicer to replace embedded function names
> >>> with "%s: ", __func__
> >>>
> >>> 			pr_warn("%s: cipher_encrypt failed\n", __func__);
> >>
> >> Do you want that I send a third patch series for the fine-tuning of these parameters?
> > 
> > If you want.
> 
> Would "a committer" fix such a small source code adjustment also without a resend of
> a patch series?

Depends on the committer.  Some might, most wouldn't.

drivers/net/ppp doesn't have a specific maintainer.

The networking maintainer generally asks for resends
of patches that don't suit his taste, but lots of
non-perfect patches still get applied there.

It's a process, and it's not immediate.  Wait to see
if these get applied as-is.  If the embedded function
name use, which is trivial, bothers you, send another
patch later on that changes it.

> Does it make sense to express such implementation details in the Linux coding
> style documentation more explicitly (besides the fact that this update suggestion
> was also triggered by a warning from the script "checkpatch.pl").

Probably not.

Overly formalized coding style rules are perhaps
more of a barrier to entry than most want.

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[SF Markus Elfring]

> It's a process, and it's not immediate.  Wait to see
> if these get applied as-is.

Thanks for your constructive feedback.


> If the embedded function name use, which is trivial, bothers you,
> send another patch later on that changes it.

Not really at the moment ...

I guess that I would prefer a general development of another semantic
patch approach according to your request with the topic "Finding embedded
function names?" a moment ago.
https://systeme.lip6.fr/pipermail/cocci/2014-December/001517.html
http://article.gmane.org/gmane.comp.version-control.coccinelle/4399

Regards,
Markus

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[terry white]

... ciao:

: on "12-4-2014" "Joe Perches" writ:
: > Does it make sense to express such implementation details in the Linux 
: > coding style documentation more explicitly (besides the fact that this 
: > update suggestion was also triggered by a warning from the script 
: > "checkpatch.pl".
: 
: Probably not.
: 
: Overly formalized coding style rules are perhaps
: more of a barrier to entry than most want.
 
   funny you should mention that.  as nothing more than a casual observer, 
i'm noticing a "TIRED" sensation reading this thread.  i have "0" 
confidence a "SERIOUS" participant's enthusiasm would remain untested.
 
   however, the "checkpatch.pl" warning suggests an assumed 'custom'. i 
can't tell if this a 'serious' issue, or "pickin' fly shit out of pepper".
 
   but from my reading of it, the "CODE" , and the "logic" driving it, is 
not the problem.

    season's best ...

-- 
... it's not what you see ,
    but in stead , notice ...

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Julia Lawall]

On Thu, 4 Dec 2014, Joe Perches wrote:

> On Thu, 2014-12-04 at 23:10 +0100, SF Markus Elfring wrote:
> > The mppe_rekey() function contained a few update candidates.
> > * Curly brackets were still used around a single function call "printk".
> > * Unwanted space characters
> > 
> > Let us improve these implementation details according to the current Linux
> > coding style convention.
> 
> trivia:
> 
> > diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> []
> > @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
> >  		setup_sg(sg_in, state->sha1_digest, state->keylen);
> >  		setup_sg(sg_out, state->session_key, state->keylen);
> >  		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
> > -					     state->keylen) != 0) {
> > -    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
> > -		}
> > +					     state->keylen) != 0)
> > +			pr_warn("mppe_rekey: cipher_encrypt failed\n");
> 
> It's generally nicer to replace embedded function names
> with "%s: ", __func__
> 
> 			pr_warn("%s: cipher_encrypt failed\n", __func__);

Doing so may potentially allow some strings to be shared, thus saving a 
little space.  Perhaps not in this case, though.

julia

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Joe Perches]

On Fri, 2014-12-05 at 08:21 +0100, Julia Lawall wrote:
> On Thu, 4 Dec 2014, Joe Perches wrote:
> > It's generally nicer to replace embedded function names
> > with "%s: ", __func__
> > 
> > 			pr_warn("%s: cipher_encrypt failed\n", __func__);
> 
> Doing so may potentially allow some strings to be shared, thus saving a 
> little space.  Perhaps not in this case, though.

It's not necessarily a code size savings in any case.

It can be, but the real benefits are stylistic
consistency and lack of mismatch between function
name and message.

If the code is refactored or copy/pasted into another
function, a moderately common defect is not modifying
the embedded function name in the message.

There may be some smallish savings if ever these
__func__ uses were converted to use %pf via some
internal standardized mechanism.

A negative to that approach is inlined functions would
take the function name of the parent not keep the
inlined function name.

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Julia Lawall]

> A negative to that approach is inlined functions would
> take the function name of the parent not keep the
> inlined function name.

I tried the following:

#include <stdio.h>

inline int foo() {
  printf("%s %x\n",__func__,0x12345);
}

int main () {
  foo();
}

The assembly code generated for main is:

0000000000400470 <main>:
  400470:       b9 45 23 01 00          mov    $0x12345,%ecx
  400475:       ba 4b 06 40 00          mov    $0x40064b,%edx
  40047a:       be 44 06 40 00          mov    $0x400644,%esi
  40047f:       bf 01 00 00 00          mov    $0x1,%edi
  400484:       31 c0                   xor    %eax,%eax
  400486:       e9 d5 ff ff ff          jmpq   400460 <__printf_chk@plt>

That is, the call to foo seems tom be inlined.

But the output is:

foo 12345

So it seems that __func__ is determined before inlining.

julia

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Joe Perches]

On Sun, 2014-12-07 at 11:44 +0100, Julia Lawall wrote:
> > A negative to that approach is inlined functions would
> > take the function name of the parent not keep the
> > inlined function name.
> 
> I tried the following:
> 
> #include <stdio.h>
> 
> inline int foo() {
>   printf("%s %x\n",__func__,0x12345);
> }
> 
> int main () {
>   foo();
> }
> 
> The assembly code generated for main is:
> 
> 0000000000400470 <main>:
>   400470:       b9 45 23 01 00          mov    $0x12345,%ecx
>   400475:       ba 4b 06 40 00          mov    $0x40064b,%edx
>   40047a:       be 44 06 40 00          mov    $0x400644,%esi
>   40047f:       bf 01 00 00 00          mov    $0x1,%edi
>   400484:       31 c0                   xor    %eax,%eax
>   400486:       e9 d5 ff ff ff          jmpq   400460 <__printf_chk@plt>
> 
> That is, the call to foo seems tom be inlined.
> 
> But the output is:
> 
> foo 12345
> 
> So it seems that __func__ is determined before inlining.

True, and that's what I intended to describe.

If you did that with a kernel module and replaced
"%s, __func__" with "%pf, __builtin_return_address(0)"
when built with kallsyms you should get:

"modname 12345" when most would expect "foo 12345"

when built without kallsyms, that output should be
"<address> 12345"

but the object code should be smaller.

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Julia Lawall]

On Sun, 7 Dec 2014, Joe Perches wrote:

> On Sun, 2014-12-07 at 11:44 +0100, Julia Lawall wrote:
> > > A negative to that approach is inlined functions would
> > > take the function name of the parent not keep the
> > > inlined function name.
> > 
> > I tried the following:
> > 
> > #include <stdio.h>
> > 
> > inline int foo() {
> >   printf("%s %x\n",__func__,0x12345);
> > }
> > 
> > int main () {
> >   foo();
> > }
> > 
> > The assembly code generated for main is:
> > 
> > 0000000000400470 <main>:
> >   400470:       b9 45 23 01 00          mov    $0x12345,%ecx
> >   400475:       ba 4b 06 40 00          mov    $0x40064b,%edx
> >   40047a:       be 44 06 40 00          mov    $0x400644,%esi
> >   40047f:       bf 01 00 00 00          mov    $0x1,%edi
> >   400484:       31 c0                   xor    %eax,%eax
> >   400486:       e9 d5 ff ff ff          jmpq   400460 <__printf_chk@plt>
> > 
> > That is, the call to foo seems tom be inlined.
> > 
> > But the output is:
> > 
> > foo 12345
> > 
> > So it seems that __func__ is determined before inlining.
> 
> True, and that's what I intended to describe.
> 
> If you did that with a kernel module and replaced
> "%s, __func__" with "%pf, __builtin_return_address(0)"
> when built with kallsyms you should get:
> 
> "modname 12345" when most would expect "foo 12345"
> 
> when built without kallsyms, that output should be
> "<address> 12345"
> 
> but the object code should be smaller.

OK.  But the semantic patch is only using __func__ and only in cases where 
the string wanted is similar to the name of the current function, so I 
think it should be OK?

julia

Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()

[Joe Perches]

On Sun, 2014-12-07 at 13:36 +0100, Julia Lawall wrote:
> the semantic patch is only using __func__ and only in cases where 
> the string wanted is similar to the name of the current function, so I 
> think it should be OK?

Yes, it'd be a good thing.

[PATCH v2 2/6] net-PPP: Fix indentation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:15:20 +0100

The implementations of the functions "mppe_alloc" and "mppe_free" contained
unwanted space characters.

Let us improve the indentation according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 84b7bce..b80af29 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -236,15 +236,15 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	return (void *)state;
 
-	out_free:
-	    if (state->sha1_digest)
+out_free:
+	if (state->sha1_digest)
 		kfree(state->sha1_digest);
-	    if (state->sha1)
+	if (state->sha1)
 		crypto_free_hash(state->sha1);
-	    if (state->arc4)
+	if (state->arc4)
 		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
-	out:
+	kfree(state);
+out:
 	return NULL;
 }
 
@@ -255,13 +255,13 @@ static void mppe_free(void *arg)
 {
 	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
 	if (state) {
-	    if (state->sha1_digest)
-		kfree(state->sha1_digest);
-	    if (state->sha1)
-		crypto_free_hash(state->sha1);
-	    if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
+		if (state->sha1_digest)
+			kfree(state->sha1_digest);
+		if (state->sha1)
+			crypto_free_hash(state->sha1);
+		if (state->arc4)
+			crypto_free_blkcipher(state->arc4);
+		kfree(state);
 	}
 }
 
-- 
2.1.3

[PATCH v2 3/6] net-PPP: Deletion of unnecessary checks before the function call "kfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:22:23 +0100

The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index b80af29..94ff216 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -237,8 +237,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	return (void *)state;
 
 out_free:
-	if (state->sha1_digest)
-		kfree(state->sha1_digest);
+	kfree(state->sha1_digest);
 	if (state->sha1)
 		crypto_free_hash(state->sha1);
 	if (state->arc4)
@@ -255,8 +254,7 @@ static void mppe_free(void *arg)
 {
 	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
 	if (state) {
-		if (state->sha1_digest)
-			kfree(state->sha1_digest);
+		kfree(state->sha1_digest);
 		if (state->sha1)
 			crypto_free_hash(state->sha1);
 		if (state->arc4)
-- 
2.1.3

[PATCH v2 4/6] net-PPP: Less function calls in mppe_alloc() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:30:20 +0100

The functions crypto_free_blkcipher((), crypto_free_hash() and kfree() could be
called in some cases by the mppe_alloc() function during error handling even
if the passed data structure element contained still a null pointer.

This implementation detail could be improved by adjustments for jump labels.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 94ff216..c82198f 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -196,11 +196,11 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	if (optlen != CILEN_MPPE + sizeof(state->master_key) ||
 	    options[0] != CI_MPPE || options[1] != CILEN_MPPE)
-		goto out;
+		return NULL;
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (state = NULL)
-		goto out;
+		return NULL;
 
 
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
@@ -212,16 +212,16 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->sha1)) {
 		state->sha1 = NULL;
-		goto out_free;
+		goto out_free_blkcipher;
 	}
 
 	digestsize = crypto_hash_digestsize(state->sha1);
 	if (digestsize < MPPE_MAX_KEY_LEN)
-		goto out_free;
+		goto out_free_hash;
 
 	state->sha1_digest = kmalloc(digestsize, GFP_KERNEL);
 	if (!state->sha1_digest)
-		goto out_free;
+		goto out_free_hash;
 
 	/* Save keys. */
 	memcpy(state->master_key, &options[CILEN_MPPE],
@@ -236,14 +236,12 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	return (void *)state;
 
+out_free_hash:
+	crypto_free_hash(state->sha1);
+out_free_blkcipher:
+	crypto_free_blkcipher(state->arc4);
 out_free:
-	kfree(state->sha1_digest);
-	if (state->sha1)
-		crypto_free_hash(state->sha1);
-	if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
 	kfree(state);
-out:
 	return NULL;
 }
 
-- 
2.1.3

[PATCH v2 5/6] net-PPP: Delete an unnecessary assignment in mppe_alloc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:33:34 +0100

The data structure element "arc4" was assigned a null pointer by the
mppe_alloc() function if a previous function call "crypto_alloc_blkcipher"
failed. This assignment became unnecessary with previous source
code adjustments.

Let us delete it from the affected implementation because the element "arc4"
will not be accessible outside the function after the detected
allocation failure.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index c82198f..b7db4b1 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -204,10 +204,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(state->arc4)) {
-		state->arc4 = NULL;
+	if (IS_ERR(state->arc4))
 		goto out_free;
-	}
 
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->sha1)) {
-- 
2.1.3

Re: [PATCH v2 5/6] net-PPP: Delete an unnecessary assignment in mppe_alloc()

[Dan Carpenter]

On Thu, Dec 04, 2014 at 11:18:41PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 4 Dec 2014 22:33:34 +0100
> 
> The data structure element "arc4" was assigned a null pointer by the
> mppe_alloc() function if a previous function call "crypto_alloc_blkcipher"
> failed.

No.  crypto_alloc_blkcipher() returns error pointers and not NULL.

This patch creates a bug.

regards,
dan carpenter

Re: [PATCH v2 5/6] net-PPP: Delete an unnecessary assignment in mppe_alloc()

[SF Markus Elfring]

>> The data structure element "arc4" was assigned a null pointer by the
>> mppe_alloc() function if a previous function call "crypto_alloc_blkcipher"
>> failed.
> 
> crypto_alloc_blkcipher() returns error pointers and not NULL.

That is true.


> This patch creates a bug.

Please explain: How?

Did you notice that the data structure element "arc4" was reset
to a null pointer if a failure was detected for the function
call "crypto_alloc_blkcipher"?

Do you find this specific assignment still necessary for exception
handling in the implementation of mppe_alloc() function?

Regards,
Markus

Re: [PATCH v2 5/6] net-PPP: Delete an unnecessary assignment in mppe_alloc()

[Dan Carpenter]

On Fri, Dec 05, 2014 at 01:44:30PM +0100, SF Markus Elfring wrote:
> >> The data structure element "arc4" was assigned a null pointer by the
> >> mppe_alloc() function if a previous function call "crypto_alloc_blkcipher"
> >> failed.
> > 
> > crypto_alloc_blkcipher() returns error pointers and not NULL.
> 
> That is true.
> 

Oh.  In that case, I misunderstood what you wrote.  Looking at it now,
this patch is actually ok.

regards,
dan carpenter

Re: [PATCH v2 5/6] net-PPP: Delete an unnecessary assignment in mppe_alloc()

[SF Markus Elfring]

>> That is true.
> 
> In that case, I misunderstood what you wrote.

I find it a bit interesting how this misunderstanding could happen here somehow.


> Looking at it now, this patch is actually ok.

Does that mean that you would like to add any tag like "Acked-by" or "Reviewed-by"
to any of the proposed six update steps?

Regards,
Markus

[PATCH v2 6/6] net-PPP: Delete another unnecessary assignment in mppe_alloc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:42:30 +0100

The data structure element "sha1" was assigned a null pointer by the
mppe_alloc() after a function call "crypto_alloc_hash" failed.
It was determined that this element was not accessed by the implementation
of the crypto_free_blkcipher() function.

Let us delete it from the affected implementation because the element "sha1"
will not be accessible outside the function after the detected
allocation failure.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index b7db4b1..32cb054 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -208,10 +208,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 		goto out_free;
 
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(state->sha1)) {
-		state->sha1 = NULL;
+	if (IS_ERR(state->sha1))
 		goto out_free_blkcipher;
-	}
 
 	digestsize = crypto_hash_digestsize(state->sha1);
 	if (digestsize < MPPE_MAX_KEY_LEN)
-- 
2.1.3

Re: [PATCH v2 6/6] net-PPP: Delete another unnecessary assignment in mppe_alloc()

[Dan Carpenter]

On Thu, Dec 04, 2014 at 11:20:21PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 4 Dec 2014 22:42:30 +0100
> 
> The data structure element "sha1" was assigned a null pointer by the
> mppe_alloc() after a function call "crypto_alloc_hash" failed.

This patch is also buggy.

regards,
dan carpenter

Re: [PATCH v2 6/6] net-PPP: Delete another unnecessary assignment in mppe_alloc()

[SF Markus Elfring]

>> The data structure element "sha1" was assigned a null pointer by the
>> mppe_alloc() after a function call "crypto_alloc_hash" failed.
> 
> This patch is also buggy.

Do you really want to keep a variable reset after a detected failure?

Regards,
Markus

Re: [PATCH v2 6/6] net-PPP: Delete another unnecessary assignment in mppe_alloc()

[Dan Carpenter]

On Fri, Dec 05, 2014 at 03:23:15PM +0300, Dan Carpenter wrote:
> On Thu, Dec 04, 2014 at 11:20:21PM +0100, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Thu, 4 Dec 2014 22:42:30 +0100
> > 
> > The data structure element "sha1" was assigned a null pointer by the
> > mppe_alloc() after a function call "crypto_alloc_hash" failed.
> 
> This patch is also buggy.

Actually it's ok.  I was just confused by the changelog.

Sorry about that, Markus.

regards,
dan carpenter

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[David Miller]

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 04 Dec 2014 23:03:30 +0100

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 4 Dec 2014 22:50:28 +0100
> 
> Further update suggestions were taken into account before and after a patch
> was applied from static source code analysis.

Generally speaking, it is advisable to not leave error pointers in data
structures, even if they are about to be free'd up in an error path
anyways.

Therefore I do not like some of the patches in this series.

Sorry.

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

> Generally speaking, it is advisable to not leave error pointers in data
> structures, even if they are about to be free'd up in an error path anyways.
>
> Therefore I do not like some of the patches in this series.

Can you give any more concrete feedback here?

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[David Miller]

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 12 Dec 2014 08:01:54 +0100

>> Generally speaking, it is advisable to not leave error pointers in data
>> structures, even if they are about to be free'd up in an error path anyways.
>>
>> Therefore I do not like some of the patches in this series.
> 
> Can you give any more concrete feedback here?

I gave you very concrete feedback, I said exactly that I don't want
error pointers left in data structure members.

I cannot describe my requirements any more precisely than that.

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

> I gave you very concrete feedback, I said exactly that I don't want
> error pointers left in data structure members.

I find that your critique affects the proposed update steps four to six,
doesn't it?
Are the other steps acceptable in principle?


> I cannot describe my requirements any more precisely than that.

I hope that a bit more constructive suggestions will be contributed by
involved
software developers around the affected source code. Now it seems
that a small code clean-up becomes a more challenging development task.

How do you prefer to redesign corresponding data structures eventually?

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[David Miller]

You are asking me to invest a lot of time for a very trivial
set of changes.

Why don't you just integrate the feedback you were given and
resubmit your patch series, just like any other developer would?

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

> You are asking me to invest a lot of time for a very trivial
> set of changes.

I find the proposed six update steps also trivial so far.


> Why don't you just integrate the feedback you were given and
> resubmit your patch series, just like any other developer would?

I find the requested redesign and reorganisation of involved data structures
not so easy and therefore more challenging at the moment.
Where should "the error pointers" be stored instead?
Is such a software improvement even a kind of add-on to my patch series?

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[David Miller]

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 12 Dec 2014 17:56:36 +0100

> Where should "the error pointers" be stored instead?

A local variable, before you assign it into the datastructure.

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

>> Where should "the error pointers" be stored instead?
> A local variable, before you assign it into the datastructure.

Will it be acceptable for you that anyone (or even me) will introduce
such a change with a seventh (and eventually eighth) update step here?
Do you want any other sequence for source code preparation of
the requested software improvement?

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[Eric Dumazet]

On Fri, 2014-12-12 at 18:22 +0100, SF Markus Elfring wrote:

> Will it be acceptable for you that anyone (or even me) will introduce
> such a change with a seventh (and eventually eighth) update step here?
> Do you want any other sequence for source code preparation of
> the requested software improvement?

The thing is : We are in the merge window, tracking bugs added in latest
dev cycle.

Having to deal with patches like yours is adding pressure
on the maintainer (and other developers) at the wrong time.

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

> We are in the merge window, tracking bugs added in latest dev cycle.

I am also curious on the software evolution about how many improvements will
arrive in the next Linux versions.


> Having to deal with patches like yours is adding pressure
> on the maintainer (and other developers) at the wrong time.

You can relax a bit eventually. More merge windows will follow, won't they?

It will be nice if a bunch of recent code clean-ups which were also
triggered
by static source code analysis will be integrated into Linux 3.19 already.
More update suggestions will be considered later again as usual.

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[David Miller]

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 12 Dec 2014 18:22:48 +0100

>>> Where should "the error pointers" be stored instead?
>> A local variable, before you assign it into the datastructure.
> 
> Will it be acceptable for you that anyone (or even me) will introduce
> such a change with a seventh (and eventually eighth) update step here?
> Do you want any other sequence for source code preparation of
> the requested software improvement?

I'd like to honestly ask why you are being so difficult?

Everyone gets their code reviewed, everyone has to modify their
changes to adhere to the subsystem maintainer's wishes.  You
are not being treated specially, and quite frankly nobody is
asking anything unreasonable of you.

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

> I'd like to honestly ask why you are being so difficult?

There are several factors which contribute to your perception of
difficulty here.

1. I try to extract from every feedback the information about the amount
of acceptance or rejection for a specific update suggestion.
   A terse feedback (like yours for this issue) makes it occasionally
harder to see the next useful steps. So another constructive discussion
is evolving around the clarification of some implementation details.

2. I prefer also different communication styles at some points.

3. I reached a point where the desired software updates were not
immediately obvious for me while other contributors might have achieved
a better understanding for the affected issues already.

4. I am on the way at the moment to get my Linux software development
system running again.
  
https://forums.opensuse.org/showthread.php/503327-System-startup-does-not-continue-after-hard-disk-detection


> Everyone gets their code reviewed, everyone has to modify their
> changes to adhere to the subsystem maintainer's wishes.

That is fine as usual.


> You are not being treated specially, and quite frankly nobody
> is asking anything unreasonable of you.

That is also true as the software development process will be continued.

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

>> Where should "the error pointers" be stored instead?
> 
> A local variable, before you assign it into the datastructure.

I have looked at the affected software infrastructure once more.
Now I find still that your data reorgansisation wish can not be resolved
in a simple way.

I imagine that your update suggestion would mean that the corresponding
pointers will be passed around by function parameters instead,
wouldn't it?

Two pointers were stored as the members "arc4" and "sha1" of the
data structure "ppp_mppe_state" for a specific reason. A pointer to
this structure is passed to the ppp_register_compressor() function.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/linux/ppp-comp.h?id`7ca46e97a1b6594b29647d98a32d545c24bdff#n32

The data structure "compressor" manages some function pointers.
I assume that this interface should not be changed at the moment, should it?

Are further ideas needed here?

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[David Miller]

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Dec 2014 18:23:08 +0100

>>> Where should "the error pointers" be stored instead?
>> 
>> A local variable, before you assign it into the datastructure.
> 
> I have looked at the affected software infrastructure once more.
> Now I find still that your data reorgansisation wish can not be resolved
> in a simple way.

I'm saying to leave the code alone.

If it goes:

	var = foo_that_returns_ptr_err()
	if (IS_ERR(var))
		return PTR_ERR(var);

	p->bar = var;

or whatever, simply keep it that way!

I'm not engaging in this conversation any further, you have already
consumed way too much of my limited time on this incredibly trivial
matter.

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

>> Now I find still that your data reorgansisation wish can not be resolved
>> in a simple way.
> 
> I'm saying to leave the code alone.

It seems that there might be a misunderstanding between us.


> If it goes:
> 
> 	var = foo_that_returns_ptr_err()
> 	if (IS_ERR(var))
> 		return PTR_ERR(var);
> 
> 	p->bar = var;
> 
> or whatever, simply keep it that way!

A simple return was not used by the mppe_alloc() function so far because
a bit of memory clean-up will also be useful after error detection,
won't it?


> I'm not engaging in this conversation any further, you have already
> consumed way too much of my limited time on this incredibly trivial matter.

It can occasionally happen that a safe clarification of specific implementation
details will need more efforts than you would like to invest at the moment.

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

> I'm saying to leave the code alone.

Do I need to try another interpretation out for your feedback?


> If it goes:
> 
> 	var = foo_that_returns_ptr_err()
> 	if (IS_ERR(var))
> 		return PTR_ERR(var);
> 
> 	p->bar = var;
> 
> or whatever, simply keep it that way!

Do you want to express here that a data structure member should
only be set after a previous function call succeeded?



> I'm not engaging in this conversation any further, you have
> already consumed way too much of my limited time on this
> incredibly trivial matter.

I hope that you will find a bit time and patience again
to clarify affected implementation details in a safer and
unambiguous way.

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[Lino Sanfilippo]

Hi Markus,

On 20.12.2014 15:45, SF Markus Elfring wrote:
>> I'm saying to leave the code alone.
> 
> Do I need to try another interpretation out for your feedback?
> 
> 
>> If it goes:
>> 
>> 	var = foo_that_returns_ptr_err()
>> 	if (IS_ERR(var))
>> 		return PTR_ERR(var);
>> 
>> 	p->bar = var;
>> 
>> or whatever, simply keep it that way!
> 
> Do you want to express here that a data structure member should
> only be set after a previous function call succeeded?
> 

I think what David said was pretty clear: If you see code like the above
there is no need to refactor it. That does not mean that this is the
_preferred_ way of error handling. Its just good enough to be left alone.

Regards,
Lino

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[SF Markus Elfring]

> I think what David said was pretty clear: If you see code like the above
> there is no need to refactor it.

I can understand this view in principle.


> That does not mean that this is the _preferred_ way of error handling.

Can your feedback help in the clarification of suggestions around
my update steps one to six for this Linux software module?

Regards,
Markus

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[David Miller]

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Dec 2014 15:45:32 +0100

> I hope that you will find a bit time and patience again
> to clarify affected implementation details in a safer and
> unambiguous way.

Sorry, another developer will have to hold your hand, as I
said I already invested too much time into this.

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[Julia Lawall]

> I hope that a bit more constructive suggestions will be contributed by
> involved
> software developers around the affected source code. Now it seems
> that a small code clean-up becomes a more challenging development task.

This is often the case.  Doing something half way is not useful.

julia

Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks

[terry white]

... ciao:

: on "12-12-2014" "SF Markus Elfring" writ:

:> I don't want error pointers left in data structure members.
:> I cannot describe my requirements any more precisely than that.

    to me, "error pointers 'left' in data structure members", seems a
little cavalier.  objection to their existence tends to suggest that not
a function of the design, or logic involved.

 
: I hope that a bit more constructive suggestions will be contributed by
: involved software developers around the affected source code. 
 
    so, this 'error pointers' issue fails to meet your 'constructive' 
suggestion threshold.  to my mind, what i choose to ignore, imposes a 
dangerous minimum. what are you willing to ignore.


: Now it seems that a small code clean-up becomes a more challenging 
: development task.
 
    first, if you've EVER dealt with imperfect code, that should be a 
given.  but more importantly to me, i never knew "code clean-up", wasn't 
"REQUIRED" ...


-- 
... it's not what you see ,
    but in stead , notice ...

[PATCH 2/3] net-PPP: Less function calls in mppe_alloc() after error detection

[SF Markus Elfring]

From 06c1a0fff81142dfa6d933479e17bb1b45ab9dc7 Mon Sep 17 00:00:00 2001
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 30 Nov 2014 17:07:34 +0100

The functions crypto_free_blkcipher((), crypto_free_hash() and kfree() could be
called in some cases by the mppe_alloc() function during error handling even
if the passed data structure element contained still a null pointer.
This implementation detail could be improved by adjustments for jump labels.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 7e44212..962c1a0 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -197,11 +197,11 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	if (optlen != CILEN_MPPE + sizeof(state->master_key) ||
 	    options[0] != CI_MPPE || options[1] != CILEN_MPPE)
-		goto out;
+		return NULL;
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (state = NULL)
-		goto out;
+		return NULL;
 
 
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
@@ -213,16 +213,16 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->sha1)) {
 		state->sha1 = NULL;
-		goto out_free;
+		goto out_free_blkcipher;
 	}
 
 	digestsize = crypto_hash_digestsize(state->sha1);
 	if (digestsize < MPPE_MAX_KEY_LEN)
-		goto out_free;
+		goto out_free_hash;
 
 	state->sha1_digest = kmalloc(digestsize, GFP_KERNEL);
 	if (!state->sha1_digest)
-		goto out_free;
+		goto out_free_hash;
 
 	/* Save keys. */
 	memcpy(state->master_key, &options[CILEN_MPPE],
@@ -237,14 +237,12 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	return (void *)state;
 
-	out_free:
-	kfree(state->sha1_digest);
-	    if (state->sha1)
-		crypto_free_hash(state->sha1);
-	    if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
-	out:
+out_free_hash:
+	crypto_free_hash(state->sha1);
+out_free_blkcipher:
+	crypto_free_blkcipher(state->arc4);
+out_free:
+	kfree(state);
 	return NULL;
 }
 
-- 
2.1.3

[PATCH 3/3] net-PPP: Delete an unnecessary assignment

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 30 Nov 2014 17:17:36 +0100

The data structure element "arc4" was assigned a null pointer by the
mppe_alloc() function if a previous function call "crypto_alloc_blkcipher"
failed. This assignment became unnecessary with previous source
code adjustments.
Let us delete it from the affected implementation because the element "arc4"
will not be accessible outside the function after the detected
allocation failure.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ppp/ppp_mppe.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 962c1a0..d913bc9 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -205,10 +205,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(state->arc4)) {
-		state->arc4 = NULL;
+	if (IS_ERR(state->arc4))
 		goto out_free;
-	}
 
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->sha1)) {
-- 
2.1.3

Re: [PATCH 3/3] net-PPP: Delete an unnecessary assignment

[Eric Dumazet]

On Sun, 2014-11-30 at 17:47 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 30 Nov 2014 17:17:36 +0100
> 
> The data structure element "arc4" was assigned a null pointer by the
> mppe_alloc() function if a previous function call "crypto_alloc_blkcipher"
> failed. This assignment became unnecessary with previous source
> code adjustments.
> Let us delete it from the affected implementation because the element "arc4"
> will not be accessible outside the function after the detected
> allocation failure.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/ppp/ppp_mppe.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> index 962c1a0..d913bc9 100644
> --- a/drivers/net/ppp/ppp_mppe.c
> +++ b/drivers/net/ppp/ppp_mppe.c
> @@ -205,10 +205,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
>  
> 
>  	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
> -	if (IS_ERR(state->arc4)) {
> -		state->arc4 = NULL;
> +	if (IS_ERR(state->arc4))
>  		goto out_free;
> -	}
>  
>  	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
>  	if (IS_ERR(state->sha1)) {

I have no idea why its a patch on its own, and why state->arc4 gets
special treatment while state->sha1 does not.

This definitely belongs to the previous patch, refactoring error
handling in mppe_alloc()

Also, it seems your patches do not fix bugs, so so you need to target
net-next tree, as explained in Documentation/networking/netdev-FAQ.txt

Re: [PATCH 3/3] net-PPP: Delete an unnecessary assignment

[SF Markus Elfring]

> I have no idea why its a patch on its own, and why state->arc4 gets
> special treatment while state->sha1 does not.

I did not fiddle with the data structure element "sha1" because
I assumed that it might be still relevant for the call of a function
like crypto_free_blkcipher().


> This definitely belongs to the previous patch, refactoring error
> handling in mppe_alloc()

I have got different preferences for change distribution over several
patches here.
I find it safer to tackle an assignment clean-up after adjustments
for jump labels.


> Also, it seems your patches do not fix bugs, so so you need to target
> net-next tree, as explained in Documentation/networking/netdev-FAQ.txt

Do you want that I resend my update suggestion?

Regards,
Markus