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?
