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

["Denis V. Lunev" <den@openvz.org>]

On 07/07/15 04:31, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-06-30 05:25:19)
>> From: Olga Krishtal <okrishtal@virtuozzo.com>
>>
>> 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}}
I'll check this later during office time. Something definitely went wrong.

> 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.
no, the discussion was a bit different AFAIR. First of all, you have 
proposed
to use unified code to perform exec. On the other hand current mechanics
with pipes is quite inconvenient for end-users of the feature for example
for interactive shell in the guest.

We have used very simple approach for our application: pipes are not
used, the application creates VirtIO serial channel and forces guest through
this API to fork/exec the child using this serial as a stdio in/out. In this
case we do receive a convenient API for shell processing.

This means that this flexibility with direct specification of the file
descriptors is necessary.

There are two solutions from my point of view:
- keep current API, it is suitable for us
- switch to "pipe only" mechanics for guest exec, i.e. the command
    will work like "ssh" with one descriptor for read and one for write
    created automatically, but in this case we do need either a way
    to connect Unix socket in host with file descriptor in guest or
    make possibility to send events from QGA to client using QMP

> I'm really sorry for chiming in right before hard freeze, very poor
> timing/planning on my part.
:( can we somehow schedule this better next time? This functionality
is mandatory for us and we can not afford to drop it or forget about
it for long. There was no pressure in winter but now I am on a hard
pressure. Thus can we at least agree on API terms and come to an
agreement?

> Will look at the fs/pci info patches tonight.
>
>> Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
>> Acked-by: Roman Kagan <rkagan@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>>   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
>>