From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54003) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCOvF-0008SQ-OW for qemu-devel@nongnu.org; Tue, 07 Jul 2015 05:12:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCOvB-0002SA-1Z for qemu-devel@nongnu.org; Tue, 07 Jul 2015 05:12:13 -0400 Received: from relay.parallels.com ([195.214.232.42]:50468) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCOvA-0002PU-Jt for qemu-devel@nongnu.org; Tue, 07 Jul 2015 05:12:08 -0400 Message-ID: <559B97E2.804@parallels.com> Date: Tue, 7 Jul 2015 12:12:02 +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> In-Reply-To: <559B8870.30704@openvz.org> 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 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'"}} >> >> {'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 >>> >