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

["Denis V. Lunev" <den@openvz.org>]

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