On Tue, 10 Dec 2019 17:06:25 +0100, Paul Chaignon wrote:
> When several BPF programs have the same tag, bpftool matches only the
> first (in ID order). This patch changes that behavior such that dump and
> show commands return all matched programs. Commands that require a single
> program (e.g., pin and attach) will error out if given a tag that matches
> several. bpftool prog dump will also error out if file or visual are
> given and several programs have the given tag.
>
> In the case of the dump command, a program header is added before each
> dump only if the tag matches several programs; this patch doesn't change
> the output if a single program matches.
How does this work? Could you add examples to the commit message?
This header idea doesn't seem correct, aren't id and other per-instance
fields only printed once?
> Signed-off-by: Paul Chaignon <paul.chaignon@xxxxxxxxxx>
> - close(fd);
> + if (nb_fds > 0) {
> + tmp = realloc(fds, (nb_fds + 1) * sizeof(int));
> + if (!tmp) {
> + p_err("failed to realloc");
> + goto err_close_fd;
> + }
> + fds = tmp;
How does this work? the new array is never returned to the caller, and
the caller will most likely access freed memory, no?
> + }
> + fds[nb_fds++] = fd;
> }
> +
> +err_close_fd:
> + close(fd);
> +err_close_fds:
> + for (nb_fds--; nb_fds >= 0; nb_fds--)
> + close(fds[nb_fds]);
> + return -1;
> }
> +int prog_parse_fd(int *argc, char ***argv)
> +{
> + int *fds = NULL;
> + int nb_fds, fd;
> +
> + fds = malloc(sizeof(int));
> + if (!fds) {
> + p_err("mem alloc failed");
> + return -1;
> + }
> + nb_fds = prog_parse_fds(argc, argv, fds);
> + if (nb_fds != 1) {
> + if (nb_fds > 1) {
> + p_err("several programs match this handle");
> + for (nb_fds--; nb_fds >= 0; nb_fds--)
nit: since you checked nb_fds is positive, while (nb_fds--) ?
> + close(fds[nb_fds]);
> + }
> + fd = -1;
> + goto err_free;
> + }
> +
> + fd = fds[0];
> +err_free:
nit: we tried to call the labels exit_xyz if the code is used on both
error and success path, but maybe that pattern got lost over time.
> + free(fds);
> + return fd;
> +}
> +
> static void show_prog_maps(int fd, u32 num_maps)
> {
> struct bpf_prog_info info = {};
> static int do_show(int argc, char **argv)
> {
> + int fd, nb_fds, i;
> + int *fds = NULL;
> __u32 id = 0;
> int err;
> - int fd;
>
> if (show_pinned)
> build_pinned_obj_table(&prog_table, BPF_OBJ_PROG);
>
> if (argc == 2) {
> - fd = prog_parse_fd(&argc, &argv);
> - if (fd < 0)
> + fds = malloc(sizeof(int));
> + if (!fds) {
> + p_err("mem alloc failed");
> return -1;
> + }
> + nb_fds = prog_parse_fds(&argc, &argv, fds);
> + if (nb_fds < 1)
> + goto err_free;
>
> - err = show_prog(fd);
> - close(fd);
> - return err;
> + if (json_output && nb_fds > 1)
> + jsonw_start_array(json_wtr); /* root array */
> + for (i = 0; i < nb_fds; i++) {
> + err = show_prog(fds[i]);
> + close(fds[i]);
> + if (err) {
> + for (i++; i < nb_fds; i++)
> + close(fds[i]);
> + goto err_free;
I'm 90% sure JSON arrays close/end themselves on exit 🤔
> + }
> + }
> + if (json_output && nb_fds > 1)
> + jsonw_end_array(json_wtr); /* root array */
> +
> + return 0;
> +
> +err_free:
> + free(fds);
> + return -1;
Perhaps move the argc == 2 code to a separate function?
> }
>
> if (argc)
> @@ -408,101 +500,32 @@ static int do_show(int argc, char **argv)
> return err;
> }
