Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1286200 > unrolled thread

[PATCH v2 00/14] perf tools: Move perf subcommand framework into lib/tools

Started byJosh Poimboeuf <jpoimboe@redhat.com>
First post2015-12-08 05:30 +0100
Last post2015-12-10 16:20 +0100
Articles 11 on this page of 31 — 5 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH v2 00/14] perf tools: Move perf subcommand framework into lib/tools Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
    [PATCH v2 02/14] perf: Use -iquote for local include paths Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
    [PATCH v2 10/14] perf: Move cmd_version() to builtin-version.c Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
      [tip:perf/core] perf tools: Move cmd_version()   to builtin-version.c tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-10 09:20 +0100
    [PATCH v2 11/14] perf: Move help_unknown_cmd() to its own file Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
    [PATCH v2 09/14] perf: Remove check for unused PERF_PAGER_IN_USE Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
    [PATCH v2 06/14] perf: Split up cache.h Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
    [PATCH v2 04/14] perf: Move term functions out of util.c Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
      Re: [PATCH v2 04/14] perf: Move term functions out of util.c Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 17:00 +0100
      [tip:perf/core] perf tools: Move term functions out of util.c tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-10 09:20 +0100
    [PATCH v2 08/14] perf: Save cmdline arguments earlier Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
      [tip:perf/core] perf tools: Save cmdline arguments earlier tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-10 09:20 +0100
    [PATCH v2 07/14] perf: Remove cache.h Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
    [PATCH v2 13/14] perf tools: Move tools/lib/string.c to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 05:30 +0100
    Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 20:20 +0100
      Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-08 20:50 +0100
        Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 22:50 +0100
          Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-08 23:30 +0100
            Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-09 00:10 +0100
              Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Ingo Molnar <mingo@kernel.org> - 2015-12-09 09:10 +0100
                Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-09 13:40 +0100
                  Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 17:00 +0100
                    Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-09 20:00 +0100
                      Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Namhyung Kim <namhyung@kernel.org> - 2015-12-10 04:00 +0100
                        Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-10 16:00 +0100
                          Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-10 22:40 +0100
                            Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-11 12:30 +0100
                      Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-10 14:00 +0100
                        Re: [PATCH v2 14/14] perf tools: Move subcommand framework and  related utils to libapi Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-10 16:20 +0100
    Re: [PATCH v2 00/14] perf tools: Move perf subcommand framework into  lib/tools Namhyung Kim <namhyung@kernel.org> - 2015-12-10 05:10 +0100
      Re: [PATCH v2 00/14] perf tools: Move perf subcommand framework into  lib/tools Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-10 16:20 +0100

Page 2 of 2 — ← Prev page 1 [2]


#1287468 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-12-09 13:40 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qDMci-8ov-9@gated-at.bofh.it>
In reply to#1287172
On Wed, Dec 09, 2015 at 09:03:43AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.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
:-)

Thoughts?

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

[toc] | [prev] | [next] | [standalone]


#1287602 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-09 17:00 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qDPjR-1RQ-43@gated-at.bofh.it>
In reply to#1287468
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@redhat.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 :-\

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

[toc] | [prev] | [next] | [standalone]


#1287736 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-12-09 20:00 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qDS81-3Fq-1@gated-at.bofh.it>
In reply to#1287602
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@redhat.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.)

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

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

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

  (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?

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

[toc] | [prev] | [next] | [standalone]


#1288153 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromNamhyung Kim <namhyung@kernel.org>
Date2015-12-10 04:00 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qDZCx-8tE-9@gated-at.bofh.it>
In reply to#1287736
Hi Josh,

On Wed, Dec 09, 2015 at 12:59:15PM -0600, Josh Poimboeuf wrote:
> 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@redhat.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.)

'subcmd' looks good to me too.

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

I vaguely recall that it's related to a locale issue.  But I don't
know what was the problem exactly.

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

Not sure it's ok to call die() or similar in a library.  The error
should be reported to the caller rather than exiting inside unless
explicitly requested like in usage_with_options() IMHO.

Thanks,
Namhyung


