From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55900) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAyUD-0006Vc-Su for qemu-devel@nongnu.org; Tue, 13 Jan 2015 05:14:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YAyU8-0005FL-Qf for qemu-devel@nongnu.org; Tue, 13 Jan 2015 05:14:09 -0500 Received: from mx2.parallels.com ([199.115.105.18]:52430) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAyU8-0004oe-Ho for qemu-devel@nongnu.org; Tue, 13 Jan 2015 05:14:04 -0500 Message-ID: <54B4EFAF.9010400@openvz.org> Date: Tue, 13 Jan 2015 13:13:03 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1420031214-6053-1-git-send-email-den@openvz.org> <20150109170621.8810.42855@loki> <54B01946.8070404@openvz.org> <20150109192938.22996.67079@loki> In-Reply-To: <20150109192938.22996.67079@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 22:29, Michael Roth wrote: > Quoting Denis V. Lunev (2015-01-09 12:09:10) >> 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. > Yah, that was ultimately my reason for dropping the use-case, and just > having interactive/non-interactive rather than explicit control over > input/output pipes. But it's the one thing that havng guest-pipe-open > potentially worthwhile, so I think there's a good cause to drop that > interface and let guest-exec pass us the FDs if we agree it isn't > worth supporting. > >>> 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. > Ultimately I ended up with 2 interfaces, guest-exec-async, > which is implemented something like above, and guest-exec, > which simplifies the common case by executing synchronously > and simply allowing a timeout to be specified as an > argument. base64-encoded stdout/stderr buffer is subsequently > returned, and limited in size to 1M (user can redirect to > file in guest as an alternative). I think this handles > the vast amount of use-cases for 2), but for processes > that generate lots of output writing to a filesystem can > be problematic. And for that use case I don't think > polling is particularly an issue, performance wise. > so I have a hard time rationalizing away the need for a > guest-exec-async at some point, even if we don't support > leveraging it for interactive shells. > > guest->host events would be an interesting way to optimize > it though, but I'm okay with making polling necessary to > read from or reap a process, and adding that as a cue to > make things more efficient later while maintaining > compatibility with existing users/interfaces. > > The pipes are indeed tedious, which is why the added setup > of using guest-pipe-open was dropped in my implementation. > You still have to deal with reading/writing/closed to FDs > returned by guest-exec-async, but that's the core use-case > for that sort of interface I think. > >> 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. > Absolutely! > >> As for your approach, I would tend to agree with >> Eric. It is not safe. > I hadn't fully considered the matter of safety. I know shell > operators are santized/unsupported by the interface, but I do > suppose even basic command-line parsing can still be ambiguous > from one program to the next so perhaps we should avoid it on > that basis at the least. > >> Regards, >> Den OK, Michael, I am a little bit confused at the moment. What will be the next step and who is waiting whom? I think that the major architectural argument from your side is pointed out by Eric and should not be taken into account. The matter regarding "sync & async" exec types is still questionable and could be omitted at the moment. We will be able to extend the interface later. Guest notifications is a more interesting thing but it is much more difficult to implement. Therefore I think that the ball is on your side and we are waiting for a review from your side. Regards, Den