Re: [Qemu-devel] [PATCH 3/5] qga: guest exec functionality

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Denis V. Lunev (2015-10-01 02:38:01)
> From: Yuri Pudgorodskiy <yur@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 <yur@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@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
>