From: Reuben Thomas <rrt@sc3d.org>
Date: Sat, 27 Jan 2018 12:43:15 +0000 (+0000)
Subject: task.c: slightly improve code and comments
X-Git-Tag: v3.7~41
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=8ba6754404fc580d417ba8897b62884145c03c77;p=recode

task.c: slightly improve code and comments

This is the fruit of an aborted investigation into using pipes all the
time (it won’t work with the present architecture with translations into a
buffer).

It could be achieved by either a) adding one more stage to write to a file,
and having the parent process read that back, or b) adding an extra pipe
back to the parent process, but that would then require non-blocking I/O,
which would involve i) a new type of communication channel (e.g. file
descriptors), and ii) reimplementation, if desired, of buffering.

An overall simpler and more portable solution would be to convert the
parallel code to use threads instead of child processes, so that the output
buffer would remain accessible to the parent process.
---

diff --git a/src/task.c b/src/task.c
index ceabbdd..d705099 100644
--- a/src/task.c
+++ b/src/task.c
@@ -263,7 +263,6 @@ perform_sequence (RECODE_TASK task, enum recode_sequence_strategy strategy)
        sequence_index++)
     {
       child_process = -1;
-      subtask->output = task->output;
 
       if (strategy == RECODE_SEQUENCE_IN_MEMORY)
 	if (sequence_index > 0)
@@ -323,19 +322,21 @@ perform_sequence (RECODE_TASK task, enum recode_sequence_strategy strategy)
 		}
 	      else
 		{
-		  /* The parent redirects the current input file to the pipe.  */
-
-		  if (dup2 (pipe_pair[0], fileno (subtask->input.file)) < 0)
+		  /* The parent redirects the current input file, if any, to the pipe.  */
+		  if (subtask->input.file)
 		    {
-		      recode_perror (NULL, "dup2 ()");
-		      recode_if_nogo (RECODE_SYSTEM_ERROR, subtask);
-		      SUBTASK_RETURN (subtask);
+		      if (dup2 (pipe_pair[0], fileno (subtask->input.file)) < 0)
+			{
+			  recode_perror (NULL, "dup2 ()");
+			  recode_if_nogo (RECODE_SYSTEM_ERROR, subtask);
+			  SUBTASK_RETURN (subtask); // FIXME: Don't leak pipe_pair!
+			}
 		    }
 		  if (close (pipe_pair[0]) < 0)
 		    {
 		      recode_perror (NULL, "close ()");
 		      recode_if_nogo (RECODE_SYSTEM_ERROR, subtask);
-		      SUBTASK_RETURN (subtask);
+		      SUBTASK_RETURN (subtask); // FIXME: Don't leak pipe_pair[1]!
 		    }
 		  if (close (pipe_pair[1]) < 0)
 		    {
@@ -351,6 +352,7 @@ perform_sequence (RECODE_TASK task, enum recode_sequence_strategy strategy)
 	{
 	  /* Prepare the final output file.  */
 
+	  subtask->output = task->output;
 	  if (subtask->output.name)
 	    {
 	      if (!*subtask->output.name)
@@ -381,7 +383,7 @@ perform_sequence (RECODE_TASK task, enum recode_sequence_strategy strategy)
 	  (*step->transform_routine) (subtask);
 
 	  if (strategy == RECODE_SEQUENCE_WITH_PIPE)
-	    break;	/* parent process: escape from loop */
+	    break;	/* child/top-level process: escape from loop */
 
 	  /* Post-step clean up for memory sequence.  */
 
@@ -483,10 +485,9 @@ perform_sequence (RECODE_TASK task, enum recode_sequence_strategy strategy)
     {
       free (input.buffer);
       free (output.buffer);
-
-      task->output = subtask->output;
     }
 
+  task->output = subtask->output;
   SUBTASK_RETURN (subtask);
 }