[PATCH v1 13/30] lib/process: Add process_get_stdout

[Samuel Mendoza-Jonas]

On Tue, 2018-07-24 at 22:15 +0000, Geoff Levand wrote:
> Add a new structure 'struct process_stdout' and optional parameter
> 'stdout' to the process_run_simple functions to allow the caller
> to get a buffer filled with the stdout from the child process.
> 
> Rename the process_run_simple functions to process_get_stdout
> and add wrappers for the old process_run_simple function names.
> 
> Signed-off-by: Geoff Levand <geoff at infradead.org>
> ---
>  lib/process/process.c | 54 ++++++++++++++++++++++++++++++++++++++-------------
>  lib/process/process.h | 26 ++++++++++++++++++++-----
>  2 files changed, 62 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/process/process.c b/lib/process/process.c
> index bc392dc..5f1f9d3 100644
> --- a/lib/process/process.c
> +++ b/lib/process/process.c
> @@ -452,48 +452,76 @@ void process_stop_async_all(void)
>  	}
>  }
>  
> -int process_run_simple_argv(void *ctx, const char *argv[])
> +int process_get_stdout_argv(void *ctx, struct process_stdout **stdout,
> +	const char *argv[])
>  {
> -	struct process *process;
> +	struct process *p;
>  	int rc;
>  
> -	process = process_create(ctx);
> +	p = process_create(NULL);
> +	p->path = argv[0];
> +	p->argv = argv;
>  
> -	process->path = argv[0];
> -	process->argv = argv;
> +	if (stdout) {
> +		p->keep_stdout = true;
> +		*stdout = NULL;
> +	}
>  
> -	rc = process_run_sync(process);
> +	rc = process_run_sync(p);
>  
>  	if (!rc)
> -		rc = process->exit_status;
> +		rc = p->exit_status;
> +	else {
> +		pb_debug("%s: process_run_sync failed: %s.\n", __func__,
> +			p->path);
> +		if (*stdout)
> +			pb_debug("%s: stdout: %s\n\n", __func__, p->stdout_buf);
> +		goto exit;

Clang catches this check here:
12:01:55 lib/process/process.c:477:7: warning: Dereference of null pointer (loaded from variable 'stdout')
12:01:55                 if (*stdout)
12:01:55                     ^~~~~~~

For example if this is called via process_run_simple_argv() then stdout
is NULL which is not caught earlier.
Since this is guarding using p->stdout_buf can I fix this up by just changing
it to `if (stdout)` instead?

Cheers,
Sam

> +	}
> +
> +	if (!stdout)
> +		goto exit;
> +
> +	*stdout = talloc(ctx, struct process_stdout);
> +
> +	if (!*stdout) {
> +		rc = -1;
> +		goto exit;
> +	}
>  
> -	process_release(process);
> +	(*stdout)->len = p->stdout_len;
> +	(*stdout)->buf = talloc_memdup(*stdout, p->stdout_buf,
> +		p->stdout_len + 1);
> +	(*stdout)->buf[p->stdout_len] = 0;
>  
> +exit:
> +	process_release(p);
>  	return rc;
>  }
>  
> -int process_run_simple(void *ctx, const char *name, ...)
> +int process_get_stdout(void *ctx, struct process_stdout **stdout,
> +	const char *path, ...)
>  {
>  	int rc, i, n_argv = 1;
>  	const char **argv;
>  	va_list ap;
>  
> -	va_start(ap, name);
> +	va_start(ap, path);
>  	while (va_arg(ap, char *))
>  		n_argv++;
>  	va_end(ap);
>  
>  	argv = talloc_array(ctx, const char *, n_argv + 1);
> -	argv[0] = name;
> +	argv[0] = path;
>  
> -	va_start(ap, name);
> +	va_start(ap, path);
>  	for (i = 1; i < n_argv; i++)
>  		argv[i] = va_arg(ap, const char *);
>  	va_end(ap);
>  
>  	argv[i] = NULL;
>  
> -	rc = process_run_simple_argv(ctx, argv);
> +	rc = process_get_stdout_argv(ctx, stdout, argv);
>  
>  	talloc_free(argv);
>  
> diff --git a/lib/process/process.h b/lib/process/process.h
> index 003ff8e..9473a0d 100644
> --- a/lib/process/process.h
> +++ b/lib/process/process.h
> @@ -27,6 +27,11 @@ struct process_info;
>  
>  typedef void	(*process_exit_cb)(struct process *);
>  
> +struct process_stdout {
> +	size_t len;
> +	char *buf;
> +};
> +
>  struct process {
>  	/* caller-provided configuration */
>  	const char		*path;
> @@ -63,13 +68,24 @@ struct process *process_create(void *ctx);
>   */
>  void process_release(struct process *process);
>  
> -/* Synchronous interface. These functions will all block while waiting for
> - * the process to exit.
> +/* Synchronous interface. The process_run_sync, process_run_simple and
> + * process_get_stdout functions will all block while waiting for the child
> + * process to exit.  Calls to the variadic versions must have a NULL terminating
> + * argument.  For the process_get_stdout calls stderr will go to the log.
>   */
>  int process_run_sync(struct process *process);
> -int process_run_simple_argv(void *ctx, const char *argv[]);
> -int process_run_simple(void *ctx, const char *name, ...)
> -	__attribute__((sentinel(0)));
> +int process_get_stdout_argv(void *ctx, struct process_stdout **stdout,
> +	const char *argv[]);
> +int process_get_stdout(void *ctx, struct process_stdout **stdout,
> +	const char *path, ...) __attribute__((sentinel(0)));
> +
> +static inline int process_run_simple_argv(void *ctx, const char *argv[])
> +{
> +	return process_get_stdout_argv(ctx, NULL, argv);
> +}
> +#define process_run_simple(_ctx, _path, args...) \
> +	process_get_stdout(_ctx, NULL, _path, args)
> +
>  
>  /* Asynchronous interface. When a process is run with process_run_async, the
>   * function returns without wait()ing for the child process to exit. If the