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

[Michael Roth]

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.

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.

> 
> These patches add the following interfaces:
> 
> guest-pipe-open
> guest-exec
> guest-exec-status
> 
> With these interfaces it's possible to:
> 
> * Open an anonymous pipe and work with it as with a file using already
> implemented interfaces guest-file-{read,write,flush,close}.
> 
> * Execute a binary or a script on a guest machine.
> We can pass arbitrary arguments and environment to a new child process.
> 
> * Pass redirected IO from/to stdin, stdout, stderr from a child process to a
> local file or an anonymous pipe.
> 
> * Check the status of a child process, get its exit status if it exited or get
> signal number if it was killed.
> 
> We plan to add support for Windows in the near future using the same 
> interfaces
> we introduce here.
> 
> Example of usage:
> 
> {"execute": "guest-pipe-open", "arguments":{"mode": "r"}}
> {'return': 1000}
> 
> {"execute":"guest-exec", "arguments":{ "path": "id", "params": ["user"],
>     "env": ["MYENV=myvalue"], "handle_stdout": 1000 }}'
> {"return": 2636}
> 
> {"execute": "guest-exec-status", "arguments": {"pid": 2636}}
> {"return":{"exit":0,"handle_stderr":-1,"handle_stdin":-1,"handle_stdout":1000,"signal":-1}}
> 
> {"execute": "guest-file-read", "arguments": {"handle": 1000, "count":128}}
> {"return":{"count":58,"buf-b64":"dWlk...","eof":true}}
> 
> {"execute": "guest-file-close", "arguments": {"handle": 1000}}
> {"return":{}}
> 
> 
> These patches are based on the patches proposed by Michael Roth in 2011:
> http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg00722.html
> 
> We made several modifications to the interfaces proposed by Michael to 
> simplify
> their usage and we also added several new functions:
> 
> * Arguments to an executable file are passed as an array of strings.
> 
>     Before that we had to pass parameters like this:
>     'params': [ {'param': '-b'}, {'param': '-n1'} ]
> 
>     Now we may pass them just as an array of strings:
>     'params': [ '-b', '-n1' ]
> 
> * Environment can be passed to a child process.
> 
>     "env": ["MYENV=myvalue"]
> 
> * Removed "detach" argument from "guest-exec" - it never waits for a child
> process to signal.  With this change it's possible to return just PID from
> "guest-exec", a user must call "guest-exec-status" to get the status of a 
> child.
> 
> * Removed "wait" argument from "guest-exec-status" - waiting for a child 
> process
> to signal is dangerous, because it may block the whole qemu-ga.  Instead, the
> command polls just once a status of a child.
> 
> * "guest-exec-status" returns exit status of a child process or a signal 
> number,
> in case a process was killed.
> 
> * Simplified the command "guest-pipe-open" - the way how it stores the pipe
> fd's.  No additional logic is needed about which end of pipe should be closed
> with "guest-file-close".  This way we avoid modifying the interface of the
> existing "guest-file-close".
> 
> In the conversation about the original patches there was a suggestion to merge
> "path" and "params" into one single list.  But we didn't do so, because having
> separate "path" is similar to exec*() family functions in UNIX.  That way it
> looks more consistent.
> 
> Changes from v1:
> - Windows version of the patchset is added
> - SIGPIPE processing is added for Unix version of the patchset
> - added guest_exec_file_busy() to prevent GuestFileHandle* object from being
>   deleted in case it's used in guest-exec command.
> 
> Signed-off-by: Semen Zolin <address@hidden>
> Signed-off-by: Olga Krishtal <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>