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 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? 

thanks,
jirka

> and we cannot turn info.syms into a global
> variable, which is unnecessary. The easiest way is to reimplement a A
> copy of libbpf_kallsyms_parse.
> 
> Modifications to other parts will be carried along with the next
> version.
> 
> -- 
> Jackie
> 
> > 
> > > +	if (!f) {
> > > +		err = -errno;
> > > +		pr_warn("failed to open /proc/kallsyms\n");
> > > +		goto free_infos;
> > > +	}
> > > +
> > > +	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;
> > > +			break;
> > > +		}
> > > +
> > > +		if (!glob_match(sym_name, res->pattern))
> > > +			continue;
> > 
> > hm, we don't need to call glob_match again, we just want to check
> > if the kallsyms symbol is in infos.syms
> > 
> > > +
> > > +		if (!bsearch(&sym_name, infos.syms, infos.cnt, sizeof(void *),
> > > +			     bsearch_compare_function))
> > > +			continue;
> > > +
> > > +		err = libbpf_ensure_mem((void **)&res->addrs, &res->cap,
> > > +					sizeof(unsigned long), res->cnt + 1);
> > > +		if (err)
> > > +			break;
> > > +
> > > +		res->addrs[res->cnt++] = (unsigned long) sym_addr;
> > > +	}
> > 
> > res->cnt is check outside for 0, so we should be find here
> > 
> > jirka
> > 
> > > +
> > > +cleanup:
> > > +	fclose(f);
> > > +free_infos:
> > > +	for (i = 0; i < infos.cnt; i++)
> > > +		free((char *)infos.syms[i]);
> > > +	free(infos.syms);
> > > +
> > > +	return err;
> > >   }
> > >   struct bpf_link *
> > > @@ -10594,7 +10690,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> > >   		return libbpf_err_ptr(-EINVAL);
> > >   	if (pattern) {
> > > -		err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);
> > > +		err = libbpf_available_kallsyms_parse(&res);
> > >   		if (err)
> > >   			goto error;
> > >   		if (!res.cnt) {
> > > -- 
> > > 2.25.1
> > >