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