On Wed, Sep 23, 2020 at 06:46:24PM +0100, Alan Maguire wrote:
+/* Chunk size we use in safe copy of data to be shown. */ +#define BTF_SHOW_OBJ_SAFE_SIZE 256
sizeof(struct btf_show) == 472 It's allocated on stack and called from bpf prog. It's a leaf function, but it still worries me a bit. I've trimmed it down to 32 and everything seems to be printing fine. There will be more calls to copy_from_kernel_nofault(), but so what? Is there a downside to make it that small?
Similarly state.name is 128 bytes. May be use 80 there? I think that should be plenty still.
- Another problem is we want to ensure the data for display is safe to
- access. To support this, the "struct obj" is used to track the data
'struct obj' doesn't exist. It's an anon field 'struct {} obj;' inside btf_show that you're referring to, right? Would be good to fix this comment.
+struct btf_show {
- u64 flags;
- void *target; /* target of show operation (seq file, buffer) */
- void (*showfn)(struct btf_show *show, const char *fmt, ...);
buildbot complained that this field needs to be annotated.
+#define btf_show(show, ...) \
- do { \
if (!show->state.depth_check) \show->showfn(show, __VA_ARGS__); \- } while (0)
Does it have to be a macro? What are you gaining from macro instead of vararg function?
+static inline const char *__btf_show_indent(struct btf_show *show)
please remove all 'inline' from .c file. There is no need to give such hints to the compiler.
+#define btf_show_indent(show) \
- ((show->flags & BTF_SHOW_COMPACT) ? "" : __btf_show_indent(show))
+#define btf_show_newline(show) \
- ((show->flags & BTF_SHOW_COMPACT) ? "" : "\n")
+#define btf_show_delim(show) \
- (show->state.depth == 0 ? "" : \
((show->flags & BTF_SHOW_COMPACT) && show->state.type && \BTF_INFO_KIND(show->state.type->info) == BTF_KIND_UNION) ? "|" : ",")+#define btf_show_type_value(show, fmt, value) \
- do { \
if ((value) != 0 || (show->flags & BTF_SHOW_ZERO) || \show->state.depth == 0) { \btf_show(show, "%s%s" fmt "%s%s", \btf_show_indent(show), \btf_show_name(show), \value, btf_show_delim(show), \btf_show_newline(show)); \if (show->state.depth > show->state.depth_to_show) \show->state.depth_to_show = show->state.depth; \} \- } while (0)
+#define btf_show_type_values(show, fmt, ...) \
- do { \
btf_show(show, "%s%s" fmt "%s%s", btf_show_indent(show), \btf_show_name(show), \__VA_ARGS__, btf_show_delim(show), \btf_show_newline(show)); \if (show->state.depth > show->state.depth_to_show) \show->state.depth_to_show = show->state.depth; \- } while (0)
+/* How much is left to copy to safe buffer after @data? */ +#define btf_show_obj_size_left(show, data) \
- (show->obj.head + show->obj.size - data)
+/* Is object pointed to by @data of @size already copied to our safe buffer? */ +#define btf_show_obj_is_safe(show, data, size) \
- (data >= show->obj.data && \
(data + size) < (show->obj.data + BTF_SHOW_OBJ_SAFE_SIZE))+/*
- If object pointed to by @data of @size falls within our safe buffer, return
- the equivalent pointer to the same safe data. Assumes
- copy_from_kernel_nofault() has already happened and our safe buffer is
- populated.
- */
+#define __btf_show_obj_safe(show, data, size) \
- (btf_show_obj_is_safe(show, data, size) ? \
show->obj.safe + (data - show->obj.data) : NULL)
Similarly I don't understand the benefit of macros. They all could have been normal functions.
+static inline void *btf_show_obj_safe(struct btf_show *show,
const struct btf_type *t,void *data)
drop 'inline' pls.
+{
- int size_left, size;
- void *safe = NULL;
- if (show->flags & BTF_SHOW_UNSAFE)
return data;- (void) btf_resolve_size(show->btf, t, &size);
Is this ok to ignore the error?