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.
>
> These patches add the following interfaces:
>
> guest-pipe-open
> guest-exec
> guest-exec-status
>
> With these interfaces it's possible to:
>
> * Open an anonymous pipe and work with it as with a file using already
> implemented interfaces guest-file-{read,write,flush,close}.
>
> * Execute a binary or a script on a guest machine.
> We can pass arbitrary arguments and environment to a new child process.
>
> * Pass redirected IO from/to stdin, stdout, stderr from a child process to a
> local file or an anonymous pipe.
>
> * Check the status of a child process, get its exit status if it exited or get
> signal number if it was killed.
>
> We plan to add support for Windows in the near future using the same
> interfaces
> we introduce here.
>
> Example of usage:
>
> {"execute": "guest-pipe-open", "arguments":{"mode": "r"}}
> {'return': 1000}
>
> {"execute":"guest-exec", "arguments":{ "path": "id", "params": ["user"],
> "env": ["MYENV=myvalue"], "handle_stdout": 1000 }}'
> {"return": 2636}
>
> {"execute": "guest-exec-status", "arguments": {"pid": 2636}}
> {"return":{"exit":0,"handle_stderr":-1,"handle_stdin":-1,"handle_stdout":1000,"signal":-1}}
>
> {"execute": "guest-file-read", "arguments": {"handle": 1000, "count":128}}
> {"return":{"count":58,"buf-b64":"dWlk...","eof":true}}
>
> {"execute": "guest-file-close", "arguments": {"handle": 1000}}
> {"return":{}}
>
>
> These patches are based on the patches proposed by Michael Roth in 2011:
> http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg00722.html
>
> We made several modifications to the interfaces proposed by Michael to
> simplify
> their usage and we also added several new functions:
>
> * Arguments to an executable file are passed as an array of strings.
>
> Before that we had to pass parameters like this:
> 'params': [ {'param': '-b'}, {'param': '-n1'} ]
>
> Now we may pass them just as an array of strings:
> 'params': [ '-b', '-n1' ]
>
> * Environment can be passed to a child process.
>
> "env": ["MYENV=myvalue"]
>
> * Removed "detach" argument from "guest-exec" - it never waits for a child
> process to signal. With this change it's possible to return just PID from
> "guest-exec", a user must call "guest-exec-status" to get the status of a
> child.
>
> * Removed "wait" argument from "guest-exec-status" - waiting for a child
> process
> to signal is dangerous, because it may block the whole qemu-ga. Instead, the
> command polls just once a status of a child.
>
> * "guest-exec-status" returns exit status of a child process or a signal
> number,
> in case a process was killed.
>
> * Simplified the command "guest-pipe-open" - the way how it stores the pipe
> fd's. No additional logic is needed about which end of pipe should be closed
> with "guest-file-close". This way we avoid modifying the interface of the
> existing "guest-file-close".
>
> In the conversation about the original patches there was a suggestion to merge
> "path" and "params" into one single list. But we didn't do so, because having
> separate "path" is similar to exec*() family functions in UNIX. That way it
> looks more consistent.
>
> Changes from v1:
> - Windows version of the patchset is added
> - SIGPIPE processing is added for Unix version of the patchset
> - added guest_exec_file_busy() to prevent GuestFileHandle* object from being
> deleted in case it's used in guest-exec command.
>
> Signed-off-by: Semen Zolin <szo...@parallels.com>
> Signed-off-by: Olga Krishtal <okrish...@parallels.com>
> Signed-off-by: Denis V. Lunev <d...@openvz.org>
