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
