| Message ID | 1d3516a2-a8e6-9e95-d438-f115fac84c7f@users.sourceforge.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | char-TPM: Adjustments for ten function implementations | expand |
On Mon, Oct 16, 2017 at 07:30:13PM +0200, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > 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
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: > > From: Markus Elfring <elfring@users.sourceforge.net> > > 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
> 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
On Mon, 2017-10-16 at 21:35 +0300, Jarkko Sakkinen wrote:
> A minor complaint: all commits are missing "Fixes:" tag.
None of these patches fix anything.
All are trivial changes without much of any impact.
>> A minor complaint: all commits are missing "Fixes:" tag. > > None of these patches fix anything. It depends on the view which you prefer. > All are trivial changes without much of any impact. I find that they improve the affected software another bit. Other adjustments can be more noticeable. Regards, Markus
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > > A minor complaint: all commits are missing "Fixes:" tag. > Fixes is only for bug fixes. These don't fix any bugs. regards, dan carpenter
On Tue, 17 Oct 2017, Dan Carpenter wrote: > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > > > > 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 >
> 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?
Regards,
Markus
On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote: > > > On Tue, 17 Oct 2017, Dan Carpenter wrote: > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > > > > > > 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
On Tue, 17 Oct 2017, Dan Carpenter wrote: > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote: > > > > > > On Tue, 17 Oct 2017, Dan Carpenter wrote: > > > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > > > > > > > > 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
Hi Julia, On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote: > > On Tue, 17 Oct 2017, Dan Carpenter wrote: > > > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote: > > > > > > > > > On Tue, 17 Oct 2017, Dan Carpenter wrote: > > > > > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > > > > > > > > > > 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
Dan Carpenter <dan.carpenter@oracle.com> writes: > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote: >> >> >> On Tue, 17 Oct 2017, Dan Carpenter wrote: >> >> > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: >> > > >> > > 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:25 +0200, SF Markus Elfring wrote: > > > > 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
>>> 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
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: > > > > > > 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. Markus' changelogs leave much to be desired.
Mimi Zohar <zohar@linux.vnet.ibm.com> writes: > On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote: >> On Tue, 17 Oct 2017, Dan Carpenter wrote: >> > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote: >> > > On Tue, 17 Oct 2017, Dan Carpenter wrote: >> > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: >> > > > > >> > > > > 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
> 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. > Markus' changelogs leave much to be desired. Would you like to help more to improve the provided information for the shown change patterns? Regards, Markus
On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote: > > 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. > > 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: > > > 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. Alexander
>> 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? >>> 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
On Wed, 18 Oct 2017, Alexander.Steffen@infineon.com wrote: > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote: > > > > 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
On Wed, 2017-10-18 at 12:00 +0200, Julia Lawall wrote: > > On Wed, 18 Oct 2017, Alexander.Steffen@infineon.com wrote: > > > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote: > > > > > 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: > > > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote: > > > > > 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. Maybe. But for the second variant the correctness is easier to check, both mentally and programmatically, because there is no need for any context (the 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 > > if (...) > x = foo1(sizeof(struct xtype)); > else > x = foo2(sizeof(*x)); > > might at least cause some unnecessary mental effort to process. > > julia Alexander
On Wed, 2017-10-18 at 10:44 +0000, Alexander.Steffen@infineon.com wrote: > > 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. > > 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). Context matters.
> 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)), 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? >> Unpleasant consequences are possible in both cases. How much do you care to reduce the failure probability further? Regards, Markus
> On Wed, 2017-10-18 at 10:44 +0000, Alexander.Steffen@infineon.com wrote: > > > 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. > > > > Maybe. But for the second variant the correctness is easier to check, > > How often should > ptr = alloc(sizeof(*ptr)) > be > ptr = alloc(sizeof(**ptr)) Never? Because in that case it probably should be *ptr=alloc(sizeof(**ptr)), unless you are doing something horrible ;-) Alexander
On Wed, 2017-10-18 at 13:00 +0200, SF Markus Elfring wrote: > > 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)), > > Would you like to get this ratio changed in any ways? No. > Available development tools could help to improve the software situation > in a desired direction, couldn't they? > > > Unpleasant consequences are possible in both cases. > How much do you care to reduce the failure probability further? Zero. The alloc style is trivially useful for new code. Existing code doesn't need change.
>>>> 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. Regards, Markus
From: SF Markus Elfring > >>>> 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. David
On Wed, 18 Oct 2017, David Laight wrote: > From: SF Markus Elfring > > >>>> 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
>> 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) Such an approach might help. >> and change: >> ptr = kmalloc(sizeof *ptr, flags); >> to: >> KMALLOC(&ptr, flags); >> >> But it is all churn for churn's sake. > > Please don't. Interesting … > Coccinelle won't find real problems with kmalloc any more if this is done. The corresponding source code analysis will become different (or more challenging) then. Are you still looking for related solutions? Regards, Markus
On Mon, Oct 16, 2017 at 10:44:18PM +0200, SF Markus Elfring wrote: > > A minor complaint: all commits are missing "Fixes:" tag. > > * Do you require it to be added to the commit messages? I don't require it. It's part of the development process: https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html > * 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
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: > > > > > > On Tue, 17 Oct 2017, Dan Carpenter wrote: > > > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > > > > > > > > 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
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: > > > > > > 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
>>> A minor complaint: all commits are missing "Fixes:" tag. >> >> * Do you require it to be added to the commit messages? > > I don't require it. It's part of the development process: > > https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html Yes. - But other contributors pointed the detail out again that not every change is qualified for using this tag. >> * 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. >> * 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
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: > > > > On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote: > > > > > > > > > > > > > > > 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
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: > > 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: > > > > > > > > > > > > > > > > > > > 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
On Wed, 18 Oct 2017 02:18:46 -0700 Joe Perches <joe@perches.com> wrote: > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote: > > > 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
On Wed, 2017-10-18 at 14:18 +1100, Michael Ellerman wrote: > Mimi Zohar <zohar@linux.vnet.ibm.com> writes: > > On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote: > >> On Tue, 17 Oct 2017, Dan Carpenter wrote: > >> > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote: > >> > > On Tue, 17 Oct 2017, Dan Carpenter wrote: > >> > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > >> > > > > > >> > > > > 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. Thank you for setting the record straight. > 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 >
>>> 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. > > Thank you for setting the record straight. > >> The doco is actually pretty well worded IMO: It seems that there are still interpretations left over for further clarification. Would any porters dare to distribute the deletion of questionable condition checks (and special error messages) to more software versions? >> 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. Would you dare to categorise any software inefficiencies with a bug type? Regards, Markus
From: Markus Elfring <elfring@users.sourceforge.net> 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(-)