* [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-06-19 16:51 [Qemu-devel] [PATCH v4 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
@ 2015-06-19 16:51 ` Denis V. Lunev
0 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-19 16:51 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
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(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b216628..e633126 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -454,10 +454,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_set(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_set(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,
@@ -468,8 +689,84 @@ GuestExec *qmp_guest_exec(const char *path,
bool has_handle_stderr, int64_t handle_stderr,
Error **errp)
{
- error_set(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)
@@ -803,8 +1100,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) {
@@ -832,4 +1128,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);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-06-19 16:57 [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
0 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-19 16:57 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
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(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b216628..e633126 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -454,10 +454,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_set(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_set(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,
@@ -468,8 +689,84 @@ GuestExec *qmp_guest_exec(const char *path,
bool has_handle_stderr, int64_t handle_stderr,
Error **errp)
{
- error_set(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)
@@ -803,8 +1100,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) {
@@ -832,4 +1128,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);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec
@ 2015-06-30 10:25 Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
` (9 more replies)
0 siblings, 10 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
Implementation of qmp_quest_get_fsinfo. This functionality will be
used together with vss-provider in order to do freeze/unfreeze
per volume (single mounted fs).
These patches for guest-agent add the functionality to execute commands on
a guest UNIX and Windows machines.
These patches add the following interfaces:
guest-pipe-open
guest-exec
guest-exec-status
With these interfaces it's possible to:
* Open an anonymous pipe and work with it as with a file using already
implemented interfaces guest-file-{read,write,flush,close}.
* Execute a binary or a script on a guest machine.
We can pass arbitrary arguments and environment to a new child process.
* Pass redirected IO from/to stdin, stdout, stderr from a child process to a
local file or an anonymous pipe.
* Check the status of a child process, get its exit status if it exited or get
signal number if it was killed.
We plan to add support for Windows in the near future using the same interfaces
we introduce here.
Example of usage:
{"execute": "guest-pipe-open", "arguments":{"mode": "r"}}
{'return': 1000}
{"execute":"guest-exec", "arguments":{ "path": "id", "params": ["user"],
"env": ["MYENV=myvalue"], "handle_stdout": 1000 }}'
{"return": 2636}
{"execute": "guest-exec-status", "arguments": {"pid": 2636}}
{"return":{"exit":0,"handle_stderr":-1,"handle_stdin":-1,"handle_stdout":1000,"signal":-1}}
{"execute": "guest-file-read", "arguments": {"handle": 1000, "count":128}}
{"return":{"count":58,"buf-b64":"dWlk...","eof":true}}
{"execute": "guest-file-close", "arguments": {"handle": 1000}}
{"return":{}}
These patches are based on the patches proposed by Michael Roth in 2011:
http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg00722.html
We made several modifications to the interfaces proposed by Michael to simplify
their usage and we also added several new functions:
* Arguments to an executable file are passed as an array of strings.
Before that we had to pass parameters like this:
'params': [ {'param': '-b'}, {'param': '-n1'} ]
Now we may pass them just as an array of strings:
'params': [ '-b', '-n1' ]
* Environment can be passed to a child process.
"env": ["MYENV=myvalue"]
* Removed "detach" argument from "guest-exec" - it never waits for a child
process to signal. With this change it's possible to return just PID from
"guest-exec", a user must call "guest-exec-status" to get the status of a child.
* Removed "wait" argument from "guest-exec-status" - waiting for a child process
to signal is dangerous, because it may block the whole qemu-ga. Instead, the
command polls just once a status of a child.
* "guest-exec-status" returns exit status of a child process or a signal number,
in case a process was killed.
* Simplified the command "guest-pipe-open" - the way how it stores the pipe
fd's. No additional logic is needed about which end of pipe should be closed
with "guest-file-close". This way we avoid modifying the interface of the
existing "guest-file-close".
In the conversation about the original patches there was a suggestion to merge
"path" and "params" into one single list. But we didn't do so, because having
separate "path" is similar to exec*() family functions in UNIX. That way it
looks more consistent.
Changes from v5:
- fixed qemu_set_fd_nonblocking. Due to unclear name of bool variable
nonblocking all if statemet had opposite effect.
- checked build of qemu-ga.exe for _WIN32_WINNT == 0x0601.
- added ifdef for different versions of Windows
- merge patches that were sent to upstream first time with changes that were
accidentally made over previous versions.
- merged to current head
- changed the schema of searching of the appropriate qemu bus type in win32
guest-get-fsinfo comman.
- changed the interface of guest-pipe-open, now the command returns struct
with read handle of pipe and write handle of pipe
- fixed spelling mistakes
- added GuestPipeMode field to GuestPipeInfo struct
- added trailing comma to static WinToLin buses array
- rename some bus types in qapi-schema
Changes from v4:
- fixed typo in Olga's e-mail in patch 4 & and wrong mail in patch 10
Changes from v3:
- fixed warnings with type mismatch
- fixed the behavior of command quest-get-fsinfo in case when we do not have
CD in CD-ROM and etc.
Before this changes command failed with error 0x15 ERROR_DEVICE_NOT_READY.
Now we are simply skipping this volume.
- merged volume info & guest exec patchsets, resolved conflicts between them
Changes from v2:
- return code of commands changed to dictionary
- ported to current HEAD
- new way for moving pipe to non-blocking state (universal)
Changes from v1:
- Windows version of the patchset is added
- SIGPIPE processing is added for Unix version of the patchset
- added guest_exec_file_busy() to prevent GuestFileHandle* object from being
deleted in case it's used in guest-exec command.
Signed-off-by: Olga Krishtal <okrishtal@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>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-07-07 8:19 ` Denis V. Lunev
2015-07-08 21:16 ` Michael Roth
2015-06-30 10:25 ` [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command Denis V. Lunev
` (8 subsequent siblings)
9 siblings, 2 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@virtuozzo.com>
guest_file_toggle_flags is a copy from semi-portable qemu_set_nonblock.
The latter is not working properly for Windows due to reduced Windows
Posix implementation.
On Windows OS there is a separate API for changing flags of file, pipes
and sockets. Portable way to change file descriptor flags requires
to detect file descriptor type and proper actions depending of that
type. The patch adds wrapper qemu_set_fd_nonblocking into Windows specific
code to handle this stuff properly.
The only problem is that qemu_set_nonblock is void but this should not
be a problem.
Signed-off-by: Olga Krishtal <okrishtal@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-posix.c | 27 ++-------------------------
util/oslib-win32.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 49 insertions(+), 30 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index befd00b..40dbe25 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -28,6 +28,7 @@
#include "qapi/qmp/qerror.h"
#include "qemu/queue.h"
#include "qemu/host-utils.h"
+#include "qemu/sockets.h"
#ifndef CONFIG_HAS_ENVIRON
#ifdef __APPLE__
@@ -376,27 +377,6 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
return NULL;
}
-static int guest_file_toggle_flags(int fd, int flags, bool set, Error **err)
-{
- int ret, old_flags;
-
- old_flags = fcntl(fd, F_GETFL);
- if (old_flags == -1) {
- error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
- "failed to fetch filehandle flags");
- return -1;
- }
-
- ret = fcntl(fd, F_SETFL, set ? (old_flags | flags) : (old_flags & ~flags));
- if (ret == -1) {
- error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
- "failed to set filehandle flags");
- return -1;
- }
-
- return ret;
-}
-
int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
Error **errp)
{
@@ -417,10 +397,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
/* set fd non-blocking to avoid common use cases (like reading from a
* named pipe) from hanging the agent
*/
- if (guest_file_toggle_flags(fileno(fh), O_NONBLOCK, true, errp) < 0) {
- fclose(fh);
- return -1;
- }
+ qemu_set_nonblock(fileno(fh));
handle = guest_file_handle_add(fh, errp);
if (handle < 0) {
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 730a670..1a6ae72 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -119,17 +119,59 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
return p;
}
-void qemu_set_block(int fd)
+static void qemu_set_fd_nonblocking(int fd, bool nonblocking)
{
- unsigned long opt = 0;
- WSAEventSelect(fd, NULL, 0);
+ HANDLE handle;
+ DWORD file_type, pipe_state;
+
+ handle = (HANDLE)_get_osfhandle(fd);
+ if (handle == INVALID_HANDLE_VALUE) {
+ return;
+ }
+
+ file_type = GetFileType(handle);
+ if (file_type != FILE_TYPE_PIPE) {
+ return;
+ }
+
+ /* If file_type == FILE_TYPE_PIPE, according to msdn
+ * the specified file is socket or named pipe */
+ if (GetNamedPipeHandleState(handle, &pipe_state, NULL,
+ NULL, NULL, NULL, 0)) {
+ /* The fd is named pipe fd */
+ if (!nonblocking == !(pipe_state & PIPE_NOWAIT)) {
+ /* In this case we do not need perform any operation, because
+ * nonblocking = true and PIPE_NOWAIT is already set or
+ * nonblocking = false and PIPE_NOWAIT is not set */
+ return;
+ }
+
+ if (nonblocking) {
+ pipe_state |= PIPE_NOWAIT;
+ } else {
+ pipe_state &= ~PIPE_NOWAIT;
+ }
+
+ SetNamedPipeHandleState(handle, &pipe_state, NULL, NULL);
+ return;
+ }
+
+ /* The fd is socket fd */
+ unsigned long opt = (unsigned long)nonblocking;
+ if (!nonblocking) {
+ WSAEventSelect(fd, NULL, 0);
+ }
ioctlsocket(fd, FIONBIO, &opt);
}
+void qemu_set_block(int fd)
+{
+ qemu_set_fd_nonblocking(fd, false);
+}
+
void qemu_set_nonblock(int fd)
{
- unsigned long opt = 1;
- ioctlsocket(fd, FIONBIO, &opt);
+ qemu_set_fd_nonblocking(fd, true);
qemu_fd_register(fd);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 03/10] qga: guest exec functionality for Unix guests Denis V. Lunev
` (7 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@virtuozzo.com>
The command creates FIFO pair that can be used with existing file
read/write interfaces to communicate with processes spawned via the
forthcoming guest-file-exec interface.
Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Acked-by: Roman Kagan <rkagan@virtuozzo.com>
CC: Eric Blake <eblake@redhat.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qga/commands-posix.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++---
qga/commands-win32.c | 8 ++++-
qga/qapi-schema.json | 43 ++++++++++++++++++++++++
3 files changed, 139 insertions(+), 5 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 40dbe25..d676686 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -212,6 +212,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
typedef struct GuestFileHandle {
uint64_t id;
FILE *fh;
+ int pipe_other_end_fd; /* if set, it's a pipe fd of the other end. */
QTAILQ_ENTRY(GuestFileHandle) next;
} GuestFileHandle;
@@ -219,7 +220,8 @@ static struct {
QTAILQ_HEAD(, GuestFileHandle) filehandles;
} guest_file_state;
-static int64_t guest_file_handle_add(FILE *fh, Error **errp)
+static int64_t guest_file_handle_add(FILE *fh, int pipe_other_end_fd,
+ Error **errp)
{
GuestFileHandle *gfh;
int64_t handle;
@@ -232,6 +234,7 @@ static int64_t guest_file_handle_add(FILE *fh, Error **errp)
gfh = g_malloc0(sizeof(GuestFileHandle));
gfh->id = handle;
gfh->fh = fh;
+ gfh->pipe_other_end_fd = pipe_other_end_fd;
QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
return handle;
@@ -399,7 +402,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
*/
qemu_set_nonblock(fileno(fh));
- handle = guest_file_handle_add(fh, errp);
+ handle = guest_file_handle_add(fh, -1, errp);
if (handle < 0) {
fclose(fh);
return -1;
@@ -409,6 +412,82 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
return handle;
}
+GuestPipeInfo *qmp_guest_pipe_open(GuestPipeMode mode, Error **errp)
+{
+ FILE *f = NULL;
+ int fd[2], this_end, other_end;
+ int64_t handle;
+ GuestPipeInfo *pipe_inf;
+ const char *op_flag;
+
+ slog("guest-pipe-open called");
+
+ if (mode > 2) {
+ error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "Only \"r\" or \"w\" are the valid modes to open a pipe");
+ return NULL;
+ }
+
+ if (pipe(fd) != 0) {
+ error_setg_errno(errp, errno, QERR_QGA_COMMAND_FAILED, "pipe() failed");
+ return NULL;
+ }
+
+ this_end = (mode == GUEST_PIPE_MODE_WRITE);
+ other_end = !this_end;
+
+ qemu_set_nonblock(fd[this_end]);
+
+ qemu_set_cloexec(fd[this_end]);
+ qemu_set_cloexec(fd[other_end]);
+
+ if (mode == GUEST_PIPE_MODE_READ) {
+ op_flag = "r";
+ } else {
+ op_flag = "w";
+ }
+ f = fdopen(fd[this_end], op_flag);
+ if (f == NULL) {
+ error_setg_errno(errp, errno, QERR_QGA_COMMAND_FAILED,
+ "fdopen() failed to open pipe handle");
+ goto fail;
+ }
+
+ handle = guest_file_handle_add(f, fd[other_end], errp);
+ if (handle == -1) {
+ goto fail;
+ }
+
+ slog("guest-pipe-open: handle: %" PRId64, handle);
+
+ pipe_inf = g_malloc0(sizeof(*pipe_inf));
+ pipe_inf->fd = handle;
+ pipe_inf->mode = mode;
+ return pipe_inf;
+
+fail:
+ if (f != NULL) {
+ fclose(f);
+ } else {
+ close(fd[this_end]);
+ }
+ close(fd[other_end]);
+ return NULL;
+}
+
+static int guest_pipe_close_other_end(GuestFileHandle *gfh)
+{
+ if (gfh->pipe_other_end_fd != -1) {
+ if (close(gfh->pipe_other_end_fd) != 0) {
+ return 1;
+ }
+
+ gfh->pipe_other_end_fd = -1;
+ }
+
+ return 0;
+}
+
void qmp_guest_file_close(int64_t handle, Error **errp)
{
GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
@@ -419,6 +498,11 @@ void qmp_guest_file_close(int64_t handle, Error **errp)
return;
}
+ if (guest_pipe_close_other_end(gfh) != 0) {
+ error_setg_errno(errp, errno, "failed to close pipe handle");
+ return;
+ }
+
ret = fclose(gfh->fh);
if (ret == EOF) {
error_setg_errno(errp, errno, "failed to close handle");
@@ -2395,7 +2479,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
"guest-suspend-hybrid", "guest-network-get-interfaces",
"guest-get-vcpus", "guest-set-vcpus",
"guest-get-memory-blocks", "guest-set-memory-blocks",
- "guest-get-memory-block-size", NULL};
+ "guest-get-memory-block-size", "guest-pipe-open", NULL};
char **p = (char **)list;
while (*p) {
@@ -2409,7 +2493,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
const char *list[] = {
"guest-get-fsinfo", "guest-fsfreeze-status",
"guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list",
- "guest-fsfreeze-thaw", "guest-get-fsinfo", NULL};
+ "guest-fsfreeze-thaw", "guest-get-fsinfo",
+ "guest-pipe-open", NULL};
char **p = (char **)list;
while (*p) {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index fbddc8b..592773b 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -154,6 +154,12 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode,
return fd;
}
+GuestPipeInfo *qmp_guest_pipe_open(GuestPipeMode mode, Error **errp)
+{
+ error_setg(errp, QERR_UNSUPPORTED);
+ return NULL;
+}
+
void qmp_guest_file_close(int64_t handle, Error **errp)
{
bool ret;
@@ -713,7 +719,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", NULL};
+ "guest-fstrim", "guest-pipe-open", NULL};
char **p = (char **)list_unsupported;
while (*p) {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index b446dc7..bdf48dd 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -215,12 +215,55 @@
'returns': 'int' }
##
+# @GuestPipeMode
+#
+# An enumeration of pipe modes
+# read: pipe is opened for writing data
+# write: pipe is opened for reading data
+#
+# Since: 2.4
+##
+{ 'enum': 'GuestPipeMode',
+ 'data': [ 'read', 'write' ] }
+
+##
+# @GuestPipeInfo
+#
+# Information about pipe.
+#
+# Since: 2.4
+##
+{ 'struct': 'GuestPipeInfo',
+ 'data': {'fd': 'int', 'mode': 'GuestPipeMode'} }
+
+##
+# @guest-pipe-open
+#
+# Open a pipe to in the guest for association with a qga-spawned processes.
+#
+# Returns: Guest file handle on success, as per guest-file-open. This
+# handle is usable with the same interfaces as a handle returned by
+# guest-file-open.
+#
+# Since: 2.4
+##
+{ 'command': 'guest-pipe-open',
+ 'data': { 'mode': 'GuestPipeMode' },
+ 'returns': 'GuestPipeInfo' }
+
+##
+##
# @guest-file-close:
#
# Close an open file in the guest
#
# @handle: filehandle returned by guest-file-open
#
+# Please note that closing the write side of a pipe will block until the read
+# side is closed. If you passed the read-side of the pipe to a qga-spawned
+# process, make sure the process has exited before attempting to close the
+# write side.
+#
# Returns: Nothing on success.
#
# Since: 0.15.0
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 03/10] qga: guest exec functionality for Unix guests
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 04/10] qga: handle possible SIGPIPE in guest-file-write Denis V. Lunev
` (6 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
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-posix.c | 237 +++++++++++++++++++++++++++++++++++++++++++++++++++
qga/commands-win32.c | 21 ++++-
qga/qapi-schema.json | 56 ++++++++++++
3 files changed, 313 insertions(+), 1 deletion(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index d676686..81701b5 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -825,6 +825,242 @@ static void build_fs_mount_list(FsMountList *mounts, Error **errp)
}
#endif
+typedef struct GuestExecInfo {
+ pid_t pid;
+ 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_info_add(pid_t pid,
+ GuestFileHandle *in, GuestFileHandle *out,
+ GuestFileHandle *error)
+{
+ GuestExecInfo *gei;
+
+ gei = g_malloc0(sizeof(*gei));
+ gei->pid = pid;
+ 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(pid_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 **err)
+{
+ GuestExecInfo *gei;
+ GuestExecStatus *ges;
+ int status, ret;
+
+ slog("guest-exec-status called, pid: %u", (uint32_t)pid);
+
+ gei = guest_exec_info_find(pid);
+ if (gei == NULL) {
+ error_setg(err, QERR_INVALID_PARAMETER, "pid");
+ return NULL;
+ }
+
+ ret = waitpid(gei->pid, &status, WNOHANG);
+ if (ret == -1) {
+ error_setg_errno(err, errno, "waitpid() failed, pid: %u", gei->pid);
+ return NULL;
+ }
+
+ ges = g_malloc0(sizeof(GuestExecStatus));
+ ges->handle_stdin = gei->gfh_stdin ? gei->gfh_stdin->id : -1;
+ ges->handle_stdout = gei->gfh_stdout ? gei->gfh_stdout->id : -1;
+ ges->handle_stderr = gei->gfh_stderr ? gei->gfh_stderr->id : -1;
+ ges->exit = -1;
+ ges->signal = -1;
+
+ if (ret != 0) {
+ if (WIFEXITED(status)) {
+ ges->exit = WEXITSTATUS(status);
+ } else if (WIFSIGNALED(status)) {
+ ges->signal = WTERMSIG(status);
+ }
+
+ QTAILQ_REMOVE(&guest_exec_state.processes, gei, next);
+ g_free(gei);
+ }
+
+ return ges;
+}
+
+/* Get environment variables or arguments array for execve(). */
+static char **guest_exec_get_args(const strList *entry)
+{
+ const strList *it;
+ int count = 1, i = 0; /* reserve for NULL terminator */
+ size_t argv_str_size = 0;
+ char **args;
+ char *argv_str; /* for array of arguments */
+
+ if (entry != NULL) {
+ argv_str_size = strlen(entry->value) + 1;
+ }
+ for (it = entry; it != NULL; it = it->next) {
+ count++;
+ argv_str_size += sizeof(" ") - 1 + strlen(it->value);
+ }
+
+ argv_str = g_malloc(argv_str_size);
+ args = g_malloc(count * sizeof(char *));
+ for (it = entry; it != NULL; it = it->next) {
+ args[i++] = it->value;
+ pstrcat(argv_str, argv_str_size, it->value);
+ pstrcat(argv_str, argv_str_size, " ");
+ }
+
+ slog("guest-exec called: \"%s\"", argv_str);
+ g_free(argv_str);
+
+ args[i] = NULL;
+ return args;
+
+}
+
+static int guest_exec_set_std(GuestFileHandle *gfh, int std_fd, int fd_null)
+{
+ int fd;
+
+ if (gfh == NULL) {
+ fd = fd_null;
+ } else if (gfh->pipe_other_end_fd != -1) {
+ fd = gfh->pipe_other_end_fd;
+ } else {
+ fd = fileno(gfh->fh);
+ }
+
+ if (dup2(fd, std_fd) == -1) {
+ slog("dup2() failed: %s", strerror(errno));
+ return -1;
+ }
+ return 0;
+}
+
+GuestExec *qmp_guest_exec(const char *path,
+ bool has_params, strList *params,
+ bool has_env, strList *env,
+ bool has_handle_stdin, int64_t handle_stdin,
+ bool has_handle_stdout, int64_t handle_stdout,
+ bool has_handle_stderr, int64_t handle_stderr,
+ Error **err)
+{
+ pid_t pid = -1;
+ int fd_null;
+ GuestExec *ge = NULL;
+ GuestFileHandle *gfh_stdin = NULL, *gfh_stdout = NULL, *gfh_stderr = NULL;
+ char **argv, **envp;
+ strList path_arg = {
+ .value = (char *)path,
+ .next = has_params ? params : NULL
+ };
+
+ argv = guest_exec_get_args(&path_arg);
+ envp = guest_exec_get_args(has_env ? env : NULL);
+
+ if (has_handle_stdin) {
+ gfh_stdin = guest_file_handle_find(handle_stdin, err);
+ if (gfh_stdin == NULL) {
+ goto done;
+ }
+ }
+
+ if (has_handle_stdout) {
+ gfh_stdout = guest_file_handle_find(handle_stdout, err);
+ if (gfh_stdout == NULL) {
+ goto done;
+ }
+ }
+
+ if (has_handle_stderr) {
+ gfh_stderr = guest_file_handle_find(handle_stderr, err);
+ if (gfh_stderr == NULL) {
+ goto done;
+ }
+ }
+
+ pid = fork();
+ if (pid < 0) {
+ error_setg(err, QERR_UNDEFINED_ERROR);
+ goto done;
+ } else if (pid == 0) {
+ setsid();
+
+ fd_null = -1;
+ if (!has_handle_stdin || !has_handle_stdout || !has_handle_stderr) {
+ fd_null = open("/dev/null", O_RDWR);
+ if (fd_null == -1) {
+ slog("guest-exec: couldn't open /dev/null: %s",
+ strerror(errno));
+ exit(1);
+ }
+ }
+
+ if (guest_exec_set_std(gfh_stdin, STDIN_FILENO, fd_null) < 0 ||
+ guest_exec_set_std(gfh_stdout, STDOUT_FILENO, fd_null) < 0 ||
+ guest_exec_set_std(gfh_stderr, STDERR_FILENO, fd_null) < 0) {
+
+ exit(1);
+ }
+
+ if (fd_null != -1 && close(fd_null) != 0) {
+ slog("guest-exec: couldn't close /dev/null: %s", strerror(errno));
+ /* exit(1); */
+ }
+
+ execvpe(path, (char * const *)argv, (char * const *)envp);
+ slog("guest-exec child failed: %s", strerror(errno));
+ exit(1);
+ }
+
+ if (gfh_stdin != NULL && guest_pipe_close_other_end(gfh_stdin) != 0) {
+ slog("close() failed to close stdin pipe handle: %s", strerror(errno));
+ }
+
+ if (gfh_stdout != NULL && guest_pipe_close_other_end(gfh_stdout) != 0) {
+ slog("close() failed to close stdout pipe handle: %s", strerror(errno));
+ }
+
+ if (gfh_stderr != NULL && guest_pipe_close_other_end(gfh_stderr) != 0) {
+ slog("close() failed to close stderr pipe handle: %s", strerror(errno));
+ }
+
+ guest_exec_info_add(pid, gfh_stdin, gfh_stdout, gfh_stderr);
+ ge = g_malloc(sizeof(*ge));
+ ge->pid = pid;
+
+done:
+ g_free(argv);
+ g_free(envp);
+ return ge;
+}
+
+static void guest_exec_init(void)
+{
+ QTAILQ_INIT(&guest_exec_state.processes);
+}
+
#if defined(CONFIG_FSFREEZE)
static char *get_pci_driver(char const *syspath, int pathlen, Error **errp)
@@ -2517,4 +2753,5 @@ void ga_command_state_init(GAState *s, GACommandState *cs)
ga_command_state_add(cs, NULL, guest_fsfreeze_cleanup);
#endif
ga_command_state_add(cs, guest_file_init, NULL);
+ ga_command_state_add(cs, guest_exec_init, NULL);
}
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 592773b..6a6939c 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -388,6 +388,24 @@ static void guest_file_init(void)
QTAILQ_INIT(&guest_file_state.filehandles);
}
+GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **errp)
+{
+ error_setg(errp, QERR_UNSUPPORTED);
+ return 0;
+}
+
+GuestExec *qmp_guest_exec(const char *path,
+ bool has_params, strList *params,
+ bool has_env, strList *env,
+ bool has_handle_stdin, int64_t handle_stdin,
+ bool has_handle_stdout, int64_t handle_stdout,
+ bool has_handle_stderr, int64_t handle_stderr,
+ Error **errp)
+{
+ error_setg(errp, QERR_UNSUPPORTED);
+ return 0;
+}
+
GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
{
error_setg(errp, QERR_UNSUPPORTED);
@@ -719,7 +737,8 @@ 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-pipe-open", NULL};
+ "guest-fstrim", "guest-pipe-open", "guest-exec-status",
+ "guest-exec", NULL};
char **p = (char **)list_unsupported;
while (*p) {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index bdf48dd..dee36b6 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -934,3 +934,59 @@
##
{ 'command': 'guest-get-memory-block-info',
'returns': 'GuestMemoryBlockInfo' }
+
+##
+# @guest-exec-status
+#
+# Check status of process associated with PID retrieved via guest-exec.
+# Reap the process and associated metadata if it has exited.
+#
+# @pid: pid returned from guest-exec
+#
+# Returns: GuestExecStatus on success. If a child process exited, "exit" is set
+# to the exit code. If a child process was killed by a signal,
+# "signal" is set to the signal number. If a child process is still
+# running, both "exit" and "signal" are set to -1. If a guest cannot
+# reliably detect exit signals, "signal" will be -1.
+#
+# Since: 2.4
+##
+{ 'struct': 'GuestExecStatus',
+ 'data': { 'exit': 'int', 'signal': 'int',
+ 'handle-stdin': 'int', 'handle-stdout': 'int',
+ 'handle-stderr': 'int' } }
+
+{ 'command': 'guest-exec-status',
+ 'data': { 'pid': 'int' },
+ 'returns': 'GuestExecStatus' }
+
+##
+# @GuestExec:
+# @pid: pid of child process in guest OS
+#
+#Since: 2.4
+##
+{ 'struct': 'GuestExec',
+ 'data': { 'pid': 'int'} }
+
+##
+# @guest-exec:
+#
+# Execute a command in the guest
+#
+# @path: path or executable name to execute
+# @params: #optional parameter list to pass to executable
+# @env: #optional environment variables to pass to executable
+# @handle_stdin: #optional handle to associate with process' stdin.
+# @handle_stdout: #optional handle to associate with process' stdout
+# @handle_stderr: #optional handle to associate with process' stderr.
+#
+# Returns: PID on success.
+#
+# Since: 2.4
+##
+{ 'command': 'guest-exec',
+ 'data': { 'path': 'str', '*params': ['str'], '*env': ['str'],
+ '*handle-stdin': 'int', '*handle-stdout': 'int',
+ '*handle-stderr': 'int' },
+ 'returns': 'GuestExec' }
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 04/10] qga: handle possible SIGPIPE in guest-file-write
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (2 preceding siblings ...)
2015-06-30 10:25 ` [Qemu-devel] [PATCH 03/10] qga: guest exec functionality for Unix guests Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 05/10] qga: guest-pipe-open for Windows guest Denis V. Lunev
` (5 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@virtuozzo.com>
qemu-ga should not exit on guest-file-write to pipe without read end
but proper error code should be returned. The behavior of the
spawned process should be default thus SIGPIPE processing should be
reset to default after fork() but before exec().
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-posix.c | 16 ++++++++++++++++
qga/main.c | 6 ++++++
2 files changed, 22 insertions(+)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 81701b5..b1478bf 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -958,6 +958,20 @@ static int guest_exec_set_std(GuestFileHandle *gfh, int std_fd, int fd_null)
return 0;
}
+/** Reset ignored signals back to default. */
+static void guest_exec_reset_child_sig(void)
+{
+ struct sigaction sigact;
+
+ memset(&sigact, 0, sizeof(struct sigaction));
+ sigact.sa_handler = SIG_DFL;
+
+ if (sigaction(SIGPIPE, &sigact, NULL) != 0) {
+ slog("sigaction() failed to reset child process's SIGPIPE: %s",
+ strerror(errno));
+ }
+}
+
GuestExec *qmp_guest_exec(const char *path,
bool has_params, strList *params,
bool has_env, strList *env,
@@ -1029,6 +1043,8 @@ GuestExec *qmp_guest_exec(const char *path,
/* exit(1); */
}
+ guest_exec_reset_child_sig();
+
execvpe(path, (char * const *)argv, (char * const *)envp);
slog("guest-exec child failed: %s", strerror(errno));
exit(1);
diff --git a/qga/main.c b/qga/main.c
index 23cde01..a2ed460 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -160,6 +160,12 @@ static gboolean register_signal_handlers(void)
g_error("error configuring signal handler: %s", strerror(errno));
}
+ sigact.sa_handler = SIG_IGN;
+ if (sigaction(SIGPIPE, &sigact, NULL) != 0) {
+ g_error("error configuring SIGPIPE signal handler: %s",
+ strerror(errno));
+ }
+
return true;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 05/10] qga: guest-pipe-open for Windows guest
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (3 preceding siblings ...)
2015-06-30 10:25 ` [Qemu-devel] [PATCH 04/10] qga: handle possible SIGPIPE in guest-file-write Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests Denis V. Lunev
` (4 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@virtuozzo.com>
The patch creates anonymous pipe that can be passed as stdin/stdout/stderr
handle to a child process spawned using forthcoming guest-file-exec
command. Working with a pipe is done using the existing guest-file-* API.
Signed-off-by: Olga Krishtal <okrishtal@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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 67 insertions(+), 4 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 6a6939c..435a049 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -21,6 +21,7 @@
#include "qga-qmp-commands.h"
#include "qapi/qmp/qerror.h"
#include "qemu/queue.h"
+#include "qemu/sockets.h"
#ifndef SHTDN_REASON_FLAG_PLANNED
#define SHTDN_REASON_FLAG_PLANNED 0x80000000
@@ -37,6 +38,7 @@
typedef struct GuestFileHandle {
int64_t id;
HANDLE fh;
+ HANDLE pipe_other_end_fd; /* if set, it's a pipe fd of the other end. */
QTAILQ_ENTRY(GuestFileHandle) next;
} GuestFileHandle;
@@ -84,7 +86,8 @@ static OpenFlags *find_open_flag(const char *mode_str)
return NULL;
}
-static int64_t guest_file_handle_add(HANDLE fh, Error **errp)
+static int64_t guest_file_handle_add(HANDLE fh, HANDLE pipe_other_end_fd,
+ Error **errp)
{
GuestFileHandle *gfh;
int64_t handle;
@@ -96,6 +99,7 @@ static int64_t guest_file_handle_add(HANDLE fh, Error **errp)
gfh = g_malloc0(sizeof(GuestFileHandle));
gfh->id = handle;
gfh->fh = fh;
+ gfh->pipe_other_end_fd = pipe_other_end_fd;
QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
return handle;
@@ -143,7 +147,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode,
return -1;
}
- fd = guest_file_handle_add(fh, errp);
+ fd = guest_file_handle_add(fh, INVALID_HANDLE_VALUE, errp);
if (fd < 0) {
CloseHandle(&fh);
error_setg(errp, "failed to add handle to qmp handle table");
@@ -154,12 +158,65 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode,
return fd;
}
+/* Create an anonymous pipe. To set NOWAIT mode is done via qemu_set_nonblock *
+ * will fail with ERROR_NO_DATA; WriteFile() will return 0 bytes written. */
GuestPipeInfo *qmp_guest_pipe_open(GuestPipeMode mode, Error **errp)
{
- error_setg(errp, QERR_UNSUPPORTED);
+ HANDLE fd[2];
+ int this_end, other_end;
+ int64_t handle;
+ GuestPipeInfo *pipe_inf;
+ SECURITY_ATTRIBUTES sa = {
+ sizeof(SECURITY_ATTRIBUTES), NULL, 0
+ };
+
+ slog("guest-pipe-open called");
+ if (mode > 2) {
+ error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "Only \"r\" or \"w\" are the valid modes to open a pipe");
+ return NULL;
+ }
+
+ if (!CreatePipe(&fd[0], &fd[1], &sa, 0)) {
+ error_setg_win32(errp, GetLastError(), "CreatePipe() failed");
+ return NULL;
+ }
+
+ this_end = (mode == GUEST_PIPE_MODE_WRITE);
+ other_end = !this_end;
+
+ qemu_set_nonblock(_open_osfhandle((intptr_t)fd[this_end], O_WRONLY));
+
+ handle = guest_file_handle_add(fd[this_end], fd[other_end], errp);
+ if (handle == -1) {
+ goto fail;
+ }
+
+ slog("guest-pipe-open: handle: %" PRId64, handle);
+
+ pipe_inf = g_malloc0(sizeof(*pipe_inf));
+ pipe_inf->fd = handle;
+ pipe_inf->mode = mode;
+ return pipe_inf;
+
+fail:
+ CloseHandle(fd[0]);
+ CloseHandle(fd[1]);
return NULL;
}
+static int guest_pipe_close_other_end(GuestFileHandle *gfh)
+{
+ if (gfh->pipe_other_end_fd != INVALID_HANDLE_VALUE) {
+ if (!CloseHandle(gfh->pipe_other_end_fd)) {
+ return 1;
+ }
+ gfh->pipe_other_end_fd = INVALID_HANDLE_VALUE;
+ }
+
+ return 0;
+ }
+
void qmp_guest_file_close(int64_t handle, Error **errp)
{
bool ret;
@@ -168,6 +225,12 @@ void qmp_guest_file_close(int64_t handle, Error **errp)
if (gfh == NULL) {
return;
}
+
+ if (guest_pipe_close_other_end(gfh) != 0) {
+ error_setg_win32(errp, GetLastError(), "failed to close pipe handle");
+ return;
+ }
+
ret = CloseHandle(gfh->fh);
if (!ret) {
error_setg_win32(errp, GetLastError(), "failed close handle");
@@ -737,7 +800,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-pipe-open", "guest-exec-status",
+ "guest-fstrim", "guest-exec-status",
"guest-exec", NULL};
char **p = (char **)list_unsupported;
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (4 preceding siblings ...)
2015-06-30 10:25 ` [Qemu-devel] [PATCH 05/10] qga: guest-pipe-open for Windows guest Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-07-07 1:31 ` Michael Roth
2015-06-30 10:25 ` [Qemu-devel] [PATCH 07/10] qga: added empty qmp_quest_get_fsinfo functionality Denis V. Lunev
` (3 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 07/10] qga: added empty qmp_quest_get_fsinfo functionality.
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (5 preceding siblings ...)
2015-06-30 10:25 ` [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 08/10] qga: added mountpoint and filesystem type for single volume Denis V. Lunev
` (2 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@virtuozzo.com>
We need qmp_quest_get_fsinfo togather with vss-provider, which works with
volumes. The call to this function is implemented via
FindFirst/NextVolumes. Moreover, volumes in Windows OS are filesystem unit,
so it will be more effective to work with them rather with devices.
Signed-off-by: Olga Krishtal <okrishtal@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 | 32 +++++++++++++++++++++++++++++---
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index ad445d9..0544ee4 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -766,10 +766,36 @@ done:
return ge;
}
+static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
+{
+ return NULL;
+}
+
GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
{
- error_setg(errp, QERR_UNSUPPORTED);
- return NULL;
+ HANDLE vol_h;
+ GuestFilesystemInfoList *new, *ret = NULL;
+ char guid[256];
+
+ vol_h = FindFirstVolume(guid, sizeof(guid));
+ if (vol_h == INVALID_HANDLE_VALUE) {
+ error_setg_win32(errp, GetLastError(), "failed to find any volume");
+ return NULL;
+ }
+
+ do {
+ new = g_malloc(sizeof(*ret));
+ new->value = build_guest_fsinfo(guid, errp);
+ new->next = ret;
+ ret = new;
+ } while (FindNextVolume(vol_h, guid, sizeof(guid)));
+
+ if (GetLastError() != ERROR_NO_MORE_FILES) {
+ error_setg_win32(errp, GetLastError(), "failed to find next volume");
+ }
+
+ FindVolumeClose(vol_h);
+ return ret;
}
/*
@@ -1096,7 +1122,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
"guest-set-user-password",
"guest-get-memory-blocks", "guest-set-memory-blocks",
"guest-get-memory-block-size",
- "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
+ "guest-fsfreeze-freeze-list",
"guest-fstrim", NULL};
char **p = (char **)list_unsupported;
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 08/10] qga: added mountpoint and filesystem type for single volume
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (6 preceding siblings ...)
2015-06-30 10:25 ` [Qemu-devel] [PATCH 07/10] qga: added empty qmp_quest_get_fsinfo functionality Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 10/10] qga: added GuestPCIAddress information Denis V. Lunev
9 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@virtuozzo.com>
We should use GetVolumeXXX api to work with volumes. This will help us to
resolve the situation with volumes without drive letter, i.e. when the
volume is mounted as a folder. Such volume is called mounted folder.
This volume is a regular mounted volume from all other points of view.
The information about non mounted volume is reported as System Reserved.
This volume is not mounted and thus it is not writable.
GuestDiskAddressList API is not used because operations are performed with
volumes but no with disks. This means that spanned disk will
be counted and handled as a single volume. It is worth mentioning
that the information about every disk in the volume can be queried
via IOCTL_VOLUME_GET_VOLUME_DISK_EXTENTS.
Signed-off-by: Olga Krishtal <okrishtal@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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 49 insertions(+), 2 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0544ee4..06e4426 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -768,7 +768,50 @@ done:
static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
{
- return NULL;
+ DWORD info_size;
+ char mnt, *mnt_point;
+ char fs_name[32];
+ char vol_info[MAX_PATH+1];
+ size_t len;
+ GuestFilesystemInfo *fs = NULL;
+
+ GetVolumePathNamesForVolumeName(guid, (LPCH)&mnt, 0, &info_size);
+ if (GetLastError() != ERROR_MORE_DATA) {
+ error_setg_win32(errp, GetLastError(), "failed to get volume name");
+ return NULL;
+ }
+
+ mnt_point = g_malloc(info_size + 1);
+ if (!GetVolumePathNamesForVolumeName(guid, mnt_point, info_size,
+ &info_size)) {
+ error_setg_win32(errp, GetLastError(), "failed to get volume name");
+ goto free;
+ }
+
+ len = strlen(mnt_point);
+ mnt_point[len] = '\\';
+ mnt_point[len+1] = 0;
+ if (!GetVolumeInformation(mnt_point, vol_info, sizeof(vol_info), NULL, NULL,
+ NULL, (LPSTR)&fs_name, sizeof(fs_name))) {
+ if (GetLastError() != ERROR_NOT_READY) {
+ error_setg_win32(errp, GetLastError(), "failed to get volume info");
+ }
+ goto free;
+ }
+
+ fs_name[sizeof(fs_name) - 1] = 0;
+ fs = g_malloc(sizeof(*fs));
+ fs->name = g_strdup(guid);
+ if (len == 0) {
+ fs->mountpoint = g_strdup("System Reserved");
+ } else {
+ fs->mountpoint = g_strndup(mnt_point, len);
+ }
+ fs->type = g_strdup(fs_name);
+ fs->disk = NULL;
+free:
+ g_free(mnt_point);
+ return fs;
}
GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
@@ -784,8 +827,12 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
}
do {
+ GuestFilesystemInfo *info = build_guest_fsinfo(guid, errp);
+ if (info == NULL) {
+ continue;
+ }
new = g_malloc(sizeof(*ret));
- new->value = build_guest_fsinfo(guid, errp);
+ new->value = info;
new->next = ret;
ret = new;
} while (FindNextVolume(vol_h, guid, sizeof(guid)));
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (7 preceding siblings ...)
2015-06-30 10:25 ` [Qemu-devel] [PATCH 08/10] qga: added mountpoint and filesystem type for single volume Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 10/10] qga: added GuestPCIAddress information Denis V. Lunev
9 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@virtuozzo.com>
According to Microsoft disk location path can be obtained via
IOCTL_SCSI_GET_ADDRESS. Unfortunately this ioctl can not be used for all
devices. There are certain bus types which could be obtained with this
API. Please, refer to the following link for more details
https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx
Bus type could be obtained using IOCTL_STORAGE_QUERY_PROPERTY. Enum
STORAGE_BUS_TYPE describes all buses supported by OS.
Windows defines more bus types than Linux. Thus some values have been added
to GuestDiskBusType.
Signed-off-by: Olga Krishtal <okrishtal@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 | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++-
qga/qapi-schema.json | 14 +++++-
2 files changed, 134 insertions(+), 2 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 06e4426..8cd7f70 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -16,6 +16,8 @@
#include <powrprof.h>
#include <stdio.h>
#include <string.h>
+#include <winioctl.h>
+#include <ntddscsi.h>
#include "qga/guest-agent-core.h"
#include "qga/vss-win32.h"
#include "qga-qmp-commands.h"
@@ -86,6 +88,37 @@ static OpenFlags *find_open_flag(const char *mode_str)
return NULL;
}
+static STORAGE_BUS_TYPE win2qemu[] = {
+ [BusTypeUnknown] = GUEST_DISK_BUS_TYPE_UNKNOWN,
+ [BusTypeScsi] = GUEST_DISK_BUS_TYPE_SCSI,
+ [BusTypeAtapi] = GUEST_DISK_BUS_TYPE_IDE,
+ [BusTypeAta] = GUEST_DISK_BUS_TYPE_IDE,
+ [BusType1394] = GUEST_DISK_BUS_TYPE_IEEE1394,
+ [BusTypeSsa] = GUEST_DISK_BUS_TYPE_SSA,
+ [BusTypeFibre] = GUEST_DISK_BUS_TYPE_SSA,
+ [BusTypeUsb] = GUEST_DISK_BUS_TYPE_USB,
+ [BusTypeRAID] = GUEST_DISK_BUS_TYPE_RAID,
+#if (_WIN32_WINNT >= 0x0600)
+ [BusTypeiScsi] = GUEST_DISK_BUS_TYPE_ISCSI,
+ [BusTypeSas] = GUEST_DISK_BUS_TYPE_SAS,
+ [BusTypeSata] = GUEST_DISK_BUS_TYPE_SATA,
+ [BusTypeSd] = GUEST_DISK_BUS_TYPE_SD,
+ [BusTypeMmc] = GUEST_DISK_BUS_TYPE_MMC,
+#endif
+#if (_WIN32_WINNT >= 0x0601)
+ [BusTypeVirtual] = GUEST_DISK_BUS_TYPE_VIRTUAL,
+ [BusTypeFileBackedVirtual] = GUEST_DISK_BUS_TYPE_FILE_BACKED_VIRTUAL,
+#endif
+};
+
+static GuestDiskBusType find_bus_type(STORAGE_BUS_TYPE bus)
+{
+ if (bus > ARRAY_SIZE(win2qemu) || (int)bus < 0) {
+ return GUEST_DISK_BUS_TYPE_UNKNOWN;
+ }
+ return win2qemu[(int)bus];
+}
+
static int64_t guest_file_handle_add(HANDLE fh, HANDLE pipe_other_end_fd,
Error **errp)
{
@@ -766,6 +799,93 @@ done:
return ge;
}
+static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
+{
+ return NULL;
+}
+
+static int get_disk_bus_type(HANDLE vol_h, Error **errp)
+{
+ STORAGE_PROPERTY_QUERY query;
+ STORAGE_DEVICE_DESCRIPTOR *dev_desc, buf;
+ DWORD received;
+
+ dev_desc = &buf;
+ dev_desc->Size = sizeof(buf);
+ query.PropertyId = StorageDeviceProperty;
+ query.QueryType = PropertyStandardQuery;
+
+ if (!DeviceIoControl(vol_h, IOCTL_STORAGE_QUERY_PROPERTY, &query,
+ sizeof(STORAGE_PROPERTY_QUERY), dev_desc,
+ dev_desc->Size, &received, NULL)) {
+ error_setg_win32(errp, GetLastError(), "failed to get bus type");
+ return -1;
+ }
+
+ return dev_desc->BusType;
+}
+
+/* VSS provider works with volumes, thus there is no difference if
+ * the volume consist of spanned disks. Info about the first disk in the
+ * volume is returned for the spanned disk group (LVM) */
+static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
+{
+ GuestDiskAddressList *list = NULL;
+ GuestDiskAddress *disk;
+ SCSI_ADDRESS addr, *scsi_ad;
+ DWORD len;
+ int bus;
+ HANDLE vol_h;
+
+ scsi_ad = &addr;
+ char *name = g_strndup(guid, strlen(guid)-1);
+
+ vol_h = CreateFile(name, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING,
+ NULL, NULL);
+ if (vol_h == INVALID_HANDLE_VALUE) {
+ error_setg_win32(errp, GetLastError(), "failed to open volume");
+ goto out_free;
+ }
+
+ bus = get_disk_bus_type(vol_h, errp);
+ if (bus < 0) {
+ goto out_close;
+ }
+
+ disk = g_malloc0(sizeof(*disk));
+ disk->bus_type = find_bus_type(bus);
+ if (bus == BusTypeScsi || bus == BusTypeAta || bus == BusTypeRAID
+#if (_WIN32_WINNT >= 0x0600)
+ /* This bus type is not supported before Windows Server 2003 SP1 */
+ || bus == BusTypeSas
+#endif
+ ) {
+ /* We are able to use the same ioctls for different bus types
+ * according to Microsoft docs
+ * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */
+ if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,
+ sizeof(SCSI_ADDRESS), &len, NULL)) {
+ disk->unit = addr.Lun;
+ disk->target = addr.TargetId;
+ disk->bus = addr.PathId;
+ disk->pci_controller = get_pci_info(name, errp);
+ }
+ /* We do not set error in this case, because we still have enough
+ * information about volume. */
+ } else {
+ disk->pci_controller = NULL;
+ }
+
+ list = g_malloc0(sizeof(*list));
+ list->value = disk;
+ list->next = NULL;
+out_close:
+ CloseHandle(vol_h);
+out_free:
+ g_free(name);
+ return list;
+}
+
static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
{
DWORD info_size;
@@ -808,7 +928,7 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
fs->mountpoint = g_strndup(mnt_point, len);
}
fs->type = g_strdup(fs_name);
- fs->disk = NULL;
+ fs->disk = build_guest_disk_info(guid, errp);;
free:
g_free(mnt_point);
return fs;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index dee36b6..409badd 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -720,12 +720,24 @@
# @uml: UML disks
# @sata: SATA disks
# @sd: SD cards
+# @unknown: Unknown bus type
+# @ieee1394: Win IEEE 1394 bus type
+# @ssa: Win SSA bus type
+# @fibre: Win fiber channel bus type
+# @raid: Win RAID bus type
+# @iscsi: Win iScsi bus type
+# @sas: Win serial-attaches SCSI bus type
+# @mmc: Win multimedia card (MMC) bus type
+# @virtual: Win virtual bus type
+# @file-backed virtual: Win file-backed bus type
#
# Since: 2.2
##
{ 'enum': 'GuestDiskBusType',
'data': [ 'ide', 'fdc', 'scsi', 'virtio', 'xen', 'usb', 'uml', 'sata',
- 'sd' ] }
+ 'sd', 'unknown', 'ieee1394', 'ssa', 'fibre', 'raid', 'iscsi',
+ 'sas', 'mmc', 'virtual', 'file-backed-virtual' ] }
+
##
# @GuestPCIAddress:
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 10/10] qga: added GuestPCIAddress information
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (8 preceding siblings ...)
2015-06-30 10:25 ` [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path Denis V. Lunev
@ 2015-06-30 10:25 ` Denis V. Lunev
9 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-06-30 10:25 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@virtuozzo.com>
PCIAddress inforfation is obtained via SetupApi, which provides the
information about address, bus, etc. We look throught entire device tree
in the system and try to find device object for given volume. For this PDO
SetupDiGetDeviceRegistryProperty is called, which reads PCI configuration
for a given devicei if it is possible.
This is the most convinient way for a userspace service. The lookup is
performed for every volume available. However, this information is
not mandatory for vss-provider.
In order to use SetupApi we need to notify linker about it. We do not need
to install additional libs, so we do not make separate configuration
option to use libsetupapi.su
SetupApi gives as the same information as kernel driver
with IRP_MN_QUERY_INTERFACE.
https://support.microsoft.com/en-us/kb/253232
Signed-off-by: Olga Krishtal <okrishtal@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>
---
configure | 2 +-
qga/commands-win32.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 97 insertions(+), 2 deletions(-)
diff --git a/configure b/configure
index 3063739..c6b2dfb 100755
--- a/configure
+++ b/configure
@@ -732,7 +732,7 @@ if test "$mingw32" = "yes" ; then
sysconfdir="\${prefix}"
local_statedir=
confsuffix=""
- libs_qga="-lws2_32 -lwinmm -lpowrprof $libs_qga"
+ libs_qga="-lsetupapi -lws2_32 -lwinmm -lpowrprof $libs_qga"
fi
werror=""
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 8cd7f70..a87c007 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -18,6 +18,8 @@
#include <string.h>
#include <winioctl.h>
#include <ntddscsi.h>
+#include <setupapi.h>
+#include <initguid.h>
#include "qga/guest-agent-core.h"
#include "qga/vss-win32.h"
#include "qga-qmp-commands.h"
@@ -29,6 +31,10 @@
#define SHTDN_REASON_FLAG_PLANNED 0x80000000
#endif
+DEFINE_GUID(GUID_DEVINTERFACE_VOLUME,
+ 0x53f5630dL, 0xb6bf, 0x11d0, 0x94, 0xf2,
+ 0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
+
/* multiple of 100 nanoseconds elapsed between windows baseline
* (1/1/1601) and Unix Epoch (1/1/1970), accounting for leap years */
#define W32_FT_OFFSET (10000000ULL * 60 * 60 * 24 * \
@@ -801,7 +807,96 @@ done:
static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
{
- return NULL;
+ HDEVINFO dev_info;
+ SP_DEVINFO_DATA dev_info_data;
+ DWORD size = 0;
+ int i;
+ char dev_name[MAX_PATH];
+ char *buffer = NULL;
+ GuestPCIAddress *pci = NULL;
+ char *name = g_strdup(&guid[4]);
+
+ if (!QueryDosDevice(name, dev_name, ARRAY_SIZE(dev_name))) {
+ error_setg_win32(errp, GetLastError(), "failed to get dos device name");
+ goto out;
+ }
+
+ dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_VOLUME, 0, 0,
+ DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
+ if (dev_info == INVALID_HANDLE_VALUE) {
+ error_setg_win32(errp, GetLastError(), "failed to get devices tree");
+ goto out;
+ }
+
+ dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
+ for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
+ DWORD addr, bus, slot, func, dev, data, size2;
+ while (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
+ SPDRP_PHYSICAL_DEVICE_OBJECT_NAME,
+ &data, (PBYTE)buffer, size,
+ &size2)) {
+ size = MAX(size, size2);
+ if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
+ g_free(buffer);
+ /* Double the size to avoid problems on
+ * W2k MBCS systems per KB 888609.
+ * https://support.microsoft.com/en-us/kb/259695 */
+ buffer = g_malloc(size * 2);
+ } else {
+ error_setg_win32(errp, GetLastError(),
+ "failed to get device name");
+ goto out;
+ }
+ }
+
+ if (g_strcmp0(buffer, dev_name)) {
+ continue;
+ }
+
+ /* There is no need to allocate buffer in the next functions. The size
+ * is known and ULONG according to
+ * https://support.microsoft.com/en-us/kb/253232
+ * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx
+ */
+ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
+ SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) {
+ break;
+ }
+
+ /* The function retrieves the device's address. This value will be
+ * transformed into device function and number */
+ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
+ SPDRP_ADDRESS, &data, (PBYTE)&addr, size, NULL)) {
+ break;
+ }
+
+ /* This call returns UINumber of DEVICE_CAPABILITIES structure.
+ * This number is typically a user-perceived slot number. */
+ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
+ SPDRP_UI_NUMBER, &data, (PBYTE)&slot, size, NULL)) {
+ break;
+ }
+
+ /* SetupApi gives us the same information as driver with
+ * IoGetDeviceProperty. According to Microsoft
+ * https://support.microsoft.com/en-us/kb/253232
+ * FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF);
+ * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF);
+ * SPDRP_ADDRESS is propertyAddress, so we do the same.*/
+
+ func = addr & 0x0000FFFF;
+ dev = (addr >> 16) & 0x0000FFFF;
+ pci = g_malloc0(sizeof(*pci));
+ pci->domain = dev;
+ pci->slot = slot;
+ pci->function = func;
+ pci->bus = bus;
+ break;
+ }
+out:
+ g_free(buffer);
+ g_free(name);
+ return pci;
}
static int get_disk_bus_type(HANDLE vol_h, Error **errp)
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-06-30 10:25 ` [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests Denis V. Lunev
@ 2015-07-07 1:31 ` Michael Roth
2015-07-07 8:06 ` Denis V. Lunev
0 siblings, 1 reply; 27+ messages in thread
From: Michael Roth @ 2015-07-07 1:31 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-07-07 1:31 ` Michael Roth
@ 2015-07-07 8:06 ` Denis V. Lunev
2015-07-07 9:12 ` Olga Krishtal
2015-07-08 22:02 ` Michael Roth
0 siblings, 2 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-07-07 8:06 UTC (permalink / raw)
To: Michael Roth; +Cc: Olga Krishtal, qemu-devel
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
>>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags
2015-06-30 10:25 ` [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
@ 2015-07-07 8:19 ` Denis V. Lunev
2015-07-08 21:26 ` Michael Roth
2015-07-08 21:16 ` Michael Roth
1 sibling, 1 reply; 27+ messages in thread
From: Denis V. Lunev @ 2015-07-07 8:19 UTC (permalink / raw)
Cc: Olga Krishtal, qemu-devel, Michael Roth
On 30/06/15 13:25, Denis V. Lunev wrote:
> From: Olga Krishtal <okrishtal@virtuozzo.com>
>
> guest_file_toggle_flags is a copy from semi-portable qemu_set_nonblock.
> The latter is not working properly for Windows due to reduced Windows
> Posix implementation.
>
> On Windows OS there is a separate API for changing flags of file, pipes
> and sockets. Portable way to change file descriptor flags requires
> to detect file descriptor type and proper actions depending of that
> type. The patch adds wrapper qemu_set_fd_nonblocking into Windows specific
> code to handle this stuff properly.
>
> The only problem is that qemu_set_nonblock is void but this should not
> be a problem.
>
> Signed-off-by: Olga Krishtal <okrishtal@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>
Michael, Eric,
can you please consider merging of this patch. This is a
semi-independent cleanup which has sense on its own.
Den
> ---
> qga/commands-posix.c | 27 ++-------------------------
> util/oslib-win32.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index befd00b..40dbe25 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -28,6 +28,7 @@
> #include "qapi/qmp/qerror.h"
> #include "qemu/queue.h"
> #include "qemu/host-utils.h"
> +#include "qemu/sockets.h"
>
> #ifndef CONFIG_HAS_ENVIRON
> #ifdef __APPLE__
> @@ -376,27 +377,6 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
> return NULL;
> }
>
> -static int guest_file_toggle_flags(int fd, int flags, bool set, Error **err)
> -{
> - int ret, old_flags;
> -
> - old_flags = fcntl(fd, F_GETFL);
> - if (old_flags == -1) {
> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
> - "failed to fetch filehandle flags");
> - return -1;
> - }
> -
> - ret = fcntl(fd, F_SETFL, set ? (old_flags | flags) : (old_flags & ~flags));
> - if (ret == -1) {
> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
> - "failed to set filehandle flags");
> - return -1;
> - }
> -
> - return ret;
> -}
> -
> int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
> Error **errp)
> {
> @@ -417,10 +397,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
> /* set fd non-blocking to avoid common use cases (like reading from a
> * named pipe) from hanging the agent
> */
> - if (guest_file_toggle_flags(fileno(fh), O_NONBLOCK, true, errp) < 0) {
> - fclose(fh);
> - return -1;
> - }
> + qemu_set_nonblock(fileno(fh));
>
> handle = guest_file_handle_add(fh, errp);
> if (handle < 0) {
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 730a670..1a6ae72 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -119,17 +119,59 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
> return p;
> }
>
> -void qemu_set_block(int fd)
> +static void qemu_set_fd_nonblocking(int fd, bool nonblocking)
> {
> - unsigned long opt = 0;
> - WSAEventSelect(fd, NULL, 0);
> + HANDLE handle;
> + DWORD file_type, pipe_state;
> +
> + handle = (HANDLE)_get_osfhandle(fd);
> + if (handle == INVALID_HANDLE_VALUE) {
> + return;
> + }
> +
> + file_type = GetFileType(handle);
> + if (file_type != FILE_TYPE_PIPE) {
> + return;
> + }
> +
> + /* If file_type == FILE_TYPE_PIPE, according to msdn
> + * the specified file is socket or named pipe */
> + if (GetNamedPipeHandleState(handle, &pipe_state, NULL,
> + NULL, NULL, NULL, 0)) {
> + /* The fd is named pipe fd */
> + if (!nonblocking == !(pipe_state & PIPE_NOWAIT)) {
> + /* In this case we do not need perform any operation, because
> + * nonblocking = true and PIPE_NOWAIT is already set or
> + * nonblocking = false and PIPE_NOWAIT is not set */
> + return;
> + }
> +
> + if (nonblocking) {
> + pipe_state |= PIPE_NOWAIT;
> + } else {
> + pipe_state &= ~PIPE_NOWAIT;
> + }
> +
> + SetNamedPipeHandleState(handle, &pipe_state, NULL, NULL);
> + return;
> + }
> +
> + /* The fd is socket fd */
> + unsigned long opt = (unsigned long)nonblocking;
> + if (!nonblocking) {
> + WSAEventSelect(fd, NULL, 0);
> + }
> ioctlsocket(fd, FIONBIO, &opt);
> }
>
> +void qemu_set_block(int fd)
> +{
> + qemu_set_fd_nonblocking(fd, false);
> +}
> +
> void qemu_set_nonblock(int fd)
> {
> - unsigned long opt = 1;
> - ioctlsocket(fd, FIONBIO, &opt);
> + qemu_set_fd_nonblocking(fd, true);
> qemu_fd_register(fd);
> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-07-07 8:06 ` Denis V. Lunev
@ 2015-07-07 9:12 ` Olga Krishtal
2015-07-07 9:59 ` Denis V. Lunev
2015-07-07 10:07 ` Olga Krishtal
2015-07-08 22:02 ` Michael Roth
1 sibling, 2 replies; 27+ messages in thread
From: Olga Krishtal @ 2015-07-07 9:12 UTC (permalink / raw)
To: Denis V. Lunev, Michael Roth; +Cc: Olga Krishtal, qemu-devel
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
>>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-07-07 9:12 ` Olga Krishtal
@ 2015-07-07 9:59 ` Denis V. Lunev
2015-07-07 10:07 ` Olga Krishtal
1 sibling, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-07-07 9:59 UTC (permalink / raw)
To: Olga Krishtal, Michael Roth; +Cc: Olga Krishtal, qemu-devel
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.
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-07-07 9:12 ` Olga Krishtal
2015-07-07 9:59 ` Denis V. Lunev
@ 2015-07-07 10:07 ` Olga Krishtal
1 sibling, 0 replies; 27+ messages in thread
From: Olga Krishtal @ 2015-07-07 10:07 UTC (permalink / raw)
To: Denis V. Lunev, Michael Roth; +Cc: Olga Krishtal, qemu-devel
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
>>>>
>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags
2015-06-30 10:25 ` [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
2015-07-07 8:19 ` Denis V. Lunev
@ 2015-07-08 21:16 ` Michael Roth
2015-07-08 22:40 ` Denis V. Lunev
1 sibling, 1 reply; 27+ messages in thread
From: Michael Roth @ 2015-07-08 21:16 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel
Quoting Denis V. Lunev (2015-06-30 05:25:14)
> From: Olga Krishtal <okrishtal@virtuozzo.com>
>
> guest_file_toggle_flags is a copy from semi-portable qemu_set_nonblock.
> The latter is not working properly for Windows due to reduced Windows
> Posix implementation.
>
> On Windows OS there is a separate API for changing flags of file, pipes
> and sockets. Portable way to change file descriptor flags requires
> to detect file descriptor type and proper actions depending of that
> type. The patch adds wrapper qemu_set_fd_nonblocking into Windows specific
> code to handle this stuff properly.
>
> The only problem is that qemu_set_nonblock is void but this should not
> be a problem.
>
> Signed-off-by: Olga Krishtal <okrishtal@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-posix.c | 27 ++-------------------------
> util/oslib-win32.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index befd00b..40dbe25 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -28,6 +28,7 @@
> #include "qapi/qmp/qerror.h"
> #include "qemu/queue.h"
> #include "qemu/host-utils.h"
> +#include "qemu/sockets.h"
>
> #ifndef CONFIG_HAS_ENVIRON
> #ifdef __APPLE__
> @@ -376,27 +377,6 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
> return NULL;
> }
>
> -static int guest_file_toggle_flags(int fd, int flags, bool set, Error **err)
> -{
> - int ret, old_flags;
> -
> - old_flags = fcntl(fd, F_GETFL);
> - if (old_flags == -1) {
> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
> - "failed to fetch filehandle flags");
> - return -1;
> - }
> -
> - ret = fcntl(fd, F_SETFL, set ? (old_flags | flags) : (old_flags & ~flags));
> - if (ret == -1) {
> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
> - "failed to set filehandle flags");
> - return -1;
> - }
> -
> - return ret;
> -}
> -
> int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
> Error **errp)
> {
> @@ -417,10 +397,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
> /* set fd non-blocking to avoid common use cases (like reading from a
> * named pipe) from hanging the agent
> */
> - if (guest_file_toggle_flags(fileno(fh), O_NONBLOCK, true, errp) < 0) {
> - fclose(fh);
> - return -1;
> - }
> + qemu_set_nonblock(fileno(fh));
>
> handle = guest_file_handle_add(fh, errp);
> if (handle < 0) {
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 730a670..1a6ae72 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -119,17 +119,59 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
> return p;
> }
>
> -void qemu_set_block(int fd)
> +static void qemu_set_fd_nonblocking(int fd, bool nonblocking)
> {
> - unsigned long opt = 0;
> - WSAEventSelect(fd, NULL, 0);
> + HANDLE handle;
> + DWORD file_type, pipe_state;
> +
> + handle = (HANDLE)_get_osfhandle(fd);
> + if (handle == INVALID_HANDLE_VALUE) {
> + return;
> + }
> +
> + file_type = GetFileType(handle);
> + if (file_type != FILE_TYPE_PIPE) {
> + return;
> + }
> +
> + /* If file_type == FILE_TYPE_PIPE, according to msdn
> + * the specified file is socket or named pipe */
> + if (GetNamedPipeHandleState(handle, &pipe_state, NULL,
> + NULL, NULL, NULL, 0)) {
> + /* The fd is named pipe fd */
> + if (!nonblocking == !(pipe_state & PIPE_NOWAIT)) {
> + /* In this case we do not need perform any operation, because
> + * nonblocking = true and PIPE_NOWAIT is already set or
> + * nonblocking = false and PIPE_NOWAIT is not set */
> + return;
> + }
> +
> + if (nonblocking) {
> + pipe_state |= PIPE_NOWAIT;
> + } else {
> + pipe_state &= ~PIPE_NOWAIT;
> + }
> +
> + SetNamedPipeHandleState(handle, &pipe_state, NULL, NULL);
> + return;
> + }
> +
> + /* The fd is socket fd */
> + unsigned long opt = (unsigned long)nonblocking;
> + if (!nonblocking) {
> + WSAEventSelect(fd, NULL, 0);
> + }
> ioctlsocket(fd, FIONBIO, &opt);
> }
>
> +void qemu_set_block(int fd)
> +{
> + qemu_set_fd_nonblocking(fd, false);
> +}
> +
> void qemu_set_nonblock(int fd)
> {
> - unsigned long opt = 1;
> - ioctlsocket(fd, FIONBIO, &opt);
> + qemu_set_fd_nonblocking(fd, true);
> qemu_fd_register(fd);
We wouldn't want to pass a non-socket FD to qemu_fd_register(), so
this should get moved to qemu_set_fd_nonblocking() at least.
I think it's confusing to have a qemu_set_nonblock() that's limited
to pipes and sockets. At least with the current code it's fairly
obvious it's intended for sockets.
It might be worthwhile if we could at least cover disk files, but
I seem to recall that being non-trivial on w32 and generally
requiring non-posix functions that support overlapped IO. So, if
we're only extending this to support pipes it should probably just
be a separate helper in qga/.
> }
>
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags
2015-07-07 8:19 ` Denis V. Lunev
@ 2015-07-08 21:26 ` Michael Roth
0 siblings, 0 replies; 27+ messages in thread
From: Michael Roth @ 2015-07-08 21:26 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel
Quoting Denis V. Lunev (2015-07-07 03:19:07)
> On 30/06/15 13:25, Denis V. Lunev wrote:
> > From: Olga Krishtal <okrishtal@virtuozzo.com>
> >
> > guest_file_toggle_flags is a copy from semi-portable qemu_set_nonblock.
> > The latter is not working properly for Windows due to reduced Windows
> > Posix implementation.
> >
> > On Windows OS there is a separate API for changing flags of file, pipes
> > and sockets. Portable way to change file descriptor flags requires
> > to detect file descriptor type and proper actions depending of that
> > type. The patch adds wrapper qemu_set_fd_nonblocking into Windows specific
> > code to handle this stuff properly.
> >
> > The only problem is that qemu_set_nonblock is void but this should not
> > be a problem.
> >
> > Signed-off-by: Olga Krishtal <okrishtal@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>
> Michael, Eric,
>
> can you please consider merging of this patch. This is a
> semi-independent cleanup which has sense on its own.
I think this is more a "feature" in the sense that it's adding
support for pipes to qemu_set_nonblock(). Since the
guest-open-pipe* stuff is likely the first user of the feature
it should probably wait on those.
>
> Den
>
>
> > ---
> > qga/commands-posix.c | 27 ++-------------------------
> > util/oslib-win32.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
> > 2 files changed, 49 insertions(+), 30 deletions(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index befd00b..40dbe25 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -28,6 +28,7 @@
> > #include "qapi/qmp/qerror.h"
> > #include "qemu/queue.h"
> > #include "qemu/host-utils.h"
> > +#include "qemu/sockets.h"
> >
> > #ifndef CONFIG_HAS_ENVIRON
> > #ifdef __APPLE__
> > @@ -376,27 +377,6 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
> > return NULL;
> > }
> >
> > -static int guest_file_toggle_flags(int fd, int flags, bool set, Error **err)
> > -{
> > - int ret, old_flags;
> > -
> > - old_flags = fcntl(fd, F_GETFL);
> > - if (old_flags == -1) {
> > - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
> > - "failed to fetch filehandle flags");
> > - return -1;
> > - }
> > -
> > - ret = fcntl(fd, F_SETFL, set ? (old_flags | flags) : (old_flags & ~flags));
> > - if (ret == -1) {
> > - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
> > - "failed to set filehandle flags");
> > - return -1;
> > - }
> > -
> > - return ret;
> > -}
> > -
> > int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
> > Error **errp)
> > {
> > @@ -417,10 +397,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
> > /* set fd non-blocking to avoid common use cases (like reading from a
> > * named pipe) from hanging the agent
> > */
> > - if (guest_file_toggle_flags(fileno(fh), O_NONBLOCK, true, errp) < 0) {
> > - fclose(fh);
> > - return -1;
> > - }
> > + qemu_set_nonblock(fileno(fh));
> >
> > handle = guest_file_handle_add(fh, errp);
> > if (handle < 0) {
> > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > index 730a670..1a6ae72 100644
> > --- a/util/oslib-win32.c
> > +++ b/util/oslib-win32.c
> > @@ -119,17 +119,59 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
> > return p;
> > }
> >
> > -void qemu_set_block(int fd)
> > +static void qemu_set_fd_nonblocking(int fd, bool nonblocking)
> > {
> > - unsigned long opt = 0;
> > - WSAEventSelect(fd, NULL, 0);
> > + HANDLE handle;
> > + DWORD file_type, pipe_state;
> > +
> > + handle = (HANDLE)_get_osfhandle(fd);
> > + if (handle == INVALID_HANDLE_VALUE) {
> > + return;
> > + }
> > +
> > + file_type = GetFileType(handle);
> > + if (file_type != FILE_TYPE_PIPE) {
> > + return;
> > + }
> > +
> > + /* If file_type == FILE_TYPE_PIPE, according to msdn
> > + * the specified file is socket or named pipe */
> > + if (GetNamedPipeHandleState(handle, &pipe_state, NULL,
> > + NULL, NULL, NULL, 0)) {
> > + /* The fd is named pipe fd */
> > + if (!nonblocking == !(pipe_state & PIPE_NOWAIT)) {
> > + /* In this case we do not need perform any operation, because
> > + * nonblocking = true and PIPE_NOWAIT is already set or
> > + * nonblocking = false and PIPE_NOWAIT is not set */
> > + return;
> > + }
> > +
> > + if (nonblocking) {
> > + pipe_state |= PIPE_NOWAIT;
> > + } else {
> > + pipe_state &= ~PIPE_NOWAIT;
> > + }
> > +
> > + SetNamedPipeHandleState(handle, &pipe_state, NULL, NULL);
> > + return;
> > + }
> > +
> > + /* The fd is socket fd */
> > + unsigned long opt = (unsigned long)nonblocking;
> > + if (!nonblocking) {
> > + WSAEventSelect(fd, NULL, 0);
> > + }
> > ioctlsocket(fd, FIONBIO, &opt);
> > }
> >
> > +void qemu_set_block(int fd)
> > +{
> > + qemu_set_fd_nonblocking(fd, false);
> > +}
> > +
> > void qemu_set_nonblock(int fd)
> > {
> > - unsigned long opt = 1;
> > - ioctlsocket(fd, FIONBIO, &opt);
> > + qemu_set_fd_nonblocking(fd, true);
> > qemu_fd_register(fd);
> > }
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-07-07 8:06 ` Denis V. Lunev
2015-07-07 9:12 ` Olga Krishtal
@ 2015-07-08 22:02 ` Michael Roth
2015-07-08 22:47 ` Denis V. Lunev
1 sibling, 1 reply; 27+ messages in thread
From: Michael Roth @ 2015-07-08 22:02 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags
2015-07-08 21:16 ` Michael Roth
@ 2015-07-08 22:40 ` Denis V. Lunev
2015-07-09 0:09 ` Michael Roth
0 siblings, 1 reply; 27+ messages in thread
From: Denis V. Lunev @ 2015-07-08 22:40 UTC (permalink / raw)
To: Michael Roth; +Cc: Olga Krishtal, qemu-devel
On 09/07/15 00:16, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-06-30 05:25:14)
>> From: Olga Krishtal <okrishtal@virtuozzo.com>
>>
>> guest_file_toggle_flags is a copy from semi-portable qemu_set_nonblock.
>> The latter is not working properly for Windows due to reduced Windows
>> Posix implementation.
>>
>> On Windows OS there is a separate API for changing flags of file, pipes
>> and sockets. Portable way to change file descriptor flags requires
>> to detect file descriptor type and proper actions depending of that
>> type. The patch adds wrapper qemu_set_fd_nonblocking into Windows specific
>> code to handle this stuff properly.
>>
>> The only problem is that qemu_set_nonblock is void but this should not
>> be a problem.
>>
>> Signed-off-by: Olga Krishtal <okrishtal@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-posix.c | 27 ++-------------------------
>> util/oslib-win32.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
>> 2 files changed, 49 insertions(+), 30 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index befd00b..40dbe25 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -28,6 +28,7 @@
>> #include "qapi/qmp/qerror.h"
>> #include "qemu/queue.h"
>> #include "qemu/host-utils.h"
>> +#include "qemu/sockets.h"
>>
>> #ifndef CONFIG_HAS_ENVIRON
>> #ifdef __APPLE__
>> @@ -376,27 +377,6 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
>> return NULL;
>> }
>>
>> -static int guest_file_toggle_flags(int fd, int flags, bool set, Error **err)
>> -{
>> - int ret, old_flags;
>> -
>> - old_flags = fcntl(fd, F_GETFL);
>> - if (old_flags == -1) {
>> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
>> - "failed to fetch filehandle flags");
>> - return -1;
>> - }
>> -
>> - ret = fcntl(fd, F_SETFL, set ? (old_flags | flags) : (old_flags & ~flags));
>> - if (ret == -1) {
>> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
>> - "failed to set filehandle flags");
>> - return -1;
>> - }
>> -
>> - return ret;
>> -}
>> -
>> int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
>> Error **errp)
>> {
>> @@ -417,10 +397,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
>> /* set fd non-blocking to avoid common use cases (like reading from a
>> * named pipe) from hanging the agent
>> */
>> - if (guest_file_toggle_flags(fileno(fh), O_NONBLOCK, true, errp) < 0) {
>> - fclose(fh);
>> - return -1;
>> - }
>> + qemu_set_nonblock(fileno(fh));
>>
>> handle = guest_file_handle_add(fh, errp);
>> if (handle < 0) {
>> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>> index 730a670..1a6ae72 100644
>> --- a/util/oslib-win32.c
>> +++ b/util/oslib-win32.c
>> @@ -119,17 +119,59 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
>> return p;
>> }
>>
>> -void qemu_set_block(int fd)
>> +static void qemu_set_fd_nonblocking(int fd, bool nonblocking)
>> {
>> - unsigned long opt = 0;
>> - WSAEventSelect(fd, NULL, 0);
>> + HANDLE handle;
>> + DWORD file_type, pipe_state;
>> +
>> + handle = (HANDLE)_get_osfhandle(fd);
>> + if (handle == INVALID_HANDLE_VALUE) {
>> + return;
>> + }
>> +
>> + file_type = GetFileType(handle);
>> + if (file_type != FILE_TYPE_PIPE) {
>> + return;
>> + }
>> +
>> + /* If file_type == FILE_TYPE_PIPE, according to msdn
>> + * the specified file is socket or named pipe */
>> + if (GetNamedPipeHandleState(handle, &pipe_state, NULL,
>> + NULL, NULL, NULL, 0)) {
>> + /* The fd is named pipe fd */
>> + if (!nonblocking == !(pipe_state & PIPE_NOWAIT)) {
>> + /* In this case we do not need perform any operation, because
>> + * nonblocking = true and PIPE_NOWAIT is already set or
>> + * nonblocking = false and PIPE_NOWAIT is not set */
>> + return;
>> + }
>> +
>> + if (nonblocking) {
>> + pipe_state |= PIPE_NOWAIT;
>> + } else {
>> + pipe_state &= ~PIPE_NOWAIT;
>> + }
>> +
>> + SetNamedPipeHandleState(handle, &pipe_state, NULL, NULL);
>> + return;
>> + }
>> +
>> + /* The fd is socket fd */
>> + unsigned long opt = (unsigned long)nonblocking;
>> + if (!nonblocking) {
>> + WSAEventSelect(fd, NULL, 0);
>> + }
>> ioctlsocket(fd, FIONBIO, &opt);
>> }
>>
>> +void qemu_set_block(int fd)
>> +{
>> + qemu_set_fd_nonblocking(fd, false);
>> +}
>> +
>> void qemu_set_nonblock(int fd)
>> {
>> - unsigned long opt = 1;
>> - ioctlsocket(fd, FIONBIO, &opt);
>> + qemu_set_fd_nonblocking(fd, true);
>> qemu_fd_register(fd);
> We wouldn't want to pass a non-socket FD to qemu_fd_register(), so
> this should get moved to qemu_set_fd_nonblocking() at least.
>
> I think it's confusing to have a qemu_set_nonblock() that's limited
> to pipes and sockets. At least with the current code it's fairly
> obvious it's intended for sockets.
>
> It might be worthwhile if we could at least cover disk files, but
> I seem to recall that being non-trivial on w32 and generally
> requiring non-posix functions that support overlapped IO. So, if
> we're only extending this to support pipes it should probably just
> be a separate helper in qga/.
>
at my opinion you are a little bit wrong here. First of all, from
Linux point of view, O_NONBLOCK for ordinary file is noop, i.e.
there is no difference in behavior with and without this flag
for such descriptors.
This means that the write will take some time and will finish
without indefinite timeout. Sockets and pipes are different,
they can hang in read/write forever and thus the flag has
some value to avoid this.
The same applies for Windows platform too. Setting nonblock
here we want to avoid infinite hang and we do not want to
implement asynchronous IO, which is implemented using
overlapped API.
There are only 2 primitives in QGA which subjects to the rules
above - sockets and pipes. Thus this common helper makes
sense. If we will need more primitives - we will add proper
distinction into these calls.
Den
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-07-08 22:02 ` Michael Roth
@ 2015-07-08 22:47 ` Denis V. Lunev
2015-07-09 1:30 ` Michael Roth
0 siblings, 1 reply; 27+ messages in thread
From: Denis V. Lunev @ 2015-07-08 22:47 UTC (permalink / raw)
To: Michael Roth; +Cc: Olga Krishtal, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5051 bytes --]
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
[-- Attachment #2: qemu-sh.c --]
[-- Type: text/x-csrc, Size: 7114 bytes --]
/* A simple shell on top of qemu-ga. */
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <unistd.h>
#include <signal.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <sys/ioctl.h>
static struct {
const char *host_ga_socket;
const char *host_io_socket;
const char *guest_io_socket;
} conf;
static struct {
int gafd;
int iofd;
int guest_io_handle;
int guest_sh_pid;
} ctx;
static void ctx_cleanup(void);
static void usage(void)
{
printf("Usage: qemu-sh HOST_GA_SOCKET HOST_IO_SOCKET GUEST_IO_SOCKET\n");
}
static void dbg(const char *msg)
{
printf("qemu-sh: debug: %s\n", msg);
}
static void warn(const char *msg)
{
fprintf(stderr, "qemu-sh: warning: %s\n", msg);
}
static void error(const char *msg)
{
fprintf(stderr, "qemu-sh: error: %s\n", msg);
ctx_cleanup();
exit(1);
}
static void prepare_ga(const char *host_ga_socket)
{
struct sockaddr_un adr;
adr.sun_family = AF_UNIX;
if (strlen(host_ga_socket) >= sizeof(adr.sun_path)) {
error("too large path to unix socket");
}
strcpy(adr.sun_path, host_ga_socket);
ctx.gafd = socket(AF_UNIX, SOCK_STREAM, 0);
if (ctx.gafd == -1) {
error("socket() GA socket on host");
}
dbg("connecting to GA...");
if (connect(ctx.gafd, (struct sockaddr *)&adr, sizeof(struct sockaddr_un)) != 0) {
error("connect() to GA");
}
dbg("connected to GA");
}
static void prepare_io(const char *host_io_socket)
{
struct sockaddr_un adr;
adr.sun_family = AF_UNIX;
if (strlen(host_io_socket) >= sizeof(adr.sun_path)) {
error("too large path to unix socket");
}
strcpy(adr.sun_path, host_io_socket);
ctx.iofd = socket(AF_UNIX, SOCK_STREAM, 0);
if (ctx.iofd == -1) {
error("socket() IO socket on host");
}
dbg("connecting to IO socket...");
if (connect(ctx.iofd, (struct sockaddr *)&adr, sizeof(struct sockaddr_un)) != 0) {
error("connect() to GA");
}
dbg("connected to IO socket");
{
int n = 1;
ioctl(ctx.iofd, FIONBIO, &n);
}
}
static void prepare_guest_io(const char *guest_io_socket)
{
char *cmd;
int cmdlen, handle;
char buf[4096];
size_t nbuf = 0, r;
cmdlen = asprintf(&cmd,
"{\"execute\": \"guest-file-open\", \"arguments\":{\"path\":\"%s\", \"mode\":\"r+b\"}}",
guest_io_socket);
if (cmdlen == -1) {
error("asprintf()");
}
dbg("writing to GA...");
if (write(ctx.gafd, cmd, cmdlen) != cmdlen) {
error("write() to GA");
}
free(cmd);
/*{"return": 1000}*/
for (;;) {
dbg("reading from GA...");
r = read(ctx.gafd, buf + nbuf, sizeof(buf) - nbuf - 1);
if (r == -1 || r == 0) {
error("read() from GA");
}
nbuf += r;
buf[nbuf] = '\0';
if (!strncmp(buf, "{\"error", sizeof("{\"error") - 1)) {
error(buf);
}
if (sscanf(buf, "{\"return\": %u}", &handle) != EOF) {
break;
}
if (nbuf == sizeof(buf) - 1) {
error("too large response from GA");
}
}
ctx.guest_io_handle = handle;
dbg("got guest_io_handle");
}
static void close_guest_io(void)
{
char *cmd;
int cmdlen, handle;
char buf[4096];
size_t nbuf = 0, r;
cmdlen = asprintf(&cmd,
"{\"execute\": \"guest-file-close\", \"arguments\":{\"handle\":%u}}",
ctx.guest_io_handle);
if (cmdlen == -1) {
warn("asprintf()");
return;
}
dbg("writing to GA...");
if (write(ctx.gafd, cmd, cmdlen) != cmdlen) {
warn("write() to GA");
return;
}
free(cmd);
for (;;) {
dbg("reading from GA...");
r = read(ctx.gafd, buf + nbuf, sizeof(buf) - nbuf - 1);
if (r == -1 || r == 0) {
warn("read() from GA");
return;
}
nbuf += r;
buf[nbuf] = '\0';
if (!strncmp(buf, "{\"error", sizeof("{\"error") - 1)) {
warn(buf);
return;
}
break;
}
ctx.guest_io_handle = -1;
dbg("closed guest_io_handle");
}
static void close_guest_sh(void)
{
char *cmd;
int cmdlen, handle;
char buf[4096];
size_t nbuf = 0, r;
cmdlen = asprintf(&cmd,
"{\"execute\": \"guest-exec-status\", \"arguments\":{\"pid\":%u}}",
ctx.guest_sh_pid);
if (cmdlen == -1) {
warn("asprintf()");
return;
}
dbg("writing to GA...");
if (write(ctx.gafd, cmd, cmdlen) != cmdlen) {
warn("write() to GA");
return;
}
free(cmd);
for (;;) {
dbg("reading from GA...");
r = read(ctx.gafd, buf + nbuf, sizeof(buf) - nbuf - 1);
if (r == -1 || r == 0) {
warn("read() from GA");
return;
}
nbuf += r;
buf[nbuf] = '\0';
if (!strncmp(buf, "{\"error", sizeof("{\"error") - 1)) {
warn(buf);
return;
}
break;
}
ctx.guest_sh_pid = 0;
dbg("closed guest_sh_pid");
}
static void exec_shell()
{
char *cmd;
int cmdlen, pid;
char buf[4096];
size_t nbuf = 0, r;
cmdlen = asprintf(&cmd,
"{\"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);
if (cmdlen == -1) {
error("asprintf()");
}
dbg("writing to GA...");
if (write(ctx.gafd, cmd, cmdlen) != cmdlen) {
error("write() to GA");
}
free(cmd);
/*{"return": 2395}*/
for (;;) {
dbg("reading from GA...");
r = read(ctx.gafd, buf + nbuf, sizeof(buf) - nbuf - 1);
if (r == -1 || r == 0) {
error("read() from GA");
}
nbuf += r;
buf[nbuf] = '\0';
if (!strncmp(buf, "{\"error", sizeof("{\"error") - 1)) {
error(buf);
}
if (sscanf(buf, "{\"return\": %u}", &pid) != EOF) {
break;
}
if (nbuf == sizeof(buf) - 1) {
error("too large response from GA");
}
}
ctx.guest_sh_pid = pid;
dbg("got guest_sh_pid");
}
static void intr_handler(int signo)
{
dbg("signal caught");
}
static void ctx_cleanup(void)
{
if (ctx.guest_io_handle != 0) {
close_guest_io();
}
if (ctx.guest_sh_pid != 0) {
close_guest_sh();
}
if (ctx.iofd != -1) {
close(ctx.iofd);
ctx.iofd = -1;
}
if (ctx.gafd != -1) {
close(ctx.gafd);
ctx.gafd = -1;
}
}
static void ev_loop(void)
{
char buf[4096];
size_t r;
fd_set rset;
for (;;) {
FD_ZERO(&rset);
FD_SET(STDIN_FILENO, &rset);
FD_SET(ctx.iofd, &rset);
r = select(ctx.iofd + 1, &rset, NULL, NULL, NULL);
if (r == -1) {
if (errno == EINTR)
break;
error("select()");
}
if (FD_ISSET(ctx.iofd, &rset)) {
r = read(ctx.iofd, buf, sizeof(buf));
if (r == -1 && (errno == EAGAIN || errno == EWOULDBLOCK))
continue;
if (r == -1)
error("read() from socket");
write(STDOUT_FILENO, buf, r);
}
if (FD_ISSET(STDIN_FILENO, &rset)) {
r = read(STDIN_FILENO, buf, sizeof(buf));
if (r == -1)
error("read() from stdin");
write(ctx.iofd, buf, r);
}
}
}
int main(int argc, char **argv, char **envp)
{
if (argc != 4) {
usage();
return 0;
}
ctx.gafd = -1;
ctx.iofd = -1;
ctx.guest_io_handle = -1;
conf.host_ga_socket = argv[1];
conf.host_io_socket = argv[2];
conf.guest_io_socket = argv[3];
{
struct sigaction sa = {0};
sa.sa_handler = &intr_handler;
if (sigaction(SIGINT, &sa, NULL) != 0) {
error("sigaction()");
}
}
prepare_ga(conf.host_ga_socket);
prepare_io(conf.host_io_socket);
prepare_guest_io(conf.guest_io_socket);
exec_shell();
dbg("Shell's started. Press Ctrl+C to exit.");
ev_loop();
ctx_cleanup();
return 0;
}
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags
2015-07-08 22:40 ` Denis V. Lunev
@ 2015-07-09 0:09 ` Michael Roth
0 siblings, 0 replies; 27+ messages in thread
From: Michael Roth @ 2015-07-09 0:09 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel
Quoting Denis V. Lunev (2015-07-08 17:40:30)
> On 09/07/15 00:16, Michael Roth wrote:
> > Quoting Denis V. Lunev (2015-06-30 05:25:14)
> >> From: Olga Krishtal <okrishtal@virtuozzo.com>
> >>
> >> guest_file_toggle_flags is a copy from semi-portable qemu_set_nonblock.
> >> The latter is not working properly for Windows due to reduced Windows
> >> Posix implementation.
> >>
> >> On Windows OS there is a separate API for changing flags of file, pipes
> >> and sockets. Portable way to change file descriptor flags requires
> >> to detect file descriptor type and proper actions depending of that
> >> type. The patch adds wrapper qemu_set_fd_nonblocking into Windows specific
> >> code to handle this stuff properly.
> >>
> >> The only problem is that qemu_set_nonblock is void but this should not
> >> be a problem.
> >>
> >> Signed-off-by: Olga Krishtal <okrishtal@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-posix.c | 27 ++-------------------------
> >> util/oslib-win32.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >> 2 files changed, 49 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> >> index befd00b..40dbe25 100644
> >> --- a/qga/commands-posix.c
> >> +++ b/qga/commands-posix.c
> >> @@ -28,6 +28,7 @@
> >> #include "qapi/qmp/qerror.h"
> >> #include "qemu/queue.h"
> >> #include "qemu/host-utils.h"
> >> +#include "qemu/sockets.h"
> >>
> >> #ifndef CONFIG_HAS_ENVIRON
> >> #ifdef __APPLE__
> >> @@ -376,27 +377,6 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
> >> return NULL;
> >> }
> >>
> >> -static int guest_file_toggle_flags(int fd, int flags, bool set, Error **err)
> >> -{
> >> - int ret, old_flags;
> >> -
> >> - old_flags = fcntl(fd, F_GETFL);
> >> - if (old_flags == -1) {
> >> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
> >> - "failed to fetch filehandle flags");
> >> - return -1;
> >> - }
> >> -
> >> - ret = fcntl(fd, F_SETFL, set ? (old_flags | flags) : (old_flags & ~flags));
> >> - if (ret == -1) {
> >> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED,
> >> - "failed to set filehandle flags");
> >> - return -1;
> >> - }
> >> -
> >> - return ret;
> >> -}
> >> -
> >> int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
> >> Error **errp)
> >> {
> >> @@ -417,10 +397,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
> >> /* set fd non-blocking to avoid common use cases (like reading from a
> >> * named pipe) from hanging the agent
> >> */
> >> - if (guest_file_toggle_flags(fileno(fh), O_NONBLOCK, true, errp) < 0) {
> >> - fclose(fh);
> >> - return -1;
> >> - }
> >> + qemu_set_nonblock(fileno(fh));
> >>
> >> handle = guest_file_handle_add(fh, errp);
> >> if (handle < 0) {
> >> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> >> index 730a670..1a6ae72 100644
> >> --- a/util/oslib-win32.c
> >> +++ b/util/oslib-win32.c
> >> @@ -119,17 +119,59 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
> >> return p;
> >> }
> >>
> >> -void qemu_set_block(int fd)
> >> +static void qemu_set_fd_nonblocking(int fd, bool nonblocking)
> >> {
> >> - unsigned long opt = 0;
> >> - WSAEventSelect(fd, NULL, 0);
> >> + HANDLE handle;
> >> + DWORD file_type, pipe_state;
> >> +
> >> + handle = (HANDLE)_get_osfhandle(fd);
> >> + if (handle == INVALID_HANDLE_VALUE) {
> >> + return;
> >> + }
> >> +
> >> + file_type = GetFileType(handle);
> >> + if (file_type != FILE_TYPE_PIPE) {
> >> + return;
> >> + }
> >> +
> >> + /* If file_type == FILE_TYPE_PIPE, according to msdn
> >> + * the specified file is socket or named pipe */
> >> + if (GetNamedPipeHandleState(handle, &pipe_state, NULL,
> >> + NULL, NULL, NULL, 0)) {
> >> + /* The fd is named pipe fd */
> >> + if (!nonblocking == !(pipe_state & PIPE_NOWAIT)) {
> >> + /* In this case we do not need perform any operation, because
> >> + * nonblocking = true and PIPE_NOWAIT is already set or
> >> + * nonblocking = false and PIPE_NOWAIT is not set */
> >> + return;
> >> + }
> >> +
> >> + if (nonblocking) {
> >> + pipe_state |= PIPE_NOWAIT;
> >> + } else {
> >> + pipe_state &= ~PIPE_NOWAIT;
> >> + }
> >> +
> >> + SetNamedPipeHandleState(handle, &pipe_state, NULL, NULL);
> >> + return;
> >> + }
> >> +
> >> + /* The fd is socket fd */
> >> + unsigned long opt = (unsigned long)nonblocking;
> >> + if (!nonblocking) {
> >> + WSAEventSelect(fd, NULL, 0);
> >> + }
> >> ioctlsocket(fd, FIONBIO, &opt);
> >> }
> >>
> >> +void qemu_set_block(int fd)
> >> +{
> >> + qemu_set_fd_nonblocking(fd, false);
> >> +}
> >> +
> >> void qemu_set_nonblock(int fd)
> >> {
> >> - unsigned long opt = 1;
> >> - ioctlsocket(fd, FIONBIO, &opt);
> >> + qemu_set_fd_nonblocking(fd, true);
> >> qemu_fd_register(fd);
> > We wouldn't want to pass a non-socket FD to qemu_fd_register(), so
> > this should get moved to qemu_set_fd_nonblocking() at least.
> >
> > I think it's confusing to have a qemu_set_nonblock() that's limited
> > to pipes and sockets. At least with the current code it's fairly
> > obvious it's intended for sockets.
> >
> > It might be worthwhile if we could at least cover disk files, but
> > I seem to recall that being non-trivial on w32 and generally
> > requiring non-posix functions that support overlapped IO. So, if
> > we're only extending this to support pipes it should probably just
> > be a separate helper in qga/.
> >
> at my opinion you are a little bit wrong here. First of all, from
> Linux point of view, O_NONBLOCK for ordinary file is noop, i.e.
> there is no difference in behavior with and without this flag
> for such descriptors.
Yes, I think you're right... I'd forgotten that the reason we use it
in guest-file-open was for dealing with pipes/chardevs/sockets rather
than anything to do with files.
>
> This means that the write will take some time and will finish
> without indefinite timeout. Sockets and pipes are different,
> they can hang in read/write forever and thus the flag has
> some value to avoid this.
>
> The same applies for Windows platform too. Setting nonblock
> here we want to avoid infinite hang and we do not want to
> implement asynchronous IO, which is implemented using
> overlapped API.
>
> There are only 2 primitives in QGA which subjects to the rules
> above - sockets and pipes. Thus this common helper makes
> sense. If we will need more primitives - we will add proper
> distinction into these calls.
There's still an important invariant here in that O_NONBLOCK ensures
EAGAIN in cases where you might otherwise wait indefinitely. That
covers named/anonymous pipes, sockets, files, chardevs, any fd.
To take the socket-only w32 implementation and only tack on pipes
seems somewhat arbitrary in comparison, but I do understand your
logic from a "FDs that make sense to use it with" standpoint. I think
you'll need to Cc: our w32 maintainer Stefan Weil <sw@weilnetz.de>
for any such change in oslib-win32.c though.
Personally I think set_pipe_nonblock() in qga/commands-win32.c is
clearer, and it seems unlikely we'll ever end up with a 2nd w32
user of it so we don't stand to gain much from a generalized
function.
>
> Den
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-07-08 22:47 ` Denis V. Lunev
@ 2015-07-09 1:30 ` Michael Roth
2015-07-10 13:23 ` Denis V. Lunev
0 siblings, 1 reply; 27+ messages in thread
From: Michael Roth @ 2015-07-09 1:30 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests
2015-07-09 1:30 ` Michael Roth
@ 2015-07-10 13:23 ` Denis V. Lunev
0 siblings, 0 replies; 27+ messages in thread
From: Denis V. Lunev @ 2015-07-10 13:23 UTC (permalink / raw)
To: Michael Roth; +Cc: Olga Krishtal, qemu-devel
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
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-07-10 13:24 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-30 10:25 [Qemu-devel] [PATCH v6 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
2015-07-07 8:19 ` Denis V. Lunev
2015-07-08 21:26 ` Michael Roth
2015-07-08 21:16 ` Michael Roth
2015-07-08 22:40 ` Denis V. Lunev
2015-07-09 0:09 ` Michael Roth
2015-06-30 10:25 ` [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 03/10] qga: guest exec functionality for Unix guests Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 04/10] qga: handle possible SIGPIPE in guest-file-write Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 05/10] qga: guest-pipe-open for Windows guest Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests Denis V. Lunev
2015-07-07 1:31 ` Michael Roth
2015-07-07 8:06 ` Denis V. Lunev
2015-07-07 9:12 ` Olga Krishtal
2015-07-07 9:59 ` Denis V. Lunev
2015-07-07 10:07 ` Olga Krishtal
2015-07-08 22:02 ` Michael Roth
2015-07-08 22:47 ` Denis V. Lunev
2015-07-09 1:30 ` Michael Roth
2015-07-10 13:23 ` Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 07/10] qga: added empty qmp_quest_get_fsinfo functionality Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 08/10] qga: added mountpoint and filesystem type for single volume Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path Denis V. Lunev
2015-06-30 10:25 ` [Qemu-devel] [PATCH 10/10] qga: added GuestPCIAddress information Denis V. Lunev
-- strict thread matches above, loose matches on Subject: below --
2015-06-19 16:57 [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests Denis V. Lunev
2015-06-19 16:51 [Qemu-devel] [PATCH v4 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
2015-06-19 16:51 ` [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests Denis V. Lunev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).