| Message ID | 20210730002953.1045142-1-fallentree@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [v4,bpf-next] libbpf: Add bpf_object__set_name(obj, name) api. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to bpf-next |
| netdev/tree_selection | success | Clearly marked for bpf-next |
On Thu, Jul 29, 2021 at 5:31 PM Yucong Sun <fallentree@fb.com> wrote: > > Tracking: libbpf/libbpf#291 Please provide a bit more elaborate commit summary. Just a link to libbpf Github PR isn't a good commit message. Please explain succinctly what is being added and why. As for the PR reference itself, please include the full link. You can use this form (which Quentin used pretty consistently already): Reference: https://github.com/libbpf/libbpf/issues/291 > > Signed-off-by: Yucong Sun <fallentree@fb.com> > > --- > > V3 -> V4: handle obj is NULL case. > V2 -> V3: fix code style errors > --- > tools/lib/bpf/libbpf.c | 10 ++++++++++ > tools/lib/bpf/libbpf.h | 1 + > tools/lib/bpf/libbpf.map | 1 + > .../selftests/bpf/prog_tests/reference_tracking.c | 7 +++++++ > 4 files changed, 19 insertions(+) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index a1ca6fb0c6d8..a628e94a41a4 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -7545,6 +7545,16 @@ const char *bpf_object__name(const struct bpf_object *obj) > return obj ? obj->name : libbpf_err_ptr(-EINVAL); > } > > +int bpf_object__set_name(struct bpf_object *obj, const char *name) > +{ > + if (!obj || !name) > + return libbpf_err(-EINVAL); I think we shouldn't be checking !obj in every bpf_object's API (even though we have a bunch of APIs like that). If user can't make sure they pass non-NULL proper pointer, it's fine for them to debug SIGSEGV. > + > + strncpy(obj->name, name, sizeof(obj->name) - 1); let's do two extra checks: 1. if bpf_object was already loaded, it's too late (maps were named with its name, etc), so error out in that case 2. if provided name is too long, we can still truncate and set it, but let's also return -E2BIG maybe? BTW, static analyzers like to complain about strncpy not zero terminating, add extra obj->name[sizeof(obj->name) - 1] = '\0'; at the end to make them happier Also, please add selftest validating that this name is actually taken into account properly. First thing that comes to mind to check this would be .data/.rodata map names check, but look around the source code to see if there is something other that can be used as a check. > + > + return 0; > +} > + > unsigned int bpf_object__kversion(const struct bpf_object *obj) > { > return obj ? obj->kern_version : 0; > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 1271d99bb7aa..36a2946e3373 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -161,6 +161,7 @@ LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr); > LIBBPF_API int bpf_object__unload(struct bpf_object *obj); > > LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj); > +LIBBPF_API int bpf_object__set_name(struct bpf_object *obj, const char *name); > LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj); > LIBBPF_API int bpf_object__set_kversion(struct bpf_object *obj, __u32 kern_version); > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index c240d488eb5e..3c15aefeb6e0 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -377,4 +377,5 @@ LIBBPF_0.5.0 { > bpf_object__gen_loader; > btf_dump__dump_type_data; > libbpf_set_strict_mode; > + bpf_object__set_name; please keep that list alphabetically sorted > } LIBBPF_0.4.0; > diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c > index de2688166696..4d3d0a4aec03 100644 > --- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c > +++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c > @@ -5,6 +5,7 @@ void test_reference_tracking(void) > { > const char *file = "test_sk_lookup_kern.o"; > const char *obj_name = "ref_track"; > + const char *obj_name2 = "ref_track2"; > DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts, > .object_name = obj_name, > .relaxed_maps = true, > @@ -23,6 +24,12 @@ void test_reference_tracking(void) > bpf_object__name(obj), obj_name)) > goto cleanup; > > + bpf_object__set_name(obj, obj_name2); > + if (CHECK(strcmp(bpf_object__name(obj), obj_name2), "obj_name", > + "wrong obj name '%s', expected '%s'\n", > + bpf_object__name(obj), obj_name2)) > + goto cleanup; > + it's a bit too simple and doesn't test much. See how obj->name is used for naming maps (and maybe something else as well), and validate that it gets propagated properly > bpf_object__for_each_program(prog, obj) { > const char *title; > > -- > 2.30.2 >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index a1ca6fb0c6d8..a628e94a41a4 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -7545,6 +7545,16 @@ const char *bpf_object__name(const struct bpf_object *obj) return obj ? obj->name : libbpf_err_ptr(-EINVAL); } +int bpf_object__set_name(struct bpf_object *obj, const char *name) +{ + if (!obj || !name) + return libbpf_err(-EINVAL); + + strncpy(obj->name, name, sizeof(obj->name) - 1); + + return 0; +} + unsigned int bpf_object__kversion(const struct bpf_object *obj) { return obj ? obj->kern_version : 0; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 1271d99bb7aa..36a2946e3373 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -161,6 +161,7 @@ LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr); LIBBPF_API int bpf_object__unload(struct bpf_object *obj); LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj); +LIBBPF_API int bpf_object__set_name(struct bpf_object *obj, const char *name); LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj); LIBBPF_API int bpf_object__set_kversion(struct bpf_object *obj, __u32 kern_version); diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index c240d488eb5e..3c15aefeb6e0 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -377,4 +377,5 @@ LIBBPF_0.5.0 { bpf_object__gen_loader; btf_dump__dump_type_data; libbpf_set_strict_mode; + bpf_object__set_name; } LIBBPF_0.4.0; diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c index de2688166696..4d3d0a4aec03 100644 --- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c +++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c @@ -5,6 +5,7 @@ void test_reference_tracking(void) { const char *file = "test_sk_lookup_kern.o"; const char *obj_name = "ref_track"; + const char *obj_name2 = "ref_track2"; DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts, .object_name = obj_name, .relaxed_maps = true, @@ -23,6 +24,12 @@ void test_reference_tracking(void) bpf_object__name(obj), obj_name)) goto cleanup; + bpf_object__set_name(obj, obj_name2); + if (CHECK(strcmp(bpf_object__name(obj), obj_name2), "obj_name", + "wrong obj name '%s', expected '%s'\n", + bpf_object__name(obj), obj_name2)) + goto cleanup; + bpf_object__for_each_program(prog, obj) { const char *title;
Tracking: libbpf/libbpf#291 Signed-off-by: Yucong Sun <fallentree@fb.com> --- V3 -> V4: handle obj is NULL case. V2 -> V3: fix code style errors --- tools/lib/bpf/libbpf.c | 10 ++++++++++ tools/lib/bpf/libbpf.h | 1 + tools/lib/bpf/libbpf.map | 1 + .../selftests/bpf/prog_tests/reference_tracking.c | 7 +++++++ 4 files changed, 19 insertions(+)