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]

Message-ID: <1511894062.1754.21.camel@gmail.com>
Date:   Tue, 28 Nov 2017 20:34:22 +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 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.

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

Clearly, I fully understand that the location of that "right balance" is pretty subjective.

Do you have a strong opinion on that?

Vlad