> From: Ankur Dwivedi [mailto:adwiv...@marvell.com]
> Sent: Thursday, 12 January 2023 12.22
>
> Adds a trace point emit function for emitting a blob. The maximum blob
> bytes which can be captured is maximum value contained in uint16_t,
> which is 65535.
>
> Also adds test case for emit array tracepoint function.
>
> Signed-off-by: Ankur Dwivedi <adwiv...@marvell.com>
> ---
Excellent, thank you.
[...]
> +#define rte_trace_point_emit_blob(in, len) \
> +do { \
> + if (unlikely(in == NULL)) \
> + return; \
> + __rte_trace_point_emit(len, uint16_t); \
> + memcpy(mem, in, len); \
> + mem = RTE_PTR_ADD(mem, len); \
> +} while (0)
I am somewhat unsure about my concerns here, so please forgive me if they are
invalid...
Is rte_trace_point_emit_blob() always called with "len" being a variable, then
this is OK.
If "len" can be a non-constant formula (e.g. len++), you need a temporary
variable:
#define rte_trace_point_emit_blob(in, len) \
do { \
uint16_t _len = len; \
if (unlikely(in == NULL)) \
return; \
__rte_trace_point_emit(_len, uint16_t); \
memcpy(mem, in, _len); \
mem = RTE_PTR_ADD(_mem, _len); \
} while (0)
But I don't think this can ever happen.
However, if "len" can be a constant (e.g. 6), does __rte_trace_point_emit(len,
uint16_t) accept it? (The __rte_trace_point_emit() macro is shown below.) A
constant has no pointer to it (i.e. &6 does not exist).
Looking deeper at it, I'm afraid this question can be generalized to all the
existing macros/functions calling __rte_trace_point_emit().
And now that I have hijacked your patch with a generalized question, I wonder
if the existing __rte_trace_point_emit() has a bug? It uses sizeof(in), but I
think it should use sizeof(type).
It looks like this:
#define __rte_trace_point_emit(in, type) \
do { \
memcpy(mem, &(in), sizeof(in)); \
mem = RTE_PTR_ADD(mem, sizeof(in)); \
} while (0)
Alternatively, __rte_trace_point_emit() should RTE_BUILD_BUG_ON(typeof(in) !=
type).
If my concerns above are invalid, then:
Acked-by: Morten Brørup <m...@smartsharesystems.com>
