On 04/21/2016 01:14 PM, Nathan Scott wrote:
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.
Excellent, thanks a lot for the review.
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.
Absolutely. Due to some stupid reason, the hunk for perfevent.conf is
missing from this patch which has specified the syntax as :
event_name [cpu_option] [scale|perf_scale].
--- 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).
Right. I have done away with fscanf() altogether and used getline()
instead in the v2.
+ 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.
Yup, replaced them with snprintf in v2.
- Thanks,
Hemant Kumar
--
Thanks,
Hemant Kumar
|