Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux

[Michael Roth]

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 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.

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