On Wed, Dec 8, 2021 at 10:26 PM John Fastabend <john.fastabend@xxxxxxxxx> wrote:
>
> Andrii Nakryiko wrote:
> > To unify libbpf APIs behavior w.r.t. log_buf and log_level, fix
> > bpf_prog_load() to follow the same logic as bpf_btf_load() and
> > high-level bpf_object__load() API will follow in the subsequent patches:
> > - if log_level is 0 and non-NULL log_buf is provided by a user, attempt
> > load operation initially with no log_buf and log_level set;
> > - if successful, we are done, return new FD;
> > - on error, retry the load operation with log_level bumped to 1 and
> > log_buf set; this way verbose logging will be requested only when we
> > are sure that there is a failure, but will be fast in the
> > common/expected success case.
> >
> > Of course, user can still specify log_level > 0 from the very beginning
> > to force log collection.
> >
> > Suggested-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
>
> [...]
>
> > @@ -366,16 +368,17 @@ int bpf_prog_load_v0_6_0(enum bpf_prog_type prog_type,
> > goto done;
> > }
> >
> > - if (log_level || !log_buf)
> > - goto done;
> > + if (log_level == 0 && !log_buf) {
> ^^^^^^^^
>
> with non-Null log buf? Seems comment and above are out of sync?
>
> Should it be, if (log_level == 0 && log_buf) { ... }
Doh... yeah, it should. Apparently inverting a boolean expression is
non-trivial :) I'll add low-level bpf_prog_load() (and maybe
bpf_btf_load() while at it) log_buf tests to log_buf.c in selftests to
catch something like this better, thanks for catching!
>
> > + /* log_level == 0 with non-NULL log_buf requires retrying on error
> > + * with log_level == 1 and log_buf/log_buf_size set, to get details of
> > + * failure
> > + */
> > + attr.log_buf = ptr_to_u64(log_buf);
> > + attr.log_size = log_size;
> > + attr.log_level = 1;
> >
> > - /* Try again with log */
> > - log_buf[0] = 0;
> > - attr.log_buf = ptr_to_u64(log_buf);
> > - attr.log_size = log_size;
> > - attr.log_level = 1;
> > -
> > - fd = sys_bpf_prog_load(&attr, sizeof(attr), attempts);
> > + fd = sys_bpf_prog_load(&attr, sizeof(attr), attempts);
> > + }
> > done:
> > /* free() doesn't affect errno, so we don't need to restore it */
> > free(finfo);
> > --
> > 2.30.2
> >
>
>
