On 4/30/2010 1:15 AM, stephane eranian wrote:
> Corey,
>
> Glad to see this patch. This functionality had been on my wishlist for some
> time. This is needed to validate the multi-group support. I have a few
> comments,
> see below.
>
>> diff --git a/perf_examples/perf_util.c b/perf_examples/perf_util.c
>> index 4de21a8..3853ddf 100644
>> --- a/perf_examples/perf_util.c
>> +++ b/perf_examples/perf_util.c
>> @@ -34,9 +34,11 @@
>> int
>> perf_setup_argv_events(char **argv, perf_event_desc_t **fd)
>> {
>> - perf_event_desc_t *fdt = NULL;
>> - int num = 0, max_fd = 0, new_max;
>> + static perf_event_desc_t *fdt = NULL;
>> + static int num = 0, max_fd = 0;
>
> I'd rather not have a static variable here.
Yeah, I don't like statics either, but it solved the problem with few code
changes.
> This can lead to error
> if someone reuses that code in a multi-threaded app. See below
> for an alternative.
>
>
>> + for (grp = 0; grp < options.num_groups; grp++) {
>> + num = perf_setup_list_events(options.events[grp], &fds);
>> + if (num < 1)
>> + exit(1);
>> + }
>>
> To avoid the static in perf_setup_list_events(), I would rather have something
> like:
>
> old_num = num;
> num = perf_setup_list_events(options.events[grp], &fds, num);
> if (num == old_num)
> exit(1);
>
> That way the function is re-entrant. And in case the return value is bogus,
> the caller can either decide to continue with fewer groups or fail.
Ok, I will rework the patch and post it again.
Thanks for the review!
--
Regards,
- Corey
Corey Ashford
Software Engineer
IBM Linux Technology Center, Linux Toolchain
Beaverton, OR
503-578-3507
[email protected]
------------------------------------------------------------------------------
_______________________________________________
perfmon2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
