Hi Andrii,
Thanks for your careful review.
On Mon, Jun 2, 2025 at 5:06 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
bool print_strings; /* print char arrays as strings */let's use "emit_strings" naming, so it's consistent with emit_zeroes?
Done.
@@ -75,6 +75,7 @@ struct btf_dump_data { [...]
bool print_strings;ditto, emit_strings (and maybe put it next to emit_zeroes then)
Done.
+static int btf_dump_string_data(struct btf_dump *d, [...]
if (!btf_is_int(skip_mods_and_typedefs(d->btf, array->type, NULL)) ||btf__resolve_size(d->btf, array->type) != 1 ||!d->typed_dump->print_strings) {pr_warn("unexpected %s() call for array type %u\n",__func__, array->type);return -EINVAL;}IMO, a bit too defensive. You literally checked that we have char[] in the caller, I think it's fine not to double-check that here, let's drop this
Done.
if (c == '\0') {/* When printing character arrays as strings, NUL bytes* are always treated as string terminators; they are* never printed.*/break;what if there are non-zero characters after the terminating zero? should we keep going and if there is any non-zero one, still emit them? or maybe that should be an extra option?... When capturing some data and dumping, it might be important to know all the contents (it might be garbage or not, but you'll still see non-garbage values before \0, so maybe it's fine to always do it?)
I was thinking of this option as being optimized for common-case pretty-printing, rather than being the ideal tool for displaying arbitrary character arrays. If there are garbage values that are worth displaying, btf_dump() without the ".emit_strings" option would still show them.
+static int find_char_array_type(struct btf *btf, int nelems) [...]
if (btf_kind(t) != BTF_KIND_ARRAY)btf_is_array()
Removed, in light of your next comment.
+static int btf_dump_string_data(struct btf *btf, struct btf_dump *d, [...]
snprintf(name, sizeof(name), "char[%zu]", ptr_sz);type_id = find_char_array_type(btf, ptr_sz);instead of trying to find a suitable type in kernel BTF, just generate a tiny custom BTF with necessary char[N] types? see btf__add_xxx() usage for an example.
Ah thanks, that's a much better approach. Fixed.
I'll send out an updated version of these changes with the comments I've received so far.
Blake