From: SF Markus Elfring <hidden> Date: 2016-09-24 13:45:19
From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 15:34:15 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
Use kmalloc_array()
Delete an error message for a failed memory allocation
Add the macro "pr_fmt"
Replace some printk() calls by pr_info()
drivers/input/joystick/joydump.c | 46 ++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 23 deletions(-)
--
2.10.0
From: SF Markus Elfring <hidden> Date: 2016-09-24 13:46:28
From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 13:50:20 +0200
* A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".
This issue was detected by using the Coccinelle software.
* Replace the specification of a data structure by a pointer dereference
to make the corresponding size determination a bit safer according to
the Linux coding style convention.
Signed-off-by: Markus Elfring <redacted>
---
drivers/input/joystick/joydump.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -79,8 +79,7 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr}timeout=gameport_time(gameport,10000);/* 10 ms */--buf=kmalloc(BUF_SIZE*sizeof(structjoydump),GFP_KERNEL);+buf=kmalloc_array(BUF_SIZE,sizeof(*buf),GFP_KERNEL);if(!buf){printk(KERN_INFO"joydump: no memory for testing\n");gotojd_end;
@@ -80,10 +80,9 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *drtimeout=gameport_time(gameport,10000);/* 10 ms */buf=kmalloc_array(BUF_SIZE,sizeof(*buf),GFP_KERNEL);-if(!buf){-printk(KERN_INFO"joydump: no memory for testing\n");+if(!buf)gotojd_end;-}+dump=buf;t=0;i=1;
From: SF Markus Elfring <hidden> Date: 2016-09-24 13:49:06
From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 14:30:44 +0200
Add a definition for the macro "pr_fmt" so that its information can be used
for consistent message output.
Signed-off-by: Markus Elfring <redacted>
---
drivers/input/joystick/joydump.c | 2 ++
1 file changed, 2 insertions(+)
From: SF Markus Elfring <hidden> Date: 2016-09-24 13:50:43
From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 15:12:21 +0200
* Prefer usage of the macro "pr_info" over the interface "printk"
in this function.
* Reduce number of output function calls.
Signed-off-by: Markus Elfring <redacted>
---
drivers/input/joystick/joydump.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
Should the desired output be the same when the relevant data are passed by a single function call
(instead of three as before)?
The desired output should not be broken in conversion, which you did. Do
you know how syslog works and why the transformation is not correct.
I am also curious as to why you are patching joydump? Are you working on
extending it?
Thanks.
--
Dmitry
On Sat, Sep 24, 2016 at 03:48:54PM +0200, SF Markus Elfring wrote:
quoted hunk
From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 14:30:44 +0200
Add a definition for the macro "pr_fmt" so that its information can be used
for consistent message output.
Signed-off-by: Markus Elfring <redacted>
---
drivers/input/joystick/joydump.c | 2 ++
1 file changed, 2 insertions(+)
On Sat, Sep 24, 2016 at 03:46:21PM +0200, SF Markus Elfring wrote:
From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 13:50:20 +0200
* A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".
-ENOPARSE.
quoted hunk
This issue was detected by using the Coccinelle software.
* Replace the specification of a data structure by a pointer dereference
to make the corresponding size determination a bit safer according to
the Linux coding style convention.
Signed-off-by: Markus Elfring <redacted>
---
drivers/input/joystick/joydump.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -79,8 +79,7 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr}timeout=gameport_time(gameport,10000);/* 10 ms */--buf=kmalloc(BUF_SIZE*sizeof(structjoydump),GFP_KERNEL);+buf=kmalloc_array(BUF_SIZE,sizeof(*buf),GFP_KERNEL);if(!buf){printk(KERN_INFO"joydump: no memory for testing\n");gotojd_end;
Should the desired output be the same when the relevant data are passed by a single function call
(instead of three as before)?
Adding a singleton for a pr_fmt #define constant string and
updating the printk subsystem to prepend that constant string
to each use of a pr_<level> at runtime would be an improvement
as it could reduce constant data used by the format strings.
That would be a _real_ improvement.
Please try to implement something like that before submitting
more of these incorrect patches.
From: SF Markus Elfring <hidden> Date: 2016-09-24 16:55:54
quoted
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
Why did you split it off from the patch where you attempt to use it?
I chose a special patch granularity once again.
What good does this change do on its own?
I find that it is a preparation. - If this addition could not be accepted,
the following update step would also be discussed under an other perspective,
wouldn't it?
Regards,
Markus
From: Joe Perches <joe@perches.com> Date: 2016-09-24 17:03:17
On Sat, 2016-09-24 at 18:55 +0200, SF Markus Elfring wrote:
quoted
quoted
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Why did you split it off from the patch where you attempt to use it?
I chose a special patch granularity once again.
quoted
What good does this change do on its own?
I find that it is a preparation. - If this addition could not be accepted,
the following update step would also be discussed under an other perspective,
wouldn't it?
It's purposeless, creates unnecessary patches to review
and generally wastes other people's time.
Please don't purposefully waste other people's time.
It makes your patch proposals _less_ likely to be applied.
From: SF Markus Elfring <hidden> Date: 2016-09-24 17:04:26
quoted
* A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".
-ENOPARSE.
Do you find an other wording better?
The script "checkpatch.pl" can point information out like the following.
WARNING: Prefer kmalloc_array over kmalloc with multiply
Regards,
Markus
From: SF Markus Elfring <hidden> Date: 2016-09-24 17:21:03
The desired output should not be broken in conversion, which you did.
I dared to change another source code place a bit too much perhaps.
Do you know how syslog works and why the transformation is not correct.
I imagine that there are chances to improve the software situation a bit more,
aren't there?
I am also curious as to why you are patching joydump?
The discussed function implementation contains update candidates.
One of them was detected by the execution of a script for the semantic patch
language (Coccinelle software).
I identified further update possibilities after the opportunity for using
the function "kmalloc_array" also in this software module.
Are you working on extending it?
I attempted just another software refactoring.
Regards,
Markus
From: SF Markus Elfring <hidden> Date: 2016-09-24 17:45:28
quoted
I find that it is a preparation. - If this addition could not be accepted,
the following update step would also be discussed under an other perspective,
wouldn't it?
It's purposeless, creates unnecessary patches to review
and generally wastes other people's time.
I have got an other opinion about this.
Please don't purposefully waste other people's time.
I do not want to "waste" your time. - But I can imagine that I stress your
software development attention to some degree as I am publishing a significant
number of update suggestions according to a bit of static source code analysis.
It makes your patch proposals _less_ likely to be applied.
The acceptance varies as usual.
I see also another option.
* Can the first three update steps from this small patch series be integrated
while the fourth needs further adjustments (where I went a bit too far)?
* Do you prefer to squash the last two update steps together?
Regards,
Markus
From: Joe Perches <joe@perches.com> Date: 2016-09-24 18:01:12
On Sat, 2016-09-24 at 19:45 +0200, SF Markus Elfring wrote:
quoted
It's purposeless, creates unnecessary patches to review
and generally wastes other people's time.
I have got an other opinion about this.
Nice for you, not nice for others that have to act on
your patch proposals to get them forwarded upstream.
quoted
Please don't purposefully waste other people's time.
I do not want to "waste" your time.
When a chorus of voices says to you that you are wasting
their time, perhaps you listen to their song.
quoted
It makes your patch proposals _less_ likely to be applied.
The acceptance varies as usual.
Usual for whom?
It seems to me your patch proposals have a relatively high
unapplied patch percentage and there is an increase in the
number of upstream maintainers that ignore you.
I see also another option.
* Can the first three update steps from this small patch series be integrated
while the fourth needs further adjustments (where I went a bit too far)?
* Do you prefer to squash the last two update steps together?
Yes, the overall number of patches should be minimized when
the suggested patches are highly related.
From: SF Markus Elfring <hidden> Date: 2016-09-24 18:45:40
When a chorus of voices says to you that you are wasting
their time, perhaps you listen to their song.
I am listening to some degree. - I guess that I am making some decisions
and conclusions in a way that you would prefer to be different.
I know that my update suggestions won't be liked by all contributors.
It seems to me your patch proposals have a relatively high
unapplied patch percentage
I agree here. - This impression can occur for a while.
I find that this situation is hardly avoidable according to my evolving number
of update suggestions.
In which time frames do you look for the corresponding software development process?
and there is an increase in the number of upstream maintainers that ignore you.
It's a pity. - I hope that the corresponding software development can still
be continued in constructive ways.
Yes, the overall number of patches should be minimized when
the suggested patches are highly related.
I have got a special opinion about patch squashing. But I can consider it
also for this update step when a corresponding consensus could be
achieved between contributors.
Regards,
Markus
From: SF Markus Elfring <hidden> Date: 2016-09-24 19:51:05
quoted
Should the desired output be the same when the relevant data are passed by a single function call
(instead of three as before)?
Adding a singleton for a pr_fmt #define constant string and
updating the printk subsystem to prepend that constant string
to each use of a pr_<level> at runtime would be an improvement
as it could reduce constant data used by the format strings.
Thanks for for this constructive feedback.
That would be a _real_ improvement.
This sounds interesting for me too.
Please try to implement something like that before submitting
more of these incorrect patches.
Nice wish!
Did any other software developer try to implement such enhanced
logging functionality already?
Regards,
Markus
From: SF Markus Elfring <hidden> Date: 2016-09-25 07:15:30
From: Markus Elfring <redacted>
Date: Sun, 25 Sep 2016 08:40:34 +0200
1. Add a definition for the macros "MY_LOG_PREFIX" and "pr_fmt" so that
their information can be used for consistent message output.
2. Prefer usage of the macro "pr_info" over the interface "printk"
in this function.
3. Reduce number of output function calls.
Signed-off-by: Markus Elfring <redacted>
---
v2: Yesterday a software development discussion pointed weaknesses out around
the previous update steps "3" and "4".
Now I propose this update variant in the hope that my second approach
for this software module will work as desired and can be accepted
a bit easier.
Will the script "checkpatch.pl" need another improvement so that
the following information can be avoided for my use case anyhow?
ERROR: Macros with complex values should be enclosed in parentheses
drivers/input/joystick/joydump.c | 40 ++++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
From: Joe Perches <joe@perches.com> Date: 2016-09-25 07:31:35
On Sun, 2016-09-25 at 09:15 +0200, SF Markus Elfring wrote:
1. Add a definition for the macros "MY_LOG_PREFIX" and "pr_fmt" so that
their information can be used for consistent message output.
2. Prefer usage of the macro "pr_info" over the interface "printk"
in this function.
3. Reduce number of output function calls.
Signed-off-by: Markus Elfring <redacted>
---
v2: Yesterday a software development discussion pointed weaknesses out around
the previous update steps "3" and "4".
Now I propose this update variant in the hope that my second approach
for this software module will work as desired and can be accepted
a bit easier.
No thank you.
This is not a good change as it messes with dmesg timestamps.
Simpler to read and more straightforward is multiple
individual function calls.