Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 29 Nov 2017 12:03:14 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> 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.

I totally agree that one shouldn't think about this level of factoring
when doing only mechanical refactoring. But the issue is what's the
best mechanical refactoring you can do to let you see new changes like
this? Note, this is not a bug fix, it becomes another clean up. Having
it as an if statement within one function, makes it stick out more than
breaking up a function into two and having all users call two
functions. The two function approach will "hide" this issue. Again,
it's not a bug in the way it was done, but we miss the change because
it is more likely not to be seen.

> 
> 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.

And I agree.
> 
> 
> > 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 ?

Yes, but I would like init_common_record_context() to be merged into
parse_record_options() in the previous patches and we add that if
statement to make profile different :-)

I see that you see many of the parts of this elephant. Some looks like
a rope, some a fire hose, some a tree trunk, and some a wall. But when
you put it all together, you see the elephant as a whole. I'm trying to
have you see the whole elephant ;-)

The point I'm making is that the way something is refactored, should
start out not breaking the flow of logic. The original code technically
had an if statement for the profile (but it was in a switch statement).
That if statement should have stayed throughout the mechanical
refactoring. And then after the refactoring was done, we could look at
seeing if more can be cleaned up. And then we would see that the if
statement was not necessary. But by breaking up the flow into two
functions, that extra code would be hidden, and we would have
unnecessarily have two functions.

The way mechanical updates happen does matter, to help improve the
less trivial updates. That's the point I'm trying to make.

-- Steve