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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Nov 2017 14:35:35 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Vladislav Valtchev <vladislav.valtchev@...il.com>
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, 28 Nov 2017 20:34:22 +0200
Vladislav Valtchev <vladislav.valtchev@...il.com> wrote:

> On Tue, 2017-11-28 at 12:14 -0500, Steven Rostedt wrote:	
> > As everything is doing both init_common_record_context() and
> > parse_record_options() why not just move the
> > init_common_record_context() into parse_record_options(). If you need
> > to do something special (like set events = 1 for profile) have that
> > done in parse_record_options() if the cmd is CMD_record. Yes, you need
> > to pass the CMD_* to parse_record_options() in this case.
> >   
> 
> Actually, I spent some effort trying to avoid exactly that:
> "if" statments in "overly generic code". In my view, the point
> of having several trace_* functions is to be able to write specific code
> for them, without conditional branches.

But not if they are not necessary.

> 
> I understand that, clearly, there is a trade-off because the extreme of that is to
> have a different copy of a function like parse_record_options() for each command.
> On the other hand, when you have an IF (cmd is PROFILE) at the beginning and an
> if (cmd is PROFILE) at the end, it looks to me a good thing to move that code in the 
> specific trace_profile() function. It's all about finding the right balance:

It's a single file. If this was a library function, then sure, I could
understand. But the if would be useful to see how things are different
in one place. The only reason you broke it up, was because of one user.
Are you sure that one user couldn't do it better?

If we left the if statement in, you would see the answer would be yes!

> the previous extreme (before the refactoring) was to have everything in trace_record()
> with much more if statements and that looked more complicated to follow, at least for me.

Obviously it wasn't for me ;-)

> 
> Clearly, I fully understand that the location of that "right balance" is pretty subjective.
> 
> Do you have a strong opinion on that?

Yes. Simply because we lost the fact that we can do it better.

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?

-- Steve