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: Denis V. Lunev
Subject: Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
Date: Tue, 13 Jan 2015 13:13:03 +0300
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 
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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]