From: David Ahern <dsahern@gmail.com>
To: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Namhyung Kim <namhyung.kim@lge.com>,
Namhyung Kim <namhyung@gmail.com>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Paul Mackerras <paulus@samba.org>, Ingo Molnar <mingo@elte.hu>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/11] perf tools: Introduce struct perf_maps_opts
Date: Mon, 13 Feb 2012 12:19:37 -0700 [thread overview]
Message-ID: <4F396249.2050001@gmail.com> (raw)
In-Reply-To: <20120213190542.GL15955@infradead.org>
On 02/13/2012 12:05 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 13, 2012 at 11:50:29AM -0700, David Ahern escreveu:
>> The cleanup might make my multiple tid/pid patch easier: e.g.,
>>
>> struct perf_target{
>> ...
>> char errmsg[128];
>> };
>>
>> Then if the tid/pid string parsing fails in perf_evlist__create_maps and
>> friends the errmsg can be put into the buffer for the callers to get a
>> more useful message to the user as to what happened.
>>
>> Today's perf if you give it an invalid pid, scandir fails and the
>> command spits out the usage statement. Which is completely confusing --
>> ie., not clear that the command failed b/c the pid does not exist.
>
> Humm, ok, but then I think we should have an enum + a strerror(3)
> equivalent, i.e.:
>
> enum perf_target_error perf_evlist__create_maps(...);
>
> int perf_target__strerror(struct perf_target *target, int errnum,
> char *buf, size_t buflen);
ok, so you are proposing an internal generation of enum error codes and
correlating them to strings rather than adding a buffer into
perf_target. If that's the case perhaps we need a libperf-wide design:
enum perf_error
perf__strerror(enum perf_error)
which effectively taps an array similar to _sys_errlist_internal based
on enum index.
>
> Please see 'man strerror_r", and make it work like the POSIX compliant
> variant.
No globals are in use, so I would expect the _r to be redundant. I have
glibc source; scanning __strerror_r implementation ....
>
> Ok, so it may be better to first process Kim's patches and then you
> rework yours?
The current patch is ready to go; I just don't like the error handling
and lack of a useful message. That said, it is no worse than what
happens today.
David
>
> - Arnaldo
next prev parent reply other threads:[~2012-02-13 19:19 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1329118064-9412-1-git-send-email-namhyung.kim@lge.com>
2012-02-13 7:27 ` [PATCH 01/11] perf tools: Introduce struct perf_maps_opts Namhyung Kim
2012-02-13 7:44 ` [RFC PATCHSET] perf: Fix cpu/thread map and group event handling Namhyung Kim
2012-02-13 18:32 ` [PATCH 01/11] perf tools: Introduce struct perf_maps_opts Arnaldo Carvalho de Melo
2012-02-13 18:50 ` David Ahern
2012-02-13 19:05 ` Arnaldo Carvalho de Melo
2012-02-13 19:19 ` David Ahern [this message]
2012-02-13 20:12 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 02/11] perf stat: Convert to perf_maps_opts Namhyung Kim
2012-02-13 18:33 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 03/11] perf top: " Namhyung Kim
2012-02-13 7:27 ` [PATCH 04/11] perf tools: Introduce check_target_maps() helper Namhyung Kim
2012-02-13 18:36 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 05/11] perf tools: Make perf_evlist__create_maps() take struct perf_maps_opts Namhyung Kim
2012-02-13 18:36 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 06/11] perf tools: Check more combinations of PID/TID, UID and CPU switches Namhyung Kim
2012-02-13 7:27 ` [PATCH 07/11] perf tools: Fix creation of cpu map Namhyung Kim
2012-02-13 7:27 ` [PATCH 08/11] perf tools: Consolidate target task/cpu checking Namhyung Kim
2012-02-13 18:39 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 09/11] perf stat: Use perf_evlist__create_maps Namhyung Kim
2012-02-13 18:40 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 10/11] perf stat: Fix event grouping on forked task Namhyung Kim
2012-02-13 18:41 ` Arnaldo Carvalho de Melo
2012-02-14 1:20 ` [PATCH] " Namhyung Kim
2012-02-13 7:27 ` [PATCH 11/11] perf record: " Namhyung Kim
2012-02-13 18:42 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F396249.2050001@gmail.com \
--to=dsahern@gmail.com \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@ghostprotocols.net \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=namhyung.kim@lge.com \
--cc=namhyung@gmail.com \
--cc=paulus@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).