Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Chuck Lever III]

> On Jul 16, 2021, at 8:55 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> 
> How about this. I'll just give you the patch and you can apply it to
> your tree. I updated it with documentation, and use memcpy instead of
> strncpy() as it is replacing memcpy() and strncpy() will cause people
> to question the code (as Linus has).
> 
> Here's my latest patch. Feel free to apply it to your tree.

Thanks. I can carry whichever version of this patch that both you
and Linus are happy with. I will apply my changes to use __string_len()
on top of it, and send the whole mess when v5.15 opens in a couple
months.


> Hopefully
> it wont conflict with other work I'm doing. But if it does, we'll work
> it out. I don't have any code that relies on it.
> 
> -- Steve
> 
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> Subject: [PATCH] tracing: Add trace_event helper macros __string_len() and
> __assign_str_len()
> 
> There's a few cases that a string that is to be recorded in a trace event,
> does not have a terminating 'nul' character, and instead, the tracepoint
> passes in the length of the string to record.
> 
> Add two helper macros to the trace event code that lets this work easier,
> than tricks with "%.*s" logic.
> 
>  __string_len() which is similar to __string() for declaration, but takes a
>                 length argument.
> 
>  __assign_str_len() which is similar to __assign_str() for assiging the
>                 string, but it too takes a length argument.
> 
> Note, the TRACE_EVENT() macro will allocate the location on the ring
> buffer to 'len + 1', that will be used to store the string into. It is a
> requirement that the 'len' used for this is a most the length of the
> string being recorded.
> 
> This string can still use __get_str() just like strings created with
> __string() can use to retrieve the string.
> 
> Link: https://lore.kernel.org/linux-nfs/20210513105018.7539996a@xxxxxxxxxxxxxxxxxx/
> 
> Tested-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
> include/trace/trace_events.h               | 22 ++++++++++++++++++
> samples/trace_events/trace-events-sample.h | 27 ++++++++++++++++++++++
> 2 files changed, 49 insertions(+)
> 
> diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
> index acc17194c160..08810a463880 100644
> --- a/include/trace/trace_events.h
> +++ b/include/trace/trace_events.h
> @@ -102,6 +102,9 @@ TRACE_MAKE_SYSTEM_STR();
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, -1)
> 
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, -1)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(char, item, -1)
> 
> @@ -197,6 +200,9 @@ TRACE_MAKE_SYSTEM_STR();
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, -1)
> 
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, -1)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
> 
> @@ -459,6 +465,9 @@ static struct trace_event_functions trace_event_type_funcs_##call = {	\
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, -1)
> 
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, -1)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
> 
> @@ -507,6 +516,9 @@ static struct trace_event_fields trace_event_fields_##call[] = {	\
> #define __string(item, src) __dynamic_array(char, item,			\
> 		    strlen((src) ? (const char *)(src) : "(null)") + 1)
> 
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)
> +
> /*
>  * __bitmask_size_in_bytes_raw is the number of bytes needed to hold
>  * num_possible_cpus().
> @@ -670,10 +682,20 @@ static inline notrace int trace_event_get_offsets_##call(		\
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, -1)
> 
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, -1)
> +
> #undef __assign_str
> #define __assign_str(dst, src)						\
> 	strcpy(__get_str(dst), (src) ? (const char *)(src) : "(null)");
> 
> +#undef __assign_str_len
> +#define __assign_str_len(dst, src, len)					\
> +	do {								\
> +		memcpy(__get_str(dst), (src), (len));			\
> +		__get_str(dst)[len] = '\0';				\
> +	} while(0)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
> 
> diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h
> index 13a35f7cbe66..e61471ab7d14 100644
> --- a/samples/trace_events/trace-events-sample.h
> +++ b/samples/trace_events/trace-events-sample.h
> @@ -141,6 +141,33 @@
>  *         In most cases, the __assign_str() macro will take the same
>  *         parameters as the __string() macro had to declare the string.
>  *
> + *   __string_len: This is a helper to a __dynamic_array, but it understands
> + *	   that the array has characters in it, and with the combined
> + *         use of __assign_str_len(), it will allocate 'len' + 1 bytes
> + *         in the ring buffer and add a '\0' to the string. This is
> + *         useful if the string being saved has no terminating '\0' byte.
> + *         It requires that the length of the string is known as it acts
> + *         like a memcpy().
> + *
> + *         Declared with:
> + *
> + *         __string_len(foo, bar, len)
> + *
> + *         To assign this string, use the helper macro __assign_str_len().
> + *
> + *         __assign_str(foo, bar, len);
> + *
> + *         Then len + 1 is allocated to the ring buffer, and a nul terminating
> + *         byte is added. This is similar to:
> + *
> + *         memcpy(__get_str(foo), bar, len);
> + *         __get_str(foo)[len] = 0;
> + *
> + *        The advantage of using this over __dynamic_array, is that it
> + *        takes care of allocating the extra byte on the ring buffer
> + *        for the '\0' terminating byte, and __get_str(foo) can be used
> + *        in the TP_printk().
> + *
>  *   __bitmask: This is another kind of __dynamic_array, but it expects
>  *         an array of longs, and the number of bits to parse. It takes
>  *         two parameters (name, nr_bits), where name is the name of the
> -- 
> 2.31.1
> 

--
Chuck Lever