Quoting Denis V. Lunev (2014-12-31 07:06:46)
hese patches for guest-agent add the functionality to execute commands on
a guest UNIX machine.
Hi Denis,
Glad to see these getting picked up. I did at some point hack up a rewrite
of the original code though which has some elements might be worth considering
incorporating into your patchset.
The main one is the use of g_spawn_async_with_pipes(), which wraps fork/exec
vs. CreateProcess() and allows for more shared code between posix/win32.
It also creates the stdin/out/err FDs for you, which I suppose we could
do ourselves manually here as well, but it also raises the question of
whether guest-pipe-open is really necessary. In my code I ended up dropping
it since I can't imagine a use-case outside of guest-exec, but in doing so
also I dropped the ability to pipe one process into another...
But thinking about it more I think you can still pipe one process into
another by dup2()'ing the stdin FD returned by g_spawn_async_with_pipes()
to whatever stdout/stderr you wish to specify from a previous process.
The other one worth considering is allowing cmdline to simply be a string,
to parse it into arguments using g_shell_parse_argv(), which should also
be cross-platform.
If you do things that the core exec code ends up looking something like
this:
static GuestExecInfo *guest_exec_spawn(const char *cmdline, bool interactive,
Error **errp)
{
GSpawnFlags default_flags = G_SPAWN_SEARCH_PATH |
G_SPAWN_DO_NOT_REAP_CHILD;
gboolean ret;
GPid gpid;
gchar **argv;
gint argc;
GError *gerr = NULL;
gint fd_in = -1, fd_out = -1, fd_err = -1;
ret = g_shell_parse_argv(cmdline, &argc, &argv, &gerr);
if (!ret || gerr) {
error_setg(errp, "failed to parse command: %s, %s", cmdline,
gerr->message);
return NULL;
}
ret = g_spawn_async_with_pipes(NULL, argv, NULL,
default_flags, NULL, NULL, &gpid,
interactive ? &fd_in : NULL, &fd_out, &fd_err,
&gerr);
if (gerr) {
error_setg(errp, "failed to execute command: %s, %s", cmdline,
gerr->message);
return NULL;
}
if (!ret) {
error_setg(errp, "failed to execute command");
return NULL;
}
return guest_exec_info_new(gpid, cmdline, fd_in, fd_out, fd_err);
}
GuestExecResponse *qmp_guest_exec_async(const char *cmdline,
bool has_interactive,
bool interactive,
Error **errp)
{
GuestExecResponse *ger;
GuestExecInfo *gei;
int32_t handle;
gei = guest_exec_spawn(cmdline, has_interactive && interactive, errp);
if (error_is_set(errp)) {
return NULL;
}
print_gei(gei);
ger = g_new0(GuestExecResponse, 1);
if (has_interactive && interactive) {
ger->has_handle_stdin = true;
ger->handle_stdin =
guest_file_handle_add_fd(gei->fd_in, "a", errp);
if (error_is_set(errp)) {
return NULL;
}
}
ger->has_handle_stdout = true;
ger->handle_stdout =
guest_file_handle_add_fd(gei->fd_out, "r", errp);
if (error_is_set(errp)) {
return NULL;
}
ger->has_handle_stderr = true;
ger->handle_stderr =
guest_file_handle_add_fd(gei->fd_err, "r", errp);
if (error_is_set(errp)) {
return NULL;
}
handle = guest_exec_info_register(gei);
ger->status = qmp_guest_exec_status(handle, false, false, false, 0, errp);
if (error_is_set(errp)) {
return NULL;
}
return ger;
}
Where GuestExecResponse takes the place of the original PID return, since
we now need to fetch the stdin/stdout/stderr handles as well. In my code
it was defined as:
{ 'type': 'GuestExecResponse',
'data': { 'status': 'GuestExecStatus',
'*handle-stdin': 'int', '*handle-stdout': 'int',
'*handle-stderr': 'int' } }
Sorry for not just posting the patchset somewhere, the code was initially lost
in an accidental wipe of /home so I currently only have vim backup files to
piece it together from atm.