[net-next,v3,2/8] selftests/net: Provide test_snprintf() helper

Commit Message

Dmitry Safonov via B4 Relay Aug. 15, 2024, 9:32 p.m. UTC

From: Dmitry Safonov <[e-mail]>

Instead of pre-allocating a fixed-sized buffer of TEST_MSG_BUFFER_SIZE
and printing into it, call vsnprintf() with str = NULL, which will
return the needed size of the buffer. This hack is documented in
man 3 vsnprintf.

Essentially, in C++ terms, it re-invents std::stringstream, which is
going to be used to print different tracing paths and formatted strings.
Use it straight away in __test_print() - which is thread-safe version of
printing in selftests.

Signed-off-by: Dmitry Safonov <[e-mail]>
---
 tools/testing/selftests/net/tcp_ao/lib/aolib.h | 59 ++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 9 deletions(-)

Comments

Simon Horman Aug. 21, 2024, 7:10 p.m. UTC | #1

On Thu, Aug 15, 2024 at 10:32:27PM +0100, Dmitry Safonov via B4 Relay wrote:
> From: Dmitry Safonov <[e-mail]>
> 
> Instead of pre-allocating a fixed-sized buffer of TEST_MSG_BUFFER_SIZE
> and printing into it, call vsnprintf() with str = NULL, which will
> return the needed size of the buffer. This hack is documented in
> man 3 vsnprintf.
> 
> Essentially, in C++ terms, it re-invents std::stringstream, which is
> going to be used to print different tracing paths and formatted strings.
> Use it straight away in __test_print() - which is thread-safe version of
> printing in selftests.
> 
> Signed-off-by: Dmitry Safonov <[e-mail]>

Hi Dmitry,

Some minor nits, as it looks like there will be a v4.

> ---
>  tools/testing/selftests/net/tcp_ao/lib/aolib.h | 59 ++++++++++++++++++++++----
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/tcp_ao/lib/aolib.h b/tools/testing/selftests/net/tcp_ao/lib/aolib.h
> index fbc7f6111815..60a63419cabb 100644
> --- a/tools/testing/selftests/net/tcp_ao/lib/aolib.h
> +++ b/tools/testing/selftests/net/tcp_ao/lib/aolib.h
> @@ -37,17 +37,58 @@ extern void __test_xfail(const char *buf);
>  extern void __test_error(const char *buf);
>  extern void __test_skip(const char *buf);
>  
> -__attribute__((__format__(__printf__, 2, 3)))
> -static inline void __test_print(void (*fn)(const char *), const char *fmt, ...)
> +static inline char *test_snprintf(const char *fmt, va_list vargs)
>  {
> -#define TEST_MSG_BUFFER_SIZE 4096
> -	char buf[TEST_MSG_BUFFER_SIZE];
> -	va_list arg;
> +	char *ret = NULL;
> +	size_t size = 0;
> +	va_list tmp;
> +	int n = 0;
>  
> -	va_start(arg, fmt);
> -	vsnprintf(buf, sizeof(buf), fmt, arg);
> -	va_end(arg);
> -	fn(buf);
> +	va_copy(tmp, vargs);
> +	n = vsnprintf(ret, size, fmt, tmp);
> +	if (n < 0)
> +		return NULL;
> +
> +	size = (size_t) n + 1;

nit: I'm not sure this cast is needed.
     But if it is, then the space after ')' should be dropped.

> +	ret = malloc(size);
> +	if (ret == NULL)

nit: if (!ret)

> +		return NULL;
> +
> +	n = vsnprintf(ret, size, fmt, vargs);
> +	if (n < 0 || n > size - 1) {
> +		free(ret);
> +		return NULL;
> +	}
> +	return ret;
> +}

...

Dmitry Safonov Aug. 21, 2024, 9:35 p.m. UTC | #2

Hi Simon,

On Wed, 21 Aug 2024 at 20:10, Simon Horman <[e-mail]> wrote:
>
> On Thu, Aug 15, 2024 at 10:32:27PM +0100, Dmitry Safonov via B4 Relay wrote:
> > From: Dmitry Safonov <[e-mail]>
> >
> > Instead of pre-allocating a fixed-sized buffer of TEST_MSG_BUFFER_SIZE
> > and printing into it, call vsnprintf() with str = NULL, which will
> > return the needed size of the buffer. This hack is documented in
> > man 3 vsnprintf.
> >
> > Essentially, in C++ terms, it re-invents std::stringstream, which is
> > going to be used to print different tracing paths and formatted strings.
> > Use it straight away in __test_print() - which is thread-safe version of
> > printing in selftests.
> >
> > Signed-off-by: Dmitry Safonov <[e-mail]>
>
> Hi Dmitry,
>
> Some minor nits, as it looks like there will be a v4.

Thanks, both seem reasonable.
Did you get them with checkpatch.pl or with your trained eyes? :)

These days I run b4 prep --check and on latest version it just gave a
bunch of fmt-strings with columns > 100.

Thanks,
             Dmitry

Simon Horman Aug. 22, 2024, 10:13 a.m. UTC | #3

