From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43535) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCPmq-0007Tv-IF for qemu-devel@nongnu.org; Tue, 07 Jul 2015 06:07:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCPmn-0005vW-3h for qemu-devel@nongnu.org; Tue, 07 Jul 2015 06:07:36 -0400 Received: from relay.parallels.com ([195.214.232.42]:32932) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCPmm-0005uy-Jg for qemu-devel@nongnu.org; Tue, 07 Jul 2015 06:07:33 -0400 Message-ID: <559BA4E1.5040800@parallels.com> Date: Tue, 7 Jul 2015 13:07:29 +0300 From: Olga Krishtal MIME-Version: 1.0 References: <1435659923-2211-1-git-send-email-den@openvz.org> <1435659923-2211-7-git-send-email-den@openvz.org> <20150707013127.17393.82688@loki> <559B8870.30704@openvz.org> <559B97E2.804@parallels.com> In-Reply-To: <559B97E2.804@parallels.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , Michael Roth Cc: Olga Krishtal , qemu-devel@nongnu.org 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 >>>> >>>> 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 >>>> Acked-by: Roman Kagan >>>> Signed-off-by: Denis V. Lunev >>>> CC: Eric Blake >>>> CC: Michael Roth >>>> --- >>>> 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 >>>> >> >