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

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

On 03/02/15 23:24, Michael Roth wrote:
> 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.
actually we are not bound 2.3 or 2.4 and there is enough
time to make code consistent. But I think that this functionality
is important because it should reach distros and this takes
additional time.

IMHO it would be better to merge something working and
after that reshape it in next release with a better implementation.

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

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