Message ID | 1443685083-6242-4-git-send-email-den@openvz.org |
---|---|
State | New |
Headers | show |
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 >
On 10/2/2015 1:59 AM, Michael Roth wrote: >> +# >> +# 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. > I do not think it's a good idea. GuestExecStatus contains mostly the data about the finished exec. Having GuestExec returns same struct may make wrong assumption that it can be synchronous - wait for exec to complete and return all data in a single call. Implementing synchronous GuestExec is not and easy job - either we occupy host-guest channel for all time until task finished, which is bad or we need to implement multiplexed messages for concurrent qga commands.
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->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) { + 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' }} + +{ 'command': 'guest-exec-status', + 'data': { 'pid': 'int' }, + 'returns': 'GuestExecStatus' } + +## +# @GuestExec: +# @pid: pid of child process in guest OS +# +#Since: 2.5 +## +{ 'struct': 'GuestExec', + 'data': { 'pid': 'int'} } + +## +# @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. +# +# Since: 2.5 +## +{ 'command': 'guest-exec', + 'data': { 'path': 'str', '*arg': ['str'], '*env': ['str'], + '*inp-data': 'str', '*capture-output': 'bool' }, + 'returns': 'GuestExec' }