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. 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.
------------------------------------------------------------------------------
_______________________________________________
perfmon2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
