On Tue, Sep 8, 2020 at 1:53 PM Stanislav Fomichev [off-list ref] wrote:

On Wed, Sep 2, 2020 at 10:00 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Fri, Aug 28, 2020 at 12:37 PM Stanislav Fomichev [off-list ref] wrote:

quoted

From: YiFei Zhu <redacted>

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 <redacted>
Signed-off-by: YiFei Zhu <redacted>
Signed-off-by: Stanislav Fomichev <redacted>
---
 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(+)

[...]

quoted

+
+       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?

we might do it similarly to PID info I added with bpf_iter: if it's
supported -- emit it, if not -- skip and still succeed. So maybe we
don't really need extra --metadata flag and should do all this always?

quoted

quoted

+       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)

quoted

+               p_err("bad metadata BTF");
+               goto out_free;
+       }
+
+       vlen = BTF_INFO_VLEN(t_datasec->info);

btf_vlen(t_datasec)

quoted

+       vsi = (struct btf_var_secinfo *)(t_datasec + 1);

btf_var_secinfos(t_datasec)

quoted

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

quoted

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

I meant to add vsi++ inside the for clause, no way to miss it:

for (i = 0; i < vlen, i++, vsi++) {
  continue/break/whatever you want, except extra i++ or vsi++
}

it's the safest way, imo

quoted

quoted

+                       t_var = btf__type_by_id(btf, vsi[i].type);

and vsi->type here and below would look a bit cleaner

quoted

+
+                       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);

[...]