* [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; 29+ 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] 29+ messages in thread
* [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec
@ 2015-06-19 16:57 Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
` (9 more replies)
0 siblings, 10 replies; 29+ 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
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 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@parallels.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] 29+ messages in thread
* [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags
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
2015-06-19 16:57 ` [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command Denis V. Lunev
` (8 subsequent siblings)
9 siblings, 0 replies; 29+ 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>
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_blocking 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 ba8de62..d0f371b 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_set_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_set_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..ab003b0 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_blocking(int fd, bool blocking)
{
- 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 (!!blocking == !(pipe_state & PIPE_NOWAIT)) {
+ /* In this case we do not need perform any operation, because
+ * blocking = true and PIPE_NOWAIT is already set or
+ * blocking = false and PIPE_NOWAIT is not set */
+ return;
+ }
+
+ if (blocking) {
+ 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)blocking;
+ if (!blocking) {
+ WSAEventSelect(fd, NULL, 0);
+ }
ioctlsocket(fd, FIONBIO, &opt);
}
+void qemu_set_block(int fd)
+{
+ qemu_set_fd_blocking(fd, false);
+}
+
void qemu_set_nonblock(int fd)
{
- unsigned long opt = 1;
- ioctlsocket(fd, FIONBIO, &opt);
+ qemu_set_fd_blocking(fd, true);
qemu_fd_register(fd);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command
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 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
2015-06-19 17:30 ` Eric Blake
2015-06-19 17:34 ` Eric Blake
2015-06-19 16:57 ` [Qemu-devel] [PATCH 03/10] qga: guest exec functionality for Unix guests Denis V. Lunev
` (7 subsequent siblings)
9 siblings, 2 replies; 29+ 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>
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 | 96 +++++++++++++++++++++++++++++++++++++++++++++++++---
qga/commands-win32.c | 8 ++++-
qga/qapi-schema.json | 44 ++++++++++++++++++++++++
3 files changed, 143 insertions(+), 5 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index d0f371b..a616996 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,85 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
return handle;
}
+GuestPipeInfoList *qmp_guest_pipe_open(GuestPipeMode mode, Error **errp)
+{
+ FILE *f = NULL;
+ int fd[2], this_end, other_end;
+ int64_t handle;
+ GuestPipeInfoList *pipe_list;
+ 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_set_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_set_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_list = g_malloc0(sizeof(*pipe_list));
+ pipe_list->value = pipe_inf;
+ pipe_list->next = NULL;
+ return pipe_list;
+
+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 +501,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 +2482,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 +2496,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 3ef0549..685dd0f 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;
}
+GuestPipeInfoList *qmp_guest_pipe_open(GuestPipeMode mode, Error **errp)
+{
+ error_set(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..8081213 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -215,12 +215,56 @@
'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'} }
+
+##
+# @guest-pipe-open
+#
+# Open a pipe to in the guest to associated with a qga-spawned processes
+# for communication.
+#
+# 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
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 03/10] qga: guest exec functionality for Unix 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 ` [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 04/10] qga: handle possible SIGPIPE in guest-file-write Denis V. Lunev
` (6 subsequent siblings)
9 siblings, 0 replies; 29+ 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-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 a616996..aa11932 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -828,6 +828,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_set(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_set(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)
@@ -2520,4 +2756,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 685dd0f..3db7255 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_set(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_set(errp, QERR_UNSUPPORTED);
+ return 0;
+}
+
GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
{
error_set(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 8081213..21997cf 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -935,3 +935,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' }
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 04/10] qga: handle possible SIGPIPE in guest-file-write
2015-06-19 16:57 [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (2 preceding siblings ...)
2015-06-19 16:57 ` [Qemu-devel] [PATCH 03/10] qga: guest exec functionality for Unix guests Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 05/10] qga: guest-pipe-open for Windows guest Denis V. Lunev
` (5 subsequent siblings)
9 siblings, 0 replies; 29+ 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>
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 aa11932..64d9b2d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -961,6 +961,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,
@@ -1032,6 +1046,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 9939a2b..bc6414c 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;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 05/10] qga: guest-pipe-open for Windows guest
2015-06-19 16:57 [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (3 preceding siblings ...)
2015-06-19 16:57 ` [Qemu-devel] [PATCH 04/10] qga: handle possible SIGPIPE in guest-file-write Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests Denis V. Lunev
` (4 subsequent siblings)
9 siblings, 0 replies; 29+ 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>
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 | 74 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 70 insertions(+), 4 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3db7255..b216628 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,68 @@ 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. */
GuestPipeInfoList *qmp_guest_pipe_open(GuestPipeMode mode, Error **errp)
{
- error_set(errp, QERR_UNSUPPORTED);
+ HANDLE fd[2];
+ int this_end, other_end;
+ int64_t handle;
+ GuestPipeInfoList *pipe_list;
+ 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_list = g_malloc0(sizeof(*pipe_list));
+ pipe_list->value = pipe_inf;
+ pipe_list->next = NULL;
+ return pipe_list;
+
+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 +228,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 +803,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;
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ 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
` (4 preceding siblings ...)
2015-06-19 16:57 ` [Qemu-devel] [PATCH 05/10] qga: guest-pipe-open for Windows guest Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 07/10] qga: added empty qmp_quest_get_fsinfo functionality Denis V. Lunev
` (3 subsequent siblings)
9 siblings, 0 replies; 29+ 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] 29+ messages in thread
* [Qemu-devel] [PATCH 07/10] qga: added empty qmp_quest_get_fsinfo functionality.
2015-06-19 16:57 [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (5 preceding siblings ...)
2015-06-19 16:57 ` [Qemu-devel] [PATCH 06/10] qga: guest exec functionality for Windows guests Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 08/10] qga: added mountpoint and filesystem type for single volume Denis V. Lunev
` (2 subsequent siblings)
9 siblings, 0 replies; 29+ 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>
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 | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index e633126..f484adc 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -769,10 +769,37 @@ done:
return ge;
}
+static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
+{
+ return NULL;
+}
+
GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
{
- error_set(errp, QERR_UNSUPPORTED);
- return NULL;
+ HANDLE vol_h;
+ GuestFilesystemInfoList *new, *ret = NULL;
+ char guid[256];
+ bool res = false;
+ 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;
+ res = FindNextVolume(vol_h, guid, sizeof(guid));
+ } while (GetLastError() != ERROR_NO_MORE_FILES && res);
+
+ if (!res && GetLastError() != ERROR_NO_MORE_FILES) {
+ error_setg_win32(errp, GetLastError(), "failed to find next volume");
+ }
+
+ FindVolumeClose(vol_h);
+ return ret;
}
/*
@@ -1099,7 +1126,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;
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 08/10] qga: added mountpoint and filesystem type for single volume
2015-06-19 16:57 [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (6 preceding siblings ...)
2015-06-19 16:57 ` [Qemu-devel] [PATCH 07/10] qga: added empty qmp_quest_get_fsinfo functionality Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 10/10] qga: added GuestPCIAddress information Denis V. Lunev
9 siblings, 0 replies; 29+ 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>
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 | 54 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 51 insertions(+), 3 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index f484adc..7acb258 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -771,7 +771,51 @@ 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];
+ bool res;
+ size_t len;
+ DWORD err_code;
+ GuestFilesystemInfo *fs;
+
+ 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_malloc0(sizeof(TCHAR)*info_size*2);
+ res = GetVolumePathNamesForVolumeName(guid, mnt_point, sizeof(mnt_point),
+ &info_size);
+ err_code = GetLastError();
+ if (!res && err_code != ERROR_SUCCESS && err_code != ERROR_MORE_DATA) {
+ error_setg_win32(errp, GetLastError(), "failed ro get volume name");
+ g_free(mnt_point);
+ return NULL;
+ }
+ len = strlen(mnt_point);
+ mnt_point[len] = '\\';
+ 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");
+ }
+ g_free(mnt_point);
+ return NULL;
+ }
+
+ fs_name[sizeof(fs_name) - 1] = 0;
+ fs = g_malloc(sizeof(*fs));
+ fs->name = g_strdup(guid);
+ fs->mountpoint = g_strndup(mnt_point, len);
+ fs->type = g_strdup(fs_name);
+ fs->disk = NULL;
+ g_free(mnt_point);
+ return fs;
+
}
GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
@@ -789,8 +833,12 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
do {
new = g_malloc(sizeof(*ret));
new->value = build_guest_fsinfo(guid, errp);
- new->next = ret;
- ret = new;
+ if (new->value != NULL) {
+ new->next = ret;
+ ret = new;
+ } else {
+ g_free(new);
+ }
res = FindNextVolume(vol_h, guid, sizeof(guid));
} while (GetLastError() != ERROR_NO_MORE_FILES && res);
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path
2015-06-19 16:57 [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (7 preceding siblings ...)
2015-06-19 16:57 ` [Qemu-devel] [PATCH 08/10] qga: added mountpoint and filesystem type for single volume Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
2015-06-19 17:10 ` Eric Blake
2015-06-19 16:57 ` [Qemu-devel] [PATCH 10/10] qga: added GuestPCIAddress information Denis V. Lunev
9 siblings, 1 reply; 29+ 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>
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 that 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 | 127 ++++++++++++++++++++++++++++++++++++++++++++++++++-
qga/qapi-schema.json | 15 +++++-
2 files changed, 139 insertions(+), 3 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7acb258..09f0e82 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,44 @@ static OpenFlags *find_open_flag(const char *mode_str)
return NULL;
}
+typedef struct WinToLin {
+ STORAGE_BUS_TYPE win;
+ GuestDiskBusType lin;
+} WinToLin;
+
+static WinToLin buses[] = {
+ {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_1394},
+ {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_I_SCSI},
+ {BusTypeSas, GUEST_DISK_BUS_TYPE_SAS},
+ {BusTypeSata, GUEST_DISK_BUS_TYPE_SATA},
+ {BusTypeSd, GUEST_DISK_BUS_TYPE_SD},
+ {BusTypeMmc, GUEST_DISK_BUS_TYPE_MMC},
+ {BusTypeVirtual, GUEST_DISK_BUS_TYPE_VIRTUAL },
+ {BusTypeFileBackedVirtuaul, GUEST_DISK_BUS_TYPE_FBIRTUAL},
+ {BusTypeSpaces, GUEST_DISK_BUS_TYPE_SPACES}
+#endif
+};
+
+static GuestDiskBusType find_bus_type(STORAGE_BUS_TYPE bus)
+{
+ int i;
+ for (i = 0; i < ARRAY_SIZE(buses); ++i) {
+ if (buses[i].win == bus) {
+ return buses[i].lin;
+ }
+ }
+ return GUEST_DISK_BUS_TYPE_MAX;
+}
+
static int64_t guest_file_handle_add(HANDLE fh, HANDLE pipe_other_end_fd,
Error **errp)
{
@@ -769,6 +809,91 @@ 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;
+}
+
+static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
+{
+ /* VSS provider works with volumes, so there is no difference if
+ * the volume consist of spanned disks. As the result we can obtain
+ * info for first disk in the spanned disks group. */
+
+ GuestDiskAddressList *list;
+ 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,
+ 0, NULL);
+ if (vol_h == INVALID_HANDLE_VALUE) {
+ error_setg_win32(errp, GetLastError(), "failed to open volume");
+ return NULL;
+ }
+ g_free(name);
+ bus = get_disk_bus_type(vol_h, errp);
+ if (bus < 0) {
+ return NULL;
+ }
+ 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 = scsi_ad->Lun;
+ disk->target = scsi_ad->TargetId;
+ disk->bus = scsi_ad->PathId;
+ disk->pci_controller = get_pci_info(name, errp);
+ }
+ /* We do 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;
+ CloseHandle(vol_h);
+ return list;
+
+}
+
static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
{
DWORD info_size;
@@ -812,7 +937,7 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
fs->name = g_strdup(guid);
fs->mountpoint = g_strndup(mnt_point, len);
fs->type = g_strdup(fs_name);
- fs->disk = NULL;
+ fs->disk = build_guest_disk_info(guid, errp);
g_free(mnt_point);
return fs;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 21997cf..59f6d9b 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -711,6 +711,7 @@
# @GuestDiskBusType
#
# An enumeration of bus type of disks
+# is devided a bit on Linux and Windows guest
#
# @ide: IDE disks
# @fdc: floppy disks
@@ -721,12 +722,22 @@
# @uml: UML disks
# @sata: SATA disks
# @sd: SD cards
-#
+# @Unknown: Unknown bus type
+# @1394: 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
+# @FB: Win file-backed bus type
# Since: 2.2
##
{ 'enum': 'GuestDiskBusType',
'data': [ 'ide', 'fdc', 'scsi', 'virtio', 'xen', 'usb', 'uml', 'sata',
- 'sd' ] }
+ 'sd', 'unknown', '1394','Ssa', 'fibre', 'RAID', 'iScsi', 'sas',
+ 'mmc', 'virtual', 'fbirtual' ] }
##
# @GuestPCIAddress:
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 10/10] qga: added GuestPCIAddress information
2015-06-19 16:57 [Qemu-devel] [PATCH v5 0/10] QGA: disk and volume info for Windows & guest exec Denis V. Lunev
` (8 preceding siblings ...)
2015-06-19 16:57 ` [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path Denis V. Lunev
@ 2015-06-19 16:57 ` Denis V. Lunev
9 siblings, 0 replies; 29+ 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>
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 | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index 222694f..bf84c5a 100755
--- a/configure
+++ b/configure
@@ -731,7 +731,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 09f0e82..f33ba7c 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 * \
@@ -811,6 +817,77 @@ done:
static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
{
+ HDEVINFO dev_info;
+ SP_DEVINFO_DATA dev_info_data;
+ int i;
+ char dev_name[MAX_PATH];
+ char *name = g_strdup(&guid[4]);
+ char *buffer = NULL;
+ if (!QueryDosDevice(name, dev_name, sizeof(dev_name)/sizeof(char))) {
+ error_setg_win32(errp, GetLastError(), "failed to get dos device name");
+ g_free(name);
+ return NULL;
+ }
+ g_free(name);
+
+ 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");
+ return NULL;
+ }
+ dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
+ for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
+ DWORD data, addr, bus, slot, func;
+ GuestPCIAddress *pci;
+ DWORD size = 0;
+
+ while (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
+ SPDRP_PHYSICAL_DEVICE_OBJECT_NAME,
+ &data, (PBYTE)buffer, size, &size)) {
+ if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
+ if (buffer) {
+ g_free(buffer);
+ }
+ buffer = g_malloc0(sizeof(char)*(size*2));
+ } else {
+ error_setg_win32(errp, GetLastError(),
+ "failed to get device name");
+ goto out;
+ }
+ }
+
+ if (g_strcmp0(buffer, dev_name)) {
+ continue;
+ }
+
+ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
+ SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, &size)) {
+ goto out;
+ }
+
+ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
+ SPDRP_ADDRESS, &data, (PBYTE)&addr, size, &size)) {
+ goto out;
+ }
+
+ slot = (addr >> 16) & 0xFFFF;
+ func = addr & 0xFFFF;
+ pci = g_malloc0(sizeof(*pci));
+ pci->domain = addr;
+ pci->slot = slot;
+ pci->function = func;
+ pci->bus = bus;
+
+ if (buffer) {
+ g_free(buffer);
+ }
+ return pci;
+ }
+out:
+ if (buffer) {
+ g_free(buffer);
+ }
return NULL;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path
2015-06-19 16:57 ` [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path Denis V. Lunev
@ 2015-06-19 17:10 ` Eric Blake
2015-06-19 18:02 ` Denis V. Lunev
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2015-06-19 17:10 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3728 bytes --]
On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
> 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 that Linux. Thus some values have been added
s/that/than/
> 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 | 127 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> qga/qapi-schema.json | 15 +++++-
> 2 files changed, 139 insertions(+), 3 deletions(-)
>
> +static WinToLin buses[] = {
> + {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_1394},
> + {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_I_SCSI},
> + {BusTypeSas, GUEST_DISK_BUS_TYPE_SAS},
> + {BusTypeSata, GUEST_DISK_BUS_TYPE_SATA},
> + {BusTypeSd, GUEST_DISK_BUS_TYPE_SD},
> + {BusTypeMmc, GUEST_DISK_BUS_TYPE_MMC},
> + {BusTypeVirtual, GUEST_DISK_BUS_TYPE_VIRTUAL },
> + {BusTypeFileBackedVirtuaul, GUEST_DISK_BUS_TYPE_FBIRTUAL},
> + {BusTypeSpaces, GUEST_DISK_BUS_TYPE_SPACES}
> +#endif
I'd use a trailing comma, so that future additions can be pure additions
rather than amending existing lines.
> +++ b/qga/qapi-schema.json
> @@ -711,6 +711,7 @@
> # @GuestDiskBusType
> #
> # An enumeration of bus type of disks
> +# is devided a bit on Linux and Windows guest
> #
s/devided/divided/
> # @ide: IDE disks
> # @fdc: floppy disks
> @@ -721,12 +722,22 @@
> # @uml: UML disks
> # @sata: SATA disks
> # @sd: SD cards
> -#
> +# @Unknown: Unknown bus type
> +# @1394: 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
> +# @FB: Win file-backed bus type
> # Since: 2.2
Missing information about when elements were added: simplest (but
redundant) by adding '(since 2.4)' everywhere, or shorter (but might
cause grief down the road if we try to automate doc generation) by doing
something like:
# Since: 2.2; 'Unknown' and all later entries since 2.4
> ##
> { 'enum': 'GuestDiskBusType',
> 'data': [ 'ide', 'fdc', 'scsi', 'virtio', 'xen', 'usb', 'uml', 'sata',
> - 'sd' ] }
> + 'sd', 'unknown', '1394','Ssa', 'fibre', 'RAID', 'iScsi', 'sas',
Uggh - '1394' is a purely numeric enum name. Not the first time (so the
qapi generator allows it), but we considered blacklisting future
additions of it:
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00229.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command
2015-06-19 16:57 ` [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command Denis V. Lunev
@ 2015-06-19 17:30 ` Eric Blake
2015-06-19 18:05 ` Denis V. Lunev
2015-06-23 10:33 ` Denis V. Lunev
2015-06-19 17:34 ` Eric Blake
1 sibling, 2 replies; 29+ messages in thread
From: Eric Blake @ 2015-06-19 17:30 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2778 bytes --]
On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
> 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 | 96 +++++++++++++++++++++++++++++++++++++++++++++++++---
> qga/commands-win32.c | 8 ++++-
> qga/qapi-schema.json | 44 ++++++++++++++++++++++++
> 3 files changed, 143 insertions(+), 5 deletions(-)
Just an interface review at this point:
> +++ b/qga/qapi-schema.json
> @@ -215,12 +215,56 @@
> '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'} }
Missing a field of type GuestPipeMode?
> +
> +##
> +# @guest-pipe-open
> +#
> +# Open a pipe to in the guest to associated with a qga-spawned processes
> +# for communication.
Reads poorly. Maybe:
Open a pipe in the guest for association with later 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'] }
I'm assuming the array will always contain two elements? Would it be
any simpler to return a single dictionary of { 'read': 'int', 'write':
'int' } for naming the two fds directly, instead of having to parse
through [ { 'fd': 1 }, { 'fd': 2 } ] ?
> +
> +##
> +##
> # @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
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command
2015-06-19 16:57 ` [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command Denis V. Lunev
2015-06-19 17:30 ` Eric Blake
@ 2015-06-19 17:34 ` Eric Blake
2015-06-19 17:59 ` Denis V. Lunev
1 sibling, 1 reply; 29+ messages in thread
From: Eric Blake @ 2015-06-19 17:34 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]
On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
> 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 | 96 +++++++++++++++++++++++++++++++++++++++++++++++++---
> qga/commands-win32.c | 8 ++++-
> qga/qapi-schema.json | 44 ++++++++++++++++++++++++
> 3 files changed, 143 insertions(+), 5 deletions(-)
>
> +
> + if (pipe(fd) != 0) {
> + error_set_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]);
Would it be better to create a named FIFO somewhere in the file system,
so that you can reopen the connection even if the qga daemon is
restarted? By using just pipe(), your fds are rather ephemeral, but if
I understand correctly, the rest of the guest-file API tries to persist
across qga restart.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command
2015-06-19 17:34 ` Eric Blake
@ 2015-06-19 17:59 ` Denis V. Lunev
0 siblings, 0 replies; 29+ messages in thread
From: Denis V. Lunev @ 2015-06-19 17:59 UTC (permalink / raw)
To: Eric Blake; +Cc: Olga Krishtal, qemu-devel, Michael Roth
On 19/06/15 20:34, Eric Blake wrote:
> On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
>> 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 | 96 +++++++++++++++++++++++++++++++++++++++++++++++++---
>> qga/commands-win32.c | 8 ++++-
>> qga/qapi-schema.json | 44 ++++++++++++++++++++++++
>> 3 files changed, 143 insertions(+), 5 deletions(-)
>>
>> +
>> + if (pipe(fd) != 0) {
>> + error_set_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]);
> Would it be better to create a named FIFO somewhere in the file system,
> so that you can reopen the connection even if the qga daemon is
> restarted? By using just pipe(), your fds are rather ephemeral, but if
> I understand correctly, the rest of the guest-file API tries to persist
> across qga restart.
>
this will not help IMHO
Let us assume that we have a FIFO on a filesystem
and we have a process which uses this FIFO as
stdout. If QGA will exit that process will blown up with
SIGPIPE on any write attempt.
Den
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path
2015-06-19 17:10 ` Eric Blake
@ 2015-06-19 18:02 ` Denis V. Lunev
0 siblings, 0 replies; 29+ messages in thread
From: Denis V. Lunev @ 2015-06-19 18:02 UTC (permalink / raw)
To: Eric Blake; +Cc: Olga Krishtal, qemu-devel, Michael Roth
On 19/06/15 20:10, Eric Blake wrote:
> On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
>> 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 that Linux. Thus some values have been added
> s/that/than/
>
>> 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 | 127 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> qga/qapi-schema.json | 15 +++++-
>> 2 files changed, 139 insertions(+), 3 deletions(-)
>>
>> +static WinToLin buses[] = {
>> + {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_1394},
>> + {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_I_SCSI},
>> + {BusTypeSas, GUEST_DISK_BUS_TYPE_SAS},
>> + {BusTypeSata, GUEST_DISK_BUS_TYPE_SATA},
>> + {BusTypeSd, GUEST_DISK_BUS_TYPE_SD},
>> + {BusTypeMmc, GUEST_DISK_BUS_TYPE_MMC},
>> + {BusTypeVirtual, GUEST_DISK_BUS_TYPE_VIRTUAL },
>> + {BusTypeFileBackedVirtuaul, GUEST_DISK_BUS_TYPE_FBIRTUAL},
>> + {BusTypeSpaces, GUEST_DISK_BUS_TYPE_SPACES}
>> +#endif
> I'd use a trailing comma, so that future additions can be pure additions
> rather than amending existing lines.
>
>
>> +++ b/qga/qapi-schema.json
>> @@ -711,6 +711,7 @@
>> # @GuestDiskBusType
>> #
>> # An enumeration of bus type of disks
>> +# is devided a bit on Linux and Windows guest
>> #
> s/devided/divided/
>
>> # @ide: IDE disks
>> # @fdc: floppy disks
>> @@ -721,12 +722,22 @@
>> # @uml: UML disks
>> # @sata: SATA disks
>> # @sd: SD cards
>> -#
>> +# @Unknown: Unknown bus type
>> +# @1394: 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
>> +# @FB: Win file-backed bus type
>> # Since: 2.2
> Missing information about when elements were added: simplest (but
> redundant) by adding '(since 2.4)' everywhere, or shorter (but might
> cause grief down the road if we try to automate doc generation) by doing
> something like:
>
> # Since: 2.2; 'Unknown' and all later entries since 2.4
>
>> ##
>> { 'enum': 'GuestDiskBusType',
>> 'data': [ 'ide', 'fdc', 'scsi', 'virtio', 'xen', 'usb', 'uml', 'sata',
>> - 'sd' ] }
>> + 'sd', 'unknown', '1394','Ssa', 'fibre', 'RAID', 'iScsi', 'sas',
> Uggh - '1394' is a purely numeric enum name. Not the first time (so the
> qapi generator allows it), but we considered blacklisting future
> additions of it:
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00229.html
>
no prob, we can change this to 'ieee1394', the rest here is clear, we'll
do that on Monday
Den
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command
2015-06-19 17:30 ` Eric Blake
@ 2015-06-19 18:05 ` Denis V. Lunev
2015-06-23 10:33 ` Denis V. Lunev
1 sibling, 0 replies; 29+ messages in thread
From: Denis V. Lunev @ 2015-06-19 18:05 UTC (permalink / raw)
To: Eric Blake; +Cc: Olga Krishtal, qemu-devel, Michael Roth
On 19/06/15 20:30, Eric Blake wrote:
> On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
>> 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 | 96 +++++++++++++++++++++++++++++++++++++++++++++++++---
>> qga/commands-win32.c | 8 ++++-
>> qga/qapi-schema.json | 44 ++++++++++++++++++++++++
>> 3 files changed, 143 insertions(+), 5 deletions(-)
> Just an interface review at this point:
>
>> +++ b/qga/qapi-schema.json
>> @@ -215,12 +215,56 @@
>> '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'} }
> Missing a field of type GuestPipeMode?
yep, nice catch :)
>> +
>> +##
>> +# @guest-pipe-open
>> +#
>> +# Open a pipe to in the guest to associated with a qga-spawned processes
>> +# for communication.
> Reads poorly. Maybe:
>
> Open a pipe in the guest for association with later qga-spawned processes.
ok
>> +#
>> +# 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'] }
> I'm assuming the array will always contain two elements? Would it be
> any simpler to return a single dictionary of { 'read': 'int', 'write':
> 'int' } for naming the two fds directly, instead of having to parse
> through [ { 'fd': 1 }, { 'fd': 2 } ] ?
this looks much better to me
>> +
>> +##
>> +##
>> # @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
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command
2015-06-19 17:30 ` Eric Blake
2015-06-19 18:05 ` Denis V. Lunev
@ 2015-06-23 10:33 ` Denis V. Lunev
1 sibling, 0 replies; 29+ messages in thread
From: Denis V. Lunev @ 2015-06-23 10:33 UTC (permalink / raw)
To: Eric Blake; +Cc: Olga Krishtal, qemu-devel, Michael Roth
On 19/06/15 20:30, Eric Blake wrote:
> On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
>> 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 | 96 +++++++++++++++++++++++++++++++++++++++++++++++++---
>> qga/commands-win32.c | 8 ++++-
>> qga/qapi-schema.json | 44 ++++++++++++++++++++++++
>> 3 files changed, 143 insertions(+), 5 deletions(-)
> Just an interface review at this point:
>
>> +++ b/qga/qapi-schema.json
>> @@ -215,12 +215,56 @@
>> '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'} }
> Missing a field of type GuestPipeMode?
>
>> +
>> +##
>> +# @guest-pipe-open
>> +#
>> +# Open a pipe to in the guest to associated with a qga-spawned processes
>> +# for communication.
> Reads poorly. Maybe:
>
> Open a pipe in the guest for association with later 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'] }
> I'm assuming the array will always contain two elements? Would it be
> any simpler to return a single dictionary of { 'read': 'int', 'write':
> 'int' } for naming the two fds directly, instead of having to parse
> through [ { 'fd': 1 }, { 'fd': 2 } ] ?
Eric,
I have mistaken here in the previous letter. The idea
is that we return here only one int, which is qemu file
handle representing the pipe as an entity.
May be this should be declared in a different way to
allow future extendability. Can you advise on this?
Regards,
Den
^ permalink raw reply [flat|nested] 29+ 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
@ 2015-06-30 10:25 ` Denis V. Lunev
2015-07-07 1:31 ` Michael Roth
0 siblings, 1 reply; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ messages in thread
end of thread, other threads:[~2015-07-10 13:24 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 01/10] util, qga: drop guest_file_toggle_flags Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command Denis V. Lunev
2015-06-19 17:30 ` Eric Blake
2015-06-19 18:05 ` Denis V. Lunev
2015-06-23 10:33 ` Denis V. Lunev
2015-06-19 17:34 ` Eric Blake
2015-06-19 17:59 ` Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 03/10] qga: guest exec functionality for Unix guests Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 04/10] qga: handle possible SIGPIPE in guest-file-write Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 05/10] qga: guest-pipe-open for Windows guest 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:57 ` [Qemu-devel] [PATCH 07/10] qga: added empty qmp_quest_get_fsinfo functionality Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 08/10] qga: added mountpoint and filesystem type for single volume Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 09/10] qga: added bus type and disk location path Denis V. Lunev
2015-06-19 17:10 ` Eric Blake
2015-06-19 18:02 ` Denis V. Lunev
2015-06-19 16:57 ` [Qemu-devel] [PATCH 10/10] qga: added GuestPCIAddress information Denis V. Lunev
-- strict thread matches above, loose matches on Subject: below --
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 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-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).