diff mbox series

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

Message ID 46b3f40fe91b725d6ca897709ebe0c353fb3a882.1532469861.git.geoff@infradead.org
State Superseded
Headers show
Series [v1,01/30] docker: Add libfdt-dev | expand

Commit Message

Geoff Levand July 24, 2018, 10:15 p.m. UTC
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@infradead.org>
---
 lib/process/process.c | 54 ++++++++++++++++++++++++++++++++++++++-------------
 lib/process/process.h | 26 ++++++++++++++++++++-----
 2 files changed, 62 insertions(+), 18 deletions(-)

Comments

samjonas July 30, 2018, 3:25 a.m. UTC | #1
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@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
Geoff Levand July 30, 2018, 7:04 p.m. UTC | #2
Hi Sam,

On 07/29/2018 08:25 PM, Samuel Mendoza-Jonas wrote:
> On Tue, 2018-07-24 at 22:15 +0000, Geoff Levand wrote:
>> +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?

Yes, that's what I had intended, to print only if a stdout was passed.

-Geoff
diff mbox series

Patch

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;
+	}
+
+	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