[PATCH v3 0/6] cmd: setexpr: add fmt format string operation

[PATCH v3 0/6] cmd: setexpr: add fmt format string operation

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>


In contrast to version 2, the "fmt" operator has been made more flexible
like its Bash counterpart, as some comments to version 2 requested.
For license compatibility reasons the code for parsing the format string
has been taken from the BusyBox project and not from Bash.

The _maxags limit for setexpr has been increased to 8, which allows up
to 4 remaining arguments for the format string.

This patch adds C like format string capabilities to the setexpr
command. Here are some examples:

  => setexpr foo fmt %d 0x100
  => echo $foo
  256
  =>

  => setexpr foo fmt 0x%08x 0x63
  => echo $foo
  0x00000063
  =>

  => setexpr foo fmt %%%o 8
  => echo $foo
  %10
  =>

  => setexpr foo fmt \"0x%08x-%s-%d-%s\" $a $b $c $d
  => echo $foo
  0x00000eff-hello-99-world
  =>

Format string handling can be turned on by enabling the
CONFIG_CMD_SETEXPR_FMT option. Enabling that option will increase code
size by 2480 bytes on ARM target and 1648 bytes on ARM Thumb2 target.
Handling of float already has been removed. A further reduction is
possible by disabling the escape character handling.

Changes in v3:
 - enable Bash like format string support for the setexpr fmt operator
 - import format string parsing routines from Busybox project

Changes in v2:
 - replace the setexpr dec operator by fmt for simple format strings


(no changes since v1)

Roland Gaudig (6):
  lib: strto: add simple_strtoll function
  cmd: printf: import busybox-1.33.1 printf.c
  cmd: printf: add helper functions from busybox
  cmd: setexpr: add format string handling
  doc: usage: add description for setexpr command
  test: cmd: setexpr: add format string tests

 MAINTAINERS           |   6 +
 cmd/Kconfig           |   8 +
 cmd/Makefile          |   1 +
 cmd/printf.c          | 647 ++++++++++++++++++++++++++++++++++++++++++
 cmd/printf.h          |   8 +
 cmd/setexpr.c         |  37 ++-
 doc/usage/index.rst   |   1 +
 doc/usage/setexpr.rst | 148 ++++++++++
 include/vsprintf.h    |   1 +
 lib/strto.c           |   8 +
 test/cmd/setexpr.c    |  84 ++++++
 11 files changed, 945 insertions(+), 4 deletions(-)
 create mode 100644 cmd/printf.c
 create mode 100644 cmd/printf.h
 create mode 100644 doc/usage/setexpr.rst

-- 
2.25.1

[PATCH v3 1/6] lib: strto: add simple_strtoll function

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Add simple_strtoll function for converting a string containing digits
into a long long int value.

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

(no changes since v1)

 include/vsprintf.h | 1 +
 lib/strto.c        | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/vsprintf.h b/include/vsprintf.h
index 2290083eba..4016de6677 100644
--- a/include/vsprintf.h
+++ b/include/vsprintf.h
@@ -41,6 +41,7 @@ int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
 unsigned long long simple_strtoull(const char *cp, char **endp,
 					unsigned int base);
 long simple_strtol(const char *cp, char **endp, unsigned int base);
