Date:   Wed, 29 Nov 2017 12:03:14 +0200
From:   Vladislav Valtchev <vladislav.valtchev@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, y.karadz@...il.com
Subject: Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile
 separate funcs

On Tue, 2017-11-28 at 14:35 -0500, Steven Rostedt wrote:
> 
> Yes. Simply because we lost the fact that we can do it better.
> 

Hi Steven,
of course we can do better, if we make a step further than just
a mechanical refactoring. I'm used to intentionally limit myself
to do such kind of mechanical refactoring, in order to reduce to the minimum
possible the chance of introducing bugs. And it really works pretty well for me.

I intentionally did not even consider to check whether events = 1 was
needed there or not: for me that kind of simplifications should be done
*after* the mechanical refactoring, not while doing it. This way, while
searching for a bug, I can order the commits by likelihood of being the
root-cause of the bug and, when possible, I'm happy to put changes as this one
at the bottom.

Also, I learned that lesson while working in vSphere, by observing (and a few
times doing myself) "innocent" refactoring changes breaking CI and system tests
because of super-subtle side-effects.

In this concrete case, the original code was:

	if ((record = (strcmp(argv[1], "record") == 0)))
		; /* do nothing */
	else if ((start = strcmp(argv[1], "start") == 0))
		; /* do nothing */
	else if ((extract = strcmp(argv[1], "extract") == 0))
		; /* do nothing */
	else if ((stream = strcmp(argv[1], "stream") == 0))
		; /* do nothing */
	else if ((profile = strcmp(argv[1], "profile") == 0)) {
		handle_init = trace_init_profile;
		events = 1;
	} else
		usage(argv);

at the beginning of trace_record().
After that, it followed the big "for" loop with its big "switch".
During the refactoring, I followed this order *strictly* and I'm sure
that my change is correct exactly as much as the original code: I just
moved the things around, but the state (ctx and global variables)
continued to change (evolve) exactly in the same order as in the original
code did.

That's why, from that point-of-view, "it was there before" was the
right answer to me.


> There's a reason I asked about why the "events = 1" was needed? And
> saying "it was there before" is not the right answer. You are changing
> the flow of the code. These are not subtle changes. These are
> legitimate changes to the flow. Even though they are only to make it
> easier to understand. The algorithm is changing. Look again, and tell
> me, why is "events = 1" needed? And for that matter, the setting of
> handle_init. If it is needed, why?
> 

Of course, I understand that the level of code improvement with my algorithm
is certainly not the best we can do: it's just the most conservative approach.
I hope that you at least appreciate my effort to make "big" changes like
this one, without even a single minor bug, thanks to a strict approach: it
still seems to me the less evil thing if compared to the introduction of a
subtle bug in a corner case because of a "too smart move".

Returning back to 'events=1', yes I agree that actually parse_record_options()
never reads its value. The same is true for handle_init. Both the variables
are used in the record_trace() part. The "events" flag is checked before doing
expand_event_list() and before:

for_all_instances(ctx->instance)
	enable_events(ctx->instance);

handle_init is used by start_thread(), which is called by record_trace().

Therefore:

	handle_init = trace_init_profile;
	ctx.events = 1;

can be safely moved after parse_record_options() and, the call of
init_common_record_context() could be moved at the beginning of
parse_record_options().

Are you fine with me doing that in a patch the follows immediately
this patch 8 ?

Vlad

Powered by blists - more mailing lists