Re: [PATCH 2/7] libperf: Introduce perf_{evsel, evlist}__open_opt with extensible struct opts

[Namhyung Kim]

Hello,

On Fri, Apr 22, 2022 at 2:44 AM Shunsuke Nakamura
<nakamura.shun@xxxxxxxxxxx> wrote:
>
> Introduce perf_{evsel, evlist}__open_opt with extensible structure opts.
> The mechanism of the extensible structure opts imitates
> tools/lib/bpf/libbpf.h. Currently, only open_flags is supported for the
> opts structure.
>
> Signed-off-by: Shunsuke Nakamura <nakamura.shun@xxxxxxxxxxx>
> ---
[SNIP]
> diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
> index 210ea7c06ce8..00c0cea43b52 100644
> --- a/tools/lib/perf/evsel.c
> +++ b/tools/lib/perf/evsel.c
> @@ -16,8 +16,12 @@
>  #include <internal/lib.h>
>  #include <linux/string.h>
>  #include <sys/ioctl.h>
> +#include <signal.h>
> +#include <fcntl.h>
> +#include <sys/types.h>
>  #include <sys/mman.h>
>  #include <asm/bug.h>
> +#include "internal.h"
>
>  void perf_evsel__init(struct perf_evsel *evsel, struct perf_event_attr *attr,
>                       int idx)
> @@ -26,6 +30,7 @@ void perf_evsel__init(struct perf_evsel *evsel, struct perf_event_attr *attr,
>         evsel->attr = *attr;
>         evsel->idx  = idx;
>         evsel->leader = evsel;
> +       evsel->open_flags = 0;

In general, you don't need to reset it to zero as it's allocated with
zalloc().

>  }
>
[SNIP]
> diff --git a/tools/lib/perf/internal.h b/tools/lib/perf/internal.h
> index 2c27e158de6b..4b91a087ed62 100644
> --- a/tools/lib/perf/internal.h
> +++ b/tools/lib/perf/internal.h
> @@ -20,4 +20,48 @@ do {                            \
>  #define pr_debug2(fmt, ...)     __pr(LIBPERF_DEBUG2, fmt, ##__VA_ARGS__)
>  #define pr_debug3(fmt, ...)     __pr(LIBPERF_DEBUG3, fmt, ##__VA_ARGS__)
>
> +static inline bool libperf_is_mem_zeroed(const char *p, ssize_t len)
> +{
> +       while (len > 0) {
> +               if (*p)
> +                       return false;
> +               p++;
> +               len--;
> +       }
> +       return true;
> +}
> +
> +static inline bool libperf_validate_opts(const char *opts,
> +                                        size_t opts_sz, size_t user_sz,
> +                                        const char *type_name)
> +{
> +       if (user_sz < sizeof(size_t)) {
> +               pr_warning("%s size (%zu) is too small\n", type_name, user_sz);
> +               return false;
> +       }
> +       if (!libperf_is_mem_zeroed(opts + opts_sz, (ssize_t)user_sz - opts_sz)) {

I don't think the cast is necessary since it'll be promoted again
to size_t due to opts_sz.  Instead, we can check if user_sz is
greater than opts_sz explicitly and call the function.

> +               pr_warning("%s has non-zero extra bytes\n", type_name);
> +               return false;
> +       }
> +       return true;
> +}
> +
> +#define offsetofend(TYPE, FIELD) \
> +       (offsetof(TYPE, FIELD) + sizeof(((TYPE *)0)->FIELD))
> +
> +#define OPTS_VALID(opts, type)                                                 \
> +       (!(opts) || libperf_validate_opts((const char *)opts,                   \
> +                                         offsetofend(struct type,              \
> +                                                     type##__last_field),      \
> +                                               (opts)->sz, #type))
> +#define OPTS_HAS(opts, field) \
> +       ((opts) && opts->sz >= offsetofend(typeof(*(opts)), field))
> +#define OPTS_GET(opts, field, fallback_value) \
> +       (OPTS_HAS(opts, field) ? (opts)->field : fallback_value)

It'd be nice if you add a blank line between the macros.

Thanks,
Namhyung


> +#define OPTS_SET(opts, field, value)           \
> +       do {                                    \
> +               if (OPTS_HAS(opts, field))      \
> +                       (opts)->field = value;  \
> +       } while (0)
> +
>  #endif /* __LIBPERF_INTERNAL_H */
> diff --git a/tools/lib/perf/libperf.map b/tools/lib/perf/libperf.map
> index 190b56ae923a..eeeb3075e092 100644
> --- a/tools/lib/perf/libperf.map
> +++ b/tools/lib/perf/libperf.map
> @@ -25,6 +25,7 @@ LIBPERF_0.0.1 {
>                 perf_evsel__enable;
>                 perf_evsel__disable;
>                 perf_evsel__open;
> +               perf_evsel__open_opts;
>                 perf_evsel__close;
>                 perf_evsel__mmap;
>                 perf_evsel__munmap;
> @@ -36,6 +37,7 @@ LIBPERF_0.0.1 {
>                 perf_evlist__new;
>                 perf_evlist__delete;
>                 perf_evlist__open;
> +               perf_evlist__open_opts;
>                 perf_evlist__close;
>                 perf_evlist__enable;
>                 perf_evlist__disable;
> --
> 2.25.1
>