> 
> - 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?
> 
> -- 
> Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1288552 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-12-10 16:00 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qEaRk-7yx-19@gated-at.bofh.it>
In reply to#1288153
On Thu, Dec 10, 2015 at 10:40:39AM +0900, Namhyung Kim wrote:
> Hi Josh,
> 
> On Wed, Dec 09, 2015 at 12:59:15PM -0600, Josh Poimboeuf wrote:
> > 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@redhat.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.)
> 
> 'subcmd' looks good to me too.

Ok, thanks!

> > 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.
> >   
> > - ctype.c: used by parse-options.c for tolower(), but it's not strictly
> >   necessary; instead the glibc version of tolower() can be used.
> 
> I vaguely recall that it's related to a locale issue.  But I don't
> know what was the problem exactly.

Ah, I think you're right.  From the ctype man page:

  If c is a uppercase letter, tolower() returns its lowercase
  equivalent, if a lowercase representation exists in the current
  locale.

In this case, tolower() is just used for sorting the printed option list
by the lowercase version of their short option.  IMO it's fine to use
the glibc locale-specific conversion here.

> > - 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.
> > 
> > - 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.
> > 
> >   (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.
> 
> Not sure it's ok to call die() or similar in a library.  The error
> should be reported to the caller rather than exiting inside unless
> explicitly requested like in usage_with_options() IMHO.

Thanks, good point.  I'll try to remove all exits from the library
(except for the explicit requests).

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

[toc] | [prev] | [next] | [standalone]


#1288943 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-12-10 22:40 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qEh6q-3f7-31@gated-at.bofh.it>
In reply to#1288552
On Thu, Dec 10, 2015 at 08:54:45AM -0600, Josh Poimboeuf wrote:
> On Thu, Dec 10, 2015 at 10:40:39AM +0900, Namhyung Kim wrote:
> > > - usage.c: used in several places for die() and error(), but these are
> > >   trivial functions which can be duplicated.
> > 
> > Not sure it's ok to call die() or similar in a library.  The error
> > should be reported to the caller rather than exiting inside unless
> > explicitly requested like in usage_with_options() IMHO.
> 
> Thanks, good point.  I'll try to remove all exits from the library
> (except for the explicit requests).

As it turns out, some special options like '--list-opts' and
'--list-cmds' are implemented within parse_options_subcommand(), which
then does an exit().  If those exit()'s were replaced with negative
return codes, we'd have to provide a way for callers to distinguish
between a normal early exit and a real error (in which the usage
printout might be appropriate).  That would be a disruptive change and
require the 40+ callers of the parse_options*() functions to have more
complexity (because they'd need to check for more return conditions).

So I'm thinking I'll leave the code as it is for now and just document
the fact that these functions can exit().

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

[toc] | [prev] | [next] | [standalone]


#1289444 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-11 12:30 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qEu3F-3z9-23@gated-at.bofh.it>
In reply to#1288943
Em Thu, Dec 10, 2015 at 03:35:24PM -0600, Josh Poimboeuf escreveu:
> On Thu, Dec 10, 2015 at 08:54:45AM -0600, Josh Poimboeuf wrote:
> > On Thu, Dec 10, 2015 at 10:40:39AM +0900, Namhyung Kim wrote:
> > > > - usage.c: used in several places for die() and error(), but these are
> > > >   trivial functions which can be duplicated.
> > > 
> > > Not sure it's ok to call die() or similar in a library.  The error
> > > should be reported to the caller rather than exiting inside unless
> > > explicitly requested like in usage_with_options() IMHO.
> > 
> > Thanks, good point.  I'll try to remove all exits from the library
> > (except for the explicit requests).
> 
> As it turns out, some special options like '--list-opts' and
> '--list-cmds' are implemented within parse_options_subcommand(), which
> then does an exit().  If those exit()'s were replaced with negative
> return codes, we'd have to provide a way for callers to distinguish
> between a normal early exit and a real error (in which the usage
> printout might be appropriate).  That would be a disruptive change and
> require the 40+ callers of the parse_options*() functions to have more
> complexity (because they'd need to check for more return conditions).

Well, what you're doing is an improvement, so lets go with it. But I'm
not afraid of at some point doing a N+ files change if that makes sense,
and I find any panic() like libary function to be horrible and in need
of removing.
 
> So I'm thinking I'll leave the code as it is for now and just document
> the fact that these functions can exit().

Fair enough, as code using this library will live in tools/ we can
always change all the callers in one go when we see fit.

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

[toc] | [prev] | [next] | [standalone]


#1288496 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-10 14:00 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qE8Zc-6lY-1@gated-at.bofh.it>
In reply to#1287736
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@redhat.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1288582 — Re: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-12-10 16:20 +0100
SubjectRe: [PATCH v2 14/14] perf tools: Move subcommand framework and related utils to libapi
Message-ID<qEbaH-7V7-41@gated-at.bofh.it>
In reply to#1288496
On Thu, Dec 10, 2015 at 09:55:53AM -0300, Arnaldo Carvalho de Melo wrote:
> 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@redhat.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" :-)

Yes, this is much better than both 'libutil' and 'libmarshmallow' ;-)

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

Thanks!  Working on v3 now.

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

[toc] | [prev] | [next] | [standalone]


#1288187 — Re: [PATCH v2 00/14] perf tools: Move perf subcommand framework into lib/tools

FromNamhyung Kim <namhyung@kernel.org>
Date2015-12-10 05:10 +0100
SubjectRe: [PATCH v2 00/14] perf tools: Move perf subcommand framework into lib/tools
Message-ID<qE0Ii-11h-23@gated-at.bofh.it>
In reply to#1286200
On Mon, Dec 07, 2015 at 10:21:38PM -0600, Josh Poimboeuf wrote:
> Ingo suggested that I factor out the perf subcommand functionality
> (originally copied from git) into tools/lib so that it can be used by
> stacktool[*] and possibly others.
> 
> All the subcommand option handling files have been moved into libapi,
> including parse-options.c, run-command.c, exec_cmd.c, help.c, usage.c,
> and their dependencies.  Since several of the dependencies are more
> general-purpose "utilities" which aren't directly related to
> subcommands, I put everything in 'tools/lib/api/util'.  They're linked
> into perf as part of the libapi.a library.
> 
> Patches 1-13 do some cleanups and splitting up of the code in
> preparation for the move.
> 
> Patch 14 does the actual moving of the files into tools/lib/api/util.
> 
> Based on tip/perf/core.
> 
> [*] https://lkml.kernel.org/r/cover.1445443144.git.jpoimboe@redhat.com

Is there a public repo that I can pull and test?

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

[toc] | [prev] | [next] | [standalone]


#1288574 — Re: [PATCH v2 00/14] perf tools: Move perf subcommand framework into lib/tools

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-12-10 16:20 +0100
SubjectRe: [PATCH v2 00/14] perf tools: Move perf subcommand framework into lib/tools
Message-ID<qEbaF-7V7-17@gated-at.bofh.it>
In reply to#1288187
On Thu, Dec 10, 2015 at 11:00:12AM +0900, Namhyung Kim wrote:
> On Mon, Dec 07, 2015 at 10:21:38PM -0600, Josh Poimboeuf wrote:
> > Ingo suggested that I factor out the perf subcommand functionality
> > (originally copied from git) into tools/lib so that it can be used by
> > stacktool[*] and possibly others.
> > 
> > All the subcommand option handling files have been moved into libapi,
> > including parse-options.c, run-command.c, exec_cmd.c, help.c, usage.c,
> > and their dependencies.  Since several of the dependencies are more
> > general-purpose "utilities" which aren't directly related to
> > subcommands, I put everything in 'tools/lib/api/util'.  They're linked
> > into perf as part of the libapi.a library.
> > 
> > Patches 1-13 do some cleanups and splitting up of the code in
> > preparation for the move.
> > 
> > Patch 14 does the actual moving of the files into tools/lib/api/util.
> > 
> > Based on tip/perf/core.
> > 
> > [*] https://lkml.kernel.org/r/cover.1445443144.git.jpoimboe@redhat.com
> 
> Is there a public repo that I can pull and test?

When I post v3 (probably later today), I'll add a repo link.

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

[toc] | [prev] | [standalone]


Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel


csiph-web