Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

btf: rename FindType to TypeByName, add TypesByName and TypeByID methods #503

Merged
merged 3 commits into from
Dec 9, 2021

Conversation

ti-mo
Copy link
Collaborator

@ti-mo ti-mo commented Nov 24, 2021

This patch adds two querying primitives to btf.Spec:

  • TypeByID() looks up a btf.Type by its ID in the btf.Spec
  • TypesByName() returns a list of btf.Types that satisfy the given essential name. This can be used to obtain all CO-RE flavors of a struct, to perform a quick lookup without retaining the result, or if the kind or signature of the type you're looking for is not known up front.

In order to simplify the internal graph bookkeeping, harmonize on a single btf.Type interface all BTF types must implement instead of the Type and NamedType interfaces. This facilitates implementing an intuitive API for querying the graph.

Further document the purpose of essentialName().

@ti-mo ti-mo requested a review from lmb November 24, 2021 14:07
@ti-mo ti-mo marked this pull request as ready for review November 24, 2021 14:07
@ti-mo ti-mo force-pushed the tb/btf-type-queries branch 2 times, most recently from fd08a06 to b669fe2 Compare November 24, 2021 14:46
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the idea behind getting rid of NamedType. Is the problem that []Type and []NamedType are not assignable to each other? The code I've written for bpf2go is nicer with NamedType vs. having to check the function return: https://github.com/lmb/ebpf/blob/7f73e6889d42ae315d66e8027140850d71c49d0d/cmd/bpf2go/output.go#L258-L292

internal/btf/btf.go Show resolved Hide resolved
internal/btf/btf.go Outdated Show resolved Hide resolved
@ti-mo
Copy link
Collaborator Author

ti-mo commented Nov 26, 2021

I don't understand the idea behind getting rid of NamedType. Is the problem that []Type and []NamedType are not assignable to each other?

Yes, the fact that they're both discrete types and that they are/were stored in separate arrays makes it slightly awkward to access them. Also, I don't think it made sense from a data modelling perspective:

  • whether a type can have a name or not is a property of a type, not a defining characteristic
  • named and unnamed types are not semantically different; even named types can be anonymous, so any code that operates on named types must account for TypeName() == "", which kind of defeats the purpose of having a separate namedType interface to begin with
  • the querying interface (here e.g. TypeByID) would need a variant for both kinds of types, which will just cause confusion and might be more painful to implement/maintain than necessary

The code I've written for bpf2go is nicer with NamedType vs. having to check the function return: https://github.com/lmb/ebpf/blob/7f73e6889d42ae315d66e8027140850d71c49d0d/cmd/bpf2go/output.go#L258-L292

Sorry, lacking context, not clear what this code is doing. (comments might help)


Now I've had time to mull it over, I think I've also changed my mind about TypeName() returning string, bool:

λ  ebpf tb/multiprog ✓ grep -R "\.TypeName()"
internal/cmd/gentypes/main.go:		if n, _ := cs.TypeName(); !ok || n != "" {
internal/btf/core.go:		localTypeName, _ := localType.TypeName()
internal/btf/ext_info.go:	name, _ := typ.TypeName()
internal/btf/btf.go:		if name, _ := typ.TypeName(); name != "" {
internal/btf/btf.go:		if n, _ := typ.TypeName(); n != name {
internal/btf/types.go:		typeName, _ := typ.TypeName()
internal/btf/format_test.go:				n, _ := t.TypeName()

Currently, there's not a call site that actually uses the bool, and I think this is just more expressive:

if typ.TypeName == "" && typ.IsNamed() {
  return errors.New("some_error")
}

WDYT?

@lmb
Copy link
Collaborator

lmb commented Nov 26, 2021

IsNamed() bool is confusing I think because it could mean both name != "" or name can be != "". Let's go with TypeName() string for now and skip the dedicated function. We can figure out what to do once we have the need.

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If dealing with struct flavours is too cumbersome we could also change the API, so that resolving struct flavours is the responsibility of the caller? Basically, export essentialName() and add an unexported wrapper for CO-RE which does two lookups. Just an idea.

internal/btf/btf.go Show resolved Hide resolved
internal/btf/btf.go Outdated Show resolved Hide resolved
internal/btf/btf.go Outdated Show resolved Hide resolved
internal/btf/btf.go Show resolved Hide resolved
internal/btf/btf_test.go Outdated Show resolved Hide resolved
internal/btf/btf_test.go Show resolved Hide resolved
…ethods

This patch adds two querying primitives to btf.Spec:
- TypeByID() looks up a btf.Type by its ID in the btf.Spec
- AnyTypesByName() returns a list of btf.Types that satisfy the given
  essential name. This can be used to obtain all CO-RE flavors of a struct,
  to narrow down on a specific one by specifying the flavor suffix, or to
  simply get all types with a given name regardless of kind.

In order to simplify the internal graph bookkeeping, harmonize on a single
btf.Type interface all BTF types must implement instead of a Type and a
NamedType interface. This facilitates implementing an intuitive API for
querying the graph.

Further document the purpose of essentialName().

Signed-off-by: Timo Beckers <timo@isovalent.com>
Before this, the caller would receive all flavors of a struct even if
a specific flavor was requested. Note that multiple type instances of a single
struct flavor can still exist if they are declared in multiple compilation
units.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Added a new test TestTypeByNameAmbiguous to encode the intended behaviour
of TypeByName in the face of ambiguity. For now, only exact type names are
matched, but this might later be extended to include a picking order.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@lmb lmb merged commit e3aa622 into cilium:master Dec 9, 2021
@ti-mo ti-mo deleted the tb/btf-type-queries branch December 9, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants