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=-14.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no 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 AD6D9C43461 for ; Tue, 8 Sep 2020 20:53:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 601C82145D for ; Tue, 8 Sep 2020 20:53:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="JgeSMlKt" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730413AbgIHUxW (ORCPT ); Tue, 8 Sep 2020 16:53:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44740 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729912AbgIHUxV (ORCPT ); Tue, 8 Sep 2020 16:53:21 -0400 Received: from mail-qt1-x842.google.com (mail-qt1-x842.google.com [IPv6:2607:f8b0:4864:20::842]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5CA50C061573 for ; Tue, 8 Sep 2020 13:53:20 -0700 (PDT) Received: by mail-qt1-x842.google.com with SMTP id e7so293819qtj.11 for ; Tue, 08 Sep 2020 13:53:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=g4YR8SuShX7QcPEBP8FeYlpCgaekc4/WOgYCkOBy+ik=; b=JgeSMlKt7NyZxFjwVlmXALXVwT/bOq1IHoGDZjzw6q7KfEx4GCypfDoFyxQnSwTTME dpcyyc9fXIKSWQSH6K7QLhI7+9WLpEPqBl9KlnvtqImshID4fGAbV+9hzpekPnin1jp8 J1OF780F8DKoY0KGAxULPwGLBlDyj8y1ExIGbg427Ofxp67xLWaepicqtBQhtbT+4qtC V1sYulxWxe35ZSw7mCha3kC3Cdp9xaGAfthwhj799oWWP+EIJRQ2ypYsWyItEUKU+wy+ SJ7SGuYYtf59htvBediUwxWyQeAmJJOevHXAZDCEnuzwdXl6KJdoXoG2KXlh+VkfhZfR Fwrg== 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=g4YR8SuShX7QcPEBP8FeYlpCgaekc4/WOgYCkOBy+ik=; b=uT3JolW8V/e3o5CPyS4RVPPUlgfHLq63wkirSYmHaAytRDqLtgvi5qHRGWg3J/vGuB 2pKklt4roNeBzZUZH0AEUGufu6APu1stRMaGB098pMMUofHZPHtsfD1q9Mccrtd5KTa1 zEtrCyaGT/wG582l8BGRKXrWc1/ep8LV9dwWDiVL/b2opVxcDP3afddWepsTqjSWbyTD m272LTmARgVit9PlkHIdrJ1HBjqL2ND8gc+JMliFs+sCjPs5r61kC50MydsQSaeInuwH +6OOwsb+qVGP5cQZmlSbJstUOVVRJyE/wLRlb87rPofqEMkzXdKYrhA8V2j16wnbH+yH J5Wg== X-Gm-Message-State: AOAM531zWqk+4M9uyBSsqpuJ736TIqIzgpVXkedA3IiMBElJU/xbfIQU 3jjQDQIkmDo92g3p0BoASCP4AN9Tkmtftb8b36YdLA== X-Google-Smtp-Source: ABdhPJztZUBgOyXfnZgLehiu4Dd8eZfPcUrHCOQ07ZJrNzRMPkbO5PKKLn6Kkt2Y1oDwvAdR2MAqsvJozlHdmdZRgwI= X-Received: by 2002:ac8:4784:: with SMTP id k4mr308069qtq.266.1599598399248; Tue, 08 Sep 2020 13:53:19 -0700 (PDT) MIME-Version: 1.0 References: <20200828193603.335512-1-sdf@google.com> <20200828193603.335512-6-sdf@google.com> In-Reply-To: From: Stanislav Fomichev Date: Tue, 8 Sep 2020 13:53:08 -0700 Message-ID: Subject: Re: [PATCH bpf-next v3 5/8] bpftool: support dumping metadata To: Andrii Nakryiko Cc: Networking , bpf , "David S. Miller" , Alexei Starovoitov , Daniel Borkmann , YiFei Zhu , YiFei Zhu Content-Type: text/plain; charset="UTF-8" Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Wed, Sep 2, 2020 at 10:00 PM Andrii Nakryiko wrote: > > On Fri, Aug 28, 2020 at 12:37 PM Stanislav Fomichev wrote: > > > > From: YiFei Zhu > > > > Added a flag "--metadata" to `bpftool prog list` to dump the metadata > > contents. For some formatting some BTF code is put directly in the > > metadata dumping. Sanity checks on the map and the kind of the btf_type > > to make sure we are actually dumping what we are expecting. > > > > A helper jsonw_reset is added to json writer so we can reuse the same > > json writer without having extraneous commas. > > > > Sample output: > > > > $ bpftool prog --metadata > > 6: cgroup_skb name prog tag bcf7977d3b93787c gpl > > [...] > > btf_id 4 > > metadata: > > metadata_a = "foo" > > metadata_b = 1 > > > > $ bpftool prog --metadata --json --pretty > > [{ > > "id": 6, > > [...] > > "btf_id": 4, > > "metadata": { > > "metadata_a": "foo", > > "metadata_b": 1 > > } > > } > > ] > > > > Cc: YiFei Zhu > > Signed-off-by: YiFei Zhu > > Signed-off-by: Stanislav Fomichev > > --- > > tools/bpf/bpftool/json_writer.c | 6 ++ > > tools/bpf/bpftool/json_writer.h | 3 + > > tools/bpf/bpftool/main.c | 10 +++ > > tools/bpf/bpftool/main.h | 1 + > > tools/bpf/bpftool/prog.c | 130 ++++++++++++++++++++++++++++++++ > > 5 files changed, 150 insertions(+) > > > > [...] > > > + > > + if (bpf_map_lookup_elem(map_fd, &key, value)) { > > + p_err("metadata map lookup failed: %s", strerror(errno)); > > + goto out_free; > > + } > > + > > + err = btf__get_from_id(map_info.btf_id, &btf); > > what if the map has no btf_id associated (e.g., because of an old > kernel?); why fail in this case? Thank you for the review, coming back at it a bit late :-( This functionality is guarded by --metadata bpftool flag (off by default). In case of no btf_id, it might be helpful to show why we don't have the metadata rather than just quietly failing. WDYT? > > + if (err || !btf) { > > + p_err("metadata BTF get failed: %s", strerror(-err)); > > + goto out_free; > > + } > > + > > + t_datasec = btf__type_by_id(btf, map_info.btf_value_type_id); > > + if (BTF_INFO_KIND(t_datasec->info) != BTF_KIND_DATASEC) { > > btf_is_datasec(t_datasec) > > > + p_err("bad metadata BTF"); > > + goto out_free; > > + } > > + > > + vlen = BTF_INFO_VLEN(t_datasec->info); > > btf_vlen(t_datasec) > > > + vsi = (struct btf_var_secinfo *)(t_datasec + 1); > > btf_var_secinfos(t_datasec) > > > + > > + /* We don't proceed to check the kinds of the elements of the DATASEC. > > + * The verifier enforce then to be BTF_KIND_VAR. > > typo: then -> them > > > + */ > > + > > + if (json_output) { > > + struct btf_dumper d = { > > + .btf = btf, > > + .jw = json_wtr, > > + .is_plain_text = false, > > + }; > > + > > + jsonw_name(json_wtr, "metadata"); > > + > > + jsonw_start_object(json_wtr); > > + for (i = 0; i < vlen; i++) { > > nit: doing ++vsi here Agreed with all the above, except this one. It feels like it's safer to do [i] in case somebody adds a 'continue' clause later and we miss that '++vsi'. Let me know if you feel strongly about it. > > + t_var = btf__type_by_id(btf, vsi[i].type); > > and vsi->type here and below would look a bit cleaner > > > + > > + jsonw_name(json_wtr, btf__name_by_offset(btf, t_var->name_off)); > > + err = btf_dumper_type(&d, t_var->type, value + vsi[i].offset); > > + if (err) { > > + p_err("btf dump failed"); > > + break; > > + } > > + } > > + jsonw_end_object(json_wtr); > > [...]