From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47032) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCHoQ-0003DT-Ak for qemu-devel@nongnu.org; Mon, 06 Jul 2015 21:36:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCHoI-000109-BM for qemu-devel@nongnu.org; Mon, 06 Jul 2015 21:36:42 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:45260) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCHoI-0000zQ-0v for qemu-devel@nongnu.org; Mon, 06 Jul 2015 21:36:34 -0400 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 6 Jul 2015 19:36:32 -0600 Received: from b03cxnp08026.gho.boulder.ibm.com (b03cxnp08026.gho.boulder.ibm.com [9.17.130.18]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id EE3693E4003F for ; Mon, 6 Jul 2015 19:36:29 -0600 (MDT) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t671a3Fd47906950 for ; Mon, 6 Jul 2015 18:36:04 -0700 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t671aT5G029169 for ; Mon, 6 Jul 2015 19:36:29 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <1435659923-2211-7-git-send-email-den@openvz.org> References: <1435659923-2211-1-git-send-email-den@openvz.org> <1435659923-2211-7-git-send-email-den@openvz.org> Message-ID: <20150707013127.17393.82688@loki> Date: Mon, 06 Jul 2015 20:31:27 -0500 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" Cc: Olga Krishtal , qemu-devel@nongnu.org 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)"}} {'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 > 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 *ou= t, > + GuestFileHandle *error) > +{ > + GuestExecInfo *gei; > + > + gei =3D g_malloc0(sizeof(GuestExecInfo)); > + gei->pid =3D pid; > + gei->phandle =3D phandle; > + gei->gfh_stdin =3D in; > + gei->gfh_stdout =3D out; > + gei->gfh_stderr =3D 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 =3D=3D 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 =3D guest_exec_info_find(pid); > + if (gei =3D=3D NULL) { > + error_setg(errp, QERR_INVALID_PARAMETER, "pid"); > + return NULL; > + } > + > + r =3D WaitForSingleObject(gei->phandle, 0); > + if (r !=3D WAIT_OBJECT_0 && r !=3D WAIT_TIMEOUT) { > + error_setg_win32(errp, GetLastError(), > + "WaitForSingleObject() failed, pid: %u", gei->p= id); > + return NULL; > + } > + > + ges =3D g_malloc0(sizeof(GuestExecStatus)); > + ges->handle_stdin =3D (gei->gfh_stdin !=3D NULL) ? gei->gfh_stdin->i= d : -1; > + ges->handle_stdout =3D (gei->gfh_stdout !=3D NULL) ? gei->gfh_stdout= ->id : -1; > + ges->handle_stderr =3D (gei->gfh_stderr !=3D NULL) ? gei->gfh_stderr= ->id : -1; > + ges->exit =3D -1; > + ges->signal =3D -1; > + > + if (r =3D=3D WAIT_OBJECT_0) { > + GetExitCodeProcess(gei->phandle, &exit_code); > + CloseHandle(gei->phandle); > + > + ges->exit =3D (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 =3D 0; > + char *argv_str; > + WCHAR *wargs; > + char *pargv; > + > + if (strchr(path, '"') !=3D NULL) { > + error_setg(errp, "path or arguments can't contain \" quotes"); > + return NULL; > + } > + > + for (it =3D params; it !=3D NULL; it =3D it->next) { > + if (strchr(it->value, '"') !=3D NULL) { > + error_setg(errp, "path or arguments can't contain \" quotes"= ); > + return NULL; > + } > + } > + > + cap +=3D strlen(path) + sizeof("\"\"") - 1; > + for (it =3D params; it !=3D NULL; it =3D it->next) { > + cap +=3D strlen(it->value) + sizeof(" \"\"") - 1; > + } > + cap++; > + > + argv_str =3D g_malloc(cap); > + pargv =3D argv_str; > + > + *pargv++ =3D '"'; > + pstrcpy(pargv, (pargv + cap) - pargv, path); > + *pargv++ =3D '"'; > + > + for (it =3D params; it !=3D NULL; it =3D it->next) { > + with_spaces =3D (strchr(it->value, ' ') !=3D NULL); > + > + *pargv++ =3D ' '; > + > + if (with_spaces) { > + *pargv++ =3D '"'; > + } > + > + pstrcpy(pargv, (pargv + cap) - pargv, it->value); > + pargv +=3D strlen(it->value); > + > + if (with_spaces) { > + *pargv++ =3D '"'; > + } > + } > + *pargv =3D '\0'; > + > + wargs =3D g_malloc(cap * sizeof(WCHAR)); > + if (utf8_to_ucs2(wargs, cap, argv_str, -1) =3D=3D 0) { > + goto fail; > + } > + > + cap =3D strlen(path) + 1; > + *wpath =3D g_malloc(cap * sizeof(WCHAR)); > + if (utf8_to_ucs2(*wpath, cap, path, -1) =3D=3D 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 =3D 0; > + WCHAR *wenv, *pwenv; > + > + for (it =3D env; it !=3D NULL; it =3D it->next) { > + cap +=3D strlen(it->value) + 1; > + } > + cap++; > + > + wenv =3D g_malloc(cap * sizeof(WCHAR)); > + pwenv =3D wenv; > + > + for (it =3D env; it !=3D NULL; it =3D it->next) { > + r =3D utf8_to_ucs2(pwenv, (wenv + cap) - pwenv, it->value, -1); > + if (r =3D=3D 0) { > + error_setg_win32(errp, GetLastError(), > + "MultiByteToWideChar() failed"); > + g_free(wenv); > + return NULL; > + } > + pwenv +=3D r - 1; > + > + *pwenv++ =3D L'\0'; > + } > + *pwenv =3D L'\0'; > + > + return wenv; > +} > + > +static HANDLE guest_exec_get_stdhandle(GuestFileHandle *gfh) > +{ > + HANDLE fd; > + > + if (gfh =3D=3D NULL) { > + return INVALID_HANDLE_VALUE; > + } > + > + if (gfh->pipe_other_end_fd !=3D INVALID_HANDLE_VALUE) { > + fd =3D gfh->pipe_other_end_fd; > + } else { > + fd =3D gfh->fh; > + } > + > + if (!SetHandleInformation(fd, HANDLE_FLAG_INHERIT, 1)) { > + slog("guest-exec: SetHandleInformation() failed to set inherit f= lag: " > + "%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 =3D -1; > + GuestExec *ge =3D NULL; > + BOOL b; > + PROCESS_INFORMATION info; > + STARTUPINFOW si =3D {0}; > + WCHAR *wpath =3D NULL, *wargs, *wenv =3D NULL; > + GuestFileHandle *gfh_stdin =3D NULL, *gfh_stdout =3D NULL, *gfh_stde= rr =3D NULL; > + > + wargs =3D guest_exec_get_args(path, &wpath, has_params ? params : NU= LL, errp); > + wenv =3D guest_exec_get_env(has_env ? env : NULL, errp); > + if (wargs =3D=3D NULL || wenv =3D=3D NULL) { > + return NULL; > + } > + > + if (has_handle_stdin) { > + gfh_stdin =3D guest_file_handle_find(handle_stdin, errp); > + if (gfh_stdin =3D=3D NULL) { > + goto done; > + } > + } > + > + if (has_handle_stdout) { > + gfh_stdout =3D guest_file_handle_find(handle_stdout, errp); > + if (gfh_stdout =3D=3D NULL) { > + goto done; > + } > + } > + > + if (has_handle_stderr) { > + gfh_stderr =3D guest_file_handle_find(handle_stderr, errp); > + if (gfh_stderr =3D=3D NULL) { > + goto done; > + } > + } > + > + si.cb =3D sizeof(STARTUPINFOW); > + > + if (has_handle_stdin || has_handle_stdout || has_handle_stderr) { > + si.dwFlags =3D STARTF_USESTDHANDLES; > + > + si.hStdInput =3D guest_exec_get_stdhandle(gfh_stdin); > + si.hStdOutput =3D guest_exec_get_stdhandle(gfh_stdout); > + si.hStdError =3D guest_exec_get_stdhandle(gfh_stderr); > + } > + > + b =3D 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 !=3D NULL && guest_pipe_close_other_end(gfh_stdin) != =3D 0) { > + slog("failed to close stdin pipe handle. error: %lu", GetLastErr= or()); > + } > + > + if (gfh_stdout !=3D NULL && guest_pipe_close_other_end(gfh_stdout) != =3D 0) { > + slog("failed to close stdout pipe handle. error: %lu", GetLastEr= ror()); > + } > + > + if (gfh_stderr !=3D NULL && guest_pipe_close_other_end(gfh_stderr) != =3D 0) { > + slog("failed to close stderr pipe handle. error: %lu", GetLastEr= ror()); > + } > + > + CloseHandle(info.hThread); > + guest_exec_info_add(info.dwProcessId, info.hProcess, gfh_stdin, gfh_= stdout, > + gfh_stderr); > + pid =3D info.dwProcessId; > + ge =3D g_malloc(sizeof(*ge)); > + ge->pid =3D 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 =3D (char **)list_unsupported; > = > while (*p) { > @@ -830,4 +1126,5 @@ void ga_command_state_init(GAState *s, GACommandStat= e *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 >=20