From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2856AC433DB for ; Fri, 12 Mar 2021 18:08:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E2DBB64F44 for ; Fri, 12 Mar 2021 18:08:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232288AbhCLSHh (ORCPT ); Fri, 12 Mar 2021 13:07:37 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50048 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231789AbhCLSHN (ORCPT ); Fri, 12 Mar 2021 13:07:13 -0500 Received: from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com [IPv6:2a00:1450:4864:20::62d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 94E28C061761 for ; Fri, 12 Mar 2021 10:07:13 -0800 (PST) Received: by mail-ej1-x62d.google.com with SMTP id dx17so55073065ejb.2 for ; Fri, 12 Mar 2021 10:07:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=isovalent-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=AOWDVraelT3q2iJa2o1XiejflpUA+riSmnoD1zMFlMA=; b=yPluzX314PIS3cxGWuVurfUFLcsmNiSRPKx378NxBWHOYob2ugJbu45pwhCEMExt+4 z6cLhtr3jEH+wjJIlJSM/eOuU6/AxYw7sSFmonUGThSw6XCunbBZliX4l0P6nq4YOHlS tS6Pjr1+33NGSxdP5gdlfojgsSxpw6JsBxTkJ/vNiupOy1Dd0fqmYSR1a05n6gApu4nS yWM7czZf3jHS/oOEcwBQoJz5swOCqGAUxLAMlY/26HrsFapcZJMJYZgGK/tEuToHLnXQ vtbxcnuLgnijVU9rgYtOTA7XlmZVZGFQgeBBwtghvVZIPbQ5LfwnvE0z60XSXyZ4Plp1 8Ziw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=AOWDVraelT3q2iJa2o1XiejflpUA+riSmnoD1zMFlMA=; b=t4rtEpiyLVfhQ+cooqJ3kmQ3bNyKS2S++7rvhVMNyjZh43KsORDAi15XrWyqaZM2lK SOmnT7yW+2f6LNSdX7SNIY5RFHcQPoL9bUOAZPTij3wTsTBvwxW7XnFykNugDreXLKtp cZkX3ZtZL30zmxJBEoReoSI/GNxrOy/XXZa6x+v9Yms+QGAdg9t4ID+k+EigYfFYEy2s IIlYoVT/4LJPcYoxx3KHLM85R+T4G3crUm2fNw4Emm3o9GOhkxSFD3zXNmg+w9FxzCcp p0YUG9WIEwK0Po5c3gvuzXa+ChOkn+QtDZg8CE8pL3bQZlEg4Wr8AAh+M3TFEh1O6sdg OneQ== X-Gm-Message-State: AOAM532pKoCRf8GwMy5h9mddPRKHPA3TkBxdQbsrPPbe1r5JXPgdlWj3 qFNGjGiRDX+5MwgbzKKRFdW1rA== X-Google-Smtp-Source: ABdhPJxljWO20JrlmCvTaWaOt3XIObOTPxfArQVQhLoUvfNNmkXrUgWKJRx5k9xRlPLeM0KUq1vuGQ== X-Received: by 2002:a17:906:da0e:: with SMTP id fi14mr10278257ejb.188.1615572432194; Fri, 12 Mar 2021 10:07:12 -0800 (PST) Received: from [192.168.1.8] ([194.35.119.67]) by smtp.gmail.com with ESMTPSA id s18sm3038742ejc.79.2021.03.12.10.07.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 12 Mar 2021 10:07:11 -0800 (PST) Subject: Re: [PATCH bpf-next 07/10] bpftool: add `gen bpfo` command to perform BPF static linking To: Andrii Nakryiko Cc: Andrii Nakryiko , bpf , Networking , Alexei Starovoitov , Daniel Borkmann , Kernel Team References: <20210310040431.916483-1-andrii@kernel.org> <20210310040431.916483-8-andrii@kernel.org> <9f44eedf-79a3-0025-0f31-ee70f2f7d98b@isovalent.com> From: Quentin Monnet Message-ID: <7c78ba67-03ff-fd84-339e-08628716abdf@isovalent.com> Date: Fri, 12 Mar 2021 18:07:10 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org 2021-03-11 10:45 UTC-0800 ~ Andrii Nakryiko > On Thu, Mar 11, 2021 at 3:31 AM Quentin Monnet wrote: >> >> 2021-03-09 20:04 UTC-0800 ~ Andrii Nakryiko >>> Add `bpftool gen bpfo ...` command to statically >>> link multiple BPF object files into a single output BPF object file. >>> >>> Similarly to existing '*.o' convention, bpftool is establishing a '*.bpfo' >>> convention for statically-linked BPF object files. Both .o and .bpfo suffixes >>> will be stripped out during BPF skeleton generation to infer BPF object name. >>> >>> Signed-off-by: Andrii Nakryiko >>> --- >>> tools/bpf/bpftool/gen.c | 46 ++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 45 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c >>> index 4033c46d83e7..8b1ed6c0a62f 100644 >>> --- a/tools/bpf/bpftool/gen.c >>> +++ b/tools/bpf/bpftool/gen.c >>> +static int do_bpfo(int argc, char **argv) >> >>> +{ >>> + struct bpf_linker *linker; >>> + const char *output_file, *file; >>> + int err; >>> + >>> + if (!REQ_ARGS(2)) { >>> + usage(); >>> + return -1; >>> + } >>> + >>> + output_file = GET_ARG(); >>> + >>> + linker = bpf_linker__new(output_file, NULL); >>> + if (!linker) { >>> + p_err("failed to create BPF linker instance"); >>> + return -1; >>> + } >>> + >>> + while (argc) { >>> + file = GET_ARG(); >>> + >>> + err = bpf_linker__add_file(linker, file); >>> + if (err) { >>> + p_err("failed to link '%s': %d", file, err); >> >> I think you mentioned before that your preference was for having just >> the error code instead of using strerror(), but I think it would be more >> user-friendly for the majority of users who don't know the error codes >> if we had something more verbose? How about having both strerror() >> output and the error code? > > Sure, I'll add strerror(). My earlier point was that those messages > are more often misleading (e.g., "file not found" for ENOENT or > something similar) than helpful. I should check if bpftool is passing > through warn-level messages from libbpf. Those are going to be very > helpful, if anything goes wrong. --verbose should pass through all of > libbpf messages, if it's not already the case. Thanks. Yes, --verbose should do it, but it's worth a double-check. >>> + goto err_out; >>> + } >>> + } >>> + >>> + err = bpf_linker__finalize(linker); >>> + if (err) { >>> + p_err("failed to finalize ELF file: %d", err); >>> + goto err_out; >>> + } >>> + >>> + return 0; >>> +err_out: >>> + bpf_linker__free(linker); >>> + return -1; >> >> Should you call bpf_linker__free() even on success? I see that >> bpf_linker__finalize() frees some of the resources, but it seems that >> bpf_linker__free() does a more thorough job? > > yep, it should really be just > > err_out: > bpf_linker__free(linker); > return err; > > >> >>> +} >>> + >>> static int do_help(int argc, char **argv) >>> { >>> if (json_output) { >>> @@ -611,6 +654,7 @@ static int do_help(int argc, char **argv) >>> >>> static const struct cmd cmds[] = { >>> { "skeleton", do_skeleton }, >>> + { "bpfo", do_bpfo }, >>> { "help", do_help }, >>> { 0 } >>> }; >>> >> >> Please update the usage help message, man page, and bash completion, >> thanks. Especially because what "bpftool gen bpfo" does is not intuitive >> (but I don't have a better name suggestion at the moment). > > Yeah, forgot about manpage and bash completions, as usual. > > re: "gen bpfo". I don't have much better naming as well. `bpftool > link` is already taken for bpf_link-related commands. It felt like > keeping this under "gen" command makes sense. But maybe `bpftool > linker link ...` would be a bit less confusing > convention? "bpftool linker" would have been nice, but having "bpftool link", I think it would be even more confusing. We can pass commands by their prefixes, so is "bpftool link" the command "link" or a prefix for "linker"? (I know it would be easy to sort out from our point of view, but for regular users I'm sure that would be confusing). I don't mind leaving it under "bpftool gen", it's probably the most relevant command we have. As for replacing the "bpfo" keyword, I've thought of "combined", "static_linked", "archive", "concat". I write them in case it's any inspiration, but I find none of them ideal :/. Quentin