From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52631) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9e0u-0000mM-Ix for qemu-devel@nongnu.org; Fri, 09 Jan 2015 13:10:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y9e0q-0005qB-49 for qemu-devel@nongnu.org; Fri, 09 Jan 2015 13:10:24 -0500 Received: from mx2.parallels.com ([199.115.105.18]:51920) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9e0n-0005T5-Gc for qemu-devel@nongnu.org; Fri, 09 Jan 2015 13:10:20 -0500 Message-ID: <54B01946.8070404@openvz.org> Date: Fri, 9 Jan 2015 21:09:10 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1420031214-6053-1-git-send-email-den@openvz.org> <20150109170621.8810.42855@loki> In-Reply-To: <20150109170621.8810.42855@loki> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit 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: Michael Roth Cc: Olga Krishtal , qemu-devel@nongnu.org, Semen Zolin On 09/01/15 20:06, Michael Roth wrote: > 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. I do not think that this will be ever used. IMHO nobody will bind processes in such a way. In the worst case the user will exec something like 'sh -c "process1 | process2"'. This is better and shorter. > 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. Frankly speaking this exact interface is not that convenient for a real life products. The necessity to poll exec status and to poll input/output pipes is really boring and really affects the performance. Actually there are 2 major scenarios for this: 1. "Enter inside VM", i.e. the user obtains shell inside VM and works in VM from host f.e. to setup network 2. Execute command inside VM and collect output in host. For both case the proposed interface with guest pipes is not convenient, in order to obtain interactive shell in guest we should poll frequently (100 times in seconds to avoid lag) and in my experience end-users likes to run a lot of automation scripts using this channel. Thus we have used virtserial as a real transport for case 1) and it is working seamlessly. This means that we open virtserial in guest for input and output and connect to Unix socket in host with shell application. Poll of exec-status is necessary evil, but it does not affect interactivity and could be done once in a second. I would be good to have some notifications from guest that some events are pending (input/output data or exec status is available). But I do not know at the moment how to implement this. At least I have not invested resources into this as I would like to hear some opinion from the community first. This patchset is made mostly to initiate the discussion. Michael, could you spend a bit of time looking into patches 1 and 2 of the patchset. They have been implemented and reviewed by us and could be merged separately in advance. They provides functional equivalent of already existing Linux (Posix) functionality. As for your approach, I would tend to agree with Eric. It is not safe. Regards, Den