From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753517AbdK1Se1 (ORCPT ); Tue, 28 Nov 2017 13:34:27 -0500 Received: from mail-wr0-f172.google.com ([209.85.128.172]:45448 "EHLO mail-wr0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752530AbdK1Se0 (ORCPT ); Tue, 28 Nov 2017 13:34:26 -0500 X-Google-Smtp-Source: AGs4zMaoGZLjzhWgqec2Kq/slnXPU9PYXz7L/k5lPTrSpkqExta4WNL2niOUqc3Z05bvUSvmCCs1RA== Message-ID: <1511894062.1754.21.camel@gmail.com> Subject: Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs From: Vladislav Valtchev To: Steven Rostedt Cc: linux-kernel@vger.kernel.org, y.karadz@gmail.com Date: Tue, 28 Nov 2017 20:34:22 +0200 In-Reply-To: <20171128121450.32db5781@gandalf.local.home> References: <20171123163335.19078-1-vladislav.valtchev@gmail.com> <20171123163335.19078-9-vladislav.valtchev@gmail.com> <20171128121450.32db5781@gandalf.local.home> Organization: VMware Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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