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

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

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.

As for your approach, I would tend to agree with
Eric. It is not safe.

Regards,
     Den