RE: [PATCH v2 bpf-next 01/12] libbpf: fix bpf_prog_load() log_buf logic for log_level 0

[John Fastabend <john.fastabend@gmail.com>]

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@kernel.org>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

[...]

> @@ -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) { ... }

> +		/* 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
>