Quoting Denis V. Lunev (2015-10-01 02:38:01)
> From: Yuri Pudgorodskiy <y...@virtuozzo.com>
>
> Guest-exec rewriten in platform-independant style with glib spawn.
>
> Child process is spawn asynchroneously and exit status can later
> be picked up by guest-exec-status command.
>
> stdin/stdout/stderr of the child now is redirected to /dev/null
> Later we will add ability to specify stdin in guest-exec command
> and to get collected stdout/stderr with guest-exec-status.
>
> Signed-off-by: Yuri Pudgorodskiy <y...@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <d...@openvz.org>
> CC: Michael Roth <mdr...@linux.vnet.ibm.com>
> ---
> qga/commands.c | 168
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> qga/qapi-schema.json | 57 +++++++++++++++++
> 2 files changed, 225 insertions(+)
>
> diff --git a/qga/commands.c b/qga/commands.c
> index 7834967..6efd6aa 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -70,3 +70,171 @@ struct GuestAgentInfo *qmp_guest_info(Error **errp)
> qmp_for_each_command(qmp_command_info, info);
> return info;
> }
> +
> +struct GuestExecInfo {
> + GPid pid;
> + gint status;
> + bool finished;
> + QTAILQ_ENTRY(GuestExecInfo) next;
> +};
> +typedef struct GuestExecInfo GuestExecInfo;
> +
> +static struct {
> + QTAILQ_HEAD(, GuestExecInfo) processes;
> +} guest_exec_state = {
> + .processes = QTAILQ_HEAD_INITIALIZER(guest_exec_state.processes),
> +};
> +
> +static GuestExecInfo *guest_exec_info_add(GPid pid)
> +{
> + GuestExecInfo *gei;
> +
> + gei = g_malloc0(sizeof(*gei));
gei = g_new0(GuestExecInfo, 1);
and same for all other g_malloc*(sizeof(...)) callers.
(Markus has been trying to get all prior g_malloc users converted over)
> + gei->pid = pid;
> + QTAILQ_INSERT_TAIL(&guest_exec_state.processes, gei, next);
> +
> + return gei;
> +}
> +
> +static GuestExecInfo *guest_exec_info_find(GPid pid)
> +{
> + GuestExecInfo *gei;
> +
> + QTAILQ_FOREACH(gei, &guest_exec_state.processes, next) {
> + if (gei->pid == pid) {
> + return gei;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
> +{
> + GuestExecInfo *gei;
> + GuestExecStatus *ges;
> +
> + slog("guest-exec-status called, pid: %u", (uint32_t)pid);
> +
> + gei = guest_exec_info_find((GPid)pid);
> + if (gei == NULL) {
> + error_setg(err, QERR_INVALID_PARAMETER, "pid");
> + return NULL;
> + }
> +
> + ges = g_malloc0(sizeof(GuestExecStatus));
> + ges->exit = ges->signal = -1;
> +
> + if (gei->finished) {
> + /* glib has no platform independent way to parse exit status */
> +#ifdef G_OS_WIN32
> + if ((uint32_t)gei->status < 0xC0000000U) {
Can you add a comment with a link to the documentation you referenced
for this? I assume this is actually for exceptions rather than normal
signals?
> + ges->exit = gei->status;
> + } else {
> + ges->signal = gei->status;
> + }
> +#else
> + if (WIFEXITED(gei->status)) {
> + ges->exit = WEXITSTATUS(gei->status);
> + } else if (WIFSIGNALED(gei->status)) {
> + ges->signal = WTERMSIG(gei->status);
> + }
> +#endif
> + QTAILQ_REMOVE(&guest_exec_state.processes, gei, next);
> + g_free(gei);
> + }
> +
> + return ges;
> +}
> +
> +/* Get environment variables or arguments array for execve(). */
> +static char **guest_exec_get_args(const strList *entry, bool log)
> +{
> + const strList *it;
> + int count = 1, i = 0; /* reserve for NULL terminator */
> + char **args;
> + char *str; /* for logging array of arguments */
> + size_t str_size = 1;
> +
> + for (it = entry; it != NULL; it = it->next) {
> + count++;
> + str_size += 1 + strlen(it->value);
> + }
> +
> + str = g_malloc(str_size);
> + *str = 0;
> + args = g_malloc(count * sizeof(char *));
> + for (it = entry; it != NULL; it = it->next) {
> + args[i++] = it->value;
> + pstrcat(str, str_size, it->value);
> + if (it->next) {
> + pstrcat(str, str_size, " ");
> + }
> + }
> + args[i] = NULL;
> +
> + if (log) {
> + slog("guest-exec called: \"%s\"", str);
> + }
> + g_free(str);
> +
> + return args;
> +}
> +
> +static void guest_exec_child_watch(GPid pid, gint status, gpointer data)
> +{
> + GuestExecInfo *gei = (GuestExecInfo *)data;
> +
> + g_debug("guest_exec_child_watch called, pid: %u, status: %u",
> + (uint32_t)pid, (uint32_t)status);
> +
> + gei->status = status;
> + gei->finished = true;
> +
> + g_spawn_close_pid(pid);
> +}
> +
> +GuestExec *qmp_guest_exec(const char *path,
> + bool has_arg, strList *arg,
> + bool has_env, strList *env,
> + bool has_inp_data, const char *inp_data,
> + bool has_capture_output, bool capture_output,
> + Error **err)
> +{
> + GPid pid;
> + GuestExec *ge = NULL;
> + GuestExecInfo *gei;
> + char **argv, **envp;
> + strList arglist;
> + gboolean ret;
> + GError *gerr = NULL;
> +
> + arglist.value = (char *)path;
> + arglist.next = has_arg ? arg : NULL;
> +
> + argv = guest_exec_get_args(&arglist, true);
> + envp = guest_exec_get_args(has_env ? env : NULL, false);
> +
> + ret = g_spawn_async_with_pipes(NULL, argv, envp,
> + G_SPAWN_SEARCH_PATH |
> + G_SPAWN_DO_NOT_REAP_CHILD |
> + G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL,
> + NULL, NULL, &pid, NULL, NULL, NULL, &gerr);
> + if (!ret) {
> + error_setg(err, QERR_QGA_COMMAND_FAILED, gerr->message);
> + g_error_free(gerr);
> + goto done;
> + }
> +
> + ge = g_malloc(sizeof(*ge));
> + ge->pid = (int64_t)pid;
> +
> + gei = guest_exec_info_add(pid);
> + g_child_watch_add(pid, guest_exec_child_watch, gei);
> +
> +done:
> + g_free(argv);
> + g_free(envp);
> +
> + return ge;
> +}
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 82894c6..ca9a633 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -930,3 +930,60 @@
> ##
> { 'command': 'guest-get-memory-block-info',
> 'returns': 'GuestMemoryBlockInfo' }
> +
> +##
> +# @guest-exec-status
> +#
> +# Check status of process associated with PID retrieved via guest-exec.
> +# Reap the process and associated metadata if it has exited.
> +#
> +# @pid: pid returned from guest-exec
> +#
> +# Returns: GuestExecStatus on success. If a child process exited, "exit" is
> set
> +# to the exit code. If a child process was killed by a signal,
> +# "signal" is set to the signal number. For Windows guest, "signal"
> is
> +# actually an unhandled exception code. If a child process is still
> +# running, both "exit" and "signal" are set to -1. If a guest cannot
> +# reliably detect exit signals, "signal" will be -1.
> +# 'out-data' and 'err-data' contains captured data when guest-exec
> was
> +# called with 'capture-output' flag.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'GuestExecStatus',
> + 'data': { 'exit': 'int', 'signal': 'int',
> + '*out-data': 'str', '*err-data': 'str' }}
This structure should be documented separately, with descriptions for
it's individual fields.
exit/signal are somewhat ambiguous in terms of the running status of the
process. I think we should have an explicit 'exited': bool that users
can check to determine whether that process is still running or not.
> +
> +{ 'command': 'guest-exec-status',
> + 'data': { 'pid': 'int' },
I would call this 'handle' since it aligns with the other interfaces and
gives us a bit more freedom if we decide not to expose actual
pids/HANDLEs.
> + 'returns': 'GuestExecStatus' }
> +
> +##
> +# @GuestExec:
> +# @pid: pid of child process in guest OS
> +#
> +#Since: 2.5
> +##
> +{ 'struct': 'GuestExec',
> + 'data': { 'pid': 'int'} }
Same here.
> +
> +##
> +# @guest-exec:
> +#
> +# Execute a command in the guest
> +#
> +# @path: path or executable name to execute
> +# @arg: #optional argument list to pass to executable
> +# @env: #optional environment variables to pass to executable
> +# @inp-data: #optional data to be passed to process stdin (base64 encoded)
> +# @capture-output: #optional bool flags to enable capture of
> +# stdout/stderr of running process
> +#
> +# Returns: PID on success.
Returns GuestExec on success
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'guest-exec',
> + 'data': { 'path': 'str', '*arg': ['str'], '*env': ['str'],
> + '*inp-data': 'str', '*capture-output': 'bool' },
> + 'returns': 'GuestExec' }
Would it make sense to just add handle (pid) to GuestExecStatus, and
have this return GuestExecStatus just the same as guest-exec-status
does? I'm not sure either way personally, just thought I'd mention it.
> --
> 2.1.4
>
