From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59002) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIk8t-00058p-So for qemu-devel@nongnu.org; Tue, 03 Feb 2015 15:32:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIk8q-0000q5-1P for qemu-devel@nongnu.org; Tue, 03 Feb 2015 15:32:15 -0500 Received: from e34.co.us.ibm.com ([32.97.110.152]:43527) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIk8p-0000pk-Ry for qemu-devel@nongnu.org; Tue, 03 Feb 2015 15:32:11 -0500 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 3 Feb 2015 13:25:41 -0700 Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id CC46EC40027 for ; Tue, 3 Feb 2015 13:16:24 -0700 (MST) Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t13KPCtZ36241456 for ; Tue, 3 Feb 2015 13:25:12 -0700 Received: from d03av05.boulder.ibm.com (localhost [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t13KPCh9001591 for ; Tue, 3 Feb 2015 13:25:12 -0700 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <54B4EFAF.9010400@openvz.org> References: <1420031214-6053-1-git-send-email-den@openvz.org> <20150109170621.8810.42855@loki> <54B01946.8070404@openvz.org> <20150109192938.22996.67079@loki> <54B4EFAF.9010400@openvz.org> Message-ID: <20150203202448.6410.36840@loki> Date: Tue, 03 Feb 2015 14:24:48 -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-13 04:13:03) > 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 comman= ds on > >>>> a guest UNIX machine. > >>> Hi Denis, > >>> > >>> Glad to see these getting picked up. I did at some point hack up a re= write > >>> of the original code though which has some elements might be worth co= nsidering > >>> incorporating into your patchset. > >>> > >>> The main one is the use of g_spawn_async_with_pipes(), which wraps fo= rk/exec > >>> vs. CreateProcess() and allows for more shared code between posix/win= 32. > >>> > >>> It also creates the stdin/out/err FDs for you, which I suppose we cou= ld > >>> 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 dr= opping > >>> it since I can't imagine a use-case outside of guest-exec, but in doi= ng 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_pip= es() > >>> 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 st= ring, > >>> to parse it into arguments using g_shell_parse_argv(), which should a= lso > >>> be cross-platform. > >>> > >>> If you do things that the core exec code ends up looking something li= ke > >>> this: > >>> > >>> static GuestExecInfo *guest_exec_spawn(const char *cmdline, bool inte= ractive, > >>> 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, &fd_err, &gerr); > >>> if (gerr) { > >>> error_setg(errp, "failed to execute command: %s, %s", cmdli= ne, > >>> 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 && interactiv= e, 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, fal= se, 0, errp); > >>> if (error_is_set(errp)) { > >>> return NULL; > >>> } > >>> > >>> return ger; > >>> } > >>> > >>> Where GuestExecResponse takes the place of the original PID return, s= ince > >>> we now need to fetch the stdin/stdout/stderr handles as well. In my c= ode > >>> 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 initi= ally lost > >>> in an accidental wipe of /home so I currently only have vim backup fi= les 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. Sorry for the delay. The main outstanding issue I have from an architectural standpoint is whether we should consider using g_spawn_sync/g_spawn_async to avoid the need to maintain OS-specific variants of the exec support. I realize that's not a good argument in the face of real/working code, and I've been working on getting my g_spawn*()-based implementation cleaned up so we can make a decision based on that. If you're still targetting 2.3 (soft-freeze is Feb 16th) for guest-exec let me know and I can try to hurry things along on my end, but for now I'm assuming the guest-exec functionality is something we can pursue for 2.4. I have been testing the w32 guest-file-* patches and they seem to be in good shape for 2.3, so please repost those as a separate patchset and I'll work to get them into the next QGA pull. > = > 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