| Message ID | 5480DBDE.7040604@users.sourceforge.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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__); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
... 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 ...
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }