Fork/Piping in a loop

[PointSix]

Hi there. I have looked up and understand the 'simplistic' method of setting up a forked process and piping a single message. I have run into a problem when it comes to using a loop to continuously read/write through a pipe. The scenario that I have presented is having a piping program that copies a file to another file (parent reads initial file and writes contents through the pipe to the child who writes the new copied file). I set this up by creating a buffer (256 bytes) and passing it through the pipe. The problem is that it appears that when the child reads, sometimes it gets overtaken by the parent's write. It was my understanding that this was synchronized, but obviously I'm doing something wrong. Any guidance would be appreciated (as well as any constructive feedback on my coding in general). Thank you in advance.

#include <sys/types.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

#define BUFFER_SIZE 25
#define READ_END 0
#define WRITE_END 1


int main(int argc, char * argv[])
{
	int fd[2];
	pid_t pid;

	/* create the pipe */
	if(pipe(fd) == -1)
	{
		fprintf(stderr, "Pipe failed\n");
		return 1;
	}

	/* fork a child process */
	pid = fork();

	if(pid < 0) /* error occurred */
	{
		fprintf(stderr, "Fork Failed\n");
		return 1;
	}	

	if(pid > 0) /* parent process */
	{
		/* Open the file to be copied */
		FILE * fp;
		if((fp = fopen(argv[1], "r")) == NULL)
		{
			fprintf(stderr, "Cannot open file: %s.\n", argv[1]);
		}				

		/* Loop through file using buffer */
		close(fd[READ_END]);
		int done = 0;
		while(done == 0)
		{		
			/* Get the string */
			char write_msg[BUFFER_SIZE];

			if((fgets(write_msg, BUFFER_SIZE, fp)) == NULL)
			{
				done = 1;
				printf("Closing file: %s\n", argv[1]);
				close(fd[WRITE_END]);
				fclose(fp);
			}
			else
			{
				/* Send the string through the write side of the pipe */
				write(fd[WRITE_END], write_msg, strlen(write_msg)+1);

				/* DEBUG */
				printf("Sending through pipe: %s\n", write_msg);				
			}
		}
	
		wait(NULL);
	}
	else /* child process */
	{
		/* Create the new file */
		FILE * fp;
		if((fp = fopen(argv[2], "w")) == NULL)
		{
			fprintf(stderr, "Cannot create new file: %s.\n", argv[2]);
		}

		/* Keep getting stuff from the pipe */
		int done = 0;
		close(fd[WRITE_END]);
		while(done == 0)
		{
			/* Read in a string from the pipe */
			char read_msg[BUFFER_SIZE];

			if((read(fd[READ_END], read_msg, BUFFER_SIZE)) == 0) /* end of file */
			{
				done = 1;					
				printf("Closing file: %s\n", argv[2]);			
				close(fd[READ_END]);
				fclose(fp);
			}
			else /* not end of file */
			{
				/* Write the data into the new file */
				fprintf(fp, read_msg);
				
				/* DEBUG */
				printf("Printing to new file: %s\n", read_msg);
			}

		
		}		
	}

	return 0;
}

[cas]

The source of your problem is that you're sending strings across the pipe. Strings, as I'm sure you know, are terminated by a null character. Thus when you send a string, you're including the null character, and when you receive one, you expect a null character. This may sound fine, but there's one huge issue: what happens if two write() calls happen before a read()?

If the writes are smaller than your buffer--say, the strings "hello" and "there" with your 25-byte buffer--then your read will grab the following data: "hello\0there". Printing that out with fprintf() interprets it as a string, and that means it will stop at the first null byte, giving you "hello". That "there" will never be seen. The solution is to use write() (or fwrite()) to write to the file, using the return value of your read() to know how many bytes to write. Of course, this means not sending the null character along, otherwise your output file will be filled with nulls.

As for general feedback on your coding:

I would really recommend changing how you do your loops. You have something like:

while(!done)
{
  if(x) done = true;
  else /* blah blah */;
}

It's very clear that your loop condition really is "x", and I think it's much clearer to simply loop on "x" in that case, as in:

while(fgets(write_msg, sizeof write_msg, fp) != NULL)
{
  write(fd[WRITE_END], write_msg, strlen(write_msg));
}
close(fd[WRITE_END]);
fclose(fp);
wait(NULL);

The effect is the same, but this is, to my eye, clearer at a glance and doesn't bury things unnecessarily inside of the loop. It's more symmetrical, too: open files; loop; close files.

Your fprintf(fp, read_msg) call is dangerous (of course, you'll have to change it to a write()/fwrite() anyway, but for future reference...) You should never use fprintf()/printf() to print out a string whose contents are unknown to you without using "%s"--that is, you should use fprintf(fp, "%s", read_msg) instead. Otherwise, if there are conversion characters in the string (%d, %s, %p, etc), junk could be printed out, or your program might even crash. For outputting a simple string, though, fputs() does the job nicely.

When using read() and write(), the return value is very important, because fewer bytes than requested might actually be read/written. It's a pain, but you have to account for that.