From: SF Markus Elfring <hidden> Date: 2017-10-16 17:31:09
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 19:12:34 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
Delete an error message for a failed memory allocation
in tpm_ascii_bios_measurements_show()
Delete an error message for a failed memory allocation in tpm_ibmvtpm_probe()
Improve a size determination in nine functions
Less checks in tpm_ibmvtpm_probe() after error detection
drivers/char/tpm/st33zp24/i2c.c | 3 +--
drivers/char/tpm/st33zp24/spi.c | 3 +--
drivers/char/tpm/st33zp24/st33zp24.c | 3 +--
drivers/char/tpm/tpm1_eventlog.c | 5 +----
drivers/char/tpm/tpm_crb.c | 2 +-
drivers/char/tpm/tpm_i2c_atmel.c | 2 +-
drivers/char/tpm/tpm_i2c_nuvoton.c | 2 +-
drivers/char/tpm/tpm_ibmvtpm.c | 23 +++++++++--------------
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_spi.c | 3 +--
10 files changed, 18 insertions(+), 30 deletions(-)
--
2.14.2
From: SF Markus Elfring <hidden> Date: 2017-10-16 17:32:22
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 17:43:55 +0200
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <redacted>
---
drivers/char/tpm/tpm1_eventlog.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: SF Markus Elfring <hidden> Date: 2017-10-16 17:33:19
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 18:08:23 +0200
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <redacted>
---
drivers/char/tpm/tpm_ibmvtpm.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: SF Markus Elfring <hidden> Date: 2017-10-16 17:34:24
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 18:28:17 +0200
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <redacted>
---
drivers/char/tpm/st33zp24/i2c.c | 3 +--
drivers/char/tpm/st33zp24/spi.c | 3 +--
drivers/char/tpm/st33zp24/st33zp24.c | 3 +--
drivers/char/tpm/tpm_crb.c | 2 +-
drivers/char/tpm/tpm_i2c_atmel.c | 2 +-
drivers/char/tpm/tpm_i2c_nuvoton.c | 2 +-
drivers/char/tpm/tpm_ibmvtpm.c | 2 +-
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_spi.c | 3 +--
9 files changed, 9 insertions(+), 13 deletions(-)
From: SF Markus Elfring <hidden> Date: 2017-10-16 17:35:18
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 19:00:34 +0200
Two pointer checks could be repeated by the tpm_ibmvtpm_probe() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.
* Return directly after a call of the function "kzalloc" failed
at the beginning.
* Adjust jump targets so that extra checks can be omitted at the end.
Signed-off-by: Markus Elfring <redacted>
---
drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
@@ -610,7 +610,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,ibmvtpm=kzalloc(sizeof(*ibmvtpm),GFP_KERNEL);if(!ibmvtpm)-gotocleanup;+return-ENOMEM;ibmvtpm->dev=dev;ibmvtpm->vdev=vio_dev;
@@ -619,7 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,crq_q->crq_addr=(structibmvtpm_crq*)get_zeroed_page(GFP_KERNEL);if(!crq_q->crq_addr){dev_err(dev,"Unable to allocate memory for crq_addr\n");-gotocleanup;+gotofree_tpm;}crq_q->num_entry=CRQ_RES_BUF_SIZE/sizeof(*crq_q->crq_addr);
From: Jarkko Sakkinen <hidden> Date: 2017-10-16 18:31:54
On Mon, Oct 16, 2017 at 07:30:13PM +0200, SF Markus Elfring wrote:
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 19:12:34 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
Delete an error message for a failed memory allocation
in tpm_ascii_bios_measurements_show()
Delete an error message for a failed memory allocation in tpm_ibmvtpm_probe()
Improve a size determination in nine functions
Less checks in tpm_ibmvtpm_probe() after error detection
drivers/char/tpm/st33zp24/i2c.c | 3 +--
drivers/char/tpm/st33zp24/spi.c | 3 +--
drivers/char/tpm/st33zp24/st33zp24.c | 3 +--
drivers/char/tpm/tpm1_eventlog.c | 5 +----
drivers/char/tpm/tpm_crb.c | 2 +-
drivers/char/tpm/tpm_i2c_atmel.c | 2 +-
drivers/char/tpm/tpm_i2c_nuvoton.c | 2 +-
drivers/char/tpm/tpm_ibmvtpm.c | 23 +++++++++--------------
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_spi.c | 3 +--
10 files changed, 18 insertions(+), 30 deletions(-)
--
2.14.2
For some sparse errors I fixed a while ago I got review feedback that
one should explain what is wrong what the fix does and not tell tool
reported. And it really does make sense to me.
Describing the tool that was used to find the issues fits to the cover
letter but not to the commits themselves.
I think I recently accepted a small fix with a "tool generated commit
message" but I don't want to take it as a practice It was a minor
mistake from my side to accept such patch.
/Jarkko
From: Jarkko Sakkinen <hidden> Date: 2017-10-16 18:35:22
On Mon, Oct 16, 2017 at 09:31:39PM +0300, Jarkko Sakkinen wrote:
On Mon, Oct 16, 2017 at 07:30:13PM +0200, SF Markus Elfring wrote:
quoted
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 19:12:34 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
Delete an error message for a failed memory allocation
in tpm_ascii_bios_measurements_show()
Delete an error message for a failed memory allocation in tpm_ibmvtpm_probe()
Improve a size determination in nine functions
Less checks in tpm_ibmvtpm_probe() after error detection
drivers/char/tpm/st33zp24/i2c.c | 3 +--
drivers/char/tpm/st33zp24/spi.c | 3 +--
drivers/char/tpm/st33zp24/st33zp24.c | 3 +--
drivers/char/tpm/tpm1_eventlog.c | 5 +----
drivers/char/tpm/tpm_crb.c | 2 +-
drivers/char/tpm/tpm_i2c_atmel.c | 2 +-
drivers/char/tpm/tpm_i2c_nuvoton.c | 2 +-
drivers/char/tpm/tpm_ibmvtpm.c | 23 +++++++++--------------
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_spi.c | 3 +--
10 files changed, 18 insertions(+), 30 deletions(-)
--
2.14.2
For some sparse errors I fixed a while ago I got review feedback that
one should explain what is wrong what the fix does and not tell tool
reported. And it really does make sense to me.
Describing the tool that was used to find the issues fits to the cover
letter but not to the commits themselves.
I think I recently accepted a small fix with a "tool generated commit
message" but I don't want to take it as a practice It was a minor
mistake from my side to accept such patch.
A minor complaint: all commits are missing "Fixes:" tag.
/Jarkko
From: SF Markus Elfring <hidden> Date: 2017-10-16 20:45:03
A minor complaint: all commits are missing "Fixes:" tag.
* Do you require it to be added to the commit messages?
* Would you like to get a finer patch granularity then?
* Do you find any more information missing?
Regards,
Markus
From: Julia Lawall <hidden> Date: 2017-10-17 08:57:41
On Tue, 17 Oct 2017, Dan Carpenter wrote:
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
quoted
A minor complaint: all commits are missing "Fixes:" tag.
Fixes is only for bug fixes. These don't fix any bugs.
0-day seems to put Fixes for everything. Should they be removed when the
old code is undesirable but doesn't actually cause a crash, eg out of date
API.
julia
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Dan Carpenter <hidden> Date: 2017-10-17 09:46:06
On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
On Tue, 17 Oct 2017, Dan Carpenter wrote:
quoted
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
quoted
A minor complaint: all commits are missing "Fixes:" tag.
Fixes is only for bug fixes. These don't fix any bugs.
0-day seems to put Fixes for everything. Should they be removed when the
old code is undesirable but doesn't actually cause a crash, eg out of date
API.
Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
regards,
dan carpenter
From: Julia Lawall <hidden> Date: 2017-10-17 10:12:00
On Tue, 17 Oct 2017, Dan Carpenter wrote:
On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
quoted
On Tue, 17 Oct 2017, Dan Carpenter wrote:
quoted
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
quoted
A minor complaint: all commits are missing "Fixes:" tag.
Fixes is only for bug fixes. These don't fix any bugs.
0-day seems to put Fixes for everything. Should they be removed when the
old code is undesirable but doesn't actually cause a crash, eg out of date
API.
Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
OK, I will remove them from the patches that go through me where they
don't seem appropriate.
thanks,
julia
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-10-17 11:09:10
On Mon, 2017-10-16 at 19:33 +0200, SF Markus Elfring wrote:
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 18:28:17 +0200
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
At the end it's Jarkko's call, though I would NAK this as I think some
one already told this to you for some other similar patch(es).
I even would suggest to stop doing this noisy stuff, which keeps people
busy for nothing.
quoted hunk
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <redacted>
---
drivers/char/tpm/st33zp24/i2c.c | 3 +--
drivers/char/tpm/st33zp24/spi.c | 3 +--
drivers/char/tpm/st33zp24/st33zp24.c | 3 +--
drivers/char/tpm/tpm_crb.c | 2 +-
drivers/char/tpm/tpm_i2c_atmel.c | 2 +-
drivers/char/tpm/tpm_i2c_nuvoton.c | 2 +-
drivers/char/tpm/tpm_ibmvtpm.c | 2 +-
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_spi.c | 3 +--
9 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/char/tpm/st33zp24/i2c.c
b/drivers/char/tpm/st33zp24/i2c.c
index be5d1abd3e8e..d0cb25688485 100644
Hi Julia,
On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote:
On Tue, 17 Oct 2017, Dan Carpenter wrote:
quoted
On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
quoted
On Tue, 17 Oct 2017, Dan Carpenter wrote:
quoted
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
quoted
A minor complaint: all commits are missing "Fixes:" tag.
Fixes is only for bug fixes. These don't fix any bugs.
0-day seems to put Fixes for everything. Should they be removed when the
old code is undesirable but doesn't actually cause a crash, eg out of date
API.
Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
OK, I will remove them from the patches that go through me where they
don't seem appropriate.
The "Fixes" tag is an indication that the patch should be backported.
The requirements for what should be backported are pretty stringent.
Mimi
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-10-17 12:26:52
Dan Carpenter [off-list ref] writes:
On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
quoted
On Tue, 17 Oct 2017, Dan Carpenter wrote:
quoted
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
quoted
A minor complaint: all commits are missing "Fixes:" tag.
Fixes is only for bug fixes. These don't fix any bugs.
0-day seems to put Fixes for everything. Should they be removed when the
old code is undesirable but doesn't actually cause a crash, eg out of date
API.
Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
I try to use the criteria of "if someone had backported commit A, would
they also want commit B" (where B Fixes: A).
So it's a bit broader than just "A had a *bug*" and this is the fix.
That's obviously still a bit of a slippery slope, but somewhat helpful I
think. eg, pretty much no one is interested in backporting spelling
fixes, so those aren't Fixes.
And generally people aren't interested in backporting commits like these
ones that just update coding style.
cheers
On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
wrote:
quoted
quoted
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
I actually prefer that style, so I'd welcome this change :)
Style changes should be reviewed and documented, like any other code
change, and added to Documentation/process/coding-style.rst or an
equivalent file.
quoted
At the end it's Jarkko's call, though I would NAK this as I think some
one already told this to you for some other similar patch(es).
I even would suggest to stop doing this noisy stuff, which keeps people
busy for nothing.
Cleaning up old code is also worth something, even if does not
change one bit in the assembly output in the end...
Wow, you're opening the door really wide for all sorts of trivial
changes! Hope you have the time and inclination to review and comment
on all of them. I certainly don't.
There is a major difference between adding these sorts of checks to
the tools in the scripts directory or even to the zero day bots that
catch different sorts of errors, BEFORE code is upstreamed, and
patches like these, after the fact.
After the code has been upstreamed, it is a lot more difficult to
justify changes like this. It impacts both code that is being
developed AND backporting bug fixes.
Mimi
From: Julia Lawall <hidden> Date: 2017-10-17 12:58:25
On Tue, 17 Oct 2017, Mimi Zohar wrote:
On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
wrote:
quoted
quoted
quoted
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
I actually prefer that style, so I'd welcome this change :)
Style changes should be reviewed and documented, like any other code
change, and added to Documentation/process/coding-style.rst or an
equivalent file.
Actually, it has been there for many years:
14) Allocating memory
---------------------
...
The preferred form for passing a size of a struct is the following:
.. code-block:: c
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.
julia
quoted
quoted
At the end it's Jarkko's call, though I would NAK this as I think some
one already told this to you for some other similar patch(es).
I even would suggest to stop doing this noisy stuff, which keeps people
busy for nothing.
Cleaning up old code is also worth something, even if does not
change one bit in the assembly output in the end...
Wow, you're opening the door really wide for all sorts of trivial
changes! Hope you have the time and inclination to review and comment
on all of them. I certainly don't.
There is a major difference between adding these sorts of checks to
the tools in the scripts directory or even to the zero day bots that
catch different sorts of errors, BEFORE code is upstreamed, and
patches like these, after the fact.
After the code has been upstreamed, it is a lot more difficult to
justify changes like this. It impacts both code that is being
developed AND backporting bug fixes.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-10-17 13:02:35
On Tue, 2017-10-17 at 08:52 -0400, Mimi Zohar wrote:
On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
wrote:
quoted
quoted
quoted
Replace the specification of data structures by pointer
dereferences
as the parameter for the operator "sizeof" to make the
corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
I actually prefer that style, so I'd welcome this change :)
Style changes should be reviewed and documented, like any other code
change, and added to Documentation/process/coding-style.rst or an
equivalent file.
+1.
quoted
quoted
At the end it's Jarkko's call, though I would NAK this as I think
some
one already told this to you for some other similar patch(es).
I even would suggest to stop doing this noisy stuff, which keeps
people
busy for nothing.
Cleaning up old code is also worth something, even if does not
change one bit in the assembly output in the end...
Wow, you're opening the door really wide for all sorts of trivial
changes! Hope you have the time and inclination to review and comment
on all of them. I certainly don't.
Moreover and not so obvious is an open door for making back port of
*real* fixes much harder!
There is a major difference between adding these sorts of checks to
the tools in the scripts directory or even to the zero day bots that
catch different sorts of errors, BEFORE code is upstreamed, and
patches like these, after the fact.
+1.
After the code has been upstreamed, it is a lot more difficult to
justify changes like this. It impacts both code that is being
developed AND backporting bug fixes.
--
Andy Shevchenko [off-list ref]
Intel Finland Oy
On Tue, 2017-10-17 at 14:58 +0200, Julia Lawall wrote:
On Tue, 17 Oct 2017, Mimi Zohar wrote:
quoted
On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
wrote:
quoted
quoted
quoted
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
I actually prefer that style, so I'd welcome this change :)
Style changes should be reviewed and documented, like any other code
change, and added to Documentation/process/coding-style.rst or an
equivalent file.
Actually, it has been there for many years:
14) Allocating memory
---------------------
...
The preferred form for passing a size of a struct is the following:
.. code-block:: c
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.
True, thanks for the reminder. Is this common in new code? Is there
a script/ or some other automated way of catching this usage before
patches are upstreamed?
Just as you're doing here, the patch description should reference this
in the patch description.
Mimi
From: Julia Lawall <hidden> Date: 2017-10-17 15:29:27
On Tue, 17 Oct 2017, Mimi Zohar wrote:
On Tue, 2017-10-17 at 14:58 +0200, Julia Lawall wrote:
quoted
On Tue, 17 Oct 2017, Mimi Zohar wrote:
quoted
On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
wrote:
quoted
quoted
quoted
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
I actually prefer that style, so I'd welcome this change :)
Style changes should be reviewed and documented, like any other code
change, and added to Documentation/process/coding-style.rst or an
equivalent file.
Actually, it has been there for many years:
14) Allocating memory
---------------------
...
The preferred form for passing a size of a struct is the following:
.. code-block:: c
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.
True, thanks for the reminder. Is this common in new code? Is there
a script/ or some other automated way of catching this usage before
patches are upstreamed?
Just as you're doing here, the patch description should reference this
in the patch description.
The comment in the documentation seems have been there since Linux 2.6.14,
ie 2005. The fact that a lot of code still doesn't use that style, 12
years later, suggests that actually it is not preferred, or not preferred
by everyone. Perhaps the paragraph in coding style should just be
dropped.
julia
From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: 2017-10-17 15:57:18
On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
quoted
Fixes is only for bug fixes. These don't fix any bugs.
How do you distinguish these in questionable source code
from other error categories or software weaknesses?
A style change is one that doesn't change the effect of the execution.
These don't actually even change the assembly, so there's programmatic
proof they're not fixing anything.
Bug means potentially user visible fault. In any bug fix commit you
should document the fault and its effects on users so those backporting
can decide if they care or not.
James
From: SF Markus Elfring <hidden> Date: 2017-10-17 16:32:56
quoted
quoted
Fixes is only for bug fixes. These don't fix any bugs.
How do you distinguish these in questionable source code
from other error categories or software weaknesses?
A style change is one that doesn't change the effect of the execution.
This can occasionally be fine, can't it?
These don't actually even change the assembly,
How did you check it?
I would expect that there are useful run time effects to consider
for three proposed update steps (in this patch series).
so there's programmatic proof they're not fixing anything.
I find that the software refactoring “Improve a size determination in nine functions”
should fit to this observation (while the source code can become a bit better).
Bug means potentially user visible fault.
Thanks for your constructive feedback.
Regards,
Markus
From: SF Markus Elfring <hidden> Date: 2017-10-17 18:41:37
quoted
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.
True, thanks for the reminder.
Will it trigger further software development considerations (besides my contributions)?
Is this common in new code?
Do you start an official survey here?
Is there a script/ or some other automated way of catching this usage
Yes. - I am using an approach for the semantic patch language. ;-)
before patches are upstreamed?
I imagine that a corresponding source code analysis variant could be applied
in more cases if sufficient acceptance could be achieved.
Just as you're doing here, the patch description should reference this
in the patch description.
Do you find my wording “This issue was detected by using the Coccinelle software.” insufficient?
Regards,
Markus
On Tue, 2017-10-17 at 20:41 +0200, SF Markus Elfring wrote:
Do you find my wording “This issue was detected by using the
Coccinelle software.” insufficient?
The question is not whether it is insufficient, but whether it is
appropriate. Detecting Coccinelle issues is one step. The next step
is deciding what to do with them. Up to now, these messages have been
sent out as informational, not as patches.
Before sending patches to change existing code, address the "problem"
so that it doesn't continue to happen. Only afterwards is it
appropriate to discuss what to do with existing code.
Mimi
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-10-17 19:37:07
On Tue, 2017-10-17 at 20:41 +0200, SF Markus Elfring wrote:
quoted
quoted
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts
readability and
introduces an opportunity for a bug when the pointer variable type
is changed
but the corresponding sizeof that is passed to a memory allocator
is not.
quoted
before patches are upstreamed?
I imagine that a corresponding source code analysis variant could be
applied
in more cases if sufficient acceptance could be achieved.
So, then instead of still keeping people busy with this noise you better
start doing something like CI integration with that for *new* code?
I'm pretty sure you may also exercise your achievements on
drivers/staging where it would be honored.
Have you talked to Fengguang (0-day LKP)? Have you talked to Arnd (I
think he is related to kernel-ci)?
--
Andy Shevchenko [off-list ref]
Intel Finland Oy
From: SF Markus Elfring <hidden> Date: 2017-10-17 20:24:44
quoted
I imagine that a corresponding source code analysis variant could be applied
in more cases if sufficient acceptance could be achieved.
So, then instead of still keeping people busy with this noise you better
start doing something like CI integration with that for *new* code?
There are various software development challenges to consider.
I'm pretty sure you may also exercise your achievements on
drivers/staging where it would be honored.
I am waiting for several improvements also for software components
in this area for a while. Would you like to take another look
at these change possibilities?
Have you talked to Fengguang (0-day LKP)?
Not directly for this topic so far.
Have you talked to Arnd (I think he is related to kernel-ci)?
I am curious on how he will respond to remaining open issues.
Regards,
Markus
From: Joe Perches <joe@perches.com> Date: 2017-10-17 22:43:38
On Tue, 2017-10-17 at 08:57 -0700, James Bottomley wrote:
On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
quoted
quoted
Fixes is only for bug fixes. These don't fix any bugs.
How do you distinguish these in questionable source code
from other error categories or software weaknesses?
A style change is one that doesn't change the effect of the execution.
These don't actually even change the assembly, so there's programmatic
proof they're not fixing anything.
The printk removals do change the objects.
The value of that type of change is only for
resource limited systems.
printk type changes should generally not be
considered fixes.
Bug means potentially user visible fault. In any bug fix commit you
should document the fault and its effects on users so those backporting
can decide if they care or not.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-10-18 03:19:04
Mimi Zohar [off-list ref] writes:
On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote:
quoted
On Tue, 17 Oct 2017, Dan Carpenter wrote:
quoted
On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
quoted
On Tue, 17 Oct 2017, Dan Carpenter wrote:
quoted
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
quoted
A minor complaint: all commits are missing "Fixes:" tag.
Fixes is only for bug fixes. These don't fix any bugs.
0-day seems to put Fixes for everything. Should they be removed when the
old code is undesirable but doesn't actually cause a crash, eg out of date
API.
Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
OK, I will remove them from the patches that go through me where they
don't seem appropriate.
The "Fixes" tag is an indication that the patch should be backported.
No it's not that strong. It's an indication that the patch fixes another
commit, which may or may not mean it should be backported depending on
the preferences of the backporter. If it *does* need backporting then
the Fixes tag helps identify where it should go.
The doco is actually pretty well worded IMO:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n183
If your patch fixes a bug in a specific commit, e.g. you found an issue using
``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
the SHA-1 ID, and the one line summary.
and:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n602
A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
is used to make it easy to determine where a bug originated, which can help
review a bug fix. This tag also assists the stable kernel team in determining
which stable kernel versions should receive your fix. This is the preferred
method for indicating a bug fixed by the patch. See :ref:`describe_changes`
for more details.
cheers
From: Joe Perches <joe@perches.com> Date: 2017-10-18 09:18:56
On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
quoted
The printk removals do change the objects.
The value of that type of change is only for resource limited systems.
I imagine that such small code adjustments are also useful for other systems.
Your imagination and mine differ.
Where do you _think_ it matters?
For instance, nothing about
sizeof(type)
vs
sizeof(*ptr)
makes it easier for a human to read the code.
This class of change now require a syntactic parser
to find instances of the use of type where previously
a grep or equivalent tool worked well.
quoted
Markus' changelogs leave much to be desired.
Would you like to help more to improve the provided information
for the shown change patterns?
I've done that for you far too many times already.
Your changelogs need to detail _why_ something is being
done, not describe any tool used to perform or find a
particular instance of any change.
On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
quoted
quoted
The printk removals do change the objects.
The value of that type of change is only for resource limited systems=
.
quoted
I imagine that such small code adjustments are also useful for other
systems.
=20
Your imagination and mine differ.
Where do you _think_ it matters?
=20
For instance, nothing about
=20
sizeof(type)
vs
sizeof(*ptr)
=20
makes it easier for a human to read the code.
If it does not make it easier to read the code for you, then maybe you shou=
ld consider that this might not be true for all humans. For me, it makes it=
much easier to see at a glance, that code like ptr=3Dmalloc(sizeof(*ptr)) =
is correct.
Alexander
From: SF Markus Elfring <hidden> Date: 2017-10-18 09:55:54
quoted
I imagine that such small code adjustments are also useful for other systems.
Your imagination and mine differ.
This can generally be.
Where do you _think_ it matters?
It seems that this discussion branch referred still to my cover letter
for possible changes in the TPM software area.
The four update steps (in this patch series) demonstrate different
change possibilities which could be desired.
Would you like to distinguish them a bit more?
For instance, nothing about
sizeof(type)
vs
sizeof(*ptr)
makes it easier for a human to read the code.
I could agree to this view (in the general short form).
But nine statements became shorter in the concrete update suggestion
so that such a reduction could help the trained eyes
of some software developers and code reviewers.
This class of change now require a syntactic parser
to find instances of the use of type where previously
a grep or equivalent tool worked well.
Does the Linux coding style convention prefer safety over this
data processing concern?
quoted
quoted
Markus' changelogs leave much to be desired.
Would you like to help more to improve the provided information
for the shown change patterns?
I've done that for you far too many times already.
I got an other impression.
You gave constructive feedback (also for me) occasionally.
There were a few cases where a desired agreement was not achieved so far.
Your changelogs need to detail _why_ something is being done,
I could improve descriptions if involved information sources
could also become clearer and really safe.
not describe any tool used to perform or find a
particular instance of any change.
This part refers to a bit of attribution.
Regards,
Markus
From: Julia Lawall <hidden> Date: 2017-10-18 10:01:19
On Wed, 18 Oct 2017, Alexander.Steffen@infineon.com wrote:
quoted
On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
quoted
quoted
The printk removals do change the objects.
The value of that type of change is only for resource limited systems.
I imagine that such small code adjustments are also useful for other
systems.
Your imagination and mine differ.
Where do you _think_ it matters?
For instance, nothing about
sizeof(type)
vs
sizeof(*ptr)
makes it easier for a human to read the code.
If it does not make it easier to read the code for you, then maybe you
should consider that this might not be true for all humans. For me, it
makes it much easier to see at a glance, that code like
ptr=malloc(sizeof(*ptr)) is correct.
I don't think there is a perfect solution. The type argument to sizeof
could have the wrong type. The expression argument to sizeof could be
missing the *. Unpleasant consequences are possible in both cases.
Probably each maintainer has a style they prefer. Perhaps it could be
useful to adjust the code to follow the dominant strategy, in cases where
there are a inconsistencies. For example
if (...)
x = foo1(sizeof(struct xtype));
else
x = foo2(sizeof(*x));
might at least cause some unnecessary mental effort to process.
julia
From: Joe Perches <joe@perches.com> Date: 2017-10-18 10:28:39
On Wed, 2017-10-18 at 12:00 +0200, Julia Lawall wrote:
On Wed, 18 Oct 2017, Alexander.Steffen@infineon.com wrote:
quoted
quoted
On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
quoted
quoted
The printk removals do change the objects.
The value of that type of change is only for resource limited systems.
I imagine that such small code adjustments are also useful for other
systems.
Your imagination and mine differ.
Where do you _think_ it matters?
For instance, nothing about
sizeof(type)
vs
sizeof(*ptr)
makes it easier for a human to read the code.
If it does not make it easier to read the code for you, then maybe you
should consider that this might not be true for all humans. For me, it
makes it much easier to see at a glance, that code like
ptr=malloc(sizeof(*ptr)) is correct.
I don't think there is a perfect solution. The type argument to sizeof
could have the wrong type. The expression argument to sizeof could be
missing the *.
Yup.
Today, even after all of Markus' patches for this style
conversion, there is still only ~2:1 preference for
ptr = k.alloc(sizeof(*ptr))
over
ptr = k.alloc(sizeof(struct foo))
in the kernel tree
Ugly grep follows:
$ grep -rohP --include=*.[ch] "\w+\s*=\s*[kv].alloc\s*\(\s*sizeof.*," * | \
sed -r -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*\*\s*\1\s*\)/foo = k.alloc(sizeof(*foo))/' \
-e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*struct\s+\w+\s*\)/foo = k.alloc(sizeof(struct foo))/' | \
sort | uniq -c | sort -rn | head -2
6123 foo = k.alloc(sizeof(*foo)),
3060 foo = k.alloc(sizeof(struct foo)),
Unpleasant consequences are possible in both cases.
Yup.
Probably each maintainer has a style they prefer. Perhaps it could be
useful to adjust the code to follow the dominant strategy, in cases where
there are a inconsistencies. For example
if (...)
x = foo1(sizeof(struct xtype));
else
x = foo2(sizeof(*x));
might at least cause some unnecessary mental effort to process.
Sure, but perhaps _only_ when there are inconsistencies
in the same compilation unit.'
On Wed, 18 Oct 2017, Alexander.Steffen@infineon.com wrote:
=20
quoted
quoted
On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
quoted
quoted
The printk removals do change the objects.
The value of that type of change is only for resource limited sys=
tems.
quoted
quoted
quoted
I imagine that such small code adjustments are also useful for othe=
r
quoted
quoted
systems.
Your imagination and mine differ.
Where do you _think_ it matters?
For instance, nothing about
sizeof(type)
vs
sizeof(*ptr)
makes it easier for a human to read the code.
If it does not make it easier to read the code for you, then maybe you
should consider that this might not be true for all humans. For me, it
makes it much easier to see at a glance, that code like
ptr=3Dmalloc(sizeof(*ptr)) is correct.
=20
I don't think there is a perfect solution.
Maybe. But for the second variant the correctness is easier to check, both =
mentally and programmatically, because there is no need for any context (th=
e type of ptr does not matter).
The type argument to sizeof
could have the wrong type. The expression argument to sizeof could be
missing the *. Unpleasant consequences are possible in both cases.
Probably each maintainer has a style they prefer. Perhaps it could be
useful to adjust the code to follow the dominant strategy, in cases where
there are a inconsistencies.
Certainly. At least within a file, there should be only one style.
For example
=20
if (...)
x =3D foo1(sizeof(struct xtype));
else
x =3D foo2(sizeof(*x));
=20
might at least cause some unnecessary mental effort to process.
=20
julia
From: Joe Perches <joe@perches.com> Date: 2017-10-18 10:49:27
On Wed, 2017-10-18 at 10:44 +0000, Alexander.Steffen@infineon.com wrote:
quoted
For instance, nothing about
quoted
quoted
sizeof(type)
vs
sizeof(*ptr)
makes it easier for a human to read the code.
If it does not make it easier to read the code for you, then maybe you
should consider that this might not be true for all humans. For me, it
makes it much easier to see at a glance, that code like
ptr=malloc(sizeof(*ptr)) is correct.
I don't think there is a perfect solution.
Maybe. But for the second variant the correctness is easier to check,
How often should
ptr = alloc(sizeof(*ptr))
be
ptr = alloc(sizeof(**ptr))
both mentally and programmatically, because there is no need for any context (the type of ptr does not matter).
Would you like to get this ratio changed in any ways?
Available development tools could help to improve the software situation
in a desired direction, couldn't they?
quoted
Unpleasant consequences are possible in both cases.
How much do you care to reduce the failure probability further?
Regards,
Markus
From: Julia Lawall <hidden> Date: 2017-10-18 13:32:08
On Wed, 18 Oct 2017, David Laight wrote:
From: SF Markus Elfring
quoted
quoted
quoted
quoted
quoted
Unpleasant consequences are possible in both cases.
How much do you care to reduce the failure probability further?
Zero.
I am interested to improve the software situation a bit more here.
There are probably better places to spend your time!
If you want 'security' for kmalloc() then:
#define KMALLOC_TYPE(flags) (type *)kmalloc(sizeof (type), flags)
#define KMALLOC(ptr, flags) *(ptr) = KMALLOC_TYPE(typeof *(ptr), flags)
and change:
ptr = kmalloc(sizeof *ptr, flags);
to:
KMALLOC(&ptr, flags);
But it is all churn for churn's sake.
Please don't. Coccinelle won't find real problems with kmalloc any more
if this is done.
julia
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 14:40:15
On Tue, Oct 17, 2017 at 02:03:02PM +0300, Andy Shevchenko wrote:
On Mon, 2017-10-16 at 19:33 +0200, SF Markus Elfring wrote:
quoted
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 18:28:17 +0200
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
At the end it's Jarkko's call, though I would NAK this as I think some
one already told this to you for some other similar patch(es).
I even would suggest to stop doing this noisy stuff, which keeps people
busy for nothing.
I favor using "sizeof(*foo)" for pointers but as a part of a commit where
something useful is done to the corresponding line of code.
So, I would say it's a NAK.
/Jarkko
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 14:48:23
On Tue, Oct 17, 2017 at 11:50:05AM +0000, Alexander.Steffen@infineon.com wrote:
quoted
quoted
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
I actually prefer that style, so I'd welcome this change :)
quoted
At the end it's Jarkko's call, though I would NAK this as I think some
one already told this to you for some other similar patch(es).
I even would suggest to stop doing this noisy stuff, which keeps people
busy for nothing.
Cleaning up old code is also worth something, even if does not change
one bit in the assembly output in the end...
Alexander
Quite insignificant clean up it is that does more harm that gives any
benefit as any new change adds debt to backporting.
Anyway, this has been a useful patch set for me in the sense that I have
clearer picture now on discarding/accepting commits. One line minor
clean up will be from now on automatic NAK unless it causes a compiler
warning or some other visible side-effect.
/Jarkko
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 14:52:55
On Tue, Oct 17, 2017 at 04:02:05PM +0300, Andy Shevchenko wrote:
On Tue, 2017-10-17 at 08:52 -0400, Mimi Zohar wrote:
quoted
On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
wrote:
quoted
quoted
quoted
Replace the specification of data structures by pointer
dereferences
as the parameter for the operator "sizeof" to make the
corresponding
size
determination a bit safer according to the Linux coding style
convention.
This patch does one style in favor of the other.
I actually prefer that style, so I'd welcome this change :)
Style changes should be reviewed and documented, like any other code
change, and added to Documentation/process/coding-style.rst or an
equivalent file.
+1.
quoted
quoted
quoted
At the end it's Jarkko's call, though I would NAK this as I think
some
one already told this to you for some other similar patch(es).
I even would suggest to stop doing this noisy stuff, which keeps
people
busy for nothing.
Cleaning up old code is also worth something, even if does not
change one bit in the assembly output in the end...
Wow, you're opening the door really wide for all sorts of trivial
changes! Hope you have the time and inclination to review and comment
on all of them. I certainly don't.
Moreover and not so obvious is an open door for making back port of
*real* fixes much harder!
Yes. This is really the key observation:
A commit must have value above the cost of fixing a merge conflict.
/Jarkko
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 14:57:46
On Tue, Oct 17, 2017 at 08:41:04PM +0200, SF Markus Elfring wrote:
Do you find my wording “This issue was detected by using the
Coccinelle software.” insufficient?
This is fine for cover letter, not for the commits.
After your analysis software finds an issue you should manually analyze
what is wrong and document that to the commit message. This applies to
sparse, coccinelle or any other tool.
Tool-based commit messages are bad for commit history where as clean
description gives idea what was done (if you have to maintain a GIT
tree).
In my opinion tool is doing all the work but the part that you should do
is absent.
* Would you like to get a finer patch granularity then?
I don't understand what you are asking.
* Do you find any more information missing?
Regards,
Markus
I think I already answered to this in my earlier responses (commit
messages).
I probably won't take "sizeof(*foo)" type of change even if it
is a recommended style if that is the only useful thing that the
commit does.
/Jarkko
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 15:08:08
On Tue, Oct 17, 2017 at 12:44:34PM +0300, Dan Carpenter wrote:
On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
quoted
On Tue, 17 Oct 2017, Dan Carpenter wrote:
quoted
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
quoted
A minor complaint: all commits are missing "Fixes:" tag.
Fixes is only for bug fixes. These don't fix any bugs.
0-day seems to put Fixes for everything. Should they be removed when the
old code is undesirable but doesn't actually cause a crash, eg out of date
API.
Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
regards,
dan carpenter
So breaking a rule documented in the style guide is not a bug? :-)
/Jarkko
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 15:10:27
On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
quoted
quoted
Fixes is only for bug fixes. These don't fix any bugs.
How do you distinguish these in questionable source code
from other error categories or software weaknesses?
A style change is one that doesn't change the effect of the execution.
These don't actually even change the assembly, so there's programmatic
proof they're not fixing anything.
Bug means potentially user visible fault. In any bug fix commit you
should document the fault and its effects on users so those backporting
can decide if they care or not.
James
OK, I'll adjust my definition of a bug :-)
/Jarkko
Yes. - But other contributors pointed the detail out again
that not every change is qualified for using this tag.
quoted
* Would you like to get a finer patch granularity then?
I don't understand what you are asking.
If you would insist on the addition of this tag to all my commits
for the discussed patch series, I imagine that I would need to split
the update step “Improve a size determination in nine functions”
into smaller parts.
quoted
* Do you find any more information missing?
I think I already answered to this in my earlier responses
(commit messages).
Partly.
I probably won't take "sizeof(*foo)" type of change even if it
is a recommended style if that is the only useful thing that the
commit does.
How much do you care for the section “14) Allocating memory”
in the document “coding-style.rst” then?
Regards,
Markus
From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: 2017-10-18 16:09:53
On Wed, 2017-10-18 at 18:10 +0300, Jarkko Sakkinen wrote:
On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
quoted
On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
quoted
quoted
Fixes is only for bug fixes. These don't fix any bugs.
How do you distinguish these in questionable source code
from other error categories or software weaknesses?
A style change is one that doesn't change the effect of the
execution.
These don't actually even change the assembly, so there's
programmatic
proof they're not fixing anything.
Bug means potentially user visible fault. In any bug fix commit
you
should document the fault and its effects on users so those
backporting
can decide if they care or not.
James
OK, I'll adjust my definition of a bug :-)
Subsystems are free to define bugs in any reasonable way. However,
there are two things to note here:
1. The style guide is just that, a guide; it's not hard and fast rules.
That means that violations aren't bugs in the usual sense.
However, new code should mostly follow it and if it doesn't, there
should be a good reason to go against the guide which should be
explained in the change log.
2. The coding style evolves, so older drivers usually don't conform.
Classifying coding style issues as bugs leads to tons of patches
"fixing" older drivers, some of which actually end up breaking the
drivers in subtle ways which take ages to be found (at least that's
what we've seen in SCSI).
James
From: SF Markus Elfring <hidden> Date: 2017-10-18 16:43:43
Commit message should just describe in plain text what you are doing
Did other contributors find the wording “Replace …”
and why.
and “… a bit safer according to the Linux coding style convention.”
sufficient often enough already?
Which description would you find more appropriate for this change pattern?
Regards,
Markus
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 17:13:22
On Wed, Oct 18, 2017 at 09:09:48AM -0700, James Bottomley wrote:
On Wed, 2017-10-18 at 18:10 +0300, Jarkko Sakkinen wrote:
quoted
On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
quoted
On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
quoted
quoted
Fixes is only for bug fixes. These don't fix any bugs.
How do you distinguish these in questionable source code
from other error categories or software weaknesses?
A style change is one that doesn't change the effect of the
execution.
These don't actually even change the assembly, so there's
programmatic
proof they're not fixing anything.
Bug means potentially user visible fault. In any bug fix commit
you
should document the fault and its effects on users so those
backporting
can decide if they care or not.
James
OK, I'll adjust my definition of a bug :-)
Subsystems are free to define bugs in any reasonable way. However,
there are two things to note here:
1. The style guide is just that, a guide; it's not hard and fast rules.
That means that violations aren't bugs in the usual sense.
However, new code should mostly follow it and if it doesn't, there
should be a good reason to go against the guide which should be
explained in the change log.
2. The coding style evolves, so older drivers usually don't conform.
Classifying coding style issues as bugs leads to tons of patches
"fixing" older drivers, some of which actually end up breaking the
drivers in subtle ways which take ages to be found (at least that's
what we've seen in SCSI).
James
Makes sense. Thanks for verbose explanation.
/Jarkko
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 17:19:08
On Wed, Oct 18, 2017 at 06:43:10PM +0200, SF Markus Elfring wrote:
quoted
Commit message should just describe in plain text what you are doing
Did other contributors find the wording “Replace …”
quoted
and why.
and “… a bit safer according to the Linux coding style convention.”
sufficient often enough already?
Which description would you find more appropriate for this change pattern?
Regards,
Markus
For 1/4 and 2/4: explain why the message can be omitted. Remove sentence
about Coccinelle. That's all.
3/4: definitive NAK, too much noise compared to value.
4/4: this a good commit message. Requires a Tested-by before can be
accepted, which I'm not able to give.
Hope this helps.
/Jarkko
From: Jarkko Sakkinen <hidden> Date: 2017-10-18 17:22:19
On Wed, Oct 18, 2017 at 08:18:58PM +0300, Jarkko Sakkinen wrote:
On Wed, Oct 18, 2017 at 06:43:10PM +0200, SF Markus Elfring wrote:
quoted
quoted
Commit message should just describe in plain text what you are doing
Did other contributors find the wording “Replace …”
quoted
and why.
and “… a bit safer according to the Linux coding style convention.”
sufficient often enough already?
Which description would you find more appropriate for this change pattern?
Regards,
Markus
For 1/4 and 2/4: explain why the message can be omitted. Remove sentence
about Coccinelle. That's all.
3/4: definitive NAK, too much noise compared to value.
4/4: this a good commit message. Requires a Tested-by before can be
accepted, which I'm not able to give.
Hope this helps.
/Jarkko
One more word of advice: send the three as separate patches. My guess is
that it takes a factor longer time to apply 4/4 than other patches
because there's more limited crowd who can test it.
From: SF Markus Elfring <hidden> Date: 2017-10-18 17:48:42
For 1/4 and 2/4: explain why the message can be omitted.
Why did you not reply directly with this request for the update steps
with the subject “Delete an error message for a failed memory allocation
in tpm_…()”?
https://patchwork.kernel.org/patch/10009405/https://patchwork.kernel.org/patch/10009415/
I find that there can be difficulty to show an appropriate information
source for the reasonable explanation of this change pattern.
Remove sentence about Coccinelle.
I got the impression that there is a bit of value in such
a kind of attribution.
That's all.
I assume that there might be also some communication challenges involved.
3/4: definitive NAK, too much noise compared to value.
I tried to reduce deviations from the Linux coding style again.
You do not like such an attempt for this software area so far.
From: Jerry Snitselaar <hidden> Date: 2017-10-18 17:54:16
On Wed Oct 18 17, SF Markus Elfring wrote:
quoted
For 1/4 and 2/4: explain why the message can be omitted.
Why did you not reply directly with this request for the update steps
with the subject “Delete an error message for a failed memory allocation
in tpm_…()”?
https://patchwork.kernel.org/patch/10009405/https://patchwork.kernel.org/patch/10009415/
I find that there can be difficulty to show an appropriate information
source for the reasonable explanation of this change pattern.
Shouldn't this information source for the explanation be the
submitter? I'd hope they understand what it is they are submitting.
quoted
Remove sentence about Coccinelle.
I got the impression that there is a bit of value in such
a kind of attribution.
quoted
That's all.
I assume that there might be also some communication challenges involved.
quoted
3/4: definitive NAK, too much noise compared to value.
I tried to reduce deviations from the Linux coding style again.
You do not like such an attempt for this software area so far.
From: SF Markus Elfring <hidden> Date: 2017-10-18 17:55:30
One more word of advice: send the three as separate patches.
I do not see a need for an immediate resend at the moment.
My guess is that it takes a factor longer time to apply 4/4
than other patches because there's more limited crowd who can test it.
This is fine for me if somebody would like to integrate
this update suggestion at all.
How do you think about to separate replies better between affected
update steps?
Regards,
Markus
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-10-18 18:03:22
On Wed, 2017-10-18 at 19:48 +0200, SF Markus Elfring wrote:
quoted
For 1/4 and 2/4: explain why the message can be omitted.
quoted
That's all.
I assume that there might be also some communication challenges
involved.
quoted
3/4: definitive NAK, too much noise compared to value.
I tried to reduce deviations from the Linux coding style again.
You do not like such an attempt for this software area so far.
The problem here in a time line or what comes first. Definitely, you are
trying to fix the code which _is_ upstream vs. the code which _might be_
upstream (exception is drivers/staging).
Why didn't you listen to what people are telling you?
Why are you spending too much time on little sense crap instead of doing
real fixes?
--
Andy Shevchenko [off-list ref]
Intel Finland Oy
From: SF Markus Elfring <hidden> Date: 2017-10-18 18:12:14
quoted
Why did you not reply directly with this request for the update steps
with the subject “Delete an error message for a failed memory allocation
in tpm_…()”?
https://patchwork.kernel.org/patch/10009405/https://patchwork.kernel.org/patch/10009415/
I find that there can be difficulty to show an appropriate information
source for the reasonable explanation of this change pattern.
Shouldn't this information source for the explanation be the submitter?
I offered a bit of information. I agree that it could become better eventually.
I'd hope they understand what it is they are submitting.
I do this to some degree. ;-)
But I would appreciate if I could refer to a single Linux document
for this change pattern around questionable error messages.
Would a corresponding link for an accepted explanation in the documentation
be nice in this case?
Regards,
Markus
From: Michal Suchánek <hidden> Date: 2017-10-18 18:27:53
On Wed, 18 Oct 2017 02:18:46 -0700
Joe Perches [off-list ref] wrote:
On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
quoted
quoted
The printk removals do change the objects.
The value of that type of change is only for resource limited
systems.
I imagine that such small code adjustments are also useful for
other systems.
Your imagination and mine differ.
Where do you _think_ it matters?
For instance, nothing about
sizeof(type)
vs
sizeof(*ptr)
makes it easier for a human to read the code.
However, it makes it less error-prone to modify the code.
If you do ptr = malloc(sizeof(*ptr)) and later you change the type of
the pointer the code is still correct whereas ptr = malloc(sizeof(some
type) no longer is.
That is the reason the source analysis tool warns about this usage and
you do not really need any more explanation for *this* change.
The others are not so clear.
Thanks
Michal
From: Michal Suchánek <hidden> Date: 2017-10-19 11:56:37
Hello,
On Mon, 16 Oct 2017 19:34:56 +0200
SF Markus Elfring [off-list ref] wrote:
quoted hunk
From: Markus Elfring <redacted>
Date: Mon, 16 Oct 2017 19:00:34 +0200
Two pointer checks could be repeated by the tpm_ibmvtpm_probe()
function during error handling even if the relevant properties can be
determined for the involved variables before by source code analysis.
* Return directly after a call of the function "kzalloc" failed
at the beginning.
* Adjust jump targets so that extra checks can be omitted at the end.
Signed-off-by: Markus Elfring <redacted>
---
drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
b/drivers/char/tpm/tpm_ibmvtpm.c index a4b462a77b99..b8dda7546f64
100644 --- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -610,7 +610,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
Just no.
I have seen many fixes that do inverse of this after a piece of code
allocating some more resources was added before code that returns
straight away because it is the first allocation in a function.
quoted hunk
ibmvtpm->dev = dev;
ibmvtpm->vdev = vio_dev;
@@ -619,7 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
I think a single cleanup section is better than many labels that just
avoid a single null check.
As long as you can tell easily which resources were already allocated
and need to be freed it is saner to keep only one cleanup section.
If the code doing the allocation is changed in the future the single
cleanup can stay whereas multiple labels have to be rewritten again.
Also just changing this just for the sake of code style does not seem
worth it whatever style you prefer.
Thanks
Michal
From: Michal Suchánek <hidden> Date: 2017-10-19 12:04:11
On Wed, 18 Oct 2017 21:03:13 +0300
Andy Shevchenko [off-list ref] wrote:
On Wed, 2017-10-18 at 19:48 +0200, SF Markus Elfring wrote:
quoted
quoted
For 1/4 and 2/4: explain why the message can be omitted.
quoted
quoted
That's all.
I assume that there might be also some communication challenges
involved.
quoted
3/4: definitive NAK, too much noise compared to value.
I tried to reduce deviations from the Linux coding style again.
You do not like such an attempt for this software area so far.
The problem here in a time line or what comes first. Definitely, you
are trying to fix the code which _is_ upstream vs. the code which
_might be_ upstream (exception is drivers/staging).
Why didn't you listen to what people are telling you?
Why are you spending too much time on little sense crap instead of
doing real fixes?
People are free to spend their time on what they like.
Even if no commit of this series lands in mainline it has been useful
to clarify what is preferred style and what is useful fix.
Thanks
Michal
From: Jarkko Sakkinen <hidden> Date: 2017-10-19 12:16:58
On Wed, Oct 18, 2017 at 07:48:06PM +0200, SF Markus Elfring wrote:
quoted
For 1/4 and 2/4: explain why the message can be omitted.
Why did you not reply directly with this request for the update steps
with the subject “Delete an error message for a failed memory allocation
in tpm_…()”?
https://patchwork.kernel.org/patch/10009405/https://patchwork.kernel.org/patch/10009415/
I find that there can be difficulty to show an appropriate information
source for the reasonable explanation of this change pattern.
quoted
Remove sentence about Coccinelle.
I got the impression that there is a bit of value in such
a kind of attribution.
quoted
That's all.
I assume that there might be also some communication challenges involved.
quoted
3/4: definitive NAK, too much noise compared to value.
I tried to reduce deviations from the Linux coding style again.
You do not like such an attempt for this software area so far.