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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

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@orange.com>

> -		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;
>  }