From: Markus Elfring <[email protected]>
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
>From 06c1a0fff81142dfa6d933479e17bb1b45ab9dc7 Mon Sep 17 00:00:00 2001
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
On Sun, 2014-11-30 at 17:47 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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
> 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
Hello.
On 11/30/2014 7:44 PM, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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
>> 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
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
From: Markus Elfring <[email protected]>
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
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__);
>> 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
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.
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
>>> 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
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
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.
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.
> 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
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
> 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
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
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 <[email protected]>
> > 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
>> 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
On Thu, Dec 04, 2014 at 11:20:21PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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
On Thu, Dec 04, 2014 at 11:18:41PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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
>> 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
>> 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
... 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 ...
> 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
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.
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
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.
From: SF Markus Elfring <[email protected]>
Date: Thu, 04 Dec 2014 23:03:30 +0100
> From: Markus Elfring <[email protected]>
> 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.
> 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
From: SF Markus Elfring <[email protected]>
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.
> 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
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?
> 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
From: SF Markus Elfring <[email protected]>
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.
>> 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
> 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
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.
From: SF Markus Elfring <[email protected]>
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.
> 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
> 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
>> 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=607ca46e97a1b6594b29647d98a32d545c24bdff#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
From: SF Markus Elfring <[email protected]>
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.
>> 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
> 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
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
> 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
From: SF Markus Elfring <[email protected]>
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.