Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1277580 > unrolled thread

Re: [PATCH 3/3] staging: lustre: Less function calls in class_register_type() after error detection

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] staging: lustre: Less function calls in  class_register_type() after error detection Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-11-25 17:40 +0100
    Re: [PATCH 3/3] staging: lustre: Less function calls in  class_register_type() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-26 08:10 +0100
      Re: [PATCH 3/3] staging: lustre: Less function calls in  class_register_type() after error detection Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-11-26 18:00 +0100
        Re: [PATCH 3/3] staging: lustre: Less function calls in  class_register_type() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-26 20:10 +0100
          Re: [PATCH 3/3] staging: lustre: Less function calls in  class_register_type() after error detection Dan Carpenter <dan.carpenter@oracle.com> - 2015-11-26 21:10 +0100

#1277580 — Re: [PATCH 3/3] staging: lustre: Less function calls in class_register_type() after error detection

On Thu, Nov 05, 2015 at 01:20:33PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 5 Nov 2015 12:48:58 +0100
> 
> The functions "kfree" and "kobject_put" were called in a few cases by the
> function "class_register_type" during error handling even if the passed
> variable contained a null pointer.
> 
> This implementation detail could be improved by the adjustment of
> jump targets.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)

Does not apply to my staging-next branch :(

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1278011

Am 25.11.2015 um 17:39 schrieb Greg Kroah-Hartman:
> On Thu, Nov 05, 2015 at 01:20:33PM +0100, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Thu, 5 Nov 2015 12:48:58 +0100
>>
>> The functions "kfree" and "kobject_put" were called in a few cases by the
>> function "class_register_type" during error handling even if the passed
>> variable contained a null pointer.
>>
>> This implementation detail could be improved by the adjustment of
>> jump targets.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
>>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> Does not apply to my staging-next branch :(

I get also a result like the following together with the software "Linux next-20151126".   ;-)

elfring@Sonne:~/Projekte/Linux/next-patched> LANG=C git apply ~/Projekte/Bau/Linux/scripts/Coccinelle/deletions1/next/20151102/Flicken/0003-staging-lustre-Less-function-calls-in-class_register.patch
error: patch failed: drivers/staging/lustre/lustre/obdclass/genops.c:214
error: drivers/staging/lustre/lustre/obdclass/genops.c: patch does not apply


Do you try this update suggestion out without integrating the corresponding previous
update suggestion "Delete unnecessary checks before two function calls"
where I proposed to remove extra checks before a few calls of the function "kobject_put"
(which seems to matter for the patch hunk in the shown error message)?
https://lkml.org/lkml/2015/11/5/276
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1013635.html

Would you like to reject the first update step from this patch series
so that I need to adapt my approach to your software design decision?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278346

On Thu, Nov 26, 2015 at 08:08:28AM +0100, SF Markus Elfring wrote:
> Am 25.11.2015 um 17:39 schrieb Greg Kroah-Hartman:
> > On Thu, Nov 05, 2015 at 01:20:33PM +0100, SF Markus Elfring wrote:
> >> From: Markus Elfring <elfring@users.sourceforge.net>
> >> Date: Thu, 5 Nov 2015 12:48:58 +0100
> >>
> >> The functions "kfree" and "kobject_put" were called in a few cases by the
> >> function "class_register_type" during error handling even if the passed
> >> variable contained a null pointer.
> >>
> >> This implementation detail could be improved by the adjustment of
> >> jump targets.
> >>
> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >> ---
> >>  drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
> >>  1 file changed, 16 insertions(+), 10 deletions(-)
> > 
> > Does not apply to my staging-next branch :(
> 
> I get also a result like the following together with the software "Linux next-20151126".   ;-)
> 
> elfring@Sonne:~/Projekte/Linux/next-patched> LANG=C git apply ~/Projekte/Bau/Linux/scripts/Coccinelle/deletions1/next/20151102/Flicken/0003-staging-lustre-Less-function-calls-in-class_register.patch
> error: patch failed: drivers/staging/lustre/lustre/obdclass/genops.c:214
> error: drivers/staging/lustre/lustre/obdclass/genops.c: patch does not apply
> 
> 
> Do you try this update suggestion out without integrating the corresponding previous
> update suggestion "Delete unnecessary checks before two function calls"
> where I proposed to remove extra checks before a few calls of the function "kobject_put"
> (which seems to matter for the patch hunk in the shown error message)?
> https://lkml.org/lkml/2015/11/5/276
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1013635.html

I guess so, I don't remember, I don't see any patches from you earlier
in my "todo" mbox.

> Would you like to reject the first update step from this patch series
> so that I need to adapt my approach to your software design decision?

I have no idea what you are talking about.  I have no recolection of
previous patches or conversations about your patches.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278402

>> Do you try this update suggestion out without integrating the corresponding previous
>> update suggestion "Delete unnecessary checks before two function calls"
>> where I proposed to remove extra checks before a few calls of the function "kobject_put"
>> (which seems to matter for the patch hunk in the shown error message)?
>> https://lkml.org/lkml/2015/11/5/276
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1013635.html
> 
> I guess so, I don't remember, I don't see any patches from you earlier
> in my "todo" mbox.

I am still waiting for further constructive feedback on a bunch
of my update suggestions which are derived from static source code analysis.

It can be the usual challenge to get a bit more attention for them.
Other software improvements will result in bigger effects than the source code
fine-tuning I propose, won't it?

I would like to acknowledge that changes like the following from this patch series
can still be applied together for the software "Linux next-20151126".
* 0001-staging-lustre-Delete-unnecessary-checks-before-two.patch
* 0003-staging-lustre-Less-function-calls-in-class_register.patch


>> Would you like to reject the first update step from this patch series
>> so that I need to adapt my approach to your software design decision?
> 
> I have no idea what you are talking about.  I have no recolection of
> previous patches or conversations about your patches.

* Dan Carpenter expressed his software design concerns around hidden sanity checks
  a few times.
  How do you think about to give the proposed changes another chance?

* Positive feedback is occasionally increasing by specific subsystem supporters
  and maintainers.
  How will our collaboration evolve further?


Do you want that I resend any mails/patches from my "waiting queue"?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278423

These patches are often correct in the same way a stopped clock is
correct twice a day, but I reject the motivation/approach/patch
description.  Just because there is a sanity check does not mean we
should use it (ie, do an insane thing).  It hurts readability to hide
the NULL check.

On the other hand, half the time the NULL checks are superflous because
the pointer is never NULL.  The other half of the time the NULL checks
are there because the code uses one err style error handling.  Also the
error handling code should mirror the allocation code so I feel like it
should be:

	if (some_feature)
		foo = allocate();
	ret = frob();
	if (ret)
		goto free_foo;

free_foo:
	if (some_feature)  // <--- as opposed to if (foo) {
		free(foo);

So anyway I often don't like the original code, but the new code is
even worse and I have never heard a good motivation for these patches
besides that they were generated using a cool tool.  It's not a good
reason.  Other maintainers are accepting these patches so you are free
to send them there.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web