On Fri, Mar 20, 2020 at 12:58 AM Mattias Rönnblom <mattias.ronnb...@ericsson.com> wrote:
Thanks for the review. > > On 2020-03-18 20:02, jer...@marvell.com wrote: > > From: Jerin Jacob <jer...@marvell.com> > > > > The trace function payloads such as rte_trace_ctf_* have > > dual functions. The first to emit the payload for the registration > > function and the second one to act as trace mem emitters aka > > provider payload. > > + > > +#define rte_trace_ctf_u64(in) __rte_trace_emit_datatype(in) > > Would it be worth to do a type check here? To avoid having someone do > something like: > > uint32_t v = 42; > > rte_trace_ctf_u64(v); > > which would spew out a 32-bit number, where there should be 64 bits. It is taken care with the register version of this macro by adding RTE_BUILD_BUG_ON. http://patches.dpdk.org/patch/66861/ See: #define rte_trace_ctf_i64(in)\ RTE_BUILD_BUG_ON(sizeof(int64_t) != sizeof(typeof(in)));\ __rte_trace_emit_ctf_field(sizeof(int64_t), RTE_STR(in), "int64_t") > > Or maybe better: do an assignment, allowing type conversion (promotion > at least), and type-checking, of sorts. The macro-generated code could > look something like: > > do { > > uint64_t _in = in; > > __rte_trace_emit_datatype(_in); > > } while (0) > > If you add a type parameter to __rte_trace_emit_datatype(), it can do > the job. > > > + > > +#define rte_trace_ctf_string(in)\ > Add the usual do { /../ } while (0) here? Ack. Will add it in the place where do { /../ } while (0) can be added in v2. > > + if (unlikely(in == NULL))\ > > + return;\ > > + rte_strscpy(mem, in, __RTE_TRACE_EMIT_STRING_LEN_MAX);\ > > + mem = RTE_PTR_ADD(mem, __RTE_TRACE_EMIT_STRING_LEN_MAX) > > + > > #endif /* _RTE_TRACE_PROVIDER_H_ */ > >
