[06/10] qga: guest exec functionality for Windows guests

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

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(-)

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}}

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 <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
>

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
>>

On 07/07/15 11:06, Denis V. Lunev wrote:
> 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)"}}
First if all what version of Windows are you using?
Secondly, you do need to specify environmental variable:
sudo virsh qemu-agent-command w2k12r2 
'{"execute":"guest-exec","arguments":{"path":"/Windows/System32/ipconfig.exe", 
"timeout": 5000, "env":["MyEnv=00"]}' :
For Windows Server 2003 we do not have to pass "env" at all, but if we 
are working with Server 2008 and older we have to pass "env" = "00" if 
we do not want to use it. 
https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/ 
59450592-aa52-4170-9742-63c84bff0010/unexpected-errorinvalidparameter 
-returned-by-createprocess-too-bad?forum=windowsgeneraldevelopmentissues
This comment where included in first version of patches and I may have 
forgotten it. Try to specify env and call exec several times. It should 
work fine.
I will look closer at guest-exec-status double call.


>>
>> {'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
>>>
>

On 07/07/15 12:12, Olga Krishtal wrote:
> On 07/07/15 11:06, Denis V. Lunev wrote:
>> 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)"}}
> First if all what version of Windows are you using?
> Secondly, you do need to specify environmental variable:
> sudo virsh qemu-agent-command w2k12r2 
> '{"execute":"guest-exec","arguments":{"path":"/Windows/System32/ipconfig.exe", 
> "timeout": 5000, "env":["MyEnv=00"]}' :

Argh.... I have missed this fact during internal discussion and review.
For sure this should be passed to the client. I think that it would
be better to add this automatically to the environment variables
passed to the exec arguments.

> For Windows Server 2003 we do not have to pass "env" at all, but if we 
> are working with Server 2008 and older we have to pass "env" = "00" if 
> we do not want to use it. 
> https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/ 
> 59450592-aa52-4170-9742-63c84bff0010/unexpected-errorinvalidparameter 
> -returned-by-createprocess-too-bad?forum=windowsgeneraldevelopmentissues
> This comment where included in first version of patches and I may have 
> forgotten it. Try to specify env and call exec several times. It 
> should work fine.
> I will look closer at guest-exec-status double call.
>
>

On 07/07/15 12:12, Olga Krishtal wrote:
> On 07/07/15 11:06, Denis V. Lunev wrote:
>> 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'"}}
I tracked this execution -  it is absolutely normal, because in options 
of ipconfig.exe there is no timeout argument,
as I saw using ipconfig -h in cmd.
However, if we use smth like calc.exe and call exec status twice - the 
output will be normal:
sudo virsh qemu-agent-command w2k12r2 
'{"execute":"guest-exec-status","arguments":{"pid":2840}}'
setlocale: No such file or directory
{"return":{"exit":-1,"handle-stdout":-1,"handle-stderr":-1,"handle-stdin":-1,"signal":-1}}

  sudo virsh qemu-agent-command w2k12r2 
'{"execute":"guest-exec-status","arguments":{"pid":2840}}'
setlocale: No such file or directory
{"return":{"exit":-1,"handle-stdout":-1,"handle-stderr":-1,"handle-stdin":-1,"signal":-1}}
>>>
>>> {'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)"}}
> First if all what version of Windows are you using?
> Secondly, you do need to specify environmental variable:
> sudo virsh qemu-agent-command w2k12r2 
> '{"execute":"guest-exec","arguments":{"path":"/Windows/System32/ipconfig.exe", 
> "timeout": 5000, "env":["MyEnv=00"]}' :
> For Windows Server 2003 we do not have to pass "env" at all, but if we 
> are working with Server 2008 and older we have to pass "env" = "00" if 
> we do not want to use it. 
> https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/ 
> 59450592-aa52-4170-9742-63c84bff0010/unexpected-errorinvalidparameter 
> -returned-by-createprocess-too-bad?forum=windowsgeneraldevelopmentissues
> This comment where included in first version of patches and I may have 
> forgotten it. Try to specify env and call exec several times. It 
> should work fine.
> I will look closer at guest-exec-status double call.
>
>
>>>
>>> {'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
>>>>
>>
>

Quoting Denis V. Lunev (2015-07-07 03:06:08)
> 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.

But if I'm understanding things correctly, you're simply *not* using the
guest-pipe-* interfaces in this case, and it's just a matter of having
the option of not overriding the child's stdio? We wouldn't
necessarilly lose that flexibility if we dropped guest-pipe-*,
specifying whether we want to wire qemu-ga into stdin/stdout could
still be done via options to guest-exec.

Do you have an example of the sort of invocation you're doing?

> 
> 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?

Yes, absolutely. Let's get the API down early and hopefully we can
get it all merged early this time.

On 09/07/15 01:02, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-07-07 03:06:08)
>> 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.
> But if I'm understanding things correctly, you're simply *not* using the
> guest-pipe-* interfaces in this case, and it's just a matter of having
> the option of not overriding the child's stdio? We wouldn't
> necessarilly lose that flexibility if we dropped guest-pipe-*,
> specifying whether we want to wire qemu-ga into stdin/stdout could
> still be done via options to guest-exec.
>
> Do you have an example of the sort of invocation you're doing?
>
>> 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?
> Yes, absolutely. Let's get the API down early and hopefully we can
> get it all merged early this time.
>
I have attached entire C program, which allows to obtain interactive (test)
shell in guest.

actually we perform
"{\"execute\": \"guest-file-open\", \"arguments\":{\"path\":\"%s\", 
\"mode\":\"r+b\"}}"
and after that
                 "{\"execute\":\"guest-exec\", 
\"arguments\":{\"path\":\"/bin/sh\","
"\"handle_stdin\":%u,\"handle_stdout\":%u,\"handle_stderr\":%u }}",
                 ctx.guest_io_handle, ctx.guest_io_handle, 
ctx.guest_io_handle);
with the handle obtained above.

Den

Quoting Denis V. Lunev (2015-07-08 17:47:51)
> On 09/07/15 01:02, Michael Roth wrote:
> > Quoting Denis V. Lunev (2015-07-07 03:06:08)
> >> 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.
> > But if I'm understanding things correctly, you're simply *not* using the
> > guest-pipe-* interfaces in this case, and it's just a matter of having
> > the option of not overriding the child's stdio? We wouldn't
> > necessarilly lose that flexibility if we dropped guest-pipe-*,
> > specifying whether we want to wire qemu-ga into stdin/stdout could
> > still be done via options to guest-exec.
> >
> > Do you have an example of the sort of invocation you're doing?
> >
> >> 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?
> > Yes, absolutely. Let's get the API down early and hopefully we can
> > get it all merged early this time.
> >
> I have attached entire C program, which allows to obtain interactive (test)
> shell in guest.
> 
> actually we perform
> "{\"execute\": \"guest-file-open\", \"arguments\":{\"path\":\"%s\", 
> \"mode\":\"r+b\"}}"
> and after that
>                  "{\"execute\":\"guest-exec\", 
> \"arguments\":{\"path\":\"/bin/sh\","
> "\"handle_stdin\":%u,\"handle_stdout\":%u,\"handle_stderr\":%u }}",
>                  ctx.guest_io_handle, ctx.guest_io_handle, 
> ctx.guest_io_handle);
> with the handle obtained above.

With even the bare-minimum API we could still do something like:

  cmd: '/bash/sh'
  arg0: '-c'
  arg1: 'sh <virt-serialpath >virt-serialpath'

From the qga perspective I think we'd just end up with empty
stdio for the life of both the parent 'sh' and child 'sh'.

As a general use case this is probably a bit worse, since we're
farming work out to a specific shell implementation instead of
figuring out a platform-agnostic API for doing it.

But if we consider this to be a niche use case then taking this
approach gives us a little more flexibility for simplifying the
API.

But I'm not really against being able to supply stdio/stdout/stderr
through the API. The main thing is that, by default, guest-exec
should just supply you with a set of pipes automatically.
This one thing let's us drop guest-pipe-open completely.

And it's just like with a normal shell: you get stdio by
default, and can redirect as needed. You don't have to do

  <cmd> </dev/stdin >/dev/stdout

unless you're explicitly wanting to redirect somewhere other
than the defaults.

> 
> Den

On 09/07/15 04:30, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-07-08 17:47:51)
>> On 09/07/15 01:02, Michael Roth wrote:
>>> Quoting Denis V. Lunev (2015-07-07 03:06:08)
>>>> 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.
>>> But if I'm understanding things correctly, you're simply *not* using the
>>> guest-pipe-* interfaces in this case, and it's just a matter of having
>>> the option of not overriding the child's stdio? We wouldn't
>>> necessarilly lose that flexibility if we dropped guest-pipe-*,
>>> specifying whether we want to wire qemu-ga into stdin/stdout could
>>> still be done via options to guest-exec.
>>>
>>> Do you have an example of the sort of invocation you're doing?
>>>
>>>> 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?
>>> Yes, absolutely. Let's get the API down early and hopefully we can
>>> get it all merged early this time.
>>>
>> I have attached entire C program, which allows to obtain interactive (test)
>> shell in guest.
>>
>> actually we perform
>> "{\"execute\": \"guest-file-open\", \"arguments\":{\"path\":\"%s\",
>> \"mode\":\"r+b\"}}"
>> and after that
>>                   "{\"execute\":\"guest-exec\",
>> \"arguments\":{\"path\":\"/bin/sh\","
>> "\"handle_stdin\":%u,\"handle_stdout\":%u,\"handle_stderr\":%u }}",
>>                   ctx.guest_io_handle, ctx.guest_io_handle,
>> ctx.guest_io_handle);
>> with the handle obtained above.
> With even the bare-minimum API we could still do something like:
>
>    cmd: '/bash/sh'
>    arg0: '-c'
>    arg1: 'sh <virt-serialpath >virt-serialpath'
>
>  From the qga perspective I think we'd just end up with empty
> stdio for the life of both the parent 'sh' and child 'sh'.
>
> As a general use case this is probably a bit worse, since we're
> farming work out to a specific shell implementation instead of
> figuring out a platform-agnostic API for doing it.
>
> But if we consider this to be a niche use case then taking this
> approach gives us a little more flexibility for simplifying the
> API.
>
> But I'm not really against being able to supply stdio/stdout/stderr
> through the API. The main thing is that, by default, guest-exec
> should just supply you with a set of pipes automatically.
> This one thing let's us drop guest-pipe-open completely.
>
> And it's just like with a normal shell: you get stdio by
> default, and can redirect as needed. You don't have to do
>
>    <cmd> </dev/stdin >/dev/stdout
>
> unless you're explicitly wanting to redirect somewhere other
> than the defaults.

This would be problematic on Windows. It is difficult to redirect
to Windows names like \\.Global\org.qemu.guest.agent.0
AFAIK only DOS names are allowed and specific call to associate
Win32 name with Dos name and disassociate them looks
overkill.

This means that ability to pass descriptors would be necessary.
There are the following options
- open without handle_stdin etc create pipes inside and returns
    them as a result
- open with handles just passed them to forked process

This seems fine to me.

Den