GNU bug report logs - #11816
sort -o: error comes late if opening the outfile fails

Previous Next

Full log

View this message in rfc822 format

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 11816 <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: bug#11816: sort -o: error comes late if opening the outfile fails
Date: Tue, 03 Jul 2012 01:08:04 +0200

On 07/03/2012 12:49 AM, Paul Eggert wrote:
> Thanks for the improvement.
> How about the following patch to simplify this a bit?
> It removes a call to fdopen, among other things.
> 
>>From 05cc1b416a47330ef296dbeadd2a4b6095fe5c7d Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Mon, 2 Jul 2012 15:47:03 -0700
> Subject: [PATCH] sort: simplify -o handling to avoid fdopen, assert
> 
> * src/sort.c (outfd): Remove.  All uses replaced by STDOUT_FILENO.
> (stream_open): When writing, use stdout rather than fdopen.
> (move_fd_or_die): Renamed from dup2_or_die, with the added functionality
> of closing its first argument.  All uses changed.
> (avoid_trashing_input): Special case for !outfile no longer needed.
> (check_output): Arrange for standard output to go to the file,
> rather than storing the fd in outfd.
> ---
>  src/sort.c |   47 ++++++++++++++++++++---------------------------
>  1 files changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/src/sort.c b/src/sort.c
> index e4067c9..f0d32c3 100644
> --- a/src/sort.c
> +++ b/src/sort.c
> @@ -357,9 +357,6 @@ static bool unique;
>  /* Nonzero if any of the input files are the standard input. */
>  static bool have_read_stdin;
>  
> -/* File descriptor associated with -o.  */
> -static int outfd = -1;
> -
>  /* List of key field comparisons to be tried.  */
>  static struct keyfield *keylist;
>  
> @@ -916,8 +913,6 @@ stream_open (char const *file, char const *how)
>  {
>    FILE *fp;
>  
> -  if (!file)
> -    return stdout;
>    if (*how == 'r')
>      {
>        if (STREQ (file, "-"))
> @@ -931,10 +926,10 @@ stream_open (char const *file, char const *how)
>      }
>    else if (*how == 'w')
>      {
> -      assert (outfd != -1);
> -      if (ftruncate (outfd, 0) != 0)
> -        error (EXIT_FAILURE, errno, _("%s: error truncating"), quote (file));
> -      fp = fdopen (outfd, how);
> +      if (file && ftruncate (STDOUT_FILENO, 0) != 0)
> +        error (EXIT_FAILURE, errno, _("%s: error truncating"),
> +               quote (file));

Hmm I know I had EXIT_FAILURE here.
Should that be SORT_FAILURE?

> +      fp = stdout;
>      }
>    else
>      assert (!"unexpected mode passed to stream_open");
> @@ -981,10 +976,14 @@ xfclose (FILE *fp, char const *file)
>  }
>  
>  static void
> -dup2_or_die (int oldfd, int newfd)
> +move_fd_or_die (int oldfd, int newfd)
>  {
> -  if (dup2 (oldfd, newfd) < 0)
> -    error (SORT_FAILURE, errno, _("dup2 failed"));
> +  if (oldfd != newfd)
> +    {
> +      if (dup2 (oldfd, newfd) < 0)
> +        error (SORT_FAILURE, errno, _("dup2 failed"));
> +      close (oldfd);
> +    }
>  }
>  
>  /* Fork a child process for piping to and do common cleanup.  The
> @@ -1095,10 +1094,8 @@ maybe_create_temp (FILE **pfp, bool survive_fd_exhaustion)
>        else if (node->pid == 0)
>          {
>            close (pipefds[1]);
> -          dup2_or_die (tempfd, STDOUT_FILENO);
> -          close (tempfd);
> -          dup2_or_die (pipefds[0], STDIN_FILENO);
> -          close (pipefds[0]);
> +          move_fd_or_die (tempfd, STDOUT_FILENO);
> +          move_fd_or_die (pipefds[0], STDIN_FILENO);
>  
>            if (execlp (compress_program, compress_program, (char *) NULL) < 0)
>              error (SORT_FAILURE, errno, _("couldn't execute %s"),
> @@ -1155,10 +1152,8 @@ open_temp (struct tempnode *temp)
>  
>      case 0:
>        close (pipefds[0]);
> -      dup2_or_die (tempfd, STDIN_FILENO);
> -      close (tempfd);
> -      dup2_or_die (pipefds[1], STDOUT_FILENO);
> -      close (pipefds[1]);
> +      move_fd_or_die (tempfd, STDIN_FILENO);
> +      move_fd_or_die (pipefds[1], STDOUT_FILENO);
>  
>        execlp (compress_program, compress_program, "-d", (char *) NULL);
>        error (SORT_FAILURE, errno, _("couldn't execute %s -d"),
> @@ -3649,10 +3644,7 @@ avoid_trashing_input (struct sortfile *files, size_t ntemps,
>          {
>            if (! got_outstat)
>              {
> -              if ((outfile
> -                   ? fstat (outfd, &outstat)
> -                   : fstat (STDOUT_FILENO, &outstat))
> -                  != 0)
> +              if (fstat (STDOUT_FILENO, &outstat) != 0)
>                  break;
>                got_outstat = true;
>              }
> @@ -3703,17 +3695,18 @@ check_inputs (char *const *files, size_t nfiles)
>  }
>  
>  /* Ensure a specified output file can be created or written to,
> -   and cache the returned descriptor in the global OUTFD variable.
> -   Otherwise exit with a diagnostic.  */
> +   and point stdout to it.  Do not truncate the file.
> +   Exit with a diagnostic on failure.  */
>  
>  static void
>  check_output (char const *outfile)
>  {
>    if (outfile)
>      {
> -      outfd = open (outfile, O_WRONLY | O_CREAT | O_BINARY, MODE_RW_UGO);
> +      int outfd = open (outfile, O_WRONLY | O_CREAT | O_BINARY, MODE_RW_UGO);
>        if (outfd < 0)
>          die (_("open failed"), outfile);
> +      move_fd_or_die (outfd, STDOUT_FILENO);
>      }
>  }
>  

Looks good!

cheers,
Pádraig.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.