From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751055AbbLHVs2 (ORCPT ); Tue, 8 Dec 2015 16:48:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58328 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750748AbbLHVs1 (ORCPT ); Tue, 8 Dec 2015 16:48:27 -0500 Date: Tue, 8 Dec 2015 15:48:25 -0600 From: Josh Poimboeuf To: Arnaldo Carvalho de Melo Cc: Jiri Olsa , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, Namhyung Kim Subject: Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi Message-ID: <20151208214825.GI14846@treble.redhat.com> References: <0374ac2777cfb277f395de8423d670265400eb1b.1449548395.git.jpoimboe@redhat.com> <20151208181625.GB18433@krava.brq.redhat.com> <20151208184953.GG14846@treble.redhat.com> <20151208190931.GK11564@kernel.org> <20151208191700.GH14846@treble.redhat.com> <20151208194026.GL11564@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20151208194026.GL11564@kernel.org> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 08, 2015 at 04:40:26PM -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Dec 08, 2015 at 01:17:00PM -0600, Josh Poimboeuf escreveu: > > On Tue, Dec 08, 2015 at 04:09:31PM -0300, Arnaldo Carvalho de Melo wrote: > > > Em Tue, Dec 08, 2015 at 12:49:53PM -0600, Josh Poimboeuf escreveu: > > > > On Tue, Dec 08, 2015 at 07:16:26PM +0100, Jiri Olsa wrote: > > > > > On Mon, Dec 07, 2015 at 10:21:52PM -0600, Josh Poimboeuf wrote: > > > > > > The perf subcommand framework is needed for other tools. Move > > > > > > parse-options.c and its dependencies over to libapi. > > > > > > > > > > > > Any function names with 'perf' have been renamed to something more > > > > > > generic. > > > > > > > > > > > > Also created a util_cfg struct for passing perf-specific configuration > > > > > > to the library. Specifying the configuration at runtime allows the same > > > > > > binary to be shared by multiple tools without having to recompile it. > > > > > > > > > > this patch is too big.. IMO it needs to be split into 3 parts > > > > > as described in above 3 paragraphs > > > > > > > > Ok, will do. > > > > > > Also please rename this util_cfg struct to something more expressive, > > > breaking down the patch may help in finding a better name, I guess. > > > > I'm certainly open to doing so, but I'm having trouble coming up with a > > better name. The current name makes sense to me, because the struct > > contains various configuration options needed by the libapi "util" code. > > > > Would 'libapi_util_config' be better? Or do you have any other > > suggestions? > > Please break it up into multiple pieces, as suggested by Jiri, in doing > so you may find some better name. > > But since several are related to command environment setup, perhaps > 'struct cmd_exec_env'? IMO, 'struct cmd_exec_env' doesn't describe the struct accurately. I think it tangentially describes some features of some of the fields, but not all of them. That seems more confusing to me. Is your complaint that the name is too vague? If so, that's actually by design, because the struct is meant to be a generic interface for providing various unrelated configuration variables to libapi. I've split the patch up into the above 3 paragraphs as Jiri suggested. But I still don't have any ideas for a name better than 'util_cfg' (other than something more verbose like 'libapi_util_config'). Instead of a single struct, we could consider splitting it into multiple structs (e.g., one for exec_cmd.c, one for parse-options.c, and one for pager.c). But the 'exec_name' field is used by multiple files, so it wouldn't necessarily be a clean split. It would also possibly create more room for error for the users of libapi, since there would then be three config interfaces instead of one. -- Josh