diff mbox series

[v2] tst-tzset: output reason when creating 4GiB file fails

Message ID 20211102233159.4094554-1-shorne@gmail.com
State Committed
Commit afbf26492a5adccc4c4eda00eb588b0b79e4290a
Headers
Series [v2] tst-tzset: output reason when creating 4GiB file fails |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Stafford Horne Nov. 2, 2021, 11:31 p.m. UTC 
  Currently, if the temporary file creation fails the create_tz_file
function returns NULL.  The NULL pointer is then passed to setenv which
causes a SIGSEGV.  Rather than failing with a SIGSEGV print a warning
and exit.

Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
Changes since v1:
 - Use PRId64 printf formatting, suggested by Adhemerval

 timezone/tst-tzset.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Adhemerval Zanella Nov. 3, 2021, 5:35 p.m. UTC | #1 
On 02/11/2021 20:31, Stafford Horne wrote:
> Currently, if the temporary file creation fails the create_tz_file
> function returns NULL.  The NULL pointer is then passed to setenv which
> causes a SIGSEGV.  Rather than failing with a SIGSEGV print a warning
> and exit.
> 
> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
> Changes since v1:
>  - Use PRId64 printf formatting, suggested by Adhemerval
> 
>  timezone/tst-tzset.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/timezone/tst-tzset.c b/timezone/tst-tzset.c
> index d6da2932bb..3dad42e041 100644
> --- a/timezone/tst-tzset.c
> +++ b/timezone/tst-tzset.c
> @@ -25,6 +25,7 @@
>  #include <time.h>
>  #include <unistd.h>
>  #include <support/check.h>
> +#include <inttypes.h>
>  
>  static int do_test (void);
>  #define TEST_FUNCTION do_test ()
> @@ -103,6 +104,13 @@ static void
>  test_tz_file (off64_t size)
>  {
>    char *path = create_tz_file (size);
> +  if (path == NULL)
> +   {
> +     printf ("creating timezone file of size: %" PRId64 "MiB failed.\n",
> +	     size / (1024 * 1024));
> +     exit (1);
> +   }
> +
>    if (setenv ("TZ", path, 1) < 0)
>      {
>        printf ("setenv failed: %m\n");
>
Paul Eggert Nov. 3, 2021, 7:14 p.m. UTC | #2 
On 11/2/21 16:31, Stafford Horne via Libc-alpha wrote:
> +   {
> +     printf ("creating timezone file of size: %" PRId64 "MiB failed.\n",
> +	     size / (1024 * 1024));

Why not simply print the size in bytes? Converting to MiB outputs less 
information due to rounding, and is hardly worth the trouble here. Other 
than that, looks fine.
Stafford Horne Nov. 3, 2021, 8:30 p.m. UTC | #3 
On Wed, Nov 03, 2021 at 12:14:52PM -0700, Paul Eggert wrote:
> On 11/2/21 16:31, Stafford Horne via Libc-alpha wrote:
> > +   {
> > +     printf ("creating timezone file of size: %" PRId64 "MiB failed.\n",
> > +	     size / (1024 * 1024));
> 
> Why not simply print the size in bytes? Converting to MiB outputs less
> information due to rounding, and is hardly worth the trouble here. Other
> than that, looks fine.

Hi Paul,

The reason for the divide by (1024 * 1024) it to make it easier to see where the
failure is at a glance.

The call to test_tz_file is done two times:

  test_tz_file (64 * 1024 * 1024);

  test_tz_file (4LL * 1024 * 1024 * 1024 - 6);

For me, it's a lot easier to tell the different between 2 and 4 digits (64MiB
and 4096MiB) than the bigger numbers.  The information lost is not too important
considering the test, in my opinion.

-Stafford
Stafford Horne Nov. 12, 2021, 11:10 p.m. UTC | #4 
On Thu, Nov 04, 2021 at 05:30:27AM +0900, Stafford Horne wrote:
> On Wed, Nov 03, 2021 at 12:14:52PM -0700, Paul Eggert wrote:
> > On 11/2/21 16:31, Stafford Horne via Libc-alpha wrote:
> > > +   {
> > > +     printf ("creating timezone file of size: %" PRId64 "MiB failed.\n",
> > > +	     size / (1024 * 1024));
> > 
> > Why not simply print the size in bytes? Converting to MiB outputs less
> > information due to rounding, and is hardly worth the trouble here. Other
> > than that, looks fine.
> 
> Hi Paul,
> 
> The reason for the divide by (1024 * 1024) it to make it easier to see where the
> failure is at a glance.
> 
> The call to test_tz_file is done two times:
> 
>   test_tz_file (64 * 1024 * 1024);
> 
>   test_tz_file (4LL * 1024 * 1024 * 1024 - 6);
> 
> For me, it's a lot easier to tell the different between 2 and 4 digits (64MiB
> and 4096MiB) than the bigger numbers.  The information lost is not too important
> considering the test, in my opinion.

Hi,

I have pushed this as is.

-Stafford
diff mbox series

Patch

diff --git a/timezone/tst-tzset.c b/timezone/tst-tzset.c
index d6da2932bb..3dad42e041 100644
--- a/timezone/tst-tzset.c
+++ b/timezone/tst-tzset.c
@@ -25,6 +25,7 @@ 
 #include <time.h>
 #include <unistd.h>
 #include <support/check.h>
+#include <inttypes.h>
 
 static int do_test (void);
 #define TEST_FUNCTION do_test ()
@@ -103,6 +104,13 @@  static void
 test_tz_file (off64_t size)
 {
   char *path = create_tz_file (size);
+  if (path == NULL)
+   {
+     printf ("creating timezone file of size: %" PRId64 "MiB failed.\n",
+	     size / (1024 * 1024));
+     exit (1);
+   }
+
   if (setenv ("TZ", path, 1) < 0)
     {
       printf ("setenv failed: %m\n");