Messages in this thread | | | Subject | Re: [PATCH v2 bpf-next 2/7] bpf: move to generic BTF show support, apply it to seq files/strings | From | Yonghong Song <> | Date | Mon, 18 May 2020 23:21:10 -0700 |
| |
On 5/18/20 2:46 AM, Alan Maguire wrote: > On Wed, 13 May 2020, Yonghong Song wrote: > >> >>> +struct btf_show { >>> + u64 flags; >>> + void *target; /* target of show operation (seq file, buffer) */ >>> + void (*showfn)(struct btf_show *show, const char *fmt, ...); >>> + const struct btf *btf; >>> + /* below are used during iteration */ >>> + struct { >>> + u8 depth; >>> + u8 depth_shown; >>> + u8 depth_check; >> >> I have some difficulties to understand the relationship between >> the above three variables. Could you add some comments here? >> > > Will do; sorry the code got a bit confusing. The goal is to track > which sub-components in a data structure we need to display. The > "depth" variable tracks where we are currently; "depth_shown" > is the depth at which we have something nonzer to display (perhaps > "depth_to_show" would be a better name?). "depth_check" tells
"depth_to_show" is indeed better.
> us whether we are currently checking depth or doing printing. > If we're checking, we don't actually print anything, we merely note > if we hit a non-zero value, and if so, we set "depth_shown" > to the depth at which we hit that value. > > When we show a struct, union or array, we will only display an > object has one or more non-zero members. But because > the struct can in turn nest a struct or array etc, we need > to recurse into the object. When we are doing that, depth_check > is set, and this tells us not to do any actual display. When > that recursion is complete, we check if "depth_shown" (depth > to show) is > depth (i.e. we found something) and if it is > we go on to display the object (setting depth_check to 0).
Thanks for the explanation. Putting them in the comments will be great.
> > There may be a better way to solve this problem of course, > but I wanted to avoid storing values where possible as > deeply-nested data structures might overrun such storage. > [...] >>> + >>> + /* >>> + * The goal here is to build up the right number of pointer and >>> + * array suffixes while ensuring the type name for a typedef >>> + * is represented. Along the way we accumulate a list of >>> + * BTF kinds we have encountered, since these will inform later >>> + * display; for example, pointer types will not require an >>> + * opening "{" for struct, we will just display the pointer value. >>> + * >>> + * We also want to accumulate the right number of pointer or array >>> + * indices in the format string while iterating until we get to >>> + * the typedef/pointee/array member target type. >>> + * >>> + * We start by pointing at the end of pointer and array suffix >>> + * strings; as we accumulate pointers and arrays we move the pointer >>> + * or array string backwards so it will show the expected number of >>> + * '*' or '[]' for the type. BTF_SHOW_MAX_ITER of nesting of pointers >>> + * and/or arrays and typedefs are supported as a precaution. >>> + * >>> + * We also want to get typedef name while proceeding to resolve >>> + * type it points to so that we can add parentheses if it is a >>> + * "typedef struct" etc. >>> + */ >>> + for (i = 0; i < BTF_SHOW_MAX_ITER; i++) { >>> + >>> + switch (BTF_INFO_KIND(t->info)) { >>> + case BTF_KIND_TYPEDEF: >>> + if (!type_name) >>> + type_name = btf_name_by_offset(show->btf, >>> + t->name_off); type_name should never be NULL for valid vmlinux BTF.
>>> + kinds |= BTF_KIND_BIT(BTF_KIND_TYPEDEF); >>> + id = t->type; >>> + break; >>> + case BTF_KIND_ARRAY: >>> + kinds |= BTF_KIND_BIT(BTF_KIND_ARRAY); >>> + parens = "["; >>> + array = btf_type_array(t); >>> + if (!array) array will never be NULL here. >>> + return show->state.type_name; >>> + if (!t) t will never be NULL here. >>> + return show->state.type_name; >>> + if (array_suffix > array_suffixes) >>> + array_suffix -= 2; >>> + id = array->type; >>> + break; >>> + case BTF_KIND_PTR: >>> + kinds |= BTF_KIND_BIT(BTF_KIND_PTR); >>> + if (ptr_suffix > ptr_suffixes) >>> + ptr_suffix -= 1; >>> + id = t->type; >>> + break; >>> + default: >>> + id = 0; >>> + break; >>> + } >>> + if (!id) >>> + break; >>> + t = btf_type_skip_qualifiers(show->btf, id); t should never be NULL here. >>> + if (!t) >>> + return show->state.type_name; >>> + } >> >> Do we do pointer tracing here? For example >> struct t { >> int *m[5]; >> } >> >> When trying to access memory, the above code may go through >> ptr->array and out of loop when hitting array element type "int"? >> > > I'm not totally sure I understand the question so I'll > try and describe how the above is supposed to work. I > think there's a bug here alright. > > In the above case, when we reach the "m" field of "struct t", > the code should start with the BTF_KIND_ARRAY, set up > the array suffix, then get the array type which is a PTR > and we will set up the ptr suffix to be "*" and we set > the id to the id associated with "int", and > btf_type_skip_qualifiers() will use that id to look up > the new value for the type used in btf_name_by_offset(). > So on the next iteration we hit the int itself and bail from > the loop, having noted that we've got a _PTR and _ARRAY set in > the "kinds" bitfield. > > Then we look up the int type using "t" with btf_name_by_offset, > so we end up displaying "(int *m[])" as the type.
Thanks for explanation. Previously I thought this somehow may be related to tracing data. Looks it is only for *constructing* type names. So it largely looks fine though.
> > However the code assumes we don't need the parentheses for > the array if we have encountered a pointer; that's never > the case. We only should eliminate the opening parens > for a struct or union "{" in such cases, as in those cases > we have a pointer to the struct rather than a nested struct. > So that needs to be fixed. Are the other problems here you're > seeing that the above doesn't cover?
A few minor comments in the above.
> >>> + /* We may not be able to represent this type; bail to be safe */ >>> + if (i == BTF_SHOW_MAX_ITER) >>> + return show->state.type_name; >>> + >>> + if (!type_name) >>> + type_name = btf_name_by_offset(show->btf, t->name_off); >>> + >>> + switch (BTF_INFO_KIND(t->info)) { >>> + case BTF_KIND_STRUCT: >>> + case BTF_KIND_UNION: >>> + prefix = BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT ? >>> + "struct" : "union"; >>> + /* if it's an array of struct/union, parens is already set */ >>> + if (!(kinds & (BTF_KIND_BIT(BTF_KIND_ARRAY)))) >>> + parens = "{"; >>> + break; >>> + case BTF_KIND_ENUM: >>> + prefix = "enum"; >>> + break; >>> + default: >>> + allow_anon = false; [...] >>> + if (elem_type && btf_type_is_int(elem_type)) { >>> + u32 int_type = btf_type_int(elem_type); >>> + >>> + encoding = BTF_INT_ENCODING(int_type); >>> + >>> + /* >>> + * BTF_INT_CHAR encoding never seems to be set for >>> + * char arrays, so if size is 1 and element is >>> + * printable as a char, we'll do that. >>> + */ >>> + if (elem_size == 1) > + encoding = >>> BTF_INT_CHAR; >> >> Some char array may be printable and some may not be printable, >> how did you differentiate this? >> > > I should probably change the logic to ensure all chars > (before a \0) are printable. I'll do that for v2. We will always > have cases (e.g. the skb cb[] field) where the char[] is not > intended as a string, but I think the utility of showing them as > strings where possible is worthwhile.
Make sense. Thanks!
> > Thanks again for reviewing! > > Alan >
| |