* Re: [PATCH] tracing/histograms: Simplify create_hist_fields()
2022-06-30 1:31 [PATCH] tracing/histograms: Simplify create_hist_fields() Zheng Yejian
@ 2022-06-30 2:24 ` Tom Rix
2022-06-30 6:33 ` Greg KH
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Tom Rix @ 2022-06-30 2:24 UTC (permalink / raw)
To: Zheng Yejian, rostedt, mingo, linux-kernel, tom.zanussi
Cc: stable, zhangjinhao2
On 6/29/22 6:31 PM, Zheng Yejian wrote:
> When I look into implements of create_hist_fields(), I think there can be
> following two simplifications:
> 1. If something wrong happened in parse_var_defs(), free_var_defs() would
> have been called in it, so no need goto free again after calling it;
> 2. After calling create_key_fields(), regardless of the value of 'ret', it
> then always runs into 'out: ', so the judge of 'ret' is redundant.
>
> No functional changes.
>
> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
> ---
> kernel/trace/trace_events_hist.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 2784951e0fc8..832c4ccf41ab 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4454,7 +4454,7 @@ static int create_hist_fields(struct hist_trigger_data *hist_data,
>
> ret = parse_var_defs(hist_data);
> if (ret)
> - goto out;
> + return ret;
>
> ret = create_val_fields(hist_data, file);
> if (ret)
> @@ -4465,8 +4465,7 @@ static int create_hist_fields(struct hist_trigger_data *hist_data,
> goto out;
>
> ret = create_key_fields(hist_data, file);
> - if (ret)
> - goto out;
> +
> out:
> free_var_defs(hist_data);
Reviewed-by: Tom Rix <trix@redhat.com>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] tracing/histograms: Simplify create_hist_fields()
2022-06-30 1:31 [PATCH] tracing/histograms: Simplify create_hist_fields() Zheng Yejian
2022-06-30 2:24 ` Tom Rix
@ 2022-06-30 6:33 ` Greg KH
2022-06-30 9:21 ` Zhengyejian (Zetta)
2022-07-12 17:49 ` Steven Rostedt
2022-07-12 17:52 ` Steven Rostedt
3 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2022-06-30 6:33 UTC (permalink / raw)
To: Zheng Yejian
Cc: rostedt, mingo, linux-kernel, tom.zanussi, trix, stable,
zhangjinhao2
On Thu, Jun 30, 2022 at 09:31:52AM +0800, Zheng Yejian wrote:
> When I look into implements of create_hist_fields(), I think there can be
> following two simplifications:
> 1. If something wrong happened in parse_var_defs(), free_var_defs() would
> have been called in it, so no need goto free again after calling it;
> 2. After calling create_key_fields(), regardless of the value of 'ret', it
> then always runs into 'out: ', so the judge of 'ret' is redundant.
>
> No functional changes.
>
> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
> ---
> kernel/trace/trace_events_hist.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 2784951e0fc8..832c4ccf41ab 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4454,7 +4454,7 @@ static int create_hist_fields(struct hist_trigger_data *hist_data,
>
> ret = parse_var_defs(hist_data);
> if (ret)
> - goto out;
> + return ret;
>
> ret = create_val_fields(hist_data, file);
> if (ret)
> @@ -4465,8 +4465,7 @@ static int create_hist_fields(struct hist_trigger_data *hist_data,
> goto out;
>
> ret = create_key_fields(hist_data, file);
> - if (ret)
> - goto out;
> +
> out:
> free_var_defs(hist_data);
>
> --
> 2.32.0
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] tracing/histograms: Simplify create_hist_fields()
2022-06-30 6:33 ` Greg KH
@ 2022-06-30 9:21 ` Zhengyejian (Zetta)
0 siblings, 0 replies; 7+ messages in thread
From: Zhengyejian (Zetta) @ 2022-06-30 9:21 UTC (permalink / raw)
To: Greg KH
Cc: rostedt, mingo, linux-kernel, tom.zanussi, trix, stable,
zhangjinhao2
On 2022/6/30 14:33, Greg KH wrote:
> On Thu, Jun 30, 2022 at 09:31:52AM +0800, Zheng Yejian wrote:
>> When I look into implements of create_hist_fields(), I think there can be
>> following two simplifications:
>> 1. If something wrong happened in parse_var_defs(), free_var_defs() would
>> have been called in it, so no need goto free again after calling it;
>> 2. After calling create_key_fields(), regardless of the value of 'ret', it
>> then always runs into 'out: ', so the judge of 'ret' is redundant.
>>
>> No functional changes.
>>
>> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
>> ---
>> kernel/trace/trace_events_hist.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
>> index 2784951e0fc8..832c4ccf41ab 100644
>> --- a/kernel/trace/trace_events_hist.c
>> +++ b/kernel/trace/trace_events_hist.c
>> @@ -4454,7 +4454,7 @@ static int create_hist_fields(struct hist_trigger_data *hist_data,
>>
>> ret = parse_var_defs(hist_data);
>> if (ret)
>> - goto out;
>> + return ret;
>>
>> ret = create_val_fields(hist_data, file);
>> if (ret)
>> @@ -4465,8 +4465,7 @@ static int create_hist_fields(struct hist_trigger_data *hist_data,
>> goto out;
>>
>> ret = create_key_fields(hist_data, file);
>> - if (ret)
>> - goto out;
>> +
>> out:
>> free_var_defs(hist_data);
>>
>> --
>> 2.32.0
>>
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read:
This patch is a cleanup, no need to include in stable kernel tree.
I accidentally copied the patch to stable mailbox, sorry for that :(
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tracing/histograms: Simplify create_hist_fields()
2022-06-30 1:31 [PATCH] tracing/histograms: Simplify create_hist_fields() Zheng Yejian
2022-06-30 2:24 ` Tom Rix
2022-06-30 6:33 ` Greg KH
@ 2022-07-12 17:49 ` Steven Rostedt
2022-07-13 10:32 ` Zheng Yejian
2022-07-12 17:52 ` Steven Rostedt
3 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2022-07-12 17:49 UTC (permalink / raw)
To: Zheng Yejian; +Cc: mingo, linux-kernel, tom.zanussi, trix, stable, zhangjinhao2
On Thu, 30 Jun 2022 09:31:52 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:
> When I look into implements of create_hist_fields(), I think there can be
> following two simplifications:
> 1. If something wrong happened in parse_var_defs(), free_var_defs() would
> have been called in it, so no need goto free again after calling it;
> 2. After calling create_key_fields(), regardless of the value of 'ret', it
> then always runs into 'out: ', so the judge of 'ret' is redundant.
>
> No functional changes.
I applied this but removed the "No functional changes" because it is a
functional change. The end result may be the same, but the flow is
different, and that means it changed functionally.
The only time "No functional changes" should be stated is if you move code
around or change #ifdefs to perform the same action. IOW, if the assembly
produced by the compiler is the same before and after your change, you can
say "No functional changes", otherwise don't ever say that.
This is important, because if a bisect lands on this, people may think the
bisect is incorrect, when in reality it could be the cause of the bug (I
just had this happen to me with another commit that had "No functional
changes" :-p )
-- Steve
>
> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tracing/histograms: Simplify create_hist_fields()
2022-07-12 17:49 ` Steven Rostedt
@ 2022-07-13 10:32 ` Zheng Yejian
0 siblings, 0 replies; 7+ messages in thread
From: Zheng Yejian @ 2022-07-13 10:32 UTC (permalink / raw)
To: rostedt
Cc: linux-kernel, mingo, stable, tom.zanussi, trix, zhangjinhao2,
zhengyejian1
On Tue, 12 Jul 2022 13:49:56 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 30 Jun 2022 09:31:52 +0800
> Zheng Yejian <zhengyejian1@huawei.com> wrote:
>
> > When I look into implements of create_hist_fields(), I think there can be
> > following two simplifications:
> > 1. If something wrong happened in parse_var_defs(), free_var_defs() would
> > have been called in it, so no need goto free again after calling it;
> > 2. After calling create_key_fields(), regardless of the value of 'ret', it
> > then always runs into 'out: ', so the judge of 'ret' is redundant.
> >
> > No functional changes.
>
> I applied this but removed the "No functional changes" because it is a
> functional change. The end result may be the same, but the flow is
> different, and that means it changed functionally.
>
> The only time "No functional changes" should be stated is if you move code
> around or change #ifdefs to perform the same action. IOW, if the assembly
> produced by the compiler is the same before and after your change, you can
> say "No functional changes", otherwise don't ever say that.
>
> This is important, because if a bisect lands on this, people may think the
> bisect is incorrect, when in reality it could be the cause of the bug (I
> just had this happen to me with another commit that had "No functional
> changes" :-p )
I learn it now and share it with my colleagues in the neighborhood.
Thanks for your patience :)
>
> -- Steve
>
>
> >
> > Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tracing/histograms: Simplify create_hist_fields()
2022-06-30 1:31 [PATCH] tracing/histograms: Simplify create_hist_fields() Zheng Yejian
` (2 preceding siblings ...)
2022-07-12 17:49 ` Steven Rostedt
@ 2022-07-12 17:52 ` Steven Rostedt
3 siblings, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2022-07-12 17:52 UTC (permalink / raw)
To: Zheng Yejian; +Cc: mingo, linux-kernel, tom.zanussi, trix, stable, zhangjinhao2
On Thu, 30 Jun 2022 09:31:52 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 2784951e0fc8..832c4ccf41ab 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4454,7 +4454,7 @@ static int create_hist_fields(struct hist_trigger_data *hist_data,
>
> ret = parse_var_defs(hist_data);
> if (ret)
> - goto out;
> + return ret;
This is a functional change.
>
> ret = create_val_fields(hist_data, file);
> if (ret)
> @@ -4465,8 +4465,7 @@ static int create_hist_fields(struct hist_trigger_data *hist_data,
> goto out;
>
> ret = create_key_fields(hist_data, file);
> - if (ret)
> - goto out;
> +
> out:
This is not a functional change ;-)
If you just had the latter, then it would have been OK to state "No
functional changes".
-- Steve
> free_var_defs(hist_data);
>
^ permalink raw reply [flat|nested] 7+ messages in thread