linux-kernel - Re: [PATCH 04/11] trace-cmd: Extract parse_record_options() from trace

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: <1511895433.1754.39.camel@gmail.com>
Date:   Tue, 28 Nov 2017 20:57:13 +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 04/11] trace-cmd: Extract parse_record_options() from
 trace_record()

On Tue, 2017-11-28 at 13:30 -0500, Steven Rostedt wrote:
> On Tue, 28 Nov 2017 20:17:46 +0200
> Vladislav Valtchev <vladislav.valtchev@...il.com> wrote:
> 
> > Since that code was profile-specific, I put it in trace_profile(),
> > without 'if' statements, clearly.
> 
> This is all about balancing. You were able to remove one if statement,
> but required two function calls by all others.
> 

I totally agree that is all about balancing.
I wrote exactly the same thing as part of my previous e-mail (before reading this message).

Are you concerned by the cost the of function calls or by the "verbosity" in calling them?

If it's about their cost, I believe that cold code paths like these, where the program is
just parsing its command line arguments, it seems absolutely OK to me to do that.
It's a one-time thing before any kind of tracing starts. And it helps a lot contributors to
better understand the code (in my opinion).

Otherwise, I believe that having those function calls that way just makes the code simpler
for new contributors. Functions, among everything else, are also "labels" for pieces of code.
Having two different labels there (init context and parse options) makes sense to me.
But, again, I understand that is a trade-off based on my subjective value system and
way of thinking, of course.

Do you have a strong opinion?

Vlad