On Wed, Aug 21, 2024 at 10:35:10PM +0100, Dmitry Safonov wrote:
> Hi Simon,
> 
> On Wed, 21 Aug 2024 at 20:10, Simon Horman <[e-mail]> wrote:
> >
> > On Thu, Aug 15, 2024 at 10:32:27PM +0100, Dmitry Safonov via B4 Relay wrote:
> > > From: Dmitry Safonov <[e-mail]>
> > >
> > > Instead of pre-allocating a fixed-sized buffer of TEST_MSG_BUFFER_SIZE
> > > and printing into it, call vsnprintf() with str = NULL, which will
> > > return the needed size of the buffer. This hack is documented in
> > > man 3 vsnprintf.
> > >
> > > Essentially, in C++ terms, it re-invents std::stringstream, which is
> > > going to be used to print different tracing paths and formatted strings.
> > > Use it straight away in __test_print() - which is thread-safe version of
> > > printing in selftests.
> > >
> > > Signed-off-by: Dmitry Safonov <[e-mail]>
> >
> > Hi Dmitry,
> >
> > Some minor nits, as it looks like there will be a v4.
> 
> Thanks, both seem reasonable.
> Did you get them with checkpatch.pl or with your trained eyes? :)
> 
> These days I run b4 prep --check and on latest version it just gave a
> bunch of fmt-strings with columns > 100.

Hi Dimitry,

For networking code I usually run:

checkpatch.pl --strict --codespell --min-conf-desc-length=80

Where 80 is, I believe, still in line with preferences for Networking code.
Although I'm not entirely sure it is applicable to this patch.

As to your question, in this case I think it is the --strict that causes
checkpatch to flag the issues I raised. Sorry for not mentioning that in my
previous email.

Dmitry Safonov Aug. 23, 2024, 2:27 p.m. UTC | #4

On Thu, 22 Aug 2024 at 11:13, Simon Horman <[e-mail]> wrote:
>
> On Wed, Aug 21, 2024 at 10:35:10PM +0100, Dmitry Safonov wrote:
> > Hi Simon,
> >
> > On Wed, 21 Aug 2024 at 20:10, Simon Horman <[e-mail]> wrote:
> > >
[..]
> > > Hi Dmitry,
> > >
> > > Some minor nits, as it looks like there will be a v4.
> >
> > Thanks, both seem reasonable.
> > Did you get them with checkpatch.pl or with your trained eyes? :)
> >
> > These days I run b4 prep --check and on latest version it just gave a
> > bunch of fmt-strings with columns > 100.
>
> Hi Dimitry,
>
> For networking code I usually run:
>
> checkpatch.pl --strict --codespell --min-conf-desc-length=80
>
> Where 80 is, I believe, still in line with preferences for Networking code.
> Although I'm not entirely sure it is applicable to this patch.
>
> As to your question, in this case I think it is the --strict that causes
> checkpatch to flag the issues I raised. Sorry for not mentioning that in my
> previous email.

Good, thanks!
Now I can see/fix them :-)

Cheers,
             Dmitry

Patch

diff --git a/tools/testing/selftests/net/tcp_ao/lib/aolib.h b/tools/testing/selftests/net/tcp_ao/lib/aolib.h
index fbc7f6111815..60a63419cabb 100644
--- a/tools/testing/selftests/net/tcp_ao/lib/aolib.h
+++ b/tools/testing/selftests/net/tcp_ao/lib/aolib.h
@@ -37,17 +37,58 @@  extern void __test_xfail(const char *buf);
 extern void __test_error(const char *buf);
 extern void __test_skip(const char *buf);
 
-__attribute__((__format__(__printf__, 2, 3)))
-static inline void __test_print(void (*fn)(const char *), const char *fmt, ...)
+static inline char *test_snprintf(const char *fmt, va_list vargs)
 {
-#define TEST_MSG_BUFFER_SIZE 4096
-	char buf[TEST_MSG_BUFFER_SIZE];
-	va_list arg;
+	char *ret = NULL;
+	size_t size = 0;
+	va_list tmp;
+	int n = 0;
 
-	va_start(arg, fmt);
-	vsnprintf(buf, sizeof(buf), fmt, arg);
-	va_end(arg);
-	fn(buf);
+	va_copy(tmp, vargs);
+	n = vsnprintf(ret, size, fmt, tmp);
+	if (n < 0)
+		return NULL;
+
+	size = (size_t) n + 1;
+	ret = malloc(size);
+	if (ret == NULL)
+		return NULL;
+
+	n = vsnprintf(ret, size, fmt, vargs);
+	if (n < 0 || n > size - 1) {
+		free(ret);
+		return NULL;
+	}
+	return ret;
+}
+
+static __printf(1, 2) inline char *test_sprintf(const char *fmt, ...)
+{
+	va_list vargs;
+	char *ret;
+
+	va_start(vargs, fmt);
+	ret = test_snprintf(fmt, vargs);
+	va_end(vargs);
+
+	return ret;
+}
+
+static __printf(2, 3) inline void __test_print(void (*fn)(const char *),
+					       const char *fmt, ...)
+{
+	va_list vargs;
+	char *msg;
+
+	va_start(vargs, fmt);
+	msg = test_snprintf(fmt, vargs);
+	va_end(vargs);
+
+	if (!msg)
+		return;
+
+	fn(msg);
+	free(msg);
 }
 
 #define test_print(fmt, ...)						\