Re: [PATCH v3 1/2] libbpf: kprobe.multi: cross filter using available_filter_functions and kallsyms

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Jul 04, 2023 at 04:07:48PM +0200, Jiri Olsa wrote:
> On Tue, Jul 04, 2023 at 09:33:15AM +0800, Jackie Liu wrote:
> 
> SNIP
> 
> > > 
> > > should you check if you found anything (infos.cnt != 0) and return early
> > > if there's nothing found
> > > 
> > > > +
> > > > +	/* sort available functions */
> > > > +	qsort(infos.syms, infos.cnt, sizeof(void *), qsort_compare_function);
> > > > +
> > > > +	f = fopen("/proc/kallsyms", "r");
> > > 
> > > why not use libbpf_kallsyms_parse for kallsyms parsing? the call below
> > > would be in its callback
> > 
> > This place cannot directly use libbpf_kallsyms_parse, because we need
> > info.syms, this value cannot be passed into the parameters of
> > libbpf_kallsyms_parse, 
> 
> hum, libbpf_kallsyms_parse takes 'void *ctx', so you can pass anything
> you want right? 

somthing like below should save some lines and ease up error handling

I'd add similar parse functions for both available_filter_functions and
available_filter_functions_addrs and add the logic to callbacks

jirka


---
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b9282ef3f8a7..04b980293240 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10559,8 +10559,31 @@ static int bsearch_compare_function(const void *a, const void *b)
 	return strcmp((const char *)a, *(const char **)b);
 }
 
+struct avail_kallsyms_data {
+	const char **syms;
+	struct kprobe_multi_resolve *res;
+};
+
+static int avail_kallsyms_cb(unsigned long long sym_addr, char sym_type,
+			     const char *sym_name, void *ctx)
+{
+	struct avail_kallsyms_data *data = ctx;
+	struct kprobe_multi_resolve *res = data->res;
+
+	if (!bsearch(&sym_name, data->syms, cnt, sizeof(void *), bsearch_compare_function))
+		continue;
+
+	err = libbpf_ensure_mem((void **)&res->addrs, &res->cap,
+				sizeof(unsigned long), res->cnt + 1);
+	if (err)
+		return err;
+	res->addrs[res->cnt++] = (unsigned long) sym_addr;
+	return 0;
+}
+
 static int libbpf_available_kallsyms_parse(struct kprobe_multi_resolve *res)
 {
+	struct avail_kallsyms_data data;
 	char sym_name[500];
 	const char *available_functions_file = tracefs_available_filter_functions();
 	FILE *f;
@@ -10614,42 +10637,13 @@ static int libbpf_available_kallsyms_parse(struct kprobe_multi_resolve *res)
 	/* sort available functions */
 	qsort(syms, cnt, sizeof(void *), qsort_compare_function);
 
-	f = fopen("/proc/kallsyms", "r");
-	if (!f) {
-		err = -errno;
-		pr_warn("failed to open /proc/kallsyms\n");
-		goto free_syms;
-	}
-
-	while (true) {
-		unsigned long long sym_addr;
-
-		ret = fscanf(f, "%llx %*c %499s%*[^\n]\n", &sym_addr, sym_name);
-		if (ret == EOF && feof(f))
-			break;
-
-		if (ret != 2) {
-			pr_warn("failed to read kallsyms entry: %d\n", ret);
-			err = -EINVAL;
-			goto cleanup;
-		}
-
-		if (!bsearch(&sym_name, syms, cnt, sizeof(void *), bsearch_compare_function))
-			continue;
-
-		err = libbpf_ensure_mem((void **)&res->addrs, &res->cap,
-					sizeof(unsigned long), res->cnt + 1);
-		if (err)
-			goto cleanup;
-
-		res->addrs[res->cnt++] = (unsigned long) sym_addr;
-	}
+	data.syms = syms;
+	data.res = res;
+	libbpf_kallsyms_parse(avail_kallsyms_cb, res);
 
 	if (!res->cnt)
 		err = -ENOENT;
 
-cleanup:
-	fclose(f);
 free_syms:
 	for (i = 0; i < cnt; i++)
 		free((char *)syms[i]);