Hi Hemant,
This series looks good to me so far. As discussed, it'd be good to
get some tests in place so this is eligible for next weeks release.
Only other questions/nitpicks are (mainly) in this final patch...
> snbep_unc_imc0::UNC_M_CAS_COUNT:WR node perf_scale
The perf_scale is optional right? (likewise the derived_scale from
patch #2?) ... so compatibility with existing config files is kept.
> --- a/src/pmdas/perfevent/perfinterface.c
> +++ b/src/pmdas/perfevent/perfinterface.c
> @@ -180,6 +186,298 @@ static void free_event_list(event_list_t *event_list)
> }
>
> /*
> + * Utility function to fetch the contents of a
> + * file(in "path") to "buf"
> + */
> +static int get_file_string(char *path, char *buf)
> +{
> + FILE *fp;
> + fp = fopen(path, "r");
> + if (NULL == fp)
> + return -1;
> + else
> + fscanf(fp, "%s", buf);
This looks like a potential buffer overrun, depending on the contents
of the path file (not likely though, given this is very specific data
coming from the kernel?). Since all callers to get_file_string alloc
buf just prior to calling, maybe we could use the %m specifier here &
get the buffer allocated for us - see EXAMPLE at end of scanf(3).
> + sprintf(events_path, "%s/events/", device_path);
Similarly, there's a few cases of sprintf use, where we'd usually go
with snprintf and ensure right-sizing of destination buffers.
cheers.
--
Nathan
|