Re: [PATCH v4 2/2] tracing: Bound histogram expression strings with seq_buf

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 21 May 2026 10:28:17 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> @@ -1778,47 +1783,56 @@ static char *expr_str(struct hist_field *field, unsigned int level)
>  	if (!expr)
>  		return ERR_PTR(-ENOMEM);
>  
> +	seq_buf_init(&s, expr, MAX_FILTER_STR_VAL);
> +
>  	if (!field->operands[0]) {
> -		expr_field_str(field, expr);
> +		if (!expr_field_str(field, &s))
> +			return ERR_PTR(-E2BIG);
> +
>  		return_ptr(expr);
>  	}
>  
>  	if (field->operator == FIELD_OP_UNARY_MINUS) {
> -		char *subexpr;
> +		char *subexpr __free(kfree) = NULL;
>  
> -		strcat(expr, "-(");
> +		seq_buf_puts(&s, "-(");
>  		subexpr = expr_str(field->operands[0], ++level);
>  		if (IS_ERR(subexpr))
>  			return subexpr;
>  
> -		strcat(expr, subexpr);
> -		strcat(expr, ")");
> +		seq_buf_puts(&s, subexpr);
> +		seq_buf_putc(&s, ')');
> +		seq_buf_str(&s);
>  
> -		kfree(subexpr);
> +		if (seq_buf_has_overflowed(&s))
> +			return ERR_PTR(-E2BIG);
>  
>  		return_ptr(expr);
>  	}

Wouldn't the above if statement be a lot nicer as:

 	if (field->operator == FIELD_OP_UNARY_MINUS) {
		char *subexpr;

		subexpr = expr_str(field->operands[0], ++level);
		if (IS_ERR(subexpr))
			return subexpr;

		seq_buf_printf(&s, "-(%s)", subexpr); 
		seq_buf_str(&s);
  		kfree(subexpr);

		if (seq_buf_has_overflowed(&s))
			return ERR_PTR(-E2BIG);
 
		return_ptr(expr);
 	}

In fact, the above is so simple, you don't even need to use the __free()
guard on subexpr.

BTW, because currently seq_buf_printf() does add a '\0' to the string, I
may update the API for seq_buf to state that you don't need to terminate
after calling that function. So you can leave out the sub_buf_str() after
calling seq_buf_printf().

-- Steve