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=-6.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 ACA78C31E5D for ; Mon, 17 Jun 2019 18:35:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 83A9F2084D for ; Mon, 17 Jun 2019 18:35:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="tADnIhed" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726074AbfFQSfl (ORCPT ); Mon, 17 Jun 2019 14:35:41 -0400 Received: from mail-qt1-f196.google.com ([209.85.160.196]:33784 "EHLO mail-qt1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725878AbfFQSfl (ORCPT ); Mon, 17 Jun 2019 14:35:41 -0400 Received: by mail-qt1-f196.google.com with SMTP id x2so12034425qtr.0; Mon, 17 Jun 2019 11:35:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=6R1eTlGzgAfQR7Dbiz2phmu63E+RU6ETeg5r7VI5LIU=; b=tADnIhed7CuC/BijpCUz/9AxD4B4fY30mvOvoiXuro/1gyvnxqIwsAqmyMHkx1bw6T rAN5BXfCItoQS3t+mmhF+LvZ+cWDruYLyZjCzD60lJb3u43+AExC9o06Lwz0aApG7Q85 7pmedRwYyat+pIBMkYsbZuqoqQYsavpL9zBfkxdYLV5sCFBeF69G8Y5xmVjoaJ8dkfb8 YZyrAyPady+YFP0NapwaPaF7rmF48Ez8dJXjwLXtUbjvSOlIpMccfRXcn0w/ZGuqM0pc Ca3Xuxf0T97t1RJ6zwRqdqrl+9SvAdOgLs3r6DJ/0CGFqRMcH5ZwyjyMCv2I3jyHr0Jq L7AQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6R1eTlGzgAfQR7Dbiz2phmu63E+RU6ETeg5r7VI5LIU=; b=rvDclBaOYxdAjXgRltClgwPYMJKdGthqSCyFcQQTL9IZac/3T7y3tdtHdgG3q4zszg z5KPRtOkxk16ysdklO3xmkzEbPALwstiLofF2Gki2FBCwSgpxc/DPnOW1OrdYxovzQ/q NS2thOWIzyW0JoeQYKC4hShFvKel7Ch95ZyXIUfBk7j6jv2RI4fhWKoB3dtq64Rg66gH pjms0qjOpx8PxAeHyoB805imXnkhnSGML0804wPc3CWYqnvGrYxoUGrbJ6iKahqcAgHW 9Op779NI5bgcVNtK7k0O7QF+Vd8jOwqiv3K2XShpSWsQGX3lboHjDxTw8UKRIeeKz7DJ U1WQ== X-Gm-Message-State: APjAAAW0hZaqXiQvK4533MwUdyZVj9p75YrYxcq1wSWODnwSpGwh5aMl RgR6G6elltrlnjkrJLiw9p7fntMZ6t/Gsn7Njr8= X-Google-Smtp-Source: APXvYqw3ayZjxjfSJGWiVqOR9NoqEhVz0jUkyjLZR6RvXr/HYuweowdEY9d4cJr0MmrsM9NARW2soXKAQ5kpvhWBjRQ= X-Received: by 2002:a0c:d0fc:: with SMTP id b57mr23098602qvh.78.1560796540158; Mon, 17 Jun 2019 11:35:40 -0700 (PDT) MIME-Version: 1.0 References: <20190611044747.44839-1-andriin@fb.com> <20190611044747.44839-3-andriin@fb.com> <8F07D61C-2751-44A6-9E89-9BE6781FEF81@fb.com> In-Reply-To: <8F07D61C-2751-44A6-9E89-9BE6781FEF81@fb.com> From: Andrii Nakryiko Date: Mon, 17 Jun 2019 11:35:29 -0700 Message-ID: Subject: Re: [PATCH bpf-next 2/8] libbpf: extract BTF loading and simplify ELF parsing logic To: Song Liu Cc: Song Liu , Andrii Nakryiko , bpf , Networking , Alexei Starovoitov , Daniel Borkmann , Kernel Team Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, Jun 17, 2019 at 11:07 AM Song Liu wrote: > > > > > On Jun 17, 2019, at 10:24 AM, Andrii Nakryiko wrote: > > > > On Sat, Jun 15, 2019 at 1:26 PM Song Liu wrote: > >> > >> On Mon, Jun 10, 2019 at 9:49 PM Andrii Nakryiko wrote: > >>> > >>> As a preparation for adding BTF-based BPF map loading, extract .BTF and > >>> .BTF.ext loading logic. Also simplify error handling in > >>> bpf_object__elf_collect() by returning early, as there is no common > >>> clean up to be done. > >>> > >>> Signed-off-by: Andrii Nakryiko > >>> --- > >>> tools/lib/bpf/libbpf.c | 137 ++++++++++++++++++++++------------------- > >>> 1 file changed, 75 insertions(+), 62 deletions(-) > >>> > >>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >>> index ba89d9727137..9e39a0a33aeb 100644 > >>> --- a/tools/lib/bpf/libbpf.c > >>> +++ b/tools/lib/bpf/libbpf.c > >>> @@ -1078,6 +1078,58 @@ static void bpf_object__sanitize_btf_ext(struct bpf_object *obj) > >>> } > >>> } > >>> > >>> +static int bpf_object__load_btf(struct bpf_object *obj, > >>> + Elf_Data *btf_data, > >>> + Elf_Data *btf_ext_data) > >>> +{ > >>> + int err = 0; > >>> + > >>> + if (btf_data) { > >>> + obj->btf = btf__new(btf_data->d_buf, btf_data->d_size); > >>> + if (IS_ERR(obj->btf)) { > >>> + pr_warning("Error loading ELF section %s: %d.\n", > >>> + BTF_ELF_SEC, err); > >>> + goto out; > >> > >> If we goto out here, we will return 0. > > > > > > Yes, it's intentional. BTF is treated as optional, so if we fail to > > load it, libbpf will emit warning, but will proceed as nothing > > happened and no BTF was supposed to be loaded. > > > >> > >>> + } > >>> + err = btf__finalize_data(obj, obj->btf); > >>> + if (err) { > >>> + pr_warning("Error finalizing %s: %d.\n", > >>> + BTF_ELF_SEC, err); > >>> + goto out; > >>> + } > >>> + bpf_object__sanitize_btf(obj); > >>> + err = btf__load(obj->btf); > >>> + if (err) { > >>> + pr_warning("Error loading %s into kernel: %d.\n", > >>> + BTF_ELF_SEC, err); > >>> + goto out; > >>> + } > >>> + } > >>> + if (btf_ext_data) { > >>> + if (!obj->btf) { > >>> + pr_debug("Ignore ELF section %s because its depending ELF section %s is not found.\n", > >>> + BTF_EXT_ELF_SEC, BTF_ELF_SEC); > >>> + goto out; > >> > >> We will also return 0 when goto out here. > > > > > > See above, it's original behavior of libbpf. > > > >> > >>> + } > >>> + obj->btf_ext = btf_ext__new(btf_ext_data->d_buf, > >>> + btf_ext_data->d_size); > >>> + if (IS_ERR(obj->btf_ext)) { > >>> + pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n", > >>> + BTF_EXT_ELF_SEC, PTR_ERR(obj->btf_ext)); > >>> + obj->btf_ext = NULL; > >>> + goto out; > >> And, here. And we will not free obj->btf. > > > > This is situation in which we successfully loaded .BTF, but failed to > > load .BTF.ext. In that case we'll warn about .BTF.ext, but will drop > > it and continue with .BTF only. > > > > Yeah, that makes sense. > > Shall we let bpf_object__load_btf() return void? Since it always > returns 0? This is split into bpf_object__init_btf and bpf_object__sanitize_and_load_btf in patch #6, both of which can return errors. So probably not worth changing just for one patch. > > Thanks, > Song > > >