Steven Rostedt wrote on Sat, Dec 02, 2023 at 08:14:09PM -0500:
> > AFAICS __entry is a local variable on stack, and array __entry->line not
> > intialized with zeros, i.e. the dump would contain trash at the end. Maybe
> > prepending memset() before memcpy()?
Well spotted!
Now I'm thinking about it we weren't initializing the source buffer
either back when we had (>32) msize allocations, so these already had
been printing garbage, but might as well get this sorted out while we're
here.
> __entry is a macro that points into the ring buffer that gets allocated
> before this is called. TRACE_EVENT() has a __dynamic_array() field that
> can handle variable length arrays. What you can do is turn this into
> something like:
>
> TRACE_EVENT(9p_protocol_dump,
> TP_PROTO(struct p9_client *clnt, struct p9_fcall *pdu),
>
> TP_ARGS(clnt, pdu),
>
> TP_STRUCT__entry(
> __field( void *, clnt
> )
> __field( __u8, type
> )
> __field( __u16, tag
> )
> __dynamic_array(unsigned char, line, min(pdu->capacity,
> P9_PROTO_DUMP_SZ) )
> ),
>
> TP_fast_assign(
> __entry->clnt = clnt;
> __entry->type = pdu->id;
> __entry->tag = pdu->tag;
> memcpy(__get_dynamic_array(line), pdu->sdata,
> min(pdu->capacity, P9_PROTO_DUMP_SZ));
> ),
> TP_printk("clnt %lu %s(tag = %d)\n%.3x: %16ph\n%.3x: %16ph\n",
> (unsigned long)__entry->clnt, show_9p_op(__entry->type),
> __entry->tag, 0, __get_dynamic_array(line), 16,
> __get_dynamic_array(line) + 16)
This was just printing garbage in the previous version but %16ph with a
dynamic alloc would be out of range (even the start of the next buffer,
_get_dynamic_array(line) + 16, can be out of range)
Also, for custom tracepoints e.g. bpftrace the program needs to know how
many bytes can be read safely even if it's just for dumping -- unless
dynamic_array is a "fat pointer" that conveys its own size?
(Sorry didn't take the time to check)
So I see two ways forward:
- We can give up on the 16 bytes split here, add the size in one of the
fields, and print with %*ph using that size.
- Or just give up and zero the tail; I'm surprised there's no "memcpy
up to x bytes and zero up to y bytes if required" helper but Christian's
suggestion of always doing memset first is probably not that bad
performance-wise if someone's dumping these out already.
I don't have a hard preference here, what do you think?
--
Dominique Martinet | Asmadeus
