On Tue, May 31, 2022 at 4:20 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> > On Tue, 2022-05-31 at 13:52 -0700, Song Liu wrote:
>
> Hi Song,
>
> Thanks a lot for the review, I'll apply the suggested changes and
> provide the v3 in one or two days. My only objection is below.
>
> > > {
> > > - int fd_prog, expected_ret, alignment_prevented_execution;
> > > + int fd_prog, btf_fd, expected_ret, alignment_prevented_execution;
> > > int prog_len, prog_type = test->prog_type;
> > > struct bpf_insn *prog = test->insns;
> > > LIBBPF_OPTS(bpf_prog_load_opts, opts); __u32 pflags;
> > > int i, err;
> > >
> > > + fd_prog = -1;
> >
> > This is not really necessary.
>
> Actually this one is necessary to avoid compiler warning, note the
> following fragment of the do_test_single function below:
>
> static void do_test_single(...)
> {
> ...
> if (...) {
> btf_fd = load_btf_for_test(...);
> if (btf_fd < 0)
> goto fail_log;
> opts.prog_btf_fd = btf_fd;
> }
> ...
> fd_prog = bpf_prog_load(..., &opts);
> ...
> close_fds:
> ...
> close(fd_prog);
> close(btf_fd);
> ...
> return;
> fail_log:
> ...
> goto close_fds;
> }
>
> If load_btf_for_test fails the goto fail_log would eventually jump to
> close_fds, where fd_prog would be in an uninitialised state.
Got it. Thanks for the explanation!
Song
