Re: [PATCH v2 2/2] trace: allocate space from temparary trace sequence buffer

[Linyu Yuan]

On 12/15/2022 9:23 PM, Steven Rostedt wrote:
> On Thu, 15 Dec 2022 12:33:27 +0800
> Linyu Yuan <quic_linyyuan@xxxxxxxxxxx> wrote:
>
> > --- a/include/linux/trace_seq.h
> > +++ b/include/linux/trace_seq.h
> > @@ -95,6 +95,7 @@ extern void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
> >   extern int trace_seq_hex_dump(struct trace_seq *s, const char *prefix_str,
> >   			      int prefix_type, int rowsize, int groupsize,
> >   			      const void *buf, size_t len, bool ascii);
> > +void *trace_seq_alloc_buffer(struct trace_seq *s, int len);
> So, I really don't like the name with "alloc" in it. That makes the
> assumption that it also needs to be freed. Which it does not, and why it
> confused me last night.
>
> A better name would be trace_seq_acquire(s, len);
>
> And it should return a char *, as it it process stings and not arbitrary
> binary data.
thanks, will update next version.
> > +/**
> > + * trace_seq_alloc_buffer - allocate seq buffer with size len
> > + * @s: trace sequence descriptor
> > + * @len: size of buffer to be allocated
> > + *
> > + * allocate space with size of @len from seq buffer for output usage,
> > + * On success, it returns start address of the allocated buffer,
> > + * user can fill data start from the address, user should make sure the
> > + * data length not exceed the @len, if it exceed, behavior is undefined.
> > + *
> > + * Returns NULL if no buffer can be allocated, it also means system will
> > + * crash, it is user responsiblity to make sure total buffer used will
> > + * not exceed PAGE_SIZE.
> > + *
> > + * it allow multiple usage in one trace output function call.
> > + */
> > +void *trace_seq_alloc_buffer(struct trace_seq *s, int len)
> char *trace_seq_acquire(struct trace_seq *s, int len)
>
> > +{
> > +	char *buf = trace_seq_buffer_ptr(s);
> > +
> You need to check the length first before committing:
>
> 	if (seq_buf_buffer_left(&s->seq) < len)
> 		return NULL;
will change next version.
> 	
> > +	seq_buf_commit(&s->seq, len);
> Because the above is a bug if len is too big.
>
> > +
> > +	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
> And then we don't need this either.
>
> I must apologize for the response last night. It was past my normal bed
> time, and I must really avoid reviewing patches when I should be going to
> bed ;-)

sorry to bring your attention near your bed time.

I accept late response next time 😁.

thanks again for all your comment today, it is very good.

> -- Steve
>
>
> > +		s->full = 1;
> > +		return NULL;
> > +	}
> > +
> > +	return (void *)buf;
> > +}
> > +EXPORT_SYMBOL(trace_seq_alloc_buffer);