Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Namhyung Kim <namhyung@kernel.org>]

Hi, Steve

On Thu, 14 Jun 2012 23:27:05 -0400, Steven Rostedt wrote:
> On Wed, 2012-06-13 at 12:02 +0900, Namhyung Kim wrote:
>> On Tue, 12 Jun 2012 14:01:54 -0400, Steven Rostedt wrote:
>> > On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
>> >> +int pevent_strerror(struct pevent *pevent, enum pevent_errno errnum,
>> >> +		    char *buf, size_t buflen)
>> >
>> > Hmm, actually I wonder if we should put the error into the pevent
>> > structure. Then we wouldn't even need to waste time to pass the data
>> > through.
>> >
>> > That is, you can simply do:
>> >
>> > 	ret = pevent_foo();
>> > 	if (ret < 0) {
>> > 		pevent_strerr(pevent, buf, buflen);
>> > 		printf("%s\n", buf);
>> > 	}
>> >
>> > Perhaps even include a pevent_perror(), to just do:
>> >
>> > 	if (ret < 0) {
>> > 		pevent_perror(pevent);
>> > 		return ret;
>> > 	}
>> >
>> 
>> I thought something like this, but worried about the thread-safety. What
>> about if more than one thread call pevent functions for a same pevent
>> concurrently?  Should we make the pevent->errno TLS?
>
> I hate threads :-)
>
> Anyway, there's a couple of things that can be done:
>
> 1) have two functions. A simple 'pevent_stderr()' that does the above,
> and perhaps a pevent_get_error() that can be passed the return value of
> a previous command, and give you the error string for it. This is thread
> safe for programs that require it. The ret is always returned.
>

I'm not sure I understood you. Are you saying the pevent_get_error
should look like:

       char *pevent_get_error(int errcode, ...)

? So, what's the difference to my original pevent_strerror ?


> 2) Make it TLS, although honestly I've never made dynamic variables
> that, and didn't even realize that you could.
>

Right, it should be TSD (thread-specific data) for dynamic ones,
I guess.


> I'm liking the explicit 'make this thread safe' method (#1). As it may
> be for a gui, a separate thread may be used to print out the error
> messages, and making it thread unique will make that difficult.
>

Yeah, I like the #1 too.


> If you need the code to be thread safe, have all errors do:
>
> 	ret = pevent_foo();
> 	if (ret < 0) {
> 		pevent_strerr_val(ret, buf, buflen);
>
>
> For programs that do not need to be thread safe, then:
>
> 	ret = pevent_foo();
> 	if (ret < 0) {
> 		pevent_strerr(pevent, buf, buflen);
>

Do we really need these two? I think having a single API is just
enough, IMHO.

Thanks,
Namhyung