Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests

[Michael Roth]

Quoting Denis V. Lunev (2015-06-30 05:25:19)
> From: Olga Krishtal <address@hidden>
> 
> Child process' stdin/stdout/stderr can be associated
> with handles for communication via read/write interfaces.
> 
> The workflow should be something like this:
> * Open an anonymous pipe through guest-pipe-open
> * Execute a binary or a script in the guest. Arbitrary arguments and
>   environment to a new child process could be passed through options
> * Read/pass information from/to executed process using
>   guest-file-read/write
> * Collect the status of a child process

Have you seen anything like this in your testing?

{'execute':'guest-exec','arguments':{'path':'/Windows/System32/ipconfig.exe',
 'timeout':5000}}
{"return": {"pid": 588}}
{'execute':'guest-exec-status','arguments':{'pid':588}}
{"return": {"exit": 0, "handle-stdout": -1, "handle-stderr": -1,
 "handle-stdin": -1, "signal": -1}}
{'execute':'guest-exec-status','arguments':{'pid':588}}
{"error": {"class": "GenericError", "desc": "Invalid parameter 'pid'"}}

{'execute':'guest-exec','arguments':{'path':'/Windows/System32/ipconfig.exe',
 'timeout':5000}}
{"error": {"class": "GenericError", "desc": "CreateProcessW() failed:
 The parameter is incorrect. (error: 57)"}}
{'execute':'guest-exec','arguments':{'path':'/Windows/System32/ipconfig.exe',
 'timeout':5000}}
{"error": {"class": "GenericError", "desc": "CreateProcessW() failed:
 The parameter is incorrect. (error: 57)"}}

{'execute':'guest-exec','arguments':{'path':'/Windows/System32/ipconfig.exe',
 'timeout':5000}}
{"return": {"pid": 1836}}

The guest-exec-status failures are expected since the first call reaps
everything, but the CreateProcessW() failures are not. Will look into it
more this evening, but it doesn't look like I'll be able to apply this in
it's current state.

I have concerns over the schema as well. I think last time we discussed
it we both seemed to agree that guest-file-open was unwieldy and
unnecessary. We should just let guest-exec return a set of file handles
instead of having users do all the plumbing.

I'm really sorry for chiming in right before hard freeze, very poor
timing/planning on my part.

Will look at the fs/pci info patches tonight.

> 
> Signed-off-by: Olga Krishtal <address@hidden>
> Acked-by: Roman Kagan <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Eric Blake <address@hidden>
> CC: Michael Roth <address@hidden>
> ---
>  qga/commands-win32.c | 309 
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 303 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 435a049..ad445d9 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -451,10 +451,231 @@ static void guest_file_init(void)
>      QTAILQ_INIT(&guest_file_state.filehandles);
>  }
> 
> +
> +typedef struct GuestExecInfo {
> +    int pid;
> +    HANDLE phandle;
> +    GuestFileHandle *gfh_stdin;
> +    GuestFileHandle *gfh_stdout;
> +    GuestFileHandle *gfh_stderr;
> +    QTAILQ_ENTRY(GuestExecInfo) next;
> +} GuestExecInfo;
> +
> +static struct {
> +    QTAILQ_HEAD(, GuestExecInfo) processes;
> +} guest_exec_state;
> +
> +static void guest_exec_init(void)
> +{
> +    QTAILQ_INIT(&guest_exec_state.processes);
> +}
> +
> +static void guest_exec_info_add(int pid, HANDLE phandle,
> +                                GuestFileHandle *in, GuestFileHandle *out,
> +                                GuestFileHandle *error)
> +{
> +    GuestExecInfo *gei;
> +
> +    gei = g_malloc0(sizeof(GuestExecInfo));
> +    gei->pid = pid;
> +    gei->phandle = phandle;
> +    gei->gfh_stdin = in;
> +    gei->gfh_stdout = out;
> +    gei->gfh_stderr = error;
> +    QTAILQ_INSERT_TAIL(&guest_exec_state.processes, gei, next);
> +}
> +
> +static GuestExecInfo *guest_exec_info_find(int64_t pid)
> +{
> +    GuestExecInfo *gei;
> +
> +    QTAILQ_FOREACH(gei, &guest_exec_state.processes, next) {
> +        if (gei->pid == pid) {
> +            return gei;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return 0;
> +    GuestExecInfo *gei;
> +    GuestExecStatus *ges;
> +    int r;
> +    DWORD exit_code;
> +
> +    slog("guest-exec-status called, pid: %" PRId64, pid);
> +
> +    gei = guest_exec_info_find(pid);
> +    if (gei == NULL) {
> +        error_setg(errp, QERR_INVALID_PARAMETER, "pid");
> +        return NULL;
> +    }
> +
> +    r = WaitForSingleObject(gei->phandle, 0);
> +    if (r != WAIT_OBJECT_0 && r != WAIT_TIMEOUT) {
> +        error_setg_win32(errp, GetLastError(),
> +                         "WaitForSingleObject() failed, pid: %u", gei->pid);
> +        return NULL;
> +    }
> +
> +    ges = g_malloc0(sizeof(GuestExecStatus));
> +    ges->handle_stdin = (gei->gfh_stdin != NULL) ? gei->gfh_stdin->id : -1;
> +    ges->handle_stdout = (gei->gfh_stdout != NULL) ? gei->gfh_stdout->id : 
> -1;
> +    ges->handle_stderr = (gei->gfh_stderr != NULL) ? gei->gfh_stderr->id : 
> -1;
> +    ges->exit = -1;
> +    ges->signal = -1;
> +
> +    if (r == WAIT_OBJECT_0) {
> +        GetExitCodeProcess(gei->phandle, &exit_code);
> +        CloseHandle(gei->phandle);
> +
> +        ges->exit = (int)exit_code;
> +
> +        QTAILQ_REMOVE(&guest_exec_state.processes, gei, next);
> +        g_free(gei);
> +    }
> +
> +    return ges;
> +}
> +
> +/* Convert UTF-8 to wide string. */
> +#define utf8_to_ucs2(dst, dst_cap, src, src_len) \
> +    MultiByteToWideChar(CP_UTF8, 0, src, (int)(src_len), dst, (int)(dst_cap))
> +
> +/* Get command-line arguments for CreateProcess().
> + * Path or arguments containing double quotes are prohibited.
> + * Arguments containing spaces are enclosed in double quotes.
> + * @wpath: @path that was converted to wchar.
> + * @argv_str: arguments in one line separated by space. */
> +static WCHAR *guest_exec_get_args(const char *path, WCHAR **wpath,
> +                                  const strList *params, Error **errp)
> +{
> +    const strList *it;
> +    bool with_spaces;
> +    size_t cap = 0;
> +    char *argv_str;
> +    WCHAR *wargs;
> +    char *pargv;
> +
> +    if (strchr(path, '"') != NULL) {
> +        error_setg(errp, "path or arguments can't contain \" quotes");
> +        return NULL;
> +    }
> +
> +    for (it = params; it != NULL; it = it->next) {
> +        if (strchr(it->value, '"') != NULL) {
> +            error_setg(errp, "path or arguments can't contain \" quotes");
> +            return NULL;
> +        }
> +    }
> +
> +    cap += strlen(path) + sizeof("\"\"") - 1;
> +    for (it = params; it != NULL; it = it->next) {
> +        cap += strlen(it->value) + sizeof(" \"\"") - 1;
> +    }
> +    cap++;
> +
> +    argv_str = g_malloc(cap);
> +    pargv = argv_str;
> +
> +    *pargv++ = '"';
> +    pstrcpy(pargv, (pargv + cap) - pargv, path);
> +    *pargv++ = '"';
> +
> +    for (it = params; it != NULL; it = it->next) {
> +        with_spaces = (strchr(it->value, ' ') != NULL);
> +
> +        *pargv++ = ' ';
> +
> +        if (with_spaces) {
> +            *pargv++ = '"';
> +        }
> +
> +        pstrcpy(pargv, (pargv + cap) - pargv, it->value);
> +        pargv += strlen(it->value);
> +
> +        if (with_spaces) {
> +            *pargv++ = '"';
> +        }
> +    }
> +    *pargv = '\0';
> +
> +    wargs = g_malloc(cap * sizeof(WCHAR));
> +    if (utf8_to_ucs2(wargs, cap, argv_str, -1) == 0) {
> +        goto fail;
> +    }
> +
> +    cap = strlen(path) + 1;
> +    *wpath = g_malloc(cap * sizeof(WCHAR));
> +    if (utf8_to_ucs2(*wpath, cap, path, -1) == 0) {
> +        g_free(*wpath);
> +        goto fail;
> +    }
> +    slog("guest-exec called: %s", argv_str);
> +    g_free(argv_str);
> +    return wargs;
> +
> +fail:
> +    error_setg_win32(errp, GetLastError(), "MultiByteToWideChar() failed");
> +    g_free(argv_str);
> +    g_free(wargs);
> +    return NULL;
> +}
> +
> +/* Prepare environment string for CreateProcess(). */
> +static WCHAR *guest_exec_get_env(strList *env, Error **errp)
> +{
> +    const strList *it;
> +    size_t r, cap = 0;
> +    WCHAR *wenv, *pwenv;
> +
> +    for (it = env; it != NULL; it = it->next) {
> +        cap += strlen(it->value) + 1;
> +    }
> +    cap++;
> +
> +    wenv = g_malloc(cap * sizeof(WCHAR));
> +    pwenv = wenv;
> +
> +    for (it = env; it != NULL; it = it->next) {
> +        r = utf8_to_ucs2(pwenv, (wenv + cap) - pwenv, it->value, -1);
> +        if (r == 0) {
> +            error_setg_win32(errp, GetLastError(),
> +                             "MultiByteToWideChar() failed");
> +            g_free(wenv);
> +            return NULL;
> +        }
> +        pwenv += r - 1;
> +
> +        *pwenv++ = L'\0';
> +    }
> +    *pwenv = L'\0';
> +
> +    return wenv;
> +}
> +
> +static HANDLE guest_exec_get_stdhandle(GuestFileHandle *gfh)
> +{
> +    HANDLE fd;
> +
> +    if (gfh == NULL) {
> +        return INVALID_HANDLE_VALUE;
> +    }
> +
> +    if (gfh->pipe_other_end_fd != INVALID_HANDLE_VALUE) {
> +        fd = gfh->pipe_other_end_fd;
> +    } else {
> +        fd = gfh->fh;
> +    }
> +
> +    if (!SetHandleInformation(fd, HANDLE_FLAG_INHERIT, 1)) {
> +        slog("guest-exec: SetHandleInformation() failed to set inherit flag: 
> "
> +             "%lu", GetLastError());
> +    }
> +
> +    return fd;
>  }
> 
>  GuestExec *qmp_guest_exec(const char *path,
> @@ -465,8 +686,84 @@ GuestExec *qmp_guest_exec(const char *path,
>                         bool has_handle_stderr, int64_t handle_stderr,
>                         Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return 0;
> +    int64_t pid = -1;
> +    GuestExec *ge = NULL;
> +    BOOL b;
> +    PROCESS_INFORMATION info;
> +    STARTUPINFOW si = {0};
> +    WCHAR *wpath = NULL, *wargs, *wenv = NULL;
> +    GuestFileHandle *gfh_stdin = NULL, *gfh_stdout = NULL, *gfh_stderr = 
> NULL;
> +
> +    wargs = guest_exec_get_args(path, &wpath, has_params ? params : NULL, 
> errp);
> +    wenv = guest_exec_get_env(has_env ? env : NULL, errp);
> +    if (wargs == NULL || wenv == NULL) {
> +        return NULL;
> +    }
> +
> +    if (has_handle_stdin) {
> +        gfh_stdin = guest_file_handle_find(handle_stdin, errp);
> +        if (gfh_stdin == NULL) {
> +            goto done;
> +        }
> +    }
> +
> +    if (has_handle_stdout) {
> +        gfh_stdout = guest_file_handle_find(handle_stdout, errp);
> +        if (gfh_stdout == NULL) {
> +            goto done;
> +        }
> +    }
> +
> +    if (has_handle_stderr) {
> +        gfh_stderr = guest_file_handle_find(handle_stderr, errp);
> +        if (gfh_stderr == NULL) {
> +            goto done;
> +        }
> +    }
> +
> +    si.cb = sizeof(STARTUPINFOW);
> +
> +    if (has_handle_stdin || has_handle_stdout || has_handle_stderr) {
> +        si.dwFlags = STARTF_USESTDHANDLES;
> +
> +        si.hStdInput = guest_exec_get_stdhandle(gfh_stdin);
> +        si.hStdOutput = guest_exec_get_stdhandle(gfh_stdout);
> +        si.hStdError = guest_exec_get_stdhandle(gfh_stderr);
> +    }
> +
> +    b = CreateProcessW(wpath, wargs, NULL, NULL, 1 /*inherit handles*/,
> +                       CREATE_UNICODE_ENVIRONMENT | DETACHED_PROCESS,
> +                       wenv, NULL /*inherited current dir*/, &si, &info);
> +    if (!b) {
> +        error_setg_win32(errp, GetLastError(),
> +                         "CreateProcessW() failed");
> +        goto done;
> +    }
> +
> +    if (gfh_stdin != NULL && guest_pipe_close_other_end(gfh_stdin) != 0) {
> +        slog("failed to close stdin pipe handle. error: %lu", 
> GetLastError());
> +    }
> +
> +    if (gfh_stdout != NULL && guest_pipe_close_other_end(gfh_stdout) != 0) {
> +        slog("failed to close stdout pipe handle. error: %lu", 
> GetLastError());
> +    }
> +
> +    if (gfh_stderr != NULL && guest_pipe_close_other_end(gfh_stderr) != 0) {
> +        slog("failed to close stderr pipe handle. error: %lu", 
> GetLastError());
> +    }
> +
> +    CloseHandle(info.hThread);
> +    guest_exec_info_add(info.dwProcessId, info.hProcess, gfh_stdin, 
> gfh_stdout,
> +                        gfh_stderr);
> +    pid = info.dwProcessId;
> +    ge = g_malloc(sizeof(*ge));
> +    ge->pid = pid;
> +
> +done:
> +    g_free(wpath);
> +    g_free(wargs);
> +    g_free(wenv);
> +    return ge;
>  }
> 
>  GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
> @@ -800,8 +1097,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
>          "guest-get-memory-blocks", "guest-set-memory-blocks",
>          "guest-get-memory-block-size",
>          "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
> -        "guest-fstrim", "guest-exec-status",
> -        "guest-exec", NULL};
> +        "guest-fstrim", NULL};
>      char **p = (char **)list_unsupported;
> 
>      while (*p) {
> @@ -830,4 +1126,5 @@ void ga_command_state_init(GAState *s, GACommandState 
> *cs)
>          ga_command_state_add(cs, NULL, guest_fsfreeze_cleanup);
>      }
>      ga_command_state_add(cs, guest_file_init, NULL);
> +    ga_command_state_add(cs, guest_exec_init, NULL);
>  }
> -- 
> 2.1.4
>