On Tue, 18 Apr 2023 at 21:01, Kui-Feng Lee <thinker.li@xxxxxxxxx> wrote:
>
> You can include an optional path after specifying the object name for the
> 'struct_ops register' subcommand.
>
> Since the commit 226bc6ae6405 ("Merge branch 'Transit between BPF TCP
> congestion controls.'") has been accepted, it is now possible to create a
> link for a struct_ops. This can be done by defining a struct_ops in
> SEC(".struct_ops.link") to make libbpf returns a real link. If we don't pin
> the links before leaving bpftool, they will disappear. To instruct bpftool
> to pin the links in a directory with the names of the maps, we need to
> provide the path of that directory.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@xxxxxxxx>
> ---
> tools/bpf/bpftool/struct_ops.c | 86 ++++++++++++++++++++++++++++------
> 1 file changed, 72 insertions(+), 14 deletions(-)
>
> diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
> index b389f4830e11..d1ae39f9d8df 100644
> --- a/tools/bpf/bpftool/struct_ops.c
> +++ b/tools/bpf/bpftool/struct_ops.c
> @@ -475,21 +475,62 @@ static int do_unregister(int argc, char **argv)
> return cmd_retval(&res, true);
> }
>
> +static int pathname_concat(char *buf, int buf_sz, const char *path,
> + const char *name)
> +{
> + int len;
> +
> + len = snprintf(buf, buf_sz, "%s/%s", path, name);
> + if (len < 0)
> + return -EINVAL;
> + if (len >= buf_sz)
> + return -ENAMETOOLONG;
> +
> + return 0;
> +}
This is nearly identical to the one in prog.c. If we do need this, we
should move it to common.c and reuse it.
> +
> +static int pin_link(struct bpf_link *link, const char *pindir,
> + const char *name)
> +{
> + char pinfile[PATH_MAX];
> + int err;
> +
> + err = pathname_concat(pinfile, sizeof(pinfile), pindir, name);
> + if (err)
> + return -1;
> +
> + err = bpf_link__pin(link, pinfile);
> + if (err)
> + return -1;
> +
> + return 0;
> +}
> +
> static int do_register(int argc, char **argv)
> {
> LIBBPF_OPTS(bpf_object_open_opts, open_opts);
> + __u32 link_info_len = sizeof(struct bpf_link_info);
> + struct bpf_link_info link_info = {};
> struct bpf_map_info info = {};
> __u32 info_len = sizeof(info);
> int nr_errs = 0, nr_maps = 0;
> + const char *pindir = NULL;
> struct bpf_object *obj;
> struct bpf_link *link;
> struct bpf_map *map;
> const char *file;
>
> - if (argc != 1)
> + if (argc != 1 && argc != 2)
> usage();
>
> file = GET_ARG();
> + if (argc == 1)
> + pindir = GET_ARG();
> +
> + if (pindir && mount_bpffs_for_pin(pindir)) {
> + p_err("can't mount bpffs for pinning");
> + return -1;
> + }
>
> if (verifier_logs)
> /* log_level1 + log_level2 + stats, but not stable UAPI */
> @@ -519,21 +560,38 @@ static int do_register(int argc, char **argv)
> }
> nr_maps++;
>
> - bpf_link__disconnect(link);
> - bpf_link__destroy(link);
> -
> - if (!bpf_map_get_info_by_fd(bpf_map__fd(map), &info,
> - &info_len))
> - p_info("Registered %s %s id %u",
> - get_kern_struct_ops_name(&info),
> - bpf_map__name(map),
> - info.id);
> - else
> + if (bpf_map_get_info_by_fd(bpf_map__fd(map), &info,
> + &info_len)) {
> /* Not p_err. The struct_ops was attached
> * successfully.
> */
> - p_info("Registered %s but can't find id: %s",
> - bpf_map__name(map), strerror(errno));
> + p_err("Registered %s but can't find id: %s",
> + bpf_map__name(map), strerror(errno));
See comment right above: p_info() is probably enough here. If for some
reason we do need to switch to an error message and change the
existing behaviour, can you please motivate it and make it a separate
commit (and update the comment)?
> + nr_errs++;
> + } else if (!(bpf_map__map_flags(map) & BPF_F_LINK)) {
> + p_info("Registered %s %s id %u",
> + get_kern_struct_ops_name(&info),
> + info.name,
> + info.id);
> + } else if (bpf_link_get_info_by_fd(bpf_link__fd(link),
> + &link_info,
> + &link_info_len)) {
> + p_err("Registered %s but can't find link id: %s",
> + bpf_map__name(map), strerror(errno));
> + nr_errs++;
> + } else if (pindir && pin_link(link, pindir, info.name)) {
Why do we have "pindir" and not a pinned path? Instead of taking a
directory name to concatenate, why not let the user specify the pinned
path directly, as we do for maps, programs, and links already? The
only existing use of dirname + concat I can think of is for "bpftool
prog loadall", but this is because we need one path to pin multiple
programs. Here we just have one, so let the user choose their path?
I would also avoid using "pin" too much in variable or function names.
I know we have "bpf_link__pin()", but I find it makes things confusing
between the concepts of pinned objects (through BPF_OBJ_PIN) and of
BPF links. How about "linkdir" or "linkpath" instead?
> + p_err("can't pin link %u for %s: %s",
> + link_info.id, info.name,
> + strerror(errno));
> + nr_errs++;
> + } else
> + p_info("Registered %s %s map id %u link id %u",
> + get_kern_struct_ops_name(&info),
> + info.name, info.id, link_info.id);
Missing curly brackets on the "else" block.
I find it not easy to follow the logic in this long "else if..."
chain, it would probably feel more natural with simple "if"s and some
"goto"s to reach the bpf_link__disconnect() call below. But maybe this
is just me.
> +
> + bpf_link__disconnect(link);
> + bpf_link__destroy(link);
> +
Nit: We don't need this empty line.
> }
>
> bpf_object__close(obj);
> @@ -562,7 +620,7 @@ static int do_help(int argc, char **argv)
> fprintf(stderr,
> "Usage: %1$s %2$s { show | list } [STRUCT_OPS_MAP]\n"
> " %1$s %2$s dump [STRUCT_OPS_MAP]\n"
> - " %1$s %2$s register OBJ\n"
> + " %1$s %2$s register OBJ [PATH]\n"
This is not enough to understand what PATH means here. I'd use
something like "LINK_DIR", or preferably "LINK_PATH" if we let users
specify the full path. And we need to update the
bpftool-struct_ops.rst man page (under bpftool's Documentation/) to
explain what this optional argument is for, can you please take care
of this?
We usually have to update the bash completion too, but it seems that
it offers filenames multiple times already after "bpftool struct_ops
register", which is not intentional but covers completion for the new
argument.
> " %1$s %2$s unregister STRUCT_OPS_MAP\n"
> " %1$s %2$s help\n"
> "\n"
> --
> 2.34.1
>
