Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 2 Apr 2023 17:17:53 +0000
From:   Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
To:     Willy Tarreau <w@1wt.eu>
Cc:     Shuah Khan <shuah@kernel.org>, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v2 0/3] tools/nolibc: add testcases for vfprintf
Message-ID: <dd8547a7-d794-409e-baa2-b431b0196360@t-8ch.de>
References: <20230328-nolibc-printf-test-v2-0-f72bdf210190@weissschuh.net>
 <ZCmwqcwxHIlODz2i@1wt.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZCmwqcwxHIlODz2i@1wt.eu>
Precedence: bulk

On 2023-04-02 18:43:21+0200, Willy Tarreau wrote:
> Thomas,
> 
> On Sun, Apr 02, 2023 at 01:02:44PM +0000, Thomas Weißschuh wrote:
> > vfprintf() is complex and so far did not have proper tests.
> > 
> > This series is based on the "dev" branch of the RCU tree.
> 
> I've just ran it with glibc to see:
> 
>   $ gcc nolibc-test.c  
>   $ ./a.out vfprintf
>   Running test 'vfprintf'
>   0 empty "" = ""                                                  [OK]
>   1 simple written(3) != read(0)                                  [FAIL]
>   2 string written(3) != read(0)                                  [FAIL]
>   3 number written(4) != read(0)                                  [FAIL]
>   4 negnumber written(5) != read(0)                               [FAIL]
>   5 unsigned written(5) != read(0)                                [FAIL]
>   6 char written(1) != read(0)                                    [FAIL]
>   7 hex written(1) != read(0)                                     [FAIL]
>   8 pointer written(5) != 3                                       [FAIL]
>   Errors during this test: 8
> 
> The main issue was that glibc uses buffered writes by default.
> 
> I could fix them with fflush() (which we don't have so it required an
> ifndef), and this also made me realize that we were missing an fclose()
> as well for compatibility with glibc. With this it got better:
> 
>   Running test 'vfprintf'
>   0 empty "" = ""                                                  [OK]
>   1 simple "foo" = "foo"                                           [OK]
>   2 string "foo" = "foo"                                           [OK]
>   3 number "1234" = "1234"                                         [OK]
>   4 negnumber "-1234" = "-1234"                                    [OK]
>   5 unsigned "12345" = "12345"                                     [OK]
>   6 char "c" = "c"                                                 [OK]
>   7 hex "f" = "f"                                                  [OK]
>   8 pointer written(5) != 3                                       [FAIL]
>   Errors during this test: 1
> 
> This is caused by glibc emitting "(nil)" while we emit "0x0" for a NULL
> pointer since we use the same code as when dumping integers. I could fix
> that one as well by printing (void*)1 instead, which shows "0x1" for both.
> 
> This gives me the patch below on top of yours, which I think would make
> sense to integrate in this form or a simplified one if we manage to add
> fflush() and fclose() earlier.
> 
> What do you think ?
> 
> Thanks,
> Willy
> 
> 
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 28a8d77078dc..2958dc3eca93 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -678,6 +678,7 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
>  	int ret, fd, w, r;
>  	char buf[100];
>  	va_list args;
> +	FILE *memfile;
>  
>  	fd = memfd_create("vfprintf", 0);
>  	if (fd == -1) {
> @@ -685,8 +686,14 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
>  		return 1;
>  	}
>  
> +	memfile = fdopen(fd, "w+");
> +	if (!memfile) {
> +		pad_spc(llen, 64, "[FAIL]\n");
> +		return 1;
> +	}
> +
>  	va_start(args, fmt);
> -	w = vfprintf(fdopen(fd, "w+"), fmt, args);
> +	w = vfprintf(memfile, fmt, args);
>  	va_end(args);
>  
>  	if (w != c) {
> @@ -695,12 +702,19 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
>  		return 1;
>  	}
>  
> +#ifndef _NOLIBC_STDIO_H
> +	fflush(memfile);
> +#endif
>  	lseek(fd, 0, SEEK_SET);
>  
>  	r = read(fd, buf, sizeof(buf) - 1);
>  	buf[r] = '\0';
>  
> +#ifndef _NOLIBC_STDIO_H
> +	fclose(memfile);
> +#else
>  	close(fd);
> +#endif

Wouldn't it be nicer to implement fflush/fclose in nolibc?
I can send a v3 with that.

>  	if (r != w) {
>  		llen += printf(" written(%d) != read(%d)", w, r);
> @@ -737,7 +751,7 @@ static int run_vfprintf(int min, int max)
>  		CASE_TEST(unsigned);     EXPECT_VFPRINTF(5, "12345", "%u", 12345); break;
>  		CASE_TEST(char);         EXPECT_VFPRINTF(1, "c", "%c", 'c'); break;
>  		CASE_TEST(hex);          EXPECT_VFPRINTF(1, "f", "%x", 0xf); break;
> -		CASE_TEST(pointer);      EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break;
> +		CASE_TEST(pointer);      EXPECT_VFPRINTF(3, "0x1", "%p", (void*)0x1); break;
>  		case __LINE__:
>  			return ret; /* must be last */
>  		/* note: do not set any defaults so as to permit holes above */