+long long simple_strtoll(const char *cp, char **endp, unsigned int base);
 
 /**
  * trailing_strtol() - extract a trailing integer from a string
diff --git a/lib/strto.c b/lib/strto.c
index c00bb5895d..f8b53d846b 100644
--- a/lib/strto.c
+++ b/lib/strto.c
@@ -143,6 +143,14 @@ unsigned long long simple_strtoull(const char *cp, char **endp,
 	return result;
 }
 
+long long simple_strtoll(const char *cp, char **endp, unsigned int base)
+{
+	if (*cp == '-')
+		return -simple_strtoull(cp + 1, endp, base);
+
+	return simple_strtoull(cp, endp, base);
+}
+
 long trailing_strtoln(const char *str, const char *end)
 {
 	const char *p;
-- 
2.25.1

[PATCH v3 2/6] cmd: printf: import busybox-1.33.1 printf.c

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Import printf.c from the Busybox project, which provides Bash like
format string handling.

  src-url: https://git.busybox.net/busybox/
  commit bcc5b0e6caca6c7602a6a41f "Bump version to 1.33.1"
  version: 1.33.1

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

(no changes since v1)

 cmd/printf.c | 455 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 455 insertions(+)
 create mode 100644 cmd/printf.c

diff --git a/cmd/printf.c b/cmd/printf.c
new file mode 100644
index 0000000000..a20fc33013
--- /dev/null
+++ b/cmd/printf.c
@@ -0,0 +1,455 @@
+/* vi: set sw=4 ts=4: */
+/*
+ * printf - format and print data
+ *
+ * Copyright 1999 Dave Cinege
+ * Portions copyright (C) 1990-1996 Free Software Foundation, Inc.
+ *
+ * Licensed under GPLv2 or later, see file LICENSE in this source tree.
+ */
+/* Usage: printf format [argument...]
+ *
+ * A front end to the printf function that lets it be used from the shell.
+ *
+ * Backslash escapes:
+ *
+ * \" = double quote
+ * \\ = backslash
+ * \a = alert (bell)
+ * \b = backspace
+ * \c = produce no further output
+ * \f = form feed
+ * \n = new line
+ * \r = carriage return
+ * \t = horizontal tab
+ * \v = vertical tab
+ * \0ooo = octal number (ooo is 0 to 3 digits)
+ * \xhhh = hexadecimal number (hhh is 1 to 3 digits)
+ *
+ * Additional directive:
+ *
+ * %b = print an argument string, interpreting backslash escapes
+ *
+ * The 'format' argument is re-used as many times as necessary
+ * to convert all of the given arguments.
+ *
+ * David MacKenzie <djm@gnu.ai.mit.edu>
+ */
+/* 19990508 Busy Boxed! Dave Cinege */
+
+//config:config PRINTF
+//config:	bool "printf (3.8 kb)"
+//config:	default y
+//config:	help
+//config:	printf is used to format and print specified strings.
+//config:	It's similar to 'echo' except it has more options.
+
+//applet:IF_PRINTF(APPLET_NOFORK(printf, printf, BB_DIR_USR_BIN, BB_SUID_DROP, printf))
+
+//kbuild:lib-$(CONFIG_PRINTF) += printf.o
+//kbuild:lib-$(CONFIG_ASH_PRINTF)  += printf.o
+//kbuild:lib-$(CONFIG_HUSH_PRINTF) += printf.o
+
+//usage:#define printf_trivial_usage
+//usage:       "FORMAT [ARG]..."
+//usage:#define printf_full_usage "\n\n"
+//usage:       "Format and print ARG(s) according to FORMAT (a-la C printf)"
+//usage:
+//usage:#define printf_example_usage
+//usage:       "$ printf \"Val=%d\\n\" 5\n"
+//usage:       "Val=5\n"
+
+#include "libbb.h"
+
+/* A note on bad input: neither bash 3.2 nor coreutils 6.10 stop on it.
+ * They report it:
+ *  bash: printf: XXX: invalid number
+ *  printf: XXX: expected a numeric value
+ *  bash: printf: 123XXX: invalid number
+ *  printf: 123XXX: value not completely converted
+ * but then they use 0 (or partially converted numeric prefix) as a value
+ * and continue. They exit with 1 in this case.
+ * Both accept insane field width/precision (e.g. %9999999999.9999999999d).
+ * Both print error message and assume 0 if %*.*f width/precision is "bad"
+ *  (but negative numbers are not "bad").
+ * Both accept negative numbers for %u specifier.
+ *
+ * We try to be compatible.
+ */
+
+typedef void FAST_FUNC (*converter)(const char *arg, void *result);
+
+static int multiconvert(const char *arg, void *result, converter convert)
+{
+	if (*arg == '"' || *arg == '\'') {
+		arg = utoa((unsigned char)arg[1]);
+	}
+	errno = 0;
+	convert(arg, result);
+	if (errno) {
+		bb_error_msg("invalid number '%s'", arg);
+		return 1;
+	}
+	return 0;
+}
+
+static void FAST_FUNC conv_strtoull(const char *arg, void *result)
+{
+	/* Allow leading '+' - bb_strtoull() by itself does not allow it,
+	 * and probably shouldn't (other callers might require purely numeric
+	 * inputs to be allowed.
+	 */
+	if (arg[0] == '+')
+		arg++;
+	*(unsigned long long*)result = bb_strtoull(arg, NULL, 0);
+	/* both coreutils 6.10 and bash 3.2:
+	 * $ printf '%x\n' -2
+	 * fffffffffffffffe
+	 * Mimic that:
+	 */
+	if (errno) {
+		*(unsigned long long*)result = bb_strtoll(arg, NULL, 0);
+	}
+}
+static void FAST_FUNC conv_strtoll(const char *arg, void *result)
+{
+	if (arg[0] == '+')
+		arg++;
+	*(long long*)result = bb_strtoll(arg, NULL, 0);
+}
+static void FAST_FUNC conv_strtod(const char *arg, void *result)
+{
+	char *end;
+	/* Well, this one allows leading whitespace... so what? */
+	/* What I like much less is that "-" accepted too! :( */
+	*(double*)result = strtod(arg, &end);
+	if (end[0]) {
+		errno = ERANGE;
+		*(double*)result = 0;
+	}
+}
+
+/* Callers should check errno to detect errors */
+static unsigned long long my_xstrtoull(const char *arg)
+{
+	unsigned long long result;
+	if (multiconvert(arg, &result, conv_strtoull))
+		result = 0;
+	return result;
+}
+static long long my_xstrtoll(const char *arg)
+{
+	long long result;
+	if (multiconvert(arg, &result, conv_strtoll))
+		result = 0;
+	return result;
+}
+static double my_xstrtod(const char *arg)
+{
+	double result;
+	multiconvert(arg, &result, conv_strtod);
+	return result;
+}
+
+/* Handles %b; return 1 if output is to be short-circuited by \c */
+static int print_esc_string(const char *str)
+{
+	char c;
+	while ((c = *str) != '\0') {
+		str++;
+		if (c == '\\') {
+			/* %b also accepts 4-digit octals of the form \0### */
+			if (*str == '0') {
+				if ((unsigned char)(str[1] - '0') < 8) {
+					/* 2nd char is 0..7: skip leading '0' */
+					str++;
+				}
+			}
+			else if (*str == 'c') {
+				return 1;
+			}
+			{
+				/* optimization: don't force arg to be on-stack,
+				 * use another variable for that. */
+				const char *z = str;
+				c = bb_process_escape_sequence(&z);
+				str = z;
+			}
+		}
+		putchar(c);
+	}
+
+	return 0;
+}
+
+static void print_direc(char *format, unsigned fmt_length,
+		int field_width, int precision,
+		const char *argument)
+{
+	long long llv;
+	double dv;
+	char saved;
+	char *have_prec, *have_width;
+
+	saved = format[fmt_length];
+	format[fmt_length] = '\0';
+
+	have_prec = strstr(format, ".*");
+	have_width = strchr(format, '*');
+	if (have_width - 1 == have_prec)
+		have_width = NULL;
+
+	/* multiconvert sets errno = 0, but %s needs it cleared */
+	errno = 0;
+
+	switch (format[fmt_length - 1]) {
+	case 'c':
+		printf(format, *argument);
+		break;
+	case 'd':
+	case 'i':
+		llv = my_xstrtoll(skip_whitespace(argument));
+ print_long:
+		if (!have_width) {
+			if (!have_prec)
+				printf(format, llv);
+			else
+				printf(format, precision, llv);
+		} else {
+			if (!have_prec)
+				printf(format, field_width, llv);
+			else
+				printf(format, field_width, precision, llv);
+		}
+		break;
+	case 'o':
+	case 'u':
+	case 'x':
+	case 'X':
+		llv = my_xstrtoull(skip_whitespace(argument));
+		/* cheat: unsigned long and long have same width, so... */
+		goto print_long;
+	case 's':
+		/* Are char* and long long the same? */
+		if (sizeof(argument) == sizeof(llv)) {
+			llv = (long long)(ptrdiff_t)argument;
+			goto print_long;
+		} else {
+			/* Hope compiler will optimize it out by moving call
+			 * instruction after the ifs... */
+			if (!have_width) {
+				if (!have_prec)
+					printf(format, argument, /*unused:*/ argument, argument);
+				else
+					printf(format, precision, argument, /*unused:*/ argument);
+			} else {
+				if (!have_prec)
+					printf(format, field_width, argument, /*unused:*/ argument);
+				else
+					printf(format, field_width, precision, argument);
+			}
+			break;
+		}
+	case 'f':
+	case 'e':
+	case 'E':
+	case 'g':
+	case 'G':
+		dv = my_xstrtod(argument);
+		if (!have_width) {
+			if (!have_prec)
+				printf(format, dv);
+			else
+				printf(format, precision, dv);
+		} else {
+			if (!have_prec)
+				printf(format, field_width, dv);
+			else
+				printf(format, field_width, precision, dv);
+		}
+		break;
+	} /* switch */
+
+	format[fmt_length] = saved;
+}
+
+/* Handle params for "%*.*f". Negative numbers are ok (compat). */
+static int get_width_prec(const char *str)
+{
+	int v = bb_strtoi(str, NULL, 10);
+	if (errno) {
+		bb_error_msg("invalid number '%s'", str);
+		v = 0;
+	}
+	return v;
+}
+
+/* Print the text in FORMAT, using ARGV for arguments to any '%' directives.
+   Return advanced ARGV.  */
+static char **print_formatted(char *f, char **argv, int *conv_err)
+{
+	char *direc_start;      /* Start of % directive.  */
+	unsigned direc_length;  /* Length of % directive.  */
+	int field_width;        /* Arg to first '*' */
+	int precision;          /* Arg to second '*' */
+	char **saved_argv = argv;
+
+	for (; *f; ++f) {
+		switch (*f) {
+		case '%':
+			direc_start = f++;
+			direc_length = 1;
+			field_width = precision = 0;
+			if (*f == '%') {
+				bb_putchar('%');
+				break;
+			}
+			if (*f == 'b') {
+				if (*argv) {
+					if (print_esc_string(*argv))
+						return saved_argv; /* causes main() to exit */
+					++argv;
+				}
+				break;
+			}
+			if (*f && strchr("-+ #", *f)) {
+				++f;
+				++direc_length;
+			}
+			if (*f == '*') {
+				++f;
+				++direc_length;
+				if (*argv)
+					field_width = get_width_prec(*argv++);
+			} else {
+				while (isdigit(*f)) {
+					++f;
+					++direc_length;
+				}
+			}
+			if (*f == '.') {
+				++f;
+				++direc_length;
+				if (*f == '*') {
+					++f;
+					++direc_length;
+					if (*argv)
+						precision = get_width_prec(*argv++);
+				} else {
+					while (isdigit(*f)) {
+						++f;
+						++direc_length;
+					}
+				}
+			}
+
+			/* Remove "lLhz" size modifiers, repeatedly.
+			 * bash does not like "%lld", but coreutils
+			 * happily takes even "%Llllhhzhhzd"!
+			 * We are permissive like coreutils */
+			while ((*f | 0x20) == 'l' || *f == 'h' || *f == 'z') {
+				overlapping_strcpy(f, f + 1);
+			}
+			/* Add "ll" if integer modifier, then print */
+			{
+				static const char format_chars[] ALIGN1 = "diouxXfeEgGcs";
+				char *p = strchr(format_chars, *f);
+				/* needed - try "printf %" without it */
+				if (p == NULL || *f == '\0') {
+					bb_error_msg("%s: invalid format", direc_start);
+					/* causes main() to exit with error */
+					return saved_argv - 1;
+				}
+				++direc_length;
+				if (p - format_chars <= 5) {
+					/* it is one of "diouxX" */
+					p = xmalloc(direc_length + 3);
+					memcpy(p, direc_start, direc_length);
+					p[direc_length + 1] = p[direc_length - 1];
+					p[direc_length - 1] = 'l';
+					p[direc_length] = 'l';
+					//bb_error_msg("<%s>", p);
+					direc_length += 2;
+					direc_start = p;
+				} else {
+					p = NULL;
+				}
+				if (*argv) {
+					print_direc(direc_start, direc_length, field_width,
+								precision, *argv++);
+				} else {
+					print_direc(direc_start, direc_length, field_width,
+								precision, "");
+				}
+				*conv_err |= errno;
+				free(p);
+			}
+			break;
+		case '\\':
+			if (*++f == 'c') {
+				return saved_argv; /* causes main() to exit */
+			}
+			bb_putchar(bb_process_escape_sequence((const char **)&f));
+			f--;
+			break;
+		default:
+			putchar(*f);
+		}
+	}
+
+	return argv;
+}
+
+int printf_main(int argc UNUSED_PARAM, char **argv)
+{
+	int conv_err;
+	char *format;
+	char **argv2;
+
+	/* We must check that stdout is not closed.
+	 * The reason for this is highly non-obvious.
+	 * printf_main is used from shell.
+	 * Shell must correctly handle 'printf "%s" foo'
+	 * if stdout is closed. With stdio, output gets shoveled into
+	 * stdout buffer, and even fflush cannot clear it out. It seems that
+	 * even if libc receives EBADF on write attempts, it feels determined
+	 * to output data no matter what. So it will try later,
+	 * and possibly will clobber future output. Not good. */
+// TODO: check fcntl() & O_ACCMODE == O_WRONLY or O_RDWR?
+	if (fcntl(1, F_GETFL) == -1)
+		return 1; /* match coreutils 6.10 (sans error msg to stderr) */
+	//if (dup2(1, 1) != 1) - old way
+	//	return 1;
+
+	/* bash builtin errors out on "printf '-%s-\n' foo",
+	 * coreutils-6.9 works. Both work with "printf -- '-%s-\n' foo".
+	 * We will mimic coreutils. */
+	if (argv[1] && argv[1][0] == '-' && argv[1][1] == '-' && !argv[1][2])
+		argv++;
+	if (!argv[1]) {
+		if (ENABLE_ASH_PRINTF
+		 && applet_name[0] != 'p'
+		) {
+			bb_simple_error_msg("usage: printf FORMAT [ARGUMENT...]");
+			return 2; /* bash compat */
+		}
+		bb_show_usage();
+	}
+
+	format = argv[1];
+	argv2 = argv + 2;
+
+	conv_err = 0;
+	do {
+		argv = argv2;
+		argv2 = print_formatted(format, argv, &conv_err);
+	} while (argv2 > argv && *argv2);
+
+	/* coreutils compat (bash doesn't do this):
+	if (*argv)
+		fprintf(stderr, "excess args ignored");
+	*/
+
+	return (argv2 < argv) /* if true, print_formatted errored out */
+		|| conv_err; /* print_formatted saw invalid number */
+}
-- 
2.25.1

[PATCH v3 3/6] cmd: printf: add helper functions from busybox

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Import the following helper functions from Busybox-1.33.1 which are
required by printf.c:

  process_escape_sequence from libbb/process_escape_sequence.c,
  skip_whitespace from libbb/skip_whitespace.c,
  overlapping_strcpy  from libbb/safe_strncpy.c

  src-url: https://git.busybox.net/busybox/
  commit bcc5b0e6caca6c7602a6a41f "Bump version to 1.33.1"
  version: 1.33.1

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

(no changes since v1)

 cmd/printf.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/cmd/printf.c b/cmd/printf.c
index a20fc33013..337ab8ce5d 100644
--- a/cmd/printf.c
+++ b/cmd/printf.c
@@ -79,6 +79,127 @@
 
 typedef void FAST_FUNC (*converter)(const char *arg, void *result);
 
