Re: [PATCH 2/9] configurator: Reimplement run using popen

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1.1: Type: text/plain, Size: 3938 bytes --]

On Sun, Sep 18, 2016 at 06:51:59PM -0600, Kevin Locke wrote:
> Rather than using fork+pipe+system+waitpid, most of which are only
> available on POSIX-like systems, use popen which is also available on
> Windows (under the name _popen).

Concept looks good, one little wart.

> 
> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> ---
>  tools/configurator/configurator.c | 77 +++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 48 deletions(-)
> 
> diff --git a/tools/configurator/configurator.c b/tools/configurator/configurator.c
> index e322c76..9817fcd 100644
> --- a/tools/configurator/configurator.c
> +++ b/tools/configurator/configurator.c
> @@ -24,14 +24,14 @@
>  #include <stdio.h>
>  #include <stdbool.h>
>  #include <stdlib.h>
> -#include <unistd.h>
>  #include <err.h>
> -#include <sys/types.h>
> -#include <sys/wait.h>
> -#include <sys/stat.h>
> -#include <fcntl.h>
>  #include <string.h>
>  
> +#ifdef _MSC_VER
> +#define popen _popen
> +#define pclose _pclose
> +#endif
> +
>  #define DEFAULT_COMPILER "cc"
>  #define DEFAULT_FLAGS "-g3 -ggdb -Wall -Wundef -Wmissing-prototypes -Wmissing-declarations -Wstrict-prototypes -Wold-style-definition"
>  
> @@ -367,20 +367,19 @@ static struct test tests[] = {
>  	},
>  };
>  
> -static char *grab_fd(int fd)
> +static char *grab_stream(FILE *file)
>  {
> -	int ret;
> -	size_t max, size = 0;
> +	size_t max, ret, size = 0;
>  	char *buffer;
>  
> -	max = 16384;
> -	buffer = malloc(max+1);
> -	while ((ret = read(fd, buffer + size, max - size)) > 0) {
> +	max = BUFSIZ;
> +	buffer = malloc(max);
> +	while ((ret = fread(buffer+size, 1, max - size, file)) == max - size) {

This assumes that fread() will never return a short read except on EOF
or error.  I expect that will be true in practice for regular files,
but I'm not sure if it's a good idea to rely on it.

>  		size += ret;
> -		if (size == max)
> -			buffer = realloc(buffer, max *= 2);
> +		buffer = realloc(buffer, max *= 2);
>  	}
> -	if (ret < 0)
> +	size += ret;
> +	if (ferror(file))
>  		err(1, "reading from command");
>  	buffer[size] = '\0';
>  	return buffer;
> @@ -388,43 +387,25 @@ static char *grab_fd(int fd)
>  
>  static char *run(const char *cmd, int *exitstatus)
>  {
> -	pid_t pid;
> -	int p[2];
> +	static const char redir[] = " 2>&1";
> +	size_t cmdlen;
> +	char *cmdredir;
> +	FILE *cmdout;
>  	char *ret;
> -	int status;
>  
> -	if (pipe(p) != 0)
> -		err(1, "creating pipe");
> -
> -	pid = fork();
> -	if (pid == -1)
> -		err(1, "forking");
> -
> -	if (pid == 0) {
> -		if (dup2(p[1], STDOUT_FILENO) != STDOUT_FILENO
> -		    || dup2(p[1], STDERR_FILENO) != STDERR_FILENO
> -		    || close(p[0]) != 0
> -		    || close(STDIN_FILENO) != 0
> -		    || open("/dev/null", O_RDONLY) != STDIN_FILENO)
> -			exit(128);
> -
> -		status = system(cmd);
> -		if (WIFEXITED(status))
> -			exit(WEXITSTATUS(status));
> -		/* Here's a hint... */
> -		exit(128 + WTERMSIG(status));
> -	}
> +	cmdlen = strlen(cmd);
> +	cmdredir = malloc(cmdlen + sizeof(redir));
> +	memcpy(cmdredir, cmd, cmdlen);
> +	memcpy(cmdredir + cmdlen, redir, sizeof(redir));
> +
> +	cmdout = popen(cmdredir, "r");
> +	if (!cmdout)
> +		err(1, "popen \"%s\"", cmdredir);
> +
> +	free(cmdredir);
>  
> -	close(p[1]);
> -	ret = grab_fd(p[0]);
> -	/* This shouldn't fail... */
> -	if (waitpid(pid, &status, 0) != pid)
> -		err(1, "Failed to wait for child");
> -	close(p[0]);
> -	if (WIFEXITED(status))
> -		*exitstatus = WEXITSTATUS(status);
> -	else
> -		*exitstatus = -WTERMSIG(status);
> +	ret = grab_stream(cmdout);
> +	*exitstatus = pclose(cmdout);
>  	return ret;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan