Message-ID: <20151210125553.GA17996@kernel.org>
Date:	Thu, 10 Dec 2015 09:55:53 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Ingo Molnar <mingo@...nel.org>, Jiri Olsa <jolsa@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	Namhyung Kim <namhyung@...nel.org>,
	Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH v2 14/14] perf tools: Move subcommand framework and
 related utils to libapi

Em Wed, Dec 09, 2015 at 12:59:15PM -0600, Josh Poimboeuf escreveu:
> On Wed, Dec 09, 2015 at 12:58:08PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Dec 09, 2015 at 06:33:15AM -0600, Josh Poimboeuf escreveu:
> > > On Wed, Dec 09, 2015 at 09:03:43AM +0100, Ingo Molnar wrote:
> > > > 
> > > > * Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > Humm, and now that you talk... libapi was supposed to be just sugar coating 
> > > > > > kernel APIs, perhaps we need to put it somewhere else in tools/lib/ than in 
> > > > > > tools/lib/api/?
> > > > > 
> > > > > Ah, I didn't realize libapi was a kernel API abstraction library.  Shall we put 
> > > > > it in tools/lib/util instead?
> > > > 
> > > > Yay, naming discussion! ;-)
> > > 
> > > Oh boy! ;-)
> > > 
> > > > So if this is about abstracting out the (Git derived) command-line option parsing 
> > > > UI and help system, 'util' sounds a bit too generic.
> > > > 
> > > > We could call it something like 'lib/cmdline', 'lib/options'?
> > > > 
> > > > The (old) argument against making too finegrained user-space libraries was that 
> > > > shared libraries do have extra runtime costs - this thinking resulted in catch-all 
> > > > super-libraries like libgtk:
> > > > 
> > > >   size /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
> > > >      text    data     bss     dec     hex filename
> > > >   7199789   57712   15128 7272629  6ef8b5 /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
> > > > 
> > > > But in tools/ we typically link the libraries statically so there's no shared 
> > > > library cost to worry about. (Build time linking is a good idea anyway, should we 
> > > > ever want to make use of link-time optimizations. It also eliminates version skew 
> > > > and library compatibility breakage.)
> > > > 
> > > > The other reason for the emergence of super-libraries was the high setup cost of 
> > > > new libraries: it's a lot easier to add yet another unrelated API to libgtk than 
> > > > to start up a whole new project and a new library. But this setup cost is very low 
> > > > in tools/ - one of the advantage of shared repositories.
> > > > 
> > > > So I think in tools/lib/ we can continue to do a clean topical separation of 
> > > > libraries, super-libraries are not needed.
> > > 
> > > I definitely agree that for the reasons you outlined, something like
> > > 'lib/cmdline' would be a good idea.  Except... there's a wrinkle, of
> > > course.
> > > 
> > > The library also includes non-cmdline-related dependencies.  And these
> > > dependencies are directly used by perf as well.  So if we name it
> > > 'cmdline', perf would have includes like:
> > > 
> > > #include <cmdline/pager.h>
> > > #include <cmdline/strbuf.h>
> > > #include <cmdline/term.h>
> > > #include <cmdline/wrapper.h>
> > > ...etc...
> > > 
> > > So it would be using several functions from the 'cmdline' library which
> > > are unrelated to 'cmdline'.
> > > 
> > > For that reason I would vote to name it 'lib/util'.  But I don't really
> > > care, I'd be ok with 'lib/marshmallow' if that's what you guys wanted
> > > :-)
> > 
> > Right, now you see why this wasn't librarised before, huh? Untangling
> > bits in a way that this gets sane takes a bit of time.
> > 
> > I'm going thru your patchkit to erode it a bit, taking uncontroversial
> > patches.
> > 
> > I also would just do one thing first, i.e. just move the cmdline parts
> > to lib/cmdline/, then we would look at the rest. I.e. reduce the problem
> > first.
> > 
> > Yeah, I haven't looked deeply how difficult that would be :-\
> 
> Ok.  I've taken a deeper look at how we could just have a 'cmdline'
> library without the extra unrelated utils.
> 
> (BTW, I actually think a name like 'subcmd' would be a better fit than
> 'cmdline'.  Because it deals not only with the cmdline, but more
> specifically with subcommands, as well as the exec'ing of external
> subcommands and other subprograms.  And any program that wants to have a
> "perf"- or "git"-like "subcommand" interface would use it, thus 'subcmd'
> is a more natural fit.)

Ack, see? We'd eventually get to some better name than "util" :-)
 
> I looked at the files which are unrelated to subcommands and which are
> used by both the subcmd code and perf:
> 
> - abspath.c: needed by exec_cmd.c for the make_nonrelative_path()
>   function, but it's a small function which can just be duplicated by
>   copying it into exec_cmd.c.

ack
   
> - ctype.c: used by parse-options.c for tolower(), but it's not strictly
>   necessary; instead the glibc version of tolower() can be used.

ack
 
> - pager.c: this isn't directly 'cmdline' related, but does fit the theme
>   of 'subcmd', since it pipes a child process to 'less'.  So it could
>   reasonably live in the library.

ack
 
> - strbuf.c: used sparingly by parse-options.c, exec_cmd.c, and help.c.
>   I think all the uses can be replaced rather easily with calls to
>   sprintf() and similar glibc string functions.

go for it, use asprintf, probably is all that is needed, it will
allocate and format, extending would be just using existing stuff plus
new call to asprintf.
 
>   (Another option would be to duplicate the ~150 lines of strbuf.c
>   inside the library.  That would require renaming all the functions and
>   structs in order to avoid duplicate symbol errors when linking with
>   perf.)
> 
> - term.c: used by help.c for the get_term_dimensions() function, which
>   is a small function which can be duplicated in help.c.
> 
> - usage.c: used in several places for die() and error(), but these are
>   trivial functions which can be duplicated.
> 
> - wrapper.c: used in a few places for ALLOC_GROW() and xrealloc(), but
>   ALLOC_GROW() can be duplicated locally and xrealloc() can be replaced
>   by the use of realloc().
> 
> So in summary, with a small amount of code duplication, and a little
> rewrite of the strbuf usage, I think I can extract a libsubcmd rather
> cleanly.
> 
> How does that sound?

Just fine, proceed :-)

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists