On Tue, Dec 10, 2019 at 12:36:25PM -0800, Jakub Kicinski wrote:
> 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?
Sorry, that was unclear. What I call the header here is the first line
from the prog show output (in the case of plain output). So the output
when multiple programs match looks as follows. When a single program
matches, the first line (with the ID, type, name, tag and license) is
omitted.
$ ./bpftool prog dump xlated tag 6deef7357e7b4530
3: cgroup_skb tag 6deef7357e7b4530 gpl
0: (bf) r6 = r1
[...]
7: (95) exit
4: cgroup_skb tag 6deef7357e7b4530 gpl
0: (bf) r6 = r1
[...]
7: (95) exit
>
> > 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?
Oh, this is bad. Yes, fds should actually be "int **" and this line
should be "*fds = tmp;". I'll fix it in v2.
[...]
> > + 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.
Seems lost in prog.c but still valid across bpftool. I'll make the
change.
[...]
Paul
>
