| Message ID | 547B496E.604@users.sourceforge.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 11/30/2014 7:44 PM, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sun, 30 Nov 2014 17:02:07 +0100 > The kfree() function tests whether its argument is NULL and then > returns immediately. Thus the test around the call is not needed. > This issue was detected by using the Coccinelle software. > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/net/ppp/ppp_mppe.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c > index 911b216..7e44212 100644 > --- a/drivers/net/ppp/ppp_mppe.c > +++ b/drivers/net/ppp/ppp_mppe.c > @@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen) > return (void *)state; > > out_free: > - if (state->sha1_digest) > - kfree(state->sha1_digest); > + kfree(state->sha1_digest); Please keep this line aligned to the others. > if (state->sha1) > crypto_free_hash(state->sha1); > if (state->arc4) [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c >> index 911b216..7e44212 100644 >> --- a/drivers/net/ppp/ppp_mppe.c >> +++ b/drivers/net/ppp/ppp_mppe.c >> @@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen) >> return (void *)state; >> >> out_free: >> - if (state->sha1_digest) >> - kfree(state->sha1_digest); >> + kfree(state->sha1_digest); > > Please keep this line aligned to the others. Can it be that the previous source code contained unwanted space characters at this place? Do you want indentation fixes as a separate update step? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2014 06:00 PM, SF Markus Elfring wrote: >>> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c >>> index 911b216..7e44212 100644 >>> --- a/drivers/net/ppp/ppp_mppe.c >>> +++ b/drivers/net/ppp/ppp_mppe.c >>> @@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen) >>> return (void *)state; >>> >>> out_free: >>> - if (state->sha1_digest) >>> - kfree(state->sha1_digest); >>> + kfree(state->sha1_digest); >> Please keep this line aligned to the others. > Can it be that the previous source code contained unwanted space > characters at this place? Yes, it seems so. > Do you want indentation fixes as a separate update step? Yes, that would be better to keep it separate. > Regards, > Markus WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:50:28 +0100
Further update suggestions were taken into account before and after a patch
was applied from static source code analysis.
Markus Elfring (6):
Replacement of a printk() call by pr_warn() in mppe_rekey()
Fix indentation
Deletion of unnecessary checks before the function call "kfree"
Less function calls in mppe_alloc() after error detection
Delete an unnecessary assignment in mppe_alloc()
Delete another unnecessary assignment in mppe_alloc()
drivers/net/ppp/ppp_mppe.c | 49 +++++++++++++++++++---------------------------
1 file changed, 20 insertions(+), 29 deletions(-)
From: SF Markus Elfring <elfring@users.sourceforge.net> Date: Thu, 04 Dec 2014 23:03:30 +0100 > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Thu, 4 Dec 2014 22:50:28 +0100 > > Further update suggestions were taken into account before and after a patch > was applied from static source code analysis. Generally speaking, it is advisable to not leave error pointers in data structures, even if they are about to be free'd up in an error path anyways. Therefore I do not like some of the patches in this series. Sorry. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Generally speaking, it is advisable to not leave error pointers in data > structures, even if they are about to be free'd up in an error path anyways. > > Therefore I do not like some of the patches in this series. Can you give any more concrete feedback here? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: SF Markus Elfring <elfring@users.sourceforge.net> Date: Fri, 12 Dec 2014 08:01:54 +0100 >> Generally speaking, it is advisable to not leave error pointers in data >> structures, even if they are about to be free'd up in an error path anyways. >> >> Therefore I do not like some of the patches in this series. > > Can you give any more concrete feedback here? I gave you very concrete feedback, I said exactly that I don't want error pointers left in data structure members. I cannot describe my requirements any more precisely than that. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I gave you very concrete feedback, I said exactly that I don't want > error pointers left in data structure members. I find that your critique affects the proposed update steps four to six, doesn't it? Are the other steps acceptable in principle? > I cannot describe my requirements any more precisely than that. I hope that a bit more constructive suggestions will be contributed by involved software developers around the affected source code. Now it seems that a small code clean-up becomes a more challenging development task. How do you prefer to redesign corresponding data structures eventually? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
You are asking me to invest a lot of time for a very trivial set of changes. Why don't you just integrate the feedback you were given and resubmit your patch series, just like any other developer would? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> You are asking me to invest a lot of time for a very trivial > set of changes. I find the proposed six update steps also trivial so far. > Why don't you just integrate the feedback you were given and > resubmit your patch series, just like any other developer would? I find the requested redesign and reorganisation of involved data structures not so easy and therefore more challenging at the moment. Where should "the error pointers" be stored instead? Is such a software improvement even a kind of add-on to my patch series? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: SF Markus Elfring <elfring@users.sourceforge.net> Date: Fri, 12 Dec 2014 17:56:36 +0100 > Where should "the error pointers" be stored instead? A local variable, before you assign it into the datastructure. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Where should "the error pointers" be stored instead? > A local variable, before you assign it into the datastructure. Will it be acceptable for you that anyone (or even me) will introduce such a change with a seventh (and eventually eighth) update step here? Do you want any other sequence for source code preparation of the requested software improvement? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I hope that a bit more constructive suggestions will be contributed by > involved > software developers around the affected source code. Now it seems > that a small code clean-up becomes a more challenging development task. This is often the case. Doing something half way is not useful. julia -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-12-12 at 18:22 +0100, SF Markus Elfring wrote: > Will it be acceptable for you that anyone (or even me) will introduce > such a change with a seventh (and eventually eighth) update step here? > Do you want any other sequence for source code preparation of > the requested software improvement? The thing is : We are in the merge window, tracking bugs added in latest dev cycle. Having to deal with patches like yours is adding pressure on the maintainer (and other developers) at the wrong time. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: SF Markus Elfring <elfring@users.sourceforge.net> Date: Fri, 12 Dec 2014 18:22:48 +0100 >>> Where should "the error pointers" be stored instead? >> A local variable, before you assign it into the datastructure. > > Will it be acceptable for you that anyone (or even me) will introduce > such a change with a seventh (and eventually eighth) update step here? > Do you want any other sequence for source code preparation of > the requested software improvement? I'd like to honestly ask why you are being so difficult? Everyone gets their code reviewed, everyone has to modify their changes to adhere to the subsystem maintainer's wishes. You are not being treated specially, and quite frankly nobody is asking anything unreasonable of you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> We are in the merge window, tracking bugs added in latest dev cycle. I am also curious on the software evolution about how many improvements will arrive in the next Linux versions. > Having to deal with patches like yours is adding pressure > on the maintainer (and other developers) at the wrong time. You can relax a bit eventually. More merge windows will follow, won't they? It will be nice if a bunch of recent code clean-ups which were also triggered by static source code analysis will be integrated into Linux 3.19 already. More update suggestions will be considered later again as usual. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I'd like to honestly ask why you are being so difficult? There are several factors which contribute to your perception of difficulty here. 1. I try to extract from every feedback the information about the amount of acceptance or rejection for a specific update suggestion. A terse feedback (like yours for this issue) makes it occasionally harder to see the next useful steps. So another constructive discussion is evolving around the clarification of some implementation details. 2. I prefer also different communication styles at some points. 3. I reached a point where the desired software updates were not immediately obvious for me while other contributors might have achieved a better understanding for the affected issues already. 4. I am on the way at the moment to get my Linux software development system running again. https://forums.opensuse.org/showthread.php/503327-System-startup-does-not-continue-after-hard-disk-detection > Everyone gets their code reviewed, everyone has to modify their > changes to adhere to the subsystem maintainer's wishes. That is fine as usual. > You are not being treated specially, and quite frankly nobody > is asking anything unreasonable of you. That is also true as the software development process will be continued. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Where should "the error pointers" be stored instead? > > A local variable, before you assign it into the datastructure. I have looked at the affected software infrastructure once more. Now I find still that your data reorgansisation wish can not be resolved in a simple way. I imagine that your update suggestion would mean that the corresponding pointers will be passed around by function parameters instead, wouldn't it? Two pointers were stored as the members "arc4" and "sha1" of the data structure "ppp_mppe_state" for a specific reason. A pointer to this structure is passed to the ppp_register_compressor() function. https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/linux/ppp-comp.h?id=607ca46e97a1b6594b29647d98a32d545c24bdff#n32 The data structure "compressor" manages some function pointers. I assume that this interface should not be changed at the moment, should it? Are further ideas needed here? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: SF Markus Elfring <elfring@users.sourceforge.net> Date: Thu, 18 Dec 2014 18:23:08 +0100 >>> Where should "the error pointers" be stored instead? >> >> A local variable, before you assign it into the datastructure. > > I have looked at the affected software infrastructure once more. > Now I find still that your data reorgansisation wish can not be resolved > in a simple way. I'm saying to leave the code alone. If it goes: var = foo_that_returns_ptr_err() if (IS_ERR(var)) return PTR_ERR(var); p->bar = var; or whatever, simply keep it that way! I'm not engaging in this conversation any further, you have already consumed way too much of my limited time on this incredibly trivial matter. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Now I find still that your data reorgansisation wish can not be resolved >> in a simple way. > > I'm saying to leave the code alone. It seems that there might be a misunderstanding between us. > If it goes: > > var = foo_that_returns_ptr_err() > if (IS_ERR(var)) > return PTR_ERR(var); > > p->bar = var; > > or whatever, simply keep it that way! A simple return was not used by the mppe_alloc() function so far because a bit of memory clean-up will also be useful after error detection, won't it? > I'm not engaging in this conversation any further, you have already > consumed way too much of my limited time on this incredibly trivial matter. It can occasionally happen that a safe clarification of specific implementation details will need more efforts than you would like to invest at the moment. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I'm saying to leave the code alone. Do I need to try another interpretation out for your feedback? > If it goes: > > var = foo_that_returns_ptr_err() > if (IS_ERR(var)) > return PTR_ERR(var); > > p->bar = var; > > or whatever, simply keep it that way! Do you want to express here that a data structure member should only be set after a previous function call succeeded? > I'm not engaging in this conversation any further, you have > already consumed way too much of my limited time on this > incredibly trivial matter. I hope that you will find a bit time and patience again to clarify affected implementation details in a safer and unambiguous way. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus, On 20.12.2014 15:45, SF Markus Elfring wrote: >> I'm saying to leave the code alone. > > Do I need to try another interpretation out for your feedback? > > >> If it goes: >> >> var = foo_that_returns_ptr_err() >> if (IS_ERR(var)) >> return PTR_ERR(var); >> >> p->bar = var; >> >> or whatever, simply keep it that way! > > Do you want to express here that a data structure member should > only be set after a previous function call succeeded? > I think what David said was pretty clear: If you see code like the above there is no need to refactor it. That does not mean that this is the _preferred_ way of error handling. Its just good enough to be left alone. Regards, Lino -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I think what David said was pretty clear: If you see code like the above > there is no need to refactor it. I can understand this view in principle. > That does not mean that this is the _preferred_ way of error handling. Can your feedback help in the clarification of suggestions around my update steps one to six for this Linux software module? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: SF Markus Elfring <elfring@users.sourceforge.net> Date: Sat, 20 Dec 2014 15:45:32 +0100 > I hope that you will find a bit time and patience again > to clarify affected implementation details in a safer and > unambiguous way. Sorry, another developer will have to hold your hand, as I said I already invested too much time into this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c index 911b216..7e44212 100644 --- a/drivers/net/ppp/ppp_mppe.c +++ b/drivers/net/ppp/ppp_mppe.c @@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen) return (void *)state; out_free: - if (state->sha1_digest) - kfree(state->sha1_digest); + kfree(state->sha1_digest); if (state->sha1) crypto_free_hash(state->sha1); if (state->arc4) @@ -256,13 +255,12 @@ static void mppe_free(void *arg) { struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg; if (state) { - if (state->sha1_digest) kfree(state->sha1_digest); - if (state->sha1) - crypto_free_hash(state->sha1); - if (state->arc4) - crypto_free_blkcipher(state->arc4); - kfree(state); + if (state->sha1) + crypto_free_hash(state->sha1); + if (state->arc4) + crypto_free_blkcipher(state->arc4); + kfree(state); } }