qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael Roth
Subject: Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
Date: Fri, 09 Jan 2015 13:29:38 -0600
User-agent: alot/0.3.4

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]