Re: [PATCH 1/3] cmd: setexpr: add fmt format string operation

[Wolfgang Denk <wd@denx.de>]

Dear Roland,

In message <20210628151750.572837-2-roland.gaudig-oss@weidmueller.com> you wrote:
>
> +		if (*format == '%') {
> +			switch (*(format + 1)) {
> +			case '%':
> +				/* found '%%', not a format specifier, skip. */
> +				format++;
> +				break;
> +			case '\0':
> +				/* found '%' at end of string,
> +				 * incomplete format specifier.
> +				 */
> +				return NULL;
> +			default:
> +				/* looks like a format specifier */
> +				return format;
> +			}
> +		}

Why do you do that here?  *printf() should be clever enough to parst
the format string.

> +static int setexpr_fmt(char *name, char *format, char *value)
> +{
> +	struct expr_arg aval;
> +	int data_size;
> +	char str[MAX_STR_LEN];
> +	int fmt_len = strlen(format);
> +
> +	if (fmt_len < 2) {
> +		printf("Error: missing format string");
> +		return CMD_RET_FAILURE;
> +	}

This is an arbitrary restriction that just limits the potential use.
Please don't do this.  Maybe I want to use:

	=> setexpr foo fmt X

This should not cause problems.

> +	/* Exactly one format specifier is required */
> +	if (!first || setexpr_fmt_spec_search(first + 1)) {
> +		printf("Error: exactly one format specifier is required\n");
> +		return CMD_RET_FAILURE;
> +	}

Please get rid of this restriction.

> +	/* Search type field of format specifier */
> +	while (*first && !isalpha(*first))
> +		first++;
> +
> +	/* Test if type is supported */
> +	if (!strchr("diouxX", *first)) {
> +		printf("Error: format type not supported\n");
> +		return CMD_RET_FAILURE;
> +	}

Get rid of all these, please!


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Man did not weave the web of life; he  is  merely  a  strand  in  it.
Whatever he does to the web, he does to himself.     - Seattle [1854]