+#define WANT_HEX_ESCAPES 0
+
+/* Usual "this only works for ascii compatible encodings" disclaimer. */
+#undef _tolower
+#define _tolower(X) ((X)|((char) 0x20))
+
+char FAST_FUNC bb_process_escape_sequence(const char **ptr)
+{
+	const char *q;
+	unsigned num_digits;
+	unsigned n;
+	unsigned base;
+
+	num_digits = n = 0;
+	base = 8;
+	q = *ptr;
+
+	if (WANT_HEX_ESCAPES && *q == 'x') {
+		++q;
+		base = 16;
+		++num_digits;
+	}
+
+	/* bash requires leading 0 in octal escapes:
+	 * \02 works, \2 does not (prints \ and 2).
+	 * We treat \2 as a valid octal escape sequence. */
+	do {
+		unsigned r;
+		unsigned d = (unsigned char)(*q) - '0';
+#if WANT_HEX_ESCAPES
+		if (d >= 10) {
+			d = (unsigned char)_tolower(*q) - 'a';
+			//d += 10;
+			/* The above would map 'A'-'F' and 'a'-'f' to 10-15,
+			 * however, some chars like '@' would map to 9 < base.
+			 * Do not allow that, map invalid chars to N > base:
+			 */
+			if ((int)d >= 0)
+				d += 10;
+		}
+#endif
+		if (d >= base) {
+			if (WANT_HEX_ESCAPES && base == 16) {
+				--num_digits;
+				if (num_digits == 0) {
+					/* \x<bad_char>: return '\',
+					 * leave ptr pointing to x */
+					return '\\';
+				}
+			}
+			break;
+		}
+
+		r = n * base + d;
+		if (r > UCHAR_MAX) {
+			break;
+		}
+
+		n = r;
+		++q;
+	} while (++num_digits < 3);
+
+	if (num_digits == 0) {
+		/* Not octal or hex escape sequence.
+		 * Is it one-letter one? */
+
+		/* bash builtin "echo -e '\ec'" interprets \e as ESC,
+		 * but coreutils "/bin/echo -e '\ec'" does not.
+		 * Manpages tend to support coreutils way.
+		 * Update: coreutils added support for \e on 28 Oct 2009. */
+		static const char charmap[] ALIGN1 = {
+			'a',  'b', 'e', 'f',  'n',  'r',  't',  'v',  '\\', '\0',
+			'\a', '\b', 27, '\f', '\n', '\r', '\t', '\v', '\\', '\\',
+		};
+		const char *p = charmap;
+		do {
+			if (*p == *q) {
+				q++;
+				break;
+			}
+		} while (*++p != '\0');
+		/* p points to found escape char or NUL,
+		 * advance it and find what it translates to.
+		 * Note that \NUL and unrecognized sequence \z return '\'
+		 * and leave ptr pointing to NUL or z. */
+		n = p[sizeof(charmap) / 2];
+	}
+
+	*ptr = q;
+
+	return (char) n;
+}
+
+char* FAST_FUNC skip_whitespace(const char *s)
+{
+	/* In POSIX/C locale (the only locale we care about: do we REALLY want
+	 * to allow Unicode whitespace in, say, .conf files? nuts!)
+	 * isspace is only these chars: "\t\n\v\f\r" and space.
+	 * "\t\n\v\f\r" happen to have ASCII codes 9,10,11,12,13.
+	 * Use that.
+	 */
+	while (*s == ' ' || (unsigned char)(*s - 9) <= (13 - 9))
+		s++;
+
+	return (char *) s;
+}
+
+/* Like strcpy but can copy overlapping strings. */
+void FAST_FUNC overlapping_strcpy(char *dst, const char *src)
+{
+	/* Cheap optimization for dst == src case -
+	 * better to have it here than in many callers.
+	 */
+	if (dst != src) {
+		while ((*dst = *src) != '\0') {
+			dst++;
+			src++;
+		}
+	}
+}
+
 static int multiconvert(const char *arg, void *result, converter convert)
 {
 	if (*arg == '"' || *arg == '\'') {
-- 
2.25.1

[PATCH v3 4/6] cmd: setexpr: add format string handling

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Add format string handling operator to the setexpr command.
It allows to use C or Bash like format string expressions to be
evaluated with the result being stored inside the environment variable
name.

  setexpr <name> fmt <format> [value]...

The following example

  setexpr foo fmt "%d, 0x%x" 0x100 ff

will result in $foo being set to "256, 0xff".

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

(no changes since v1)

 MAINTAINERS   |   5 +
 cmd/Kconfig   |   8 +
 cmd/Makefile  |   1 +
 cmd/printf.c  | 419 +++++++++++++++++++++++++++++---------------------
 cmd/printf.h  |   8 +
 cmd/setexpr.c |  37 ++++-
 6 files changed, 300 insertions(+), 178 deletions(-)
 create mode 100644 cmd/printf.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a6b49b54b9..fe53698878 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1049,6 +1049,11 @@ F:	arch/sandbox/
 F:	doc/arch/sandbox.rst
 F:	include/dt-bindings/*/sandbox*.h
 
+SETEXPR
+M:	Roland Gaudig <roland.gaudig@weidmueller.com>
+S:	Maintained
+F:	cmd/printf.c
+
 SH
 M:	Marek Vasut <marek.vasut+renesas@gmail.com>
 M:	Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
diff --git a/cmd/Kconfig b/cmd/Kconfig
index e40d390f88..f1bcf9ebde 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1414,6 +1414,14 @@ config CMD_SETEXPR
 	  Also supports loading the value at a memory location into a variable.
 	  If CONFIG_REGEX is enabled, setexpr also supports a gsub function.
 
+config CMD_SETEXPR_FMT
+	bool "setexpr_fmt"
+	default n
+	depends on CMD_SETEXPR
+	help
+	  Evaluate format string expression and store result in an environment
+	    variable.
+
 endmenu
 
 menu "Android support commands"
diff --git a/cmd/Makefile b/cmd/Makefile
index 9d10e07f0e..ed3669411e 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -141,6 +141,7 @@ obj-$(CONFIG_CMD_SF) += sf.o
 obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
 obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
 obj-$(CONFIG_CMD_SETEXPR) += setexpr.o
+obj-$(CONFIG_CMD_SETEXPR_FMT) += printf.o
 obj-$(CONFIG_CMD_SPI) += spi.o
 obj-$(CONFIG_CMD_STRINGS) += strings.o
 obj-$(CONFIG_CMD_SMC) += smccc.o
diff --git a/cmd/printf.c b/cmd/printf.c
index 337ab8ce5d..e024676743 100644
--- a/cmd/printf.c
+++ b/cmd/printf.c
@@ -1,12 +1,21 @@
-/* vi: set sw=4 ts=4: */
+// SPDX-License-Identifier: GPL-2.0+
 /*
- * printf - format and print data
+ * Copyright (C) 2021 Weidmüller Interface GmbH & Co. KG
+ * Roland Gaudig <roland.gaudig@weidmueller.com>
  *
  * Copyright 1999 Dave Cinege
  * Portions copyright (C) 1990-1996 Free Software Foundation, Inc.
  *
  * Licensed under GPLv2 or later, see file LICENSE in this source tree.
  */
+/*
+ * This file provides a shell printf like format string expansion as required
+ * for the setexpr <name> fmt <format> <value> command.
+ * This source file was mostly taken from the BusyBox project (www.busybox.net)
+ * In contrast to the original sources the output is not written to stdout
+ * anymore but into a char array, which can be used as input for the env_set()
+ * function.
+ */
 /* Usage: printf format [argument...]
  *
  * A front end to the printf function that lets it be used from the shell.
@@ -59,8 +68,6 @@
 //usage:       "$ printf \"Val=%d\\n\" 5\n"
 //usage:       "Val=5\n"
 
-#include "libbb.h"
-
 /* A note on bad input: neither bash 3.2 nor coreutils 6.10 stop on it.
  * They report it:
  *  bash: printf: XXX: invalid number
@@ -77,22 +84,97 @@
  * We try to be compatible.
  */
 
-typedef void FAST_FUNC (*converter)(const char *arg, void *result);
+#include <common.h>
+#include <ctype.h>
+#include <errno.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
 
 #define WANT_HEX_ESCAPES 0
+#define PRINT_CONVERSION_ERROR 1
+#define PRINT_TRUNCATED_ERROR 2
+#define PRINT_SIZE_ERROR 4
+
+struct print_inf {
+	char *str;
+	size_t size;
+	size_t offset;
+	unsigned int error;
+};
+
+typedef void (*converter)(const char *arg, void *result);
+
+/**
+ * printf_str() - print formatted into char array with length checks
+ *
+ * This function povides a printf like function for printing into a char array
+ * with checking the boundaries.
+ * Unlike snprintf, all checks are performed inside this function and status
+ * reports are stored inside the print_inf struct. That way, this function can
+ * be used almost as drop-in replacement without needing much code changes.
+ * Unlike snprintf errors are not reported by return value, but inside the
+ * error member of struct print_inf. The output stored inside the struct
+ * print_inf str member shall only be used when the error member is 0.
+ *
+ * @inf: Info structure for print operation
+ * @char: format string with optional arguments
+ */
+static void printf_str(struct print_inf *inf, char *format, ...)
+{
+	va_list args;
+	int i;
 
-/* Usual "this only works for ascii compatible encodings" disclaimer. */
-#undef _tolower
-#define _tolower(X) ((X)|((char) 0x20))
+	if (!inf)
+		return;
+
+	/* Do not write anything if previous error is pending */
+	if (inf->error)
+		return;
+
+	/* Check if end of receiving buffer is already reached */
+	if (inf->offset >= inf->size) {
+		inf->error |= PRINT_SIZE_ERROR;
+		return;
+	}
+
+	size_t remaining = inf->size - inf->offset;
+
+	va_start(args, format);
+	i = vsnprintf(inf->str + inf->offset, remaining, format, args);
+	va_end(args);
+
+	if (i >= remaining)
+		inf->error |= PRINT_TRUNCATED_ERROR;
+	else if (i < 0)
+		inf->error |= PRINT_CONVERSION_ERROR;
+	else
+		inf->offset += i;
+}
+
+/**
+ * putchar_str() - Print single character into char array with length checks
+ *
+ * This function provices a putchar like function, which stores the output
+ * into a char array with checking boundaries.
+ *
+ * @inf: Info structure for print operation
+ * @char: Single character to be printed
+ */
+static void putchar_str(struct print_inf *inf, char c)
+{
+	printf_str(inf, "%c", c);
+}
 
-char FAST_FUNC bb_process_escape_sequence(const char **ptr)
+static char process_escape_sequence(const char **ptr)
 {
 	const char *q;
-	unsigned num_digits;
-	unsigned n;
-	unsigned base;
+	unsigned int num_digits;
+	unsigned int n;
+	unsigned int base;
 
-	num_digits = n = 0;
+	num_digits = 0;
+	n = 0;
 	base = 8;
 	q = *ptr;
 
@@ -104,13 +186,14 @@ char FAST_FUNC bb_process_escape_sequence(const char **ptr)
 
 	/* bash requires leading 0 in octal escapes:
 	 * \02 works, \2 does not (prints \ and 2).
-	 * We treat \2 as a valid octal escape sequence. */
+	 * We treat \2 as a valid octal escape sequence.
+	 */
 	do {
-		unsigned r;
-		unsigned d = (unsigned char)(*q) - '0';
+		unsigned int r;
+		unsigned int d = (unsigned char)(*q) - '0';
 #if WANT_HEX_ESCAPES
 		if (d >= 10) {
-			d = (unsigned char)_tolower(*q) - 'a';
+			d = (unsigned char)tolower(*q) - 'a';
 			//d += 10;
 			/* The above would map 'A'-'F' and 'a'-'f' to 10-15,
 			 * however, some chars like '@' would map to 9 < base.
@@ -125,7 +208,8 @@ char FAST_FUNC bb_process_escape_sequence(const char **ptr)
 				--num_digits;
 				if (num_digits == 0) {
 					/* \x<bad_char>: return '\',
-					 * leave ptr pointing to x */
+					 * leave ptr pointing to x
+					 */
 					return '\\';
 				}
 			}
@@ -133,9 +217,8 @@ char FAST_FUNC bb_process_escape_sequence(const char **ptr)
 		}
 
 		r = n * base + d;
-		if (r > UCHAR_MAX) {
+		if (r > 255)
 			break;
-		}
 
 		n = r;
 		++q;
@@ -143,17 +226,20 @@ char FAST_FUNC bb_process_escape_sequence(const char **ptr)
 
 	if (num_digits == 0) {
 		/* Not octal or hex escape sequence.
-		 * Is it one-letter one? */
-
+		 * Is it one-letter one?
+		 */
 		/* bash builtin "echo -e '\ec'" interprets \e as ESC,
 		 * but coreutils "/bin/echo -e '\ec'" does not.
 		 * Manpages tend to support coreutils way.
-		 * Update: coreutils added support for \e on 28 Oct 2009. */
-		static const char charmap[] ALIGN1 = {
+		 * Update: coreutils added support for \e on 28 Oct 2009.
+		 */
+		static const char charmap[] = {
 			'a',  'b', 'e', 'f',  'n',  'r',  't',  'v',  '\\', '\0',
 			'\a', '\b', 27, '\f', '\n', '\r', '\t', '\v', '\\', '\\',
 		};
+
 		const char *p = charmap;
+
 		do {
 			if (*p == *q) {
 				q++;
@@ -163,16 +249,17 @@ char FAST_FUNC bb_process_escape_sequence(const char **ptr)
 		/* p points to found escape char or NUL,
 		 * advance it and find what it translates to.
 		 * Note that \NUL and unrecognized sequence \z return '\'
-		 * and leave ptr pointing to NUL or z. */
+		 * and leave ptr pointing to NUL or z.
+		 */
 		n = p[sizeof(charmap) / 2];
 	}
 
 	*ptr = q;
 
-	return (char) n;
+	return (char)n;
 }
 
-char* FAST_FUNC skip_whitespace(const char *s)
+static char *skip_whitespace(const char *s)
 {
 	/* In POSIX/C locale (the only locale we care about: do we REALLY want
 	 * to allow Unicode whitespace in, say, .conf files? nuts!)
@@ -183,11 +270,11 @@ char* FAST_FUNC skip_whitespace(const char *s)
 	while (*s == ' ' || (unsigned char)(*s - 9) <= (13 - 9))
 		s++;
 
-	return (char *) s;
+	return (char *)s;
 }
 
 /* Like strcpy but can copy overlapping strings. */
-void FAST_FUNC overlapping_strcpy(char *dst, const char *src)
+static void overlapping_strcpy(char *dst, const char *src)
 {
 	/* Cheap optimization for dst == src case -
 	 * better to have it here than in many callers.
@@ -202,80 +289,72 @@ void FAST_FUNC overlapping_strcpy(char *dst, const char *src)
 
 static int multiconvert(const char *arg, void *result, converter convert)
 {
-	if (*arg == '"' || *arg == '\'') {
-		arg = utoa((unsigned char)arg[1]);
-	}
-	errno = 0;
+	if (*arg == '"' || *arg == '\'')
+		sprintf((char *)arg + strlen(arg), "%u", (unsigned char)arg[1]);
+	//errno = 0;
 	convert(arg, result);
-	if (errno) {
-		bb_error_msg("invalid number '%s'", arg);
-		return 1;
-	}
+	/* Unlike their Posix counterparts, simple_strtoll and
+	 * simple_strtoull do not set errno
+	 *
+	 * if (errno) {
+	 *	printf("error invalid number '%s'", arg);
+	 *	return 1;
+	 * }
+	 */
 	return 0;
 }
 
-static void FAST_FUNC conv_strtoull(const char *arg, void *result)
+static void conv_strtoull(const char *arg, void *result)
 {
-	/* Allow leading '+' - bb_strtoull() by itself does not allow it,
-	 * and probably shouldn't (other callers might require purely numeric
-	 * inputs to be allowed.
-	 */
-	if (arg[0] == '+')
-		arg++;
-	*(unsigned long long*)result = bb_strtoull(arg, NULL, 0);
 	/* both coreutils 6.10 and bash 3.2:
 	 * $ printf '%x\n' -2
 	 * fffffffffffffffe
 	 * Mimic that:
 	 */
-	if (errno) {
-		*(unsigned long long*)result = bb_strtoll(arg, NULL, 0);
+	if (arg[0] == '-') {
+		*(unsigned long long *)result = simple_strtoll(arg, NULL, 16);
+		return;
 	}
-}
-static void FAST_FUNC conv_strtoll(const char *arg, void *result)
-{
+	/* Allow leading '+' - simple_strtoull() by itself does not allow it,
+	 * and probably shouldn't (other callers might require purely numeric
+	 * inputs to be allowed.
+	 */
 	if (arg[0] == '+')
 		arg++;
-	*(long long*)result = bb_strtoll(arg, NULL, 0);
+	*(unsigned long long *)result = simple_strtoull(arg, NULL, 16);
 }
-static void FAST_FUNC conv_strtod(const char *arg, void *result)
+
+static void conv_strtoll(const char *arg, void *result)
 {
-	char *end;
-	/* Well, this one allows leading whitespace... so what? */
-	/* What I like much less is that "-" accepted too! :( */
-	*(double*)result = strtod(arg, &end);
-	if (end[0]) {
-		errno = ERANGE;
-		*(double*)result = 0;
-	}
+	if (arg[0] == '+')
+		arg++;
+	*(long long *)result = simple_strtoll(arg, NULL, 16);
 }
 
 /* Callers should check errno to detect errors */
 static unsigned long long my_xstrtoull(const char *arg)
 {
 	unsigned long long result;
+
 	if (multiconvert(arg, &result, conv_strtoull))
 		result = 0;
 	return result;
 }
+
 static long long my_xstrtoll(const char *arg)
 {
 	long long result;
+
 	if (multiconvert(arg, &result, conv_strtoll))
 		result = 0;
 	return result;
 }
-static double my_xstrtod(const char *arg)
-{
-	double result;
-	multiconvert(arg, &result, conv_strtod);
-	return result;
-}
 
 /* Handles %b; return 1 if output is to be short-circuited by \c */
-static int print_esc_string(const char *str)
+static int print_esc_string(struct print_inf *inf, const char *str)
 {
 	char c;
+
 	while ((c = *str) != '\0') {
 		str++;
 		if (c == '\\') {
@@ -285,30 +364,30 @@ static int print_esc_string(const char *str)
 					/* 2nd char is 0..7: skip leading '0' */
 					str++;
 				}
-			}
-			else if (*str == 'c') {
+			} else if (*str == 'c') {
 				return 1;
 			}
 			{
 				/* optimization: don't force arg to be on-stack,
-				 * use another variable for that. */
+				 * use another variable for that.
+				 */
 				const char *z = str;
-				c = bb_process_escape_sequence(&z);
+
+				c = process_escape_sequence(&z);
 				str = z;
 			}
 		}
-		putchar(c);
+		putchar_str(inf, c);
 	}
 
 	return 0;
 }
 
-static void print_direc(char *format, unsigned fmt_length,
-		int field_width, int precision,
-		const char *argument)
+static void print_direc(struct print_inf *inf, char *format, unsigned int fmt_length,
+			int field_width, int precision,
+			const char *argument)
 {
 	long long llv;
-	double dv;
 	char saved;
 	char *have_prec, *have_width;
 
@@ -325,7 +404,7 @@ static void print_direc(char *format, unsigned fmt_length,
 
 	switch (format[fmt_length - 1]) {
 	case 'c':
-		printf(format, *argument);
+		printf_str(inf, format, *argument);
 		break;
 	case 'd':
 	case 'i':
@@ -333,14 +412,14 @@ static void print_direc(char *format, unsigned fmt_length,
  print_long:
 		if (!have_width) {
 			if (!have_prec)
-				printf(format, llv);
+				printf_str(inf, format, llv);
 			else
-				printf(format, precision, llv);
+				printf_str(inf, format, precision, llv);
 		} else {
 			if (!have_prec)
-				printf(format, field_width, llv);
+				printf_str(inf, format, field_width, llv);
 			else
-				printf(format, field_width, precision, llv);
+				printf_str(inf, format, field_width, precision, llv);
 		}
 		break;
 	case 'o':
@@ -357,37 +436,25 @@ static void print_direc(char *format, unsigned fmt_length,
 			goto print_long;
 		} else {
 			/* Hope compiler will optimize it out by moving call
-			 * instruction after the ifs... */
+			 * instruction after the ifs...
+			 */
 			if (!have_width) {
 				if (!have_prec)
-					printf(format, argument, /*unused:*/ argument, argument);
+					printf_str(inf, format, argument,
+						   /*unused:*/ argument, argument);
 				else
-					printf(format, precision, argument, /*unused:*/ argument);
+					printf_str(inf, format, precision,
+						   argument, /*unused:*/ argument);
 			} else {
 				if (!have_prec)
-					printf(format, field_width, argument, /*unused:*/ argument);
+					printf_str(inf, format, field_width,
+						   argument, /*unused:*/ argument);
 				else
-					printf(format, field_width, precision, argument);
+					printf_str(inf, format, field_width,
+						   precision, argument);
 			}
 			break;
 		}
-	case 'f':
-	case 'e':
-	case 'E':
-	case 'g':
-	case 'G':
-		dv = my_xstrtod(argument);
-		if (!have_width) {
-			if (!have_prec)
-				printf(format, dv);
-			else
-				printf(format, precision, dv);
-		} else {
-			if (!have_prec)
-				printf(format, field_width, dv);
-			else
-				printf(format, field_width, precision, dv);
-		}
 		break;
 	} /* switch */
 
@@ -397,22 +464,27 @@ static void print_direc(char *format, unsigned fmt_length,
 /* Handle params for "%*.*f". Negative numbers are ok (compat). */
 static int get_width_prec(const char *str)
 {
-	int v = bb_strtoi(str, NULL, 10);
-	if (errno) {
-		bb_error_msg("invalid number '%s'", str);
-		v = 0;
-	}
-	return v;
+	long v = simple_strtol(str, NULL, 10);
+
+	/* Unlike its Posix counterpart, simple_strtol does not set errno
+	 *
+	 * if (errno) {
+	 *	printf("error invalid number '%s'", str);
+	 *	v = 0;
+	 * }
+	 */
+	return (int)v;
 }
 
 /* Print the text in FORMAT, using ARGV for arguments to any '%' directives.
-   Return advanced ARGV.  */
-static char **print_formatted(char *f, char **argv, int *conv_err)
+ * Return advanced ARGV.
+ */
+static char **print_formatted(struct print_inf *inf, char *f, char **argv, int *conv_err)
 {
-	char *direc_start;      /* Start of % directive.  */
-	unsigned direc_length;  /* Length of % directive.  */
-	int field_width;        /* Arg to first '*' */
-	int precision;          /* Arg to second '*' */
+	char *direc_start;          /* Start of % directive.  */
+	unsigned int direc_length;  /* Length of % directive.  */
+	int field_width;            /* Arg to first '*' */
+	int precision;              /* Arg to second '*' */
 	char **saved_argv = argv;
 
 	for (; *f; ++f) {
@@ -420,14 +492,15 @@ static char **print_formatted(char *f, char **argv, int *conv_err)
 		case '%':
 			direc_start = f++;
 			direc_length = 1;
-			field_width = precision = 0;
+			field_width = 0;
+			precision = 0;
 			if (*f == '%') {
-				bb_putchar('%');
+				putchar_str(inf, '%');
 				break;
 			}
 			if (*f == 'b') {
 				if (*argv) {
-					if (print_esc_string(*argv))
+					if (print_esc_string(inf, *argv))
 						return saved_argv; /* causes main() to exit */
 					++argv;
 				}
@@ -467,24 +540,28 @@ static char **print_formatted(char *f, char **argv, int *conv_err)
 			/* Remove "lLhz" size modifiers, repeatedly.
 			 * bash does not like "%lld", but coreutils
 			 * happily takes even "%Llllhhzhhzd"!
-			 * We are permissive like coreutils */
-			while ((*f | 0x20) == 'l' || *f == 'h' || *f == 'z') {
+			 * We are permissive like coreutils
+			 */
+			while ((*f | 0x20) == 'l' || *f == 'h' || *f == 'z')
 				overlapping_strcpy(f, f + 1);
-			}
 			/* Add "ll" if integer modifier, then print */
 			{
-				static const char format_chars[] ALIGN1 = "diouxXfeEgGcs";
+				static const char format_chars[] = "diouxXcs";
 				char *p = strchr(format_chars, *f);
 				/* needed - try "printf %" without it */
-				if (p == NULL || *f == '\0') {
-					bb_error_msg("%s: invalid format", direc_start);
+				if (!p || *f == '\0') {
+					printf("`%s': invalid format\n", direc_start);
 					/* causes main() to exit with error */
 					return saved_argv - 1;
 				}
 				++direc_length;
 				if (p - format_chars <= 5) {
 					/* it is one of "diouxX" */
-					p = xmalloc(direc_length + 3);
+					p = malloc(direc_length + 3);
+					if (!p) {
+						/* exit with error */
+						return saved_argv - 1;
+					}
 					memcpy(p, direc_start, direc_length);
 					p[direc_length + 1] = p[direc_length - 1];
 					p[direc_length - 1] = 'l';
@@ -496,81 +573,75 @@ static char **print_formatted(char *f, char **argv, int *conv_err)
 					p = NULL;
 				}
 				if (*argv) {
-					print_direc(direc_start, direc_length, field_width,
-								precision, *argv++);
+					print_direc(inf, direc_start, direc_length,
+						    field_width, precision, *argv++);
 				} else {
-					print_direc(direc_start, direc_length, field_width,
-								precision, "");
+					print_direc(inf, direc_start, direc_length,
+						    field_width, precision, "");
 				}
 				*conv_err |= errno;
 				free(p);
 			}
 			break;
 		case '\\':
-			if (*++f == 'c') {
+			if (*++f == 'c')
 				return saved_argv; /* causes main() to exit */
-			}
-			bb_putchar(bb_process_escape_sequence((const char **)&f));
+			putchar_str(inf, process_escape_sequence((const char **)&f));
 			f--;
 			break;
 		default:
-			putchar(*f);
+			putchar_str(inf, *f);
 		}
 	}
 
 	return argv;
 }
 
-int printf_main(int argc UNUSED_PARAM, char **argv)
+/**
+ * printf_setexpr() - Implements the setexpr <name> fmt <format> command
+ *
+ * This function implements the format string evaluation for the
+ * setexpr <name> fmt <format> <value> command.
+ *
+ * @str: Output string of the evaluated expression
+ * @size: Length of @str buffer
+ * @argc: Number of arguments
+ * @argv: Argument list
+ * @return: 0 if OK, 1 on error
+ */
+int printf_setexpr(char *str, size_t size, int argc, char *const *argv)
 {
 	int conv_err;
 	char *format;
 	char **argv2;
+	struct print_inf inf = {
+		.str = str,
+		.size = size,
+		.offset = 0,
+		.error = 0,
+	};
+
+	if (!str || !size)
+		return 1;
 
-	/* We must check that stdout is not closed.
-	 * The reason for this is highly non-obvious.
-	 * printf_main is used from shell.
-	 * Shell must correctly handle 'printf "%s" foo'
-	 * if stdout is closed. With stdio, output gets shoveled into
-	 * stdout buffer, and even fflush cannot clear it out. It seems that
-	 * even if libc receives EBADF on write attempts, it feels determined
-	 * to output data no matter what. So it will try later,
-	 * and possibly will clobber future output. Not good. */
-// TODO: check fcntl() & O_ACCMODE == O_WRONLY or O_RDWR?
-	if (fcntl(1, F_GETFL) == -1)
-		return 1; /* match coreutils 6.10 (sans error msg to stderr) */
-	//if (dup2(1, 1) != 1) - old way
-	//	return 1;
-
-	/* bash builtin errors out on "printf '-%s-\n' foo",
-	 * coreutils-6.9 works. Both work with "printf -- '-%s-\n' foo".
-	 * We will mimic coreutils. */
-	if (argv[1] && argv[1][0] == '-' && argv[1][1] == '-' && !argv[1][2])
-		argv++;
-	if (!argv[1]) {
-		if (ENABLE_ASH_PRINTF
-		 && applet_name[0] != 'p'
-		) {
-			bb_simple_error_msg("usage: printf FORMAT [ARGUMENT...]");
-			return 2; /* bash compat */
-		}
-		bb_show_usage();
-	}
+	inf.str[0] = '\0';
 
-	format = argv[1];
-	argv2 = argv + 2;
+	format = argv[0];
+	argv2 = (char **)argv + 1;
 
 	conv_err = 0;
-	do {
-		argv = argv2;
-		argv2 = print_formatted(format, argv, &conv_err);
-	} while (argv2 > argv && *argv2);
+	argv = argv2;
+	/* In case any print_str call raises an error inf.error will be
+	 * set after print_formatted returns.
+	 */
+	argv2 = print_formatted(&inf, format, (char **)argv, &conv_err);
 
 	/* coreutils compat (bash doesn't do this):
-	if (*argv)
-		fprintf(stderr, "excess args ignored");
-	*/
+	 *if (*argv)
+	 *	fprintf(stderr, "excess args ignored");
+	 */
 
-	return (argv2 < argv) /* if true, print_formatted errored out */
-		|| conv_err; /* print_formatted saw invalid number */
+	return (argv2 < argv) || /* if true, print_formatted errored out */
+		conv_err || /* print_formatted saw invalid number */
+		inf.error;  /* print_str reported error */
 }
diff --git a/cmd/printf.h b/cmd/printf.h
new file mode 100644
index 0000000000..dcaff6d097
--- /dev/null
+++ b/cmd/printf.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef __PRINTF_H
+#define __PRINTF_H
+
+int printf_setexpr(char *str, size_t size, int argc, char *const *argv);
+
+#endif
diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index e828be3970..1eb67e2eff 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -11,11 +11,15 @@
 #include <common.h>
 #include <config.h>
 #include <command.h>
+#include <ctype.h>
 #include <env.h>
 #include <log.h>
 #include <malloc.h>
 #include <mapmem.h>
 #include <linux/sizes.h>
+#include "printf.h"
+
+#define MAX_STR_LEN 128
 
 /**
  * struct expr_arg: Holds an argument to an expression
@@ -370,15 +374,16 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 	int w;
 
 	/*
-	 * We take 3, 5, or 6 arguments:
+	 * We take 3, 5, or 6 arguments, except fmt operation, which
+	 * takes 4 to 8 arguments (limited by _maxargs):
 	 * 3 : setexpr name value
 	 * 5 : setexpr name val1 op val2
 	 *     setexpr name [g]sub r s
 	 * 6 : setexpr name [g]sub r s t
+	 *     setexpr name fmt format [val1] [val2] [val3] [val4]
 	 */
 
-	/* > 6 already tested by max command args */
-	if ((argc < 3) || (argc == 4))
+	if (argc < 3)
 		return CMD_RET_USAGE;
 
 	w = cmd_get_data_size(argv[0], 4);
@@ -386,6 +391,24 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (get_arg(argv[2], w, &aval))
 		return CMD_RET_FAILURE;
 
+	/* format string assignment: "setexpr name fmt %d value" */
+	if (strcmp(argv[2], "fmt") == 0 && IS_ENABLED(CONFIG_CMD_SETEXPR_FMT)) {
+		char str[MAX_STR_LEN];
+		int result;
+
+		if (argc == 3)
+			return CMD_RET_USAGE;
+
+		result = printf_setexpr(str, sizeof(str), argc - 3, &argv[3]);
+		if (result)
+			return result;
+
+		return env_set(argv[1], str);
+	}
+
+	if (argc == 4 || argc > 6)
+		return CMD_RET_USAGE;
+
 	/* plain assignment: "setexpr name value" */
 	if (argc == 3) {
 		if (w == CMD_DATA_SIZE_STR) {
@@ -495,7 +518,7 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 }
 
 U_BOOT_CMD(
-	setexpr, 6, 0, do_setexpr,
+	setexpr, 8, 0, do_setexpr,
 	"set environment variable as the result of eval expression",
 	"[.b, .w, .l, .s] name [*]value1 <op> [*]value2\n"
 	"    - set environment variable 'name' to the result of the evaluated\n"
@@ -505,6 +528,12 @@ U_BOOT_CMD(
 	"      memory addresses (*)\n"
 	"setexpr[.b, .w, .l] name [*]value\n"
 	"    - load a value into a variable"
+#ifdef CONFIG_CMD_SETEXPR_FMT
+	"\n"
+	"setexpr name fmt <format> [value1] [value2] [value3] [value4]\n"
+	"    - set environment variable 'name' to the result of the bash like\n"
+	"      format string evaluation of value."
+#endif
 #ifdef CONFIG_REGEX
 	"\n"
 	"setexpr name gsub r s [t]\n"
-- 
2.25.1

[PATCH v3 5/6] doc: usage: add description for setexpr command

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Add usage for the setexpr command. It has been added to describe
mainly the new setexpr format string operation.

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

(no changes since v1)

 MAINTAINERS           |   1 +
 doc/usage/index.rst   |   1 +
 doc/usage/setexpr.rst | 148 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 150 insertions(+)
 create mode 100644 doc/usage/setexpr.rst

diff --git a/MAINTAINERS b/MAINTAINERS
index fe53698878..57c3349ef5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1053,6 +1053,7 @@ SETEXPR
 M:	Roland Gaudig <roland.gaudig@weidmueller.com>
 S:	Maintained
 F:	cmd/printf.c
+F:	doc/usage/setexpr.rst
 
 SH
 M:	Marek Vasut <marek.vasut+renesas@gmail.com>
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 40b796a3a9..719b2c90b9 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -43,6 +43,7 @@ Shell commands
    reset
    sbi
    scp03
+   setexpr
    size
    true
    ums
diff --git a/doc/usage/setexpr.rst b/doc/usage/setexpr.rst
new file mode 100644
index 0000000000..2e511b1290
--- /dev/null
+++ b/doc/usage/setexpr.rst
@@ -0,0 +1,148 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+setexpr command
+===============
+
+Synopsis
+--------
+
+::
+
+    setexpr[.b, .w, .l .s] <name> [*]<value> <op> [*]<value2>
+    setexpr[.b, .w, .l] <name> [*]<value>
+    setexpr <name> fmt <format> [value]...
+    setexpr <name> gsub r s [t]
+    setexpr <name> sub r s [t]
+
+Description
+-----------
+
+The setexpr command is used to set an environment variable to the result
+of an evaluation.
+
+setexpr[.b, .w, .l .s] <name> [*]<value> <op> [*]<value2>
+     Set environment variable <name> to the result of the evaluated
+     expression specified by <op>.
+
+setexpr[.b, .w, .l] name [*]value
+     Load <value> into environment variable <name>
+
+setexpr name fmt <format> value
+     Set environment variable <name> to the result of the C like
+     format string <format> evaluation of <value>.
+
+setexpr name gsub <r> <s> [<t>]
+     For each substring matching the regular expression <r> in the
+     string <t>, substitute the string <s>.
+     The result is assigned to <name>.
+     If <t> is not supplied, use the old value of <name>.
+
+setexpr name sub <r> <s> [<t>]
+     Just like gsub(), but replace only the first matching substring
+
+The setexpr command takes the following arguments:
+
+format
+    This parameter contains a C or Bash like format string.
+    The number of arguments is limited to 4.
+    The following format types are supported:
+
+    c
+        single character
+    d, i
+        decimal value
+    o
+        octal value
+    s
+        string
+    u
+        unsigned decimal value
+    x, X
+        hexadecimal value
+    '%'
+        no conversion, instead a % character will be written
+
+    Backslash escapes:
+
+    \" = double quote
+    \\ = backslash
+    \a = alert (bell)
+    \b = backspace
+    \c = produce no further output
+    \f = form feed
+    \n = new line
+    \r = carriage return
+    \t = horizontal tab
+    \v = vertical tab
+    \NNN = octal number (NNN is 0 to 3 digits)
+
+name
+    The name of the environment variable to be set
+
+op
+    '|'
+        name = value | value2
+    '&'
+        name = value & value2
+    '+'
+        name = value + value2
+        (This is the only operator supported for strings.
+	It acts as concatenation operator on strings)
+    '^'
+        name = value ^ value2
+    '-'
+        name = value - value2
+    '*'
+        name = value * value2
+    '/'
+        name = value / value2
+    '%'
+        name = value % value2
+
+r
+    Regular expression
+
+s
+    Substitution string
+
+t
+    string
+
+value
+    Can either be an integer value, a string.
+    If the pointer prefix '*' is given value is treated as memory address.
+
+value2
+    See value
+
+Example
+-------
+
+::
+
+    => setexpr foo fmt %d 0x100
+    => echo $foo
+    256
+    =>
+
+    => setexpr foo fmt 0x%08x 63
+    => echo $foo
+    0x00000063
+    =>
+
+    => setexpr foo fmt %%%o 8
+    => echo $foo
+    %10
+    =>
+
+Configuration
+-------------
+
+The setexpr gsub and sub operations are only available if CONFIG_REGEX=y.
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the operation was successful.
+
+If an error occurs, the return value $? is set to 1 (false).
-- 
2.25.1

[PATCH v3 6/6] test: cmd: setexpr: add format string tests

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Add test cases for the setexpr format string operator.

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

(no changes since v1)

 test/cmd/setexpr.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index 08b6e6e724..0dc94f7e61 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -386,6 +386,90 @@ static int setexpr_test_str_long(struct unit_test_state *uts)
 }
 SETEXPR_TEST(setexpr_test_str_long, UT_TESTF_CONSOLE_REC);
 
+#ifdef CONFIG_CMD_SETEXPR_FMT
+/* Test 'setexpr' command with simply setting integers */
+static int setexpr_test_fmt(struct unit_test_state *uts)
+{
+	u8 *buf;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	memset(buf, '\xff', BUF_SIZE);
+
+	/* Test decimal conversion */
+	ut_assertok(run_command("setexpr fred fmt %d 0xff", 0));
+	ut_asserteq_str("255", env_get("fred"));
+	/* Test hexadecimal conversion with 0x prefix and 4 digits */
+	ut_assertok(run_command("setexpr fred fmt 0x%04x 257", 0));
+	ut_asserteq_str("0x0257", env_get("fred"));
+	/* Test octal conversion with % prefix */
+	ut_assertok(run_command("setexpr fred fmt %%%o 8", 0));
+	ut_asserteq_str("%10", env_get("fred"));
+	/* Test argument surrounded by %% */
+	ut_assertok(run_command("setexpr fred fmt %%%x%% 0xff", 0));
+	ut_asserteq_str("%ff%", env_get("fred"));
+	/* Test escape sequence */
+	ut_assertok(run_command("setexpr fred fmt \"hello\\040world\"", 0));
+	ut_asserteq_str("hello world", env_get("fred"));
+	/* Test %b with string containing octal escape sequence */
+	ut_assertok(run_command("setexpr fred fmt oh%bno \137", 0));
+	ut_asserteq_str("oh_no", env_get("fred"));
+	/* Test %b with string containing \c escape sequence */
+	ut_assertok(run_command("setexpr fred fmt hello%bworld \"\\c\"", 0));
+	ut_asserteq_str("hello", env_get("fred"));
+	/* Test multiple arguments referencing environment varialbes */
+	ut_assertok(run_command("setenv a eff", 0));
+	ut_assertok(run_command("setenv b hello", 0));
+	ut_assertok(run_command("setenv c 0x63", 0));
+	ut_assertok(run_command("setenv d world", 0));
+	ut_assertok(run_command("setexpr fred fmt \"0x%08x-%s-%d-%s\" $a $b $c $d", 0));
+	ut_asserteq_str("0x00000eff-hello-99-world", env_get("fred"));
+	/* Test with two format specifiers, but only one argument */
+	ut_assertok(run_command("setexpr fred fmt %d_%x 100", 0));
+	ut_asserteq_str("256_0", env_get("fred"));
+	/* Test maximum string length */
+	ut_assertok(run_command("setexpr fred fmt \"%0127d\" 7b", 0));
+	ut_asserteq_str("0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000123", env_get("fred"));
+	/* Test maximum unsigned integer size */
+	ut_assertok(run_command("setexpr fred fmt %u ffffffffffffffff", 0));
+	ut_asserteq_str("18446744073709551615", env_get("fred"));
+	/* Test maximum positive integer size */
+	ut_assertok(run_command("setexpr fred fmt %d 7fffffffffffffff", 0));
+	ut_asserteq_str("9223372036854775807", env_get("fred"));
+	/* Test maximum negative integer size */
+	ut_assertok(run_command("setexpr fred fmt %d 8000000000000000", 0));
+	ut_asserteq_str("-9223372036854775808", env_get("fred"));
+	/* Test minimum negative integer size */
+	ut_assertok(run_command("setexpr fred fmt %d ffffffffffffffff", 0));
+	ut_asserteq_str("-1", env_get("fred"));
+	/* Test signed value with + sign */
+	ut_assertok(run_command("setexpr fred fmt %d +5", 0));
+	ut_asserteq_str("5", env_get("fred"));
+	/* Test signed value with - sign */
+	ut_assertok(run_command("setexpr fred fmt %d -4", 0));
+	ut_asserteq_str("-4", env_get("fred"));
+	/* Test unsigned value with + sign */
+	ut_assertok(run_command("setexpr fred fmt %u +3", 0));
+	ut_asserteq_str("3", env_get("fred"));
+	/* Test unsigned value with - sign */
+	ut_assertok(run_command("setexpr fred fmt %x -2", 0));
+	ut_asserteq_str("fffffffffffffffe", env_get("fred"));
+	/* Error test with missing format specifier */
+	ut_asserteq(1, run_command("setexpr fred fmd hello 0xff", 0));
+	/* Error test with invalid format type */
+	ut_asserteq(1, run_command("setexpr fred fmt %a 0xff", 0));
+	/* Error test with incomplete format specifier */
+	ut_asserteq(1, run_command("setexpr fred fmt hello% bf", 0));
+	/* Error exceeding maximum string length */
+	ut_asserteq(1, run_command("setexpr fred fmt \"%0128d\" 456", 0));
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+
+SETEXPR_TEST(setexpr_test_fmt, UT_TESTF_CONSOLE_REC);
+#endif
+
 int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct unit_test *tests = UNIT_TEST_SUITE_START(setexpr_test);
-- 
2.25.1

Re: [PATCH v3 1/6] lib: strto: add simple_strtoll function

[Simon Glass]

On Fri, 23 Jul 2021 at 06:29, Roland Gaudig
<roland.gaudig-oss@weidmueller.com> wrote:
>
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>
> Add simple_strtoll function for converting a string containing digits
> into a long long int value.
>
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
>
> (no changes since v1)
>
>  include/vsprintf.h | 1 +
>  lib/strto.c        | 8 ++++++++
>  2 files changed, 9 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v3 4/6] cmd: setexpr: add format string handling

[Simon Glass]

Hi Roland,

On Fri, 23 Jul 2021 at 06:30, Roland Gaudig
<roland.gaudig-oss@weidmueller.com> wrote:
>
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>
> Add format string handling operator to the setexpr command.
> It allows to use C or Bash like format string expressions to be
> evaluated with the result being stored inside the environment variable
> name.
>
>   setexpr <name> fmt <format> [value]...
>
> The following example
>
>   setexpr foo fmt "%d, 0x%x" 0x100 ff
>
> will result in $foo being set to "256, 0xff".
>
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
>
> (no changes since v1)
>
>  MAINTAINERS   |   5 +
>  cmd/Kconfig   |   8 +
>  cmd/Makefile  |   1 +
>  cmd/printf.c  | 419 +++++++++++++++++++++++++++++---------------------
>  cmd/printf.h  |   8 +
>  cmd/setexpr.c |  37 ++++-
>  6 files changed, 300 insertions(+), 178 deletions(-)
>  create mode 100644 cmd/printf.h

Reviewed-by: Simon Glass <sjg@chromium.org>

nit below

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a6b49b54b9..fe53698878 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1049,6 +1049,11 @@ F:       arch/sandbox/
>  F:     doc/arch/sandbox.rst
>  F:     include/dt-bindings/*/sandbox*.h
>
> +SETEXPR
> +M:     Roland Gaudig <roland.gaudig@weidmueller.com>
> +S:     Maintained
> +F:     cmd/printf.c
> +
>  SH
>  M:     Marek Vasut <marek.vasut+renesas@gmail.com>
>  M:     Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index e40d390f88..f1bcf9ebde 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1414,6 +1414,14 @@ config CMD_SETEXPR
>           Also supports loading the value at a memory location into a variable.
>           If CONFIG_REGEX is enabled, setexpr also supports a gsub function.
>
> +config CMD_SETEXPR_FMT
> +       bool "setexpr_fmt"
> +       default n
> +       depends on CMD_SETEXPR
> +       help
> +         Evaluate format string expression and store result in an environment
> +           variable.
> +
>  endmenu
>
>  menu "Android support commands"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 9d10e07f0e..ed3669411e 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_CMD_SF) += sf.o
>  obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
>  obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
>  obj-$(CONFIG_CMD_SETEXPR) += setexpr.o
> +obj-$(CONFIG_CMD_SETEXPR_FMT) += printf.o
>  obj-$(CONFIG_CMD_SPI) += spi.o
>  obj-$(CONFIG_CMD_STRINGS) += strings.o
>  obj-$(CONFIG_CMD_SMC) += smccc.o
> diff --git a/cmd/printf.c b/cmd/printf.c
> index 337ab8ce5d..e024676743 100644
> --- a/cmd/printf.c
> +++ b/cmd/printf.c
> @@ -1,12 +1,21 @@
> -/* vi: set sw=4 ts=4: */
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
> - * printf - format and print data
> + * Copyright (C) 2021 Weidmüller Interface GmbH & Co. KG
> + * Roland Gaudig <roland.gaudig@weidmueller.com>
>   *
>   * Copyright 1999 Dave Cinege
>   * Portions copyright (C) 1990-1996 Free Software Foundation, Inc.
>   *
>   * Licensed under GPLv2 or later, see file LICENSE in this source tree.
>   */
> +/*
> + * This file provides a shell printf like format string expansion as required

This is a bit confusing with all the words run together.

Does it mean a printf-like, format-string expansion feature ?

Re: [PATCH v3 5/6] doc: usage: add description for setexpr command

[Simon Glass]

Hi Roland,

On Fri, 23 Jul 2021 at 06:30, Roland Gaudig
<roland.gaudig-oss@weidmueller.com> wrote:
>
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>
> Add usage for the setexpr command. It has been added to describe
> mainly the new setexpr format string operation.
>
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
>
> (no changes since v1)
>
>  MAINTAINERS           |   1 +
>  doc/usage/index.rst   |   1 +
>  doc/usage/setexpr.rst | 148 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 150 insertions(+)
>  create mode 100644 doc/usage/setexpr.rst

Reviewed-by: Simon Glass <sjg@chromium.org>

optional nits below

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe53698878..57c3349ef5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1053,6 +1053,7 @@ SETEXPR
>  M:     Roland Gaudig <roland.gaudig@weidmueller.com>
>  S:     Maintained
>  F:     cmd/printf.c
> +F:     doc/usage/setexpr.rst
>
>  SH
>  M:     Marek Vasut <marek.vasut+renesas@gmail.com>
> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index 40b796a3a9..719b2c90b9 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -43,6 +43,7 @@ Shell commands
>     reset
>     sbi
>     scp03
> +   setexpr
>     size
>     true
>     ums
> diff --git a/doc/usage/setexpr.rst b/doc/usage/setexpr.rst
> new file mode 100644
> index 0000000000..2e511b1290
> --- /dev/null
> +++ b/doc/usage/setexpr.rst
> @@ -0,0 +1,148 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +setexpr command
> +===============
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    setexpr[.b, .w, .l .s] <name> [*]<value> <op> [*]<value2>
> +    setexpr[.b, .w, .l] <name> [*]<value>
> +    setexpr <name> fmt <format> [value]...
> +    setexpr <name> gsub r s [t]
> +    setexpr <name> sub r s [t]
> +
> +Description
> +-----------
> +
> +The setexpr command is used to set an environment variable to the result
> +of an evaluation.
> +
> +setexpr[.b, .w, .l .s] <name> [*]<value> <op> [*]<value2>
> +     Set environment variable <name> to the result of the evaluated
> +     expression specified by <op>.
> +
> +setexpr[.b, .w, .l] name [*]value
> +     Load <value> into environment variable <name>
> +
> +setexpr name fmt <format> value
> +     Set environment variable <name> to the result of the C like
> +     format string <format> evaluation of <value>.
> +
> +setexpr name gsub <r> <s> [<t>]
> +     For each substring matching the regular expression <r> in the
> +     string <t>, substitute the string <s>.
> +     The result is assigned to <name>.
> +     If <t> is not supplied, use the old value of <name>.
> +
> +setexpr name sub <r> <s> [<t>]
> +     Just like gsub(), but replace only the first matching substring
> +
> +The setexpr command takes the following arguments:
> +
> +format
> +    This parameter contains a C or Bash like format string.

C- or Bash-like ?

or: C or Bash-like ?

Definitely needs at least one hyphen.

> +    The number of arguments is limited to 4.
> +    The following format types are supported:
> +
> +    c
> +        single character
> +    d, i
> +        decimal value
> +    o
> +        octal value
> +    s
> +        string
> +    u
> +        unsigned decimal value
> +    x, X
> +        hexadecimal value
> +    '%'
> +        no conversion, instead a % character will be written
> +
> +    Backslash escapes:
> +
> +    \" = double quote
> +    \\ = backslash
> +    \a = alert (bell)
> +    \b = backspace
> +    \c = produce no further output
> +    \f = form feed
> +    \n = new line
> +    \r = carriage return
> +    \t = horizontal tab
> +    \v = vertical tab
> +    \NNN = octal number (NNN is 0 to 3 digits)

For me this list comes out wrong...may need an extra \ ?

> +
> +name
> +    The name of the environment variable to be set
> +
> +op
> +    '|'
> +        name = value | value2
> +    '&'
> +        name = value & value2
> +    '+'
> +        name = value + value2
> +        (This is the only operator supported for strings.
> +       It acts as concatenation operator on strings)
> +    '^'
> +        name = value ^ value2
> +    '-'
> +        name = value - value2
> +    '*'
> +        name = value * value2
> +    '/'
> +        name = value / value2
> +    '%'
> +        name = value % value2

Those ops might look better as a table.

> +
> +r
> +    Regular expression
> +
> +s
> +    Substitution string
> +
> +t
> +    string
> +
> +value
> +    Can either be an integer value, a string.
> +    If the pointer prefix '*' is given value is treated as memory address.
> +
> +value2
> +    See value
> +
> +Example
> +-------
> +
> +::
> +
> +    => setexpr foo fmt %d 0x100
> +    => echo $foo
> +    256
> +    =>
> +
> +    => setexpr foo fmt 0x%08x 63
> +    => echo $foo
> +    0x00000063
> +    =>
> +
> +    => setexpr foo fmt %%%o 8
> +    => echo $foo
> +    %10
> +    =>
> +
> +Configuration
> +-------------
> +
> +The setexpr gsub and sub operations are only available if CONFIG_REGEX=y.

Should we use `CONFIG_REGEX=y` ?


> +
> +Return value
> +------------
> +
> +The return value $? is set to 0 (true) if the operation was successful.
> +
> +If an error occurs, the return value $? is set to 1 (false).
> --
> 2.25.1
>

Re: [PATCH v3 6/6] test: cmd: setexpr: add format string tests

[Simon Glass]

Hi Roland,

On Fri, 23 Jul 2021 at 06:30, Roland Gaudig
<roland.gaudig-oss@weidmueller.com> wrote:
>
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>
> Add test cases for the setexpr format string operator.
>
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
>
> (no changes since v1)
>
>  test/cmd/setexpr.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>
> diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
> index 08b6e6e724..0dc94f7e61 100644
> --- a/test/cmd/setexpr.c
> +++ b/test/cmd/setexpr.c
> @@ -386,6 +386,90 @@ static int setexpr_test_str_long(struct unit_test_state *uts)
>  }
>  SETEXPR_TEST(setexpr_test_str_long, UT_TESTF_CONSOLE_REC);
>
> +#ifdef CONFIG_CMD_SETEXPR_FMT
> +/* Test 'setexpr' command with simply setting integers */

comment needs an update

> +static int setexpr_test_fmt(struct unit_test_state *uts)
> +{
> +       u8 *buf;
> +
> +       buf = map_sysmem(0, BUF_SIZE);
> +       memset(buf, '\xff', BUF_SIZE);
> +
> +       /* Test decimal conversion */
> +       ut_assertok(run_command("setexpr fred fmt %d 0xff", 0));
> +       ut_asserteq_str("255", env_get("fred"));

I suggest a blank line before each comment to make this easier to read.

Also how about adding a helper that does the run_command() and returns
the value of env_get("fred") ? Then your tests might fit in one
statement, and perhaps even one line in some cases.

> +       /* Test hexadecimal conversion with 0x prefix and 4 digits */
> +       ut_assertok(run_command("setexpr fred fmt 0x%04x 257", 0));

nit: that should really be %#04x so we teach people to use that
feature. Another one below.

> +       ut_asserteq_str("0x0257", env_get("fred"));
> +       /* Test octal conversion with % prefix */
> +       ut_assertok(run_command("setexpr fred fmt %%%o 8", 0));
> +       ut_asserteq_str("%10", env_get("fred"));
> +       /* Test argument surrounded by %% */
> +       ut_assertok(run_command("setexpr fred fmt %%%x%% 0xff", 0));
> +       ut_asserteq_str("%ff%", env_get("fred"));
> +       /* Test escape sequence */
> +       ut_assertok(run_command("setexpr fred fmt \"hello\\040world\"", 0));
> +       ut_asserteq_str("hello world", env_get("fred"));
> +       /* Test %b with string containing octal escape sequence */
> +       ut_assertok(run_command("setexpr fred fmt oh%bno \137", 0));
> +       ut_asserteq_str("oh_no", env_get("fred"));
> +       /* Test %b with string containing \c escape sequence */
> +       ut_assertok(run_command("setexpr fred fmt hello%bworld \"\\c\"", 0));
> +       ut_asserteq_str("hello", env_get("fred"));
> +       /* Test multiple arguments referencing environment varialbes */
> +       ut_assertok(run_command("setenv a eff", 0));
> +       ut_assertok(run_command("setenv b hello", 0));
> +       ut_assertok(run_command("setenv c 0x63", 0));
> +       ut_assertok(run_command("setenv d world", 0));
> +       ut_assertok(run_command("setexpr fred fmt \"0x%08x-%s-%d-%s\" $a $b $c $d", 0));
> +       ut_asserteq_str("0x00000eff-hello-99-world", env_get("fred"));
> +       /* Test with two format specifiers, but only one argument */
> +       ut_assertok(run_command("setexpr fred fmt %d_%x 100", 0));
> +       ut_asserteq_str("256_0", env_get("fred"));
> +       /* Test maximum string length */
> +       ut_assertok(run_command("setexpr fred fmt \"%0127d\" 7b", 0));
> +       ut_asserteq_str("0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000123", env_get("fred"));
> +       /* Test maximum unsigned integer size */
> +       ut_assertok(run_command("setexpr fred fmt %u ffffffffffffffff", 0));

I think this needs to check sizeof(ulong) because this will only work
on 64-bit machines.

How about splitting these ones out into two separate test functions,
one for 32-bit and one for 64-bit?

> +       ut_asserteq_str("18446744073709551615", env_get("fred"));
> +       /* Test maximum positive integer size */
> +       ut_assertok(run_command("setexpr fred fmt %d 7fffffffffffffff", 0));
> +       ut_asserteq_str("9223372036854775807", env_get("fred"));
> +       /* Test maximum negative integer size */
> +       ut_assertok(run_command("setexpr fred fmt %d 8000000000000000", 0));
> +       ut_asserteq_str("-9223372036854775808", env_get("fred"));
> +       /* Test minimum negative integer size */
> +       ut_assertok(run_command("setexpr fred fmt %d ffffffffffffffff", 0));
> +       ut_asserteq_str("-1", env_get("fred"));
> +       /* Test signed value with + sign */
> +       ut_assertok(run_command("setexpr fred fmt %d +5", 0));
> +       ut_asserteq_str("5", env_get("fred"));
> +       /* Test signed value with - sign */
> +       ut_assertok(run_command("setexpr fred fmt %d -4", 0));
> +       ut_asserteq_str("-4", env_get("fred"));
> +       /* Test unsigned value with + sign */
> +       ut_assertok(run_command("setexpr fred fmt %u +3", 0));
> +       ut_asserteq_str("3", env_get("fred"));
> +       /* Test unsigned value with - sign */
> +       ut_assertok(run_command("setexpr fred fmt %x -2", 0));
> +       ut_asserteq_str("fffffffffffffffe", env_get("fred"));
> +       /* Error test with missing format specifier */
> +       ut_asserteq(1, run_command("setexpr fred fmd hello 0xff", 0));
> +       /* Error test with invalid format type */
> +       ut_asserteq(1, run_command("setexpr fred fmt %a 0xff", 0));
> +       /* Error test with incomplete format specifier */
> +       ut_asserteq(1, run_command("setexpr fred fmt hello% bf", 0));
> +       /* Error exceeding maximum string length */
> +       ut_asserteq(1, run_command("setexpr fred fmt \"%0128d\" 456", 0));
> +
> +       unmap_sysmem(buf);
> +
> +       return 0;
> +}
> +
> +SETEXPR_TEST(setexpr_test_fmt, UT_TESTF_CONSOLE_REC);
> +#endif
> +
>  int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>         struct unit_test *tests = UNIT_TEST_SUITE_START(setexpr_test);
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH v3 2/6] cmd: printf: import busybox-1.33.1 printf.c

[Simon Glass]

Hi Roland,

On Fri, 23 Jul 2021 at 06:30, Roland Gaudig
<roland.gaudig-oss@weidmueller.com> wrote:
>
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>
> Import printf.c from the Busybox project, which provides Bash like
> format string handling.
>
>   src-url: https://git.busybox.net/busybox/
>   commit bcc5b0e6caca6c7602a6a41f "Bump version to 1.33.1"
>   version: 1.33.1
>
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
>
> (no changes since v1)
>
>  cmd/printf.c | 455 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 455 insertions(+)
>  create mode 100644 cmd/printf.c

I think I missed the discussion on this. Why can we not use U-Boot's
existing printf()?

If we can't, dot we need to remove the existing U-Boot printf()?
Again, sorry if I missed some discussion.

Regards,
Simon

Re: [PATCH v3 2/6] cmd: printf: import busybox-1.33.1 printf.c

[Roland Gaudig (OSS)]

Hi Simon,

On 23.07.21 21:41, Simon Glass wrote:
> On Fri, 23 Jul 2021 at 06:30, Roland Gaudig
> <roland.gaudig-oss@weidmueller.com> wrote:
>>
>> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>>
>> Import printf.c from the Busybox project, which provides Bash like
>> format string handling.
>>
>>   src-url: https://git.busybox.net/busybox/
>>   commit bcc5b0e6caca6c7602a6a41f "Bump version to 1.33.1"
>>   version: 1.33.1
>>
>> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
>> ---
>>
>> (no changes since v1)
>>
>>  cmd/printf.c | 455 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 455 insertions(+)
>>  create mode 100644 cmd/printf.c
> 
> I think I missed the discussion on this.

Short summary: We wanted to pass a U-Boot environment variable which
contained a hexadecimal value to the kernel bootargs which required a
decimal value. Therefore I created a dec operator to the setexpr command
for converting a value to decimal. Then came the proposal to use a
format string instead of a dec operator. To keep codesize low I
implemented a very limited version, allowing only one argument. Then
came the request to make it more flexible like its Bash counterpart and
allowing multiple arguments. This is what this version tries to do.

> Why can we not use U-Boot's existing printf()?

The existing printf in the lib directroy implements the C library
function, whereas my cmd printf implements a Bash like printf.
Both are on the first glance similar but behave differently in detail.
The C printf requires, that the requested format matches the type of
the argument. If I for example request a %s format and pass an int
type bad things will happen.

In contrast the shell does not have such a strict concept of types,
when requesting a %d format the argument might contain a string
representing a Number. Therefore the shell printf implementation
has to convert this string first into an integer variable, which
then can be passed to the C lib printf function.

> If we can't, dot we need to remove the existing U-Boot printf()?
> Again, sorry if I missed some discussion.

No, we can't. The cmd printf is only a front-end performing the
necessary conversion. It is relying on the lib printf, which does the
actual work.
 
Regards,
Roland

Re: [PATCH v3 2/6] cmd: printf: import busybox-1.33.1 printf.c

[Simon Glass]

Hi Roland,

On Mon, 26 Jul 2021 at 02:11, Roland Gaudig (OSS)
<roland.gaudig-oss@weidmueller.com> wrote:
>
> Hi Simon,
>
> On 23.07.21 21:41, Simon Glass wrote:
> > On Fri, 23 Jul 2021 at 06:30, Roland Gaudig
> > <roland.gaudig-oss@weidmueller.com> wrote:
> >>
> >> From: Roland Gaudig <roland.gaudig@weidmueller.com>
> >>
> >> Import printf.c from the Busybox project, which provides Bash like
> >> format string handling.
> >>
> >>   src-url: https://git.busybox.net/busybox/
> >>   commit bcc5b0e6caca6c7602a6a41f "Bump version to 1.33.1"
> >>   version: 1.33.1
> >>
> >> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >>  cmd/printf.c | 455 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 455 insertions(+)
> >>  create mode 100644 cmd/printf.c
> >
> > I think I missed the discussion on this.
>
> Short summary: We wanted to pass a U-Boot environment variable which
> contained a hexadecimal value to the kernel bootargs which required a
> decimal value. Therefore I created a dec operator to the setexpr command
> for converting a value to decimal. Then came the proposal to use a
> format string instead of a dec operator. To keep codesize low I
> implemented a very limited version, allowing only one argument. Then
> came the request to make it more flexible like its Bash counterpart and
> allowing multiple arguments. This is what this version tries to do.
>
> > Why can we not use U-Boot's existing printf()?
>
> The existing printf in the lib directroy implements the C library
> function, whereas my cmd printf implements a Bash like printf.
> Both are on the first glance similar but behave differently in detail.
> The C printf requires, that the requested format matches the type of
> the argument. If I for example request a %s format and pass an int
> type bad things will happen.
>
> In contrast the shell does not have such a strict concept of types,
> when requesting a %d format the argument might contain a string
> representing a Number. Therefore the shell printf implementation
> has to convert this string first into an integer variable, which
> then can be passed to the C lib printf function.
>
> > If we can't, dot we need to remove the existing U-Boot printf()?
> > Again, sorry if I missed some discussion.
>
> No, we can't. The cmd printf is only a front-end performing the
> necessary conversion. It is relying on the lib printf, which does the
> actual work.

OK. I think you should add some comments to the top of both impls
explaining their purpose and why they are separate from the other one.

Regards,
Simon

Re: [PATCH v3 1/6] lib: strto: add simple_strtoll function

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Fri, Jul 23, 2021 at 12:29:18PM +0000, Roland Gaudig wrote:

> From: Roland Gaudig <roland.gaudig@weidmueller.com>
> 
> Add simple_strtoll function for converting a string containing digits
> into a long long int value.
> 
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

For the series, applied to u-boot/master (and enabled on sandbox so the
tests run), thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]