From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38594) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9fFm-0003NU-Kx for qemu-devel@nongnu.org; Fri, 09 Jan 2015 14:29:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y9fFj-0003F8-KR for qemu-devel@nongnu.org; Fri, 09 Jan 2015 14:29:50 -0500 Received: from e39.co.us.ibm.com ([32.97.110.160]:52556) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9fFj-0003Er-A1 for qemu-devel@nongnu.org; Fri, 09 Jan 2015 14:29:47 -0500 Received: from /spool/local by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 9 Jan 2015 12:29:44 -0700 Received: from b01cxnp22035.gho.pok.ibm.com (b01cxnp22035.gho.pok.ibm.com [9.57.198.25]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 05BD86E803C for ; Fri, 9 Jan 2015 14:21:35 -0500 (EST) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by b01cxnp22035.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t09JTfcY26411012 for ; Fri, 9 Jan 2015 19:29:41 GMT Received: from d01av04.pok.ibm.com (localhost [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t09JTfWo007374 for ; Fri, 9 Jan 2015 14:29:41 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <54B01946.8070404@openvz.org> References: <1420031214-6053-1-git-send-email-den@openvz.org> <20150109170621.8810.42855@loki> <54B01946.8070404@openvz.org> Message-ID: <20150109192938.22996.67079@loki> Date: Fri, 09 Jan 2015 13:29:38 -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 (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 rewr= ite > > of the original code though which has some elements might be worth cons= idering > > 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 drop= ping > > 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 stri= ng, > > 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 intera= ctive, > > 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_ou= t, &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 =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, errp); > > if (error_is_set(errp)) { > > return NULL; > > } > > > > return ger; > > } > > > > Where GuestExecResponse takes the place of the original PID return, sin= ce > > 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 initial= ly lost > > in an accidental wipe of /home so I currently only have vim backup file= s 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