From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35235) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9d17-0007gT-Nf for qemu-devel@nongnu.org; Fri, 09 Jan 2015 12:06:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y9d12-00054j-Ub for qemu-devel@nongnu.org; Fri, 09 Jan 2015 12:06:33 -0500 Received: from e38.co.us.ibm.com ([32.97.110.159]:53670) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9d12-00054N-Me for qemu-devel@nongnu.org; Fri, 09 Jan 2015 12:06:28 -0500 Received: from /spool/local by e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 9 Jan 2015 10:06:26 -0700 Received: from b03cxnp07029.gho.boulder.ibm.com (b03cxnp07029.gho.boulder.ibm.com [9.17.130.16]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 6604519D803D for ; Fri, 9 Jan 2015 09:55:01 -0700 (MST) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t09H6Oge54657096 for ; Fri, 9 Jan 2015 10:06:24 -0700 Received: from d03av02.boulder.ibm.com (localhost [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t09H6NwN010277 for ; Fri, 9 Jan 2015 10:06:23 -0700 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <1420031214-6053-1-git-send-email-den@openvz.org> References: <1420031214-6053-1-git-send-email-den@openvz.org> Message-ID: <20150109170621.8810.42855@loki> Date: Fri, 09 Jan 2015 11:06:21 -0600 Subject: Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Olga Krishtal , qemu-devel@nongnu.org, Semen Zolin 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 consider= ing 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 interactiv= e, Error **errp) { GSpawnFlags default_flags =3D G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP= _CHILD; gboolean ret; GPid gpid; gchar **argv; gint argc; GError *gerr =3D NULL; gint fd_in =3D -1, fd_out =3D -1, fd_err =3D -1; ret =3D 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 =3D g_spawn_async_with_pipes(NULL, argv, NULL, default_flags, NULL, NULL, &gpid, interactive ? &fd_in : NULL, &fd_out, &f= d_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 =3D guest_exec_spawn(cmdline, has_interactive && interactive, errp); if (error_is_set(errp)) { return NULL; } = print_gei(gei); ger =3D g_new0(GuestExecResponse, 1); = if (has_interactive && interactive) { ger->has_handle_stdin =3D true; ger->handle_stdin =3D guest_file_handle_add_fd(gei->fd_in, "a", errp); if (error_is_set(errp)) { return NULL; } } = ger->has_handle_stdout =3D true; ger->handle_stdout =3D guest_file_handle_add_fd(gei->fd_out, "r", errp); if (error_is_set(errp)) { return NULL; } = ger->has_handle_stderr =3D true; ger->handle_stderr =3D guest_file_handle_add_fd(gei->fd_err, "r", errp); if (error_is_set(errp)) { return NULL; } = handle =3D guest_exec_info_register(gei); ger->status =3D qmp_guest_exec_status(handle, false, false, false, 0, e= rrp); 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 l= ost 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 t= o a > local file or an anonymous pipe. > = > * Check the status of a child process, get its exit status if it exited o= r get > signal number if it was killed. > = > We plan to add support for Windows in the near future using the same inte= rfaces > 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=3Dmyvalue"], "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 si= mplify > 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=3Dmyvalue"] > = > * 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 pi= pe > fd's. No additional logic is needed about which end of pipe should be cl= osed > 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 h= aving > 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 be= ing > deleted in case it's used in guest-exec command. > = > Signed-off-by: Semen Zolin > Signed-off-by: Olga Krishtal > Signed-off-by: Denis V. Lunev