Messages in this thread | | | Subject | Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs | From | Vladislav Valtchev <> | Date | Wed, 29 Nov 2017 12:03:14 +0200 |
| |
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
| |