Re: [PATCH bpf-next 1/3] bpftool: match several programs with same tag

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

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