* [Qemu-devel] [PATCH 1/8] qga: fixed warning in qemu-ga.exe for mingw >= 4.9.1
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
@ 2014-12-31 13:06 ` Denis V. Lunev
2015-02-03 21:29 ` Eric Blake
2014-12-31 13:06 ` [Qemu-devel] [PATCH 2/8] qga: implement file commands for Windows guest Denis V. Lunev
` (8 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Denis V. Lunev @ 2014-12-31 13:06 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@parallels.com>
strtok_r was redefined before the patch
Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
include/sysemu/os-win32.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index af3fbc4..84e229b 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -81,7 +81,9 @@ struct tm *gmtime_r(const time_t *timep, struct tm *result);
#undef localtime_r
struct tm *localtime_r(const time_t *timep, struct tm *result);
+#if defined __MINGW32__ && !(__GNUC__ >= 4 && __GNUC_MINOR__ >= 9)
char *strtok_r(char *str, const char *delim, char **saveptr);
+#endif
static inline void os_setup_signal_handling(void) {}
static inline void os_daemonize(void) {}
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] qga: fixed warning in qemu-ga.exe for mingw >= 4.9.1
2014-12-31 13:06 ` [Qemu-devel] [PATCH 1/8] qga: fixed warning in qemu-ga.exe for mingw >= 4.9.1 Denis V. Lunev
@ 2015-02-03 21:29 ` Eric Blake
2015-02-04 14:25 ` Olga Krishtal
0 siblings, 1 reply; 24+ messages in thread
From: Eric Blake @ 2015-02-03 21:29 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, Michael Roth, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]
On 12/31/2014 06:06 AM, Denis V. Lunev wrote:
> From: Olga Krishtal <okrishtal@parallels.com>
>
> strtok_r was redefined before the patch
>
> Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> include/sysemu/os-win32.h | 2 ++
> 1 file changed, 2 insertions(+)
What's the actual compiler error you get without this patch?
>
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index af3fbc4..84e229b 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -81,7 +81,9 @@ struct tm *gmtime_r(const time_t *timep, struct tm *result);
> #undef localtime_r
> struct tm *localtime_r(const time_t *timep, struct tm *result);
>
> +#if defined __MINGW32__ && !(__GNUC__ >= 4 && __GNUC_MINOR__ >= 9)
> char *strtok_r(char *str, const char *delim, char **saveptr);
It feels weird to key off of the gcc version. Whether strtok_r is
defined is a property of the system headers, not the compiler. For
example, I'm aware a but where older mingw <pthreads.h> would pollute
the namespace with a broken #define of strtok_r; but that has been fixed
in newer mingw headers (on Fedora, I know that F20 mingw has the bug,
F21 does not). But that is the version of mingw, not the version of
gcc, that determined that problem. Therefore, I'm not sure this is the
right patch, but without knowing the symptoms it fixes, I don't know if
anything is better.
--
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] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 1/8] qga: fixed warning in qemu-ga.exe for mingw >= 4.9.1
2015-02-03 21:29 ` Eric Blake
@ 2015-02-04 14:25 ` Olga Krishtal
0 siblings, 0 replies; 24+ messages in thread
From: Olga Krishtal @ 2015-02-04 14:25 UTC (permalink / raw)
To: Eric Blake, Denis V. Lunev; +Cc: Michael Roth, qemu-devel
On 04/02/15 00:29, Eric Blake wrote:
> On 12/31/2014 06:06 AM, Denis V. Lunev wrote:
>> From: Olga Krishtal <okrishtal@parallels.com>
>>
>> strtok_r was redefined before the patch
>>
>> Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>> include/sysemu/os-win32.h | 2 ++
>> 1 file changed, 2 insertions(+)
> What's the actual compiler error you get without this patch?
/mingw/include/string.h:88:9: note: previous declaration of 'strtok_r'
was here
char *strtok_r(char * __restrict__ _Str, const char * __restrict__
_Delim, char ** __restrict__ __last);
/include/sysemu/os-win32.h:83:7: warning: redundant redeclaration of
'strtok_r' [-Wredundant-decls]
char *strtok_r(char *str, const char *delim, char **saveptr);
>
>> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
>> index af3fbc4..84e229b 100644
>> --- a/include/sysemu/os-win32.h
>> +++ b/include/sysemu/os-win32.h
>> @@ -81,7 +81,9 @@ struct tm *gmtime_r(const time_t *timep, struct tm *result);
>> #undef localtime_r
>> struct tm *localtime_r(const time_t *timep, struct tm *result);
>>
>> +#if defined __MINGW32__ && !(__GNUC__ >= 4 && __GNUC_MINOR__ >= 9)
>> char *strtok_r(char *str, const char *delim, char **saveptr);
> It feels weird to key off of the gcc version. Whether strtok_r is
> defined is a property of the system headers, not the compiler. For
> example, I'm aware a but where older mingw <pthreads.h> would pollute
> the namespace with a broken #define of strtok_r; but that has been fixed
> in newer mingw headers (on Fedora, I know that F20 mingw has the bug,
> F21 does not). But that is the version of mingw, not the version of
> gcc, that determined that problem. Therefore, I'm not sure this is the
> right patch, but without knowing the symptoms it fixes, I don't know if
> anything is better.
>
There is an idea to get rid of the strtok_r() because it is used only
once, so we can replace it with other functions to get the same
functionality.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 2/8] qga: implement file commands for Windows guest
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
2014-12-31 13:06 ` [Qemu-devel] [PATCH 1/8] qga: fixed warning in qemu-ga.exe for mingw >= 4.9.1 Denis V. Lunev
@ 2014-12-31 13:06 ` Denis V. Lunev
2015-02-03 21:15 ` Michael Roth
2014-12-31 13:06 ` [Qemu-devel] [PATCH 3/8] guest agent: guest-file-open: refactoring Denis V. Lunev
` (7 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Denis V. Lunev @ 2014-12-31 13:06 UTC (permalink / raw)
Cc: Michael Roth, Olga Krishtal, qemu-devel, Denis V. Lunev
From: Olga Krishtal <okrishtal@parallels.com>
The following commands are implemented:
- guest_file_open
- guest_file_close
- guest_file_write
- guest_file_read
- guest_file_seek
- guest_file_flush
Motivation is quite simple: Windows guests should be supported with the
same set of features as Linux one. Also this patch is a prerequisite for
Windows guest-exec command support.
Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qga/commands-win32.c | 271 +++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 250 insertions(+), 21 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3bcbeae..5db96ef 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -14,10 +14,13 @@
#include <glib.h>
#include <wtypes.h>
#include <powrprof.h>
+#include <stdio.h>
+#include <string.h>
#include "qga/guest-agent-core.h"
#include "qga/vss-win32.h"
#include "qga-qmp-commands.h"
#include "qapi/qmp/qerror.h"
+#include "qemu/queue.h"
#ifndef SHTDN_REASON_FLAG_PLANNED
#define SHTDN_REASON_FLAG_PLANNED 0x80000000
@@ -29,6 +32,146 @@
(365 * (1970 - 1601) + \
(1970 - 1601) / 4 - 3))
+#define INVALID_SET_FILE_POINTER ((DWORD)-1)
+
+typedef struct GuestFileHandle {
+ int64_t id;
+ HANDLE fh;
+ QTAILQ_ENTRY(GuestFileHandle) next;
+} GuestFileHandle;
+
+static struct {
+ QTAILQ_HEAD(, GuestFileHandle) filehandles;
+} guest_file_state;
+
+
+typedef struct OpenFlags {
+ const char *forms;
+ DWORD desired_access;
+ DWORD creation_disposition;
+} OpenFlags;
+static OpenFlags guest_file_open_modes[] = {
+ {"r", GENERIC_READ, OPEN_EXISTING},
+ {"rb", GENERIC_READ, OPEN_EXISTING},
+ {"w", GENERIC_WRITE, CREATE_ALWAYS},
+ {"wb", GENERIC_WRITE, CREATE_ALWAYS},
+ {"a", GENERIC_WRITE, OPEN_ALWAYS },
+ {"r+", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
+ {"rb+", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
+ {"r+b", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
+ {"w+", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
+ {"wb+", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
+ {"w+b", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
+ {"a+", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS },
+ {"ab+", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS },
+ {"a+b", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS }
+};
+
+static OpenFlags *find_open_flag(const char *mode_str)
+{
+ int mode;
+ Error **errp = NULL;
+
+ for (mode = 0; mode < ARRAY_SIZE(guest_file_open_modes); ++mode) {
+ OpenFlags *flags = guest_file_open_modes + mode;
+
+ if (strcmp(flags->forms, mode_str) == 0) {
+ return flags;
+ }
+ }
+
+ error_setg(errp, "invalid file open mode '%s'", mode_str);
+ return NULL;
+}
+
+static int64_t guest_file_handle_add(HANDLE fh, Error **errp)
+{
+ GuestFileHandle *gfh;
+ int64_t handle;
+
+ handle = ga_get_fd_handle(ga_state, errp);
+ if (handle < 0) {
+ return -1;
+ }
+ gfh = g_malloc0(sizeof(GuestFileHandle));
+ gfh->id = handle;
+ gfh->fh = fh;
+ QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
+
+ return handle;
+}
+
+static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
+{
+ GuestFileHandle *gfh;
+ QTAILQ_FOREACH(gfh, &guest_file_state.filehandles, next) {
+ if (gfh->id == id) {
+ return gfh;
+ }
+ }
+ error_setg(errp, "handle '%" PRId64 "' has not been found", id);
+ return NULL;
+}
+
+int64_t qmp_guest_file_open(const char *path, bool has_mode,
+ const char *mode, Error **errp)
+{
+ int64_t fd;
+ HANDLE fh;
+ HANDLE templ_file = NULL;
+ DWORD share_mode = FILE_SHARE_READ;
+ DWORD flags_and_attr = FILE_ATTRIBUTE_NORMAL;
+ LPSECURITY_ATTRIBUTES sa_attr = NULL;
+ OpenFlags *guest_flags;
+
+ if (!has_mode) {
+ mode = "r";
+ }
+ slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
+ guest_flags = find_open_flag(mode);
+ if (guest_flags == NULL) {
+ error_setg(errp, "invalid file open mode");
+ return -1;
+ }
+
+ fh = CreateFile(path, guest_flags->desired_access, share_mode, sa_attr,
+ guest_flags->creation_disposition, flags_and_attr,
+ templ_file);
+ if (fh == INVALID_HANDLE_VALUE) {
+ error_setg_errno(errp, GetLastError(), "failed to open file '%s'",
+ path);
+ return -1;
+ }
+
+ fd = guest_file_handle_add(fh, errp);
+ if (fd < 0) {
+ CloseHandle(&fh);
+ error_setg(errp, "failed to add handle to qmp handle table");
+ return -1;
+ }
+
+ slog("guest-file-open, handle: % " PRId64, fd);
+ return fd;
+}
+
+void qmp_guest_file_close(int64_t handle, Error **errp)
+{
+ bool ret;
+ GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
+ slog("guest-file-close called, handle: %" PRId64, handle);
+ if (gfh == NULL) {
+ return;
+ }
+ ret = CloseHandle(gfh->fh);
+ if (!ret) {
+ error_setg_errno(errp, GetLastError(), "failed close handle");
+ return;
+ }
+
+ QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
+ g_free(gfh);
+}
+
static void acquire_privilege(const char *name, Error **errp)
{
HANDLE token = NULL;
@@ -113,43 +256,130 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
}
}
-int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
- Error **errp)
-{
- error_set(errp, QERR_UNSUPPORTED);
- return 0;
-}
-
-void qmp_guest_file_close(int64_t handle, Error **errp)
-{
- error_set(errp, QERR_UNSUPPORTED);
-}
-
GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
int64_t count, Error **errp)
{
- error_set(errp, QERR_UNSUPPORTED);
- return 0;
+ GuestFileRead *read_data = NULL;
+ guchar *buf;
+ HANDLE fh;
+ bool is_ok;
+ DWORD read_count;
+ GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
+
+ if (!gfh) {
+ return NULL;
+ }
+ if (!has_count) {
+ count = QGA_READ_COUNT_DEFAULT;
+ } else if (count < 0) {
+ error_setg(errp, "value '%" PRId64
+ "' is invalid for argument count", count);
+ return NULL;
+ }
+
+ fh = gfh->fh;
+ buf = g_malloc0(count+1);
+ is_ok = ReadFile(fh, buf, count, &read_count, NULL);
+ if (!is_ok) {
+ error_setg_errno(errp, GetLastError(), "failed to read file");
+ slog("guest-file-read failed, handle %" PRId64, handle);
+ } else {
+ buf[read_count] = 0;
+ read_data = g_malloc0(sizeof(GuestFileRead));
+ read_data->count = (size_t)read_count;
+ read_data->eof = read_count == 0;
+
+ if (read_count != 0) {
+ read_data->buf_b64 = g_base64_encode(buf, read_count);
+ }
+ }
+ g_free(buf);
+
+ return read_data;
}
GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
bool has_count, int64_t count,
Error **errp)
{
- error_set(errp, QERR_UNSUPPORTED);
- return 0;
+ GuestFileWrite *write_data = NULL;
+ guchar *buf;
+ gsize buf_len;
+ bool is_ok;
+ DWORD write_count;
+ GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
+ HANDLE fh;
+
+ if (!gfh) {
+ return NULL;
+ }
+ fh = gfh->fh;
+ buf = g_base64_decode(buf_b64, &buf_len);
+
+ if (!has_count) {
+ count = buf_len;
+ } else if (count < 0 || count > buf_len) {
+ error_setg(errp, "value '%" PRId64
+ "' is invalid for argument count", count);
+ goto done;
+ }
+
+ is_ok = WriteFile(fh, buf, count, &write_count, NULL);
+ if (!is_ok) {
+ error_setg_errno(errp, GetLastError(), "failed to write to file");
+ slog("guest-file-write-failed, handle: %" PRId64, handle);
+ } else {
+ write_data = g_malloc0(sizeof(GuestFileWrite));
+ write_data->count = (size_t) write_count;
+ }
+
+done:
+ g_free(buf);
+ return write_data;
}
GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
int64_t whence, Error **errp)
{
- error_set(errp, QERR_UNSUPPORTED);
- return 0;
+ GuestFileHandle *gfh;
+ GuestFileSeek *seek_data;
+ HANDLE fh;
+ LARGE_INTEGER new_pos, off_pos;
+ off_pos.QuadPart = offset;
+ BOOL res;
+ gfh = guest_file_handle_find(handle, errp);
+ if (!gfh) {
+ return NULL;
+ }
+
+ fh = gfh->fh;
+ res = SetFilePointerEx(fh, off_pos, &new_pos, whence);
+ if (!res) {
+ error_setg_errno(errp, GetLastError(), "failed to seek file");
+ return NULL;
+ }
+ seek_data = g_new0(GuestFileSeek, 1);
+ seek_data->position = new_pos.QuadPart;
+ return seek_data;
}
void qmp_guest_file_flush(int64_t handle, Error **errp)
{
- error_set(errp, QERR_UNSUPPORTED);
+ HANDLE fh;
+ GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
+ if (!gfh) {
+ return;
+ }
+
+ fh = gfh->fh;
+ if (!FlushFileBuffers(fh)) {
+ error_setg_errno(errp, GetLastError(), "failed to flush file");
+ }
+}
+
+static void guest_file_init(void)
+{
+ QTAILQ_INIT(&guest_file_state.filehandles);
}
GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
@@ -450,8 +680,6 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
GList *ga_command_blacklist_init(GList *blacklist)
{
const char *list_unsupported[] = {
- "guest-file-open", "guest-file-close", "guest-file-read",
- "guest-file-write", "guest-file-seek", "guest-file-flush",
"guest-suspend-hybrid", "guest-network-get-interfaces",
"guest-get-vcpus", "guest-set-vcpus",
"guest-fsfreeze-freeze-list", "guest-get-fsinfo",
@@ -482,4 +710,5 @@ void ga_command_state_init(GAState *s, GACommandState *cs)
if (!vss_initialized()) {
ga_command_state_add(cs, NULL, guest_fsfreeze_cleanup);
}
+ ga_command_state_add(cs, guest_file_init, NULL);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 2/8] qga: implement file commands for Windows guest
2014-12-31 13:06 ` [Qemu-devel] [PATCH 2/8] qga: implement file commands for Windows guest Denis V. Lunev
@ 2015-02-03 21:15 ` Michael Roth
0 siblings, 0 replies; 24+ messages in thread
From: Michael Roth @ 2015-02-03 21:15 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel
Quoting Denis V. Lunev (2014-12-31 07:06:48)
> From: Olga Krishtal <okrishtal@parallels.com>
>
> The following commands are implemented:
> - guest_file_open
> - guest_file_close
> - guest_file_write
> - guest_file_read
> - guest_file_seek
> - guest_file_flush
>
> Motivation is quite simple: Windows guests should be supported with the
> same set of features as Linux one. Also this patch is a prerequisite for
> Windows guest-exec command support.
>
> Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qga/commands-win32.c | 271 +++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 250 insertions(+), 21 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3bcbeae..5db96ef 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -14,10 +14,13 @@
> #include <glib.h>
> #include <wtypes.h>
> #include <powrprof.h>
> +#include <stdio.h>
> +#include <string.h>
> #include "qga/guest-agent-core.h"
> #include "qga/vss-win32.h"
> #include "qga-qmp-commands.h"
> #include "qapi/qmp/qerror.h"
> +#include "qemu/queue.h"
>
> #ifndef SHTDN_REASON_FLAG_PLANNED
> #define SHTDN_REASON_FLAG_PLANNED 0x80000000
> @@ -29,6 +32,146 @@
> (365 * (1970 - 1601) + \
> (1970 - 1601) / 4 - 3))
>
> +#define INVALID_SET_FILE_POINTER ((DWORD)-1)
> +
> +typedef struct GuestFileHandle {
> + int64_t id;
> + HANDLE fh;
> + QTAILQ_ENTRY(GuestFileHandle) next;
> +} GuestFileHandle;
> +
> +static struct {
> + QTAILQ_HEAD(, GuestFileHandle) filehandles;
> +} guest_file_state;
> +
> +
> +typedef struct OpenFlags {
> + const char *forms;
> + DWORD desired_access;
> + DWORD creation_disposition;
> +} OpenFlags;
> +static OpenFlags guest_file_open_modes[] = {
> + {"r", GENERIC_READ, OPEN_EXISTING},
> + {"rb", GENERIC_READ, OPEN_EXISTING},
> + {"w", GENERIC_WRITE, CREATE_ALWAYS},
> + {"wb", GENERIC_WRITE, CREATE_ALWAYS},
> + {"a", GENERIC_WRITE, OPEN_ALWAYS },
> + {"r+", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
> + {"rb+", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
> + {"r+b", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
> + {"w+", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
> + {"wb+", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
> + {"w+b", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
> + {"a+", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS },
> + {"ab+", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS },
> + {"a+b", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS }
> +};
> +
> +static OpenFlags *find_open_flag(const char *mode_str)
> +{
> + int mode;
> + Error **errp = NULL;
> +
> + for (mode = 0; mode < ARRAY_SIZE(guest_file_open_modes); ++mode) {
> + OpenFlags *flags = guest_file_open_modes + mode;
> +
> + if (strcmp(flags->forms, mode_str) == 0) {
> + return flags;
> + }
> + }
> +
> + error_setg(errp, "invalid file open mode '%s'", mode_str);
> + return NULL;
> +}
> +
> +static int64_t guest_file_handle_add(HANDLE fh, Error **errp)
> +{
> + GuestFileHandle *gfh;
> + int64_t handle;
> +
> + handle = ga_get_fd_handle(ga_state, errp);
> + if (handle < 0) {
> + return -1;
> + }
> + gfh = g_malloc0(sizeof(GuestFileHandle));
> + gfh->id = handle;
> + gfh->fh = fh;
> + QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> +
> + return handle;
> +}
> +
> +static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
> +{
> + GuestFileHandle *gfh;
> + QTAILQ_FOREACH(gfh, &guest_file_state.filehandles, next) {
> + if (gfh->id == id) {
> + return gfh;
> + }
> + }
> + error_setg(errp, "handle '%" PRId64 "' has not been found", id);
> + return NULL;
> +}
> +
> +int64_t qmp_guest_file_open(const char *path, bool has_mode,
> + const char *mode, Error **errp)
> +{
> + int64_t fd;
> + HANDLE fh;
> + HANDLE templ_file = NULL;
> + DWORD share_mode = FILE_SHARE_READ;
> + DWORD flags_and_attr = FILE_ATTRIBUTE_NORMAL;
> + LPSECURITY_ATTRIBUTES sa_attr = NULL;
> + OpenFlags *guest_flags;
> +
> + if (!has_mode) {
> + mode = "r";
> + }
> + slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
> + guest_flags = find_open_flag(mode);
> + if (guest_flags == NULL) {
> + error_setg(errp, "invalid file open mode");
> + return -1;
> + }
> +
> + fh = CreateFile(path, guest_flags->desired_access, share_mode, sa_attr,
> + guest_flags->creation_disposition, flags_and_attr,
> + templ_file);
> + if (fh == INVALID_HANDLE_VALUE) {
> + error_setg_errno(errp, GetLastError(), "failed to open file '%s'",
> + path);
For working with GetLastError() I think error_setg_win32() is what you
want. Same for all the other occurrences.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 3/8] guest agent: guest-file-open: refactoring
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
2014-12-31 13:06 ` [Qemu-devel] [PATCH 1/8] qga: fixed warning in qemu-ga.exe for mingw >= 4.9.1 Denis V. Lunev
2014-12-31 13:06 ` [Qemu-devel] [PATCH 2/8] qga: implement file commands for Windows guest Denis V. Lunev
@ 2014-12-31 13:06 ` Denis V. Lunev
2015-02-03 22:04 ` Eric Blake
2014-12-31 13:06 ` [Qemu-devel] [PATCH 4/8] guest agent: add guest-pipe-open Denis V. Lunev
` (6 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Denis V. Lunev @ 2014-12-31 13:06 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, Simon Zolin, Michael Roth
From: Simon Zolin <szolin@parallels.com>
Moved the code that sets non-blocking flag on fd into a separate function.
Signed-off-by: Simon Zolin <szolin@parallels.com>
Acked-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qga/commands-posix.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index f6f3e3c..fd746db 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -376,13 +376,33 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
return NULL;
}
+static int guest_file_toggle_flags(int fd, long 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)
{
FILE *fh;
Error *local_err = NULL;
- int fd;
- int64_t ret = -1, handle;
+ int64_t handle;
if (!has_mode) {
mode = "r";
@@ -397,12 +417,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
*/
- fd = fileno(fh);
- ret = fcntl(fd, F_GETFL);
- ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
- if (ret == -1) {
- error_setg_errno(errp, errno, "failed to make file '%s' non-blocking",
- path);
+ if (guest_file_toggle_flags(fileno(fh), O_NONBLOCK, true, errp) < 0) {
fclose(fh);
return -1;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 3/8] guest agent: guest-file-open: refactoring
2014-12-31 13:06 ` [Qemu-devel] [PATCH 3/8] guest agent: guest-file-open: refactoring Denis V. Lunev
@ 2015-02-03 22:04 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2015-02-03 22:04 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, Simon Zolin, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]
On 12/31/2014 06:06 AM, Denis V. Lunev wrote:
> From: Simon Zolin <szolin@parallels.com>
>
> Moved the code that sets non-blocking flag on fd into a separate function.
>
> Signed-off-by: Simon Zolin <szolin@parallels.com>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qga/commands-posix.c | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f6f3e3c..fd746db 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -376,13 +376,33 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
> return NULL;
> }
>
> +static int guest_file_toggle_flags(int fd, long flags, bool set, Error **err)
> +{
Why is 'flags' a long?
> + 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));
Bug. 'int | long' is a long, but on 64-bit platforms, passing a 'long'
as the var-arg third argument of fcntl where the interface expects 'int'
is liable to corrupt things depending on endianness. You MUST pass an
'int' for F_SETFL.
--
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] 24+ messages in thread
* [Qemu-devel] [PATCH 4/8] guest agent: add guest-pipe-open
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
` (2 preceding siblings ...)
2014-12-31 13:06 ` [Qemu-devel] [PATCH 3/8] guest agent: guest-file-open: refactoring Denis V. Lunev
@ 2014-12-31 13:06 ` Denis V. Lunev
2015-02-03 21:57 ` Eric Blake
2014-12-31 13:06 ` [Qemu-devel] [PATCH 5/8] guest agent: add guest-exec and guest-exec-status interfaces Denis V. Lunev
` (5 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Denis V. Lunev @ 2014-12-31 13:06 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, Simon Zolin, Michael Roth
From: Simon Zolin <szolin@parallels.com>
Creates a 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: Simon Zolin <szolin@parallels.com>
Acked-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qga/commands-posix.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++--
qga/commands-win32.c | 8 ++++-
qga/qapi-schema.json | 21 ++++++++++++++
3 files changed, 108 insertions(+), 3 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index fd746db..bf14518 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -208,9 +208,11 @@ 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;
@@ -218,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;
@@ -231,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;
@@ -422,7 +426,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
return -1;
}
- handle = guest_file_handle_add(fh, errp);
+ handle = guest_file_handle_add(fh, -1, errp);
if (handle < 0) {
fclose(fh);
return -1;
@@ -432,6 +436,75 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
return handle;
}
+int64_t qmp_guest_pipe_open(const char *mode, Error **err)
+{
+ FILE *f = NULL;
+ int fd[2], this_end, other_end;
+ int64_t handle;
+
+ slog("guest-pipe-open called");
+
+ if ((mode[0] != 'r' && mode[0] != 'w') || mode[1] != '\0') {
+ error_set(err, ERROR_CLASS_GENERIC_ERROR,
+ "Only \"r\" or \"w\" are the valid modes to open a pipe");
+ return -1;
+ }
+
+ if (pipe(fd) != 0) {
+ error_set_errno(err, errno, QERR_QGA_COMMAND_FAILED, "pipe() failed");
+ return -1;
+ }
+
+ this_end = (mode[0] == 'w');
+ other_end = !this_end;
+
+ if (guest_file_toggle_flags(fd[this_end], O_NONBLOCK, true, err) == -1) {
+ goto fail;
+ }
+
+ qemu_set_cloexec(fd[this_end]);
+ qemu_set_cloexec(fd[other_end]);
+
+ f = fdopen(fd[this_end], mode);
+ if (f == NULL) {
+ error_set_errno(err, errno, QERR_QGA_COMMAND_FAILED,
+ "fdopen() failed to open pipe handle");
+ goto fail;
+ }
+
+ handle = guest_file_handle_add(f, fd[other_end], err);
+ if (handle == -1) {
+ goto fail;
+ }
+
+ slog("guest-pipe-open: handle: %" PRId64, handle);
+
+ return handle;
+
+fail:
+ if (f != NULL) {
+ fclose(f);
+ } else {
+ close(fd[this_end]);
+ }
+ close(fd[other_end]);
+
+ return -1;
+}
+
+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);
@@ -442,6 +515,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");
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 5db96ef..5cb7946 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;
}
+int64_t qmp_guest_pipe_open(const char *mode, Error **errp)
+{
+ error_set(errp, QERR_UNSUPPORTED);
+ return 0;
+}
+
void qmp_guest_file_close(int64_t handle, Error **errp)
{
bool ret;
@@ -683,7 +689,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
"guest-suspend-hybrid", "guest-network-get-interfaces",
"guest-get-vcpus", "guest-set-vcpus",
"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 376e79f..67d3b72 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -212,12 +212,33 @@
'returns': '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 useable with the same interfaces as a handle returned by
+# guest-file-open.
+#
+# Since: 2.3
+##
+{ 'command': 'guest-pipe-open',
+ 'data': { 'mode': 'str' },
+ 'returns': 'int' }
+
+##
# @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] 24+ messages in thread* Re: [Qemu-devel] [PATCH 4/8] guest agent: add guest-pipe-open
2014-12-31 13:06 ` [Qemu-devel] [PATCH 4/8] guest agent: add guest-pipe-open Denis V. Lunev
@ 2015-02-03 21:57 ` Eric Blake
2015-02-03 22:06 ` Eric Blake
0 siblings, 1 reply; 24+ messages in thread
From: Eric Blake @ 2015-02-03 21:57 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, Simon Zolin, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]
On 12/31/2014 06:06 AM, Denis V. Lunev wrote:
> From: Simon Zolin <szolin@parallels.com>
>
> Creates a 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: Simon Zolin <szolin@parallels.com>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> +++ b/qga/qapi-schema.json
> @@ -212,12 +212,33 @@
> 'returns': '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 useable with the same interfaces as a handle returned by
s/useable/usable/
> +# guest-file-open.
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'guest-pipe-open',
> + 'data': { 'mode': 'str' },
> + 'returns': 'int' }
I'm not a fan of returning a bare 'int' - it is not extensible. Better
is returning a dictionary, such as 'returns': { 'fd': 'int' }. That
way, if we ever find a reason to return multiple pieces of information,
we just return a larger dictionary.
Yeah, I know guest-pipe-open breaks the rules here, and so consistency
may be an argument in favor of also breaking the rules.
I don't like 'mode' encoded as a raw string. Make it an enum type (as
in { 'enum':'PipeMode', 'data':['read', 'write']} ... 'mode':'PipeMode')
or even a bool (as in 'read':'bool')
This only returns ONE end of a pipe (good for when the host is piping
data into the child, or when the child is piping data into the host).
But isn't your goal to also make it possible to string together multiple
child processes where the output of one is the input of the other (no
host involvement)? How would you wire that up?
> +
> +##
> # @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.
How does one pass the read side of a pipe to a spawned child? Can you
design the spawning API so that close cannot deadlock?
> +#
> # 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] 24+ messages in thread* Re: [Qemu-devel] [PATCH 4/8] guest agent: add guest-pipe-open
2015-02-03 21:57 ` Eric Blake
@ 2015-02-03 22:06 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2015-02-03 22:06 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, Simon Zolin, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 882 bytes --]
On 02/03/2015 02:57 PM, Eric Blake wrote:
>> +# Returns: Guest file handle on success, as per guest-file-open. This
>> +# handle is useable with the same interfaces as a handle returned by
>
>> + 'returns': 'int' }
>
> I'm not a fan of returning a bare 'int' - it is not extensible. Better
> is returning a dictionary, such as 'returns': { 'fd': 'int' }. That
> way, if we ever find a reason to return multiple pieces of information,
> we just return a larger dictionary.
>
> Yeah, I know guest-pipe-open breaks the rules here, and so consistency
> may be an argument in favor of also breaking the rules.
I meant to say 'guest-file-open' breaks the rules, and that you are
proposing that 'guest-pipe-open' be consistent with 'guest-file-open'.
--
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] 24+ messages in thread
* [Qemu-devel] [PATCH 5/8] guest agent: add guest-exec and guest-exec-status interfaces
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
` (3 preceding siblings ...)
2014-12-31 13:06 ` [Qemu-devel] [PATCH 4/8] guest agent: add guest-pipe-open Denis V. Lunev
@ 2014-12-31 13:06 ` Denis V. Lunev
2015-02-03 21:45 ` Eric Blake
2014-12-31 13:06 ` [Qemu-devel] [PATCH 6/8] guest agent: ignore SIGPIPE signal Denis V. Lunev
` (4 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Denis V. Lunev @ 2014-12-31 13:06 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, Simon Zolin, Michael Roth
From: Simon Zolin <szolin@parallels.com>
Interfaces to execute/manage processes in the guest. Child process'
stdin/stdout/stderr can be associated with handles for communication
via read/write interfaces.
Signed-off-by: Simon Zolin <szolin@parallels.com>
Acked-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qga/commands-posix.c | 297 ++++++++++++++++++++++++++++++++++++++++++++++++++-
qga/commands-win32.c | 21 +++-
qga/qapi-schema.json | 47 ++++++++
3 files changed, 361 insertions(+), 4 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index bf14518..d8e4ecf 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -505,6 +505,8 @@ static int guest_pipe_close_other_end(GuestFileHandle *gfh)
return 0;
}
+static bool guest_exec_file_busy(GuestFileHandle *gfh);
+
void qmp_guest_file_close(int64_t handle, Error **errp)
{
GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
@@ -520,9 +522,19 @@ void qmp_guest_file_close(int64_t handle, Error **errp)
return;
}
- ret = fclose(gfh->fh);
- if (ret == EOF) {
- error_setg_errno(errp, errno, "failed to close handle");
+ if (gfh->fh != NULL) {
+ ret = fclose(gfh->fh);
+ if (ret == EOF) {
+ error_setg_errno(errp, errno, "failed to close handle");
+ return;
+ }
+
+ gfh->fh = NULL;
+ }
+
+ if (guest_exec_file_busy(gfh)) {
+ error_setg_errno(errp, errno, "can't close handle %" PRId64 ", "
+ "because it's used by guest-exec", handle);
return;
}
@@ -842,6 +854,284 @@ 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;
+}
+
+/* Return TRUE if the specified file is used in guest-exec command.
+ * We must not free the memory associated with GuestFileHandle* until
+ * guest-exec-status is called. */
+static bool guest_exec_file_busy(GuestFileHandle *gfh)
+{
+ GuestExecInfo *gei;
+
+ QTAILQ_FOREACH(gei, &guest_exec_state.processes, next) {
+
+ if (gei->gfh_stdin == gfh ||
+ gei->gfh_stdout == gfh ||
+ gei->gfh_stderr == gfh) {
+
+ return true;
+ }
+ }
+
+ return false;
+}
+
+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 array for execve(). */
+static char **guest_exec_get_envp(const strList *env)
+{
+ const strList *it;
+ char **envp;
+ size_t i = 0, count = 1;
+
+ for (it = env; it != NULL; it = it->next) {
+ count++;
+ }
+
+ envp = g_malloc(count * sizeof(char *));
+
+ for (it = env; it != NULL; it = it->next) {
+ envp[i++] = it->value;
+ }
+
+ envp[i] = NULL;
+ return envp;
+}
+
+/* Get array of arguments for execve().
+ * @argv_str: arguments in one line separated by space. */
+static char **guest_exec_get_argv(const char *path, const strList *entry,
+ char **argv_str)
+{
+ const strList *it;
+ int i = 0, count = 2; /* reserve 2 for path and NULL terminator */
+ size_t argv_str_size;
+ char **argv;
+
+ argv_str_size = strlen(path) + 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);
+ pstrcpy(*argv_str, argv_str_size, path);
+
+ argv = g_malloc(count * sizeof(char *));
+ argv[i++] = (char *)path;
+
+ for (it = entry; it != NULL; it = it->next) {
+ argv[i++] = it->value;
+ pstrcat(*argv_str, argv_str_size, " ");
+ pstrcat(*argv_str, argv_str_size, it->value);
+ }
+
+ argv[i] = NULL;
+ return argv;
+}
+
+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;
+}
+
+int64_t 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;
+ GuestFileHandle *gfh_stdin = NULL, *gfh_stdout = NULL, *gfh_stderr = NULL;
+ char **argv, *argv_str, **envp;
+
+ argv = guest_exec_get_argv(path, has_params ? params : NULL, &argv_str);
+
+ slog("guest-exec called: \"%s\"", argv_str);
+ g_free(argv_str);
+
+ envp = guest_exec_get_envp(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);
+
+done:
+ g_free(argv);
+ g_free(envp);
+ return pid;
+}
+
+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)
@@ -2096,4 +2386,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 5cb7946..8fb29fc 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;
+}
+
+int64_t 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);
@@ -689,7 +707,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
"guest-suspend-hybrid", "guest-network-get-interfaces",
"guest-get-vcpus", "guest-set-vcpus",
"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 67d3b72..ea09565 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -759,3 +759,50 @@
##
{ 'command': 'guest-get-fsinfo',
'returns': ['GuestFilesystemInfo'] }
+
+##
+# @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. On Windows,
+# "signal" is always set to -1.
+#
+# Since: 2.3
+##
+{ 'type': 'GuestExecStatus',
+ 'data': { 'exit': 'int', 'signal': 'int',
+ 'handle_stdin': 'int', 'handle_stdout': 'int',
+ 'handle_stderr': 'int' } }
+
+{ 'command': 'guest-exec-status',
+ 'data': { 'pid': 'int' },
+ 'returns': 'GuestExecStatus' }
+
+##
+# @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.3
+##
+{ 'command': 'guest-exec',
+ 'data': { 'path': 'str', '*params': ['str'], '*env': ['str'],
+ '*handle_stdin': 'int', '*handle_stdout': 'int',
+ '*handle_stderr': 'int' },
+ 'returns': 'int' }
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 5/8] guest agent: add guest-exec and guest-exec-status interfaces
2014-12-31 13:06 ` [Qemu-devel] [PATCH 5/8] guest agent: add guest-exec and guest-exec-status interfaces Denis V. Lunev
@ 2015-02-03 21:45 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2015-02-03 21:45 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, Simon Zolin, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3393 bytes --]
On 12/31/2014 06:06 AM, Denis V. Lunev wrote:
> From: Simon Zolin <szolin@parallels.com>
>
> Interfaces to execute/manage processes in the guest. Child process'
> stdin/stdout/stderr can be associated with handles for communication
> via read/write interfaces.
>
> Signed-off-by: Simon Zolin <szolin@parallels.com>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> +++ b/qga/qapi-schema.json
> @@ -759,3 +759,50 @@
> ##
> { 'command': 'guest-get-fsinfo',
> 'returns': ['GuestFilesystemInfo'] }
> +
> +##
> +# @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. On Windows,
> +# "signal" is always set to -1.
That last sentence feels a bit too specific, and might even be wrong (it
IS possible to tell in Windows if something died due to Ctrl-C, which is
effectively SIGINT); so it might be better worded as 'If a guest cannot
reliably detect exit signals, "signal" will be -1'.
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'GuestExecStatus',
> + 'data': { 'exit': 'int', 'signal': 'int',
> + 'handle_stdin': 'int', 'handle_stdout': 'int',
> + 'handle_stderr': 'int' } }
s/_/-/ throughout (that is, prefer dash, not underscore, for
handle-stdin and friends)
> +
> +{ 'command': 'guest-exec-status',
> + 'data': { 'pid': 'int' },
> + 'returns': 'GuestExecStatus' }
Is there any way to query which pids are currently being tracked?
Suppose that the host's connection to qga is interrupted; on reconnect,
how can the host learn which pids are in use, to know how to grab status
of those pids?
> +
> +##
> +# @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
Inconsistent trailing '.'. If any handle is omitted, what is used in
its place, effectively /dev/null?
> +#
> +# Returns: PID on success.
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'guest-exec',
> + 'data': { 'path': 'str', '*params': ['str'], '*env': ['str'],
I guess 'env' matches the usual exec*() interface, where the user is
responsible for passing 'name=value' pairs and behavior is not
necessarily defined if any of those strings aren't a name=value?
> + '*handle_stdin': 'int', '*handle_stdout': 'int',
> + '*handle_stderr': 'int' },
again, s/_/-/
> + 'returns': 'int' }
>
--
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] 24+ messages in thread
* [Qemu-devel] [PATCH 6/8] guest agent: ignore SIGPIPE signal
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
` (4 preceding siblings ...)
2014-12-31 13:06 ` [Qemu-devel] [PATCH 5/8] guest agent: add guest-exec and guest-exec-status interfaces Denis V. Lunev
@ 2014-12-31 13:06 ` Denis V. Lunev
2014-12-31 13:06 ` [Qemu-devel] [PATCH 7/8] guest agent: add guest-pipe-open command on Windows Denis V. Lunev
` (3 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Denis V. Lunev @ 2014-12-31 13:06 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, Simon Zolin, Michael Roth
From: Simon Zolin <szolin@parallels.com>
If write operation fails on a pipe whose reading end is closed, qemu-ga
won't be terminated, but instead write() will fail with error EPIPE.
execve() inherits signals that are ignored, so reset SIGPIPE to its default
handler before calling execve() in a forked process.
Signed-off-by: Simon Zolin <szolin@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
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 d8e4ecf..bf9f79f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1030,6 +1030,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));
+ }
+}
+
int64_t qmp_guest_exec(const char *path,
bool has_params, strList *params,
bool has_env, strList *env,
@@ -1102,6 +1116,8 @@ int64_t 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] 24+ messages in thread* [Qemu-devel] [PATCH 7/8] guest agent: add guest-pipe-open command on Windows
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
` (5 preceding siblings ...)
2014-12-31 13:06 ` [Qemu-devel] [PATCH 6/8] guest agent: ignore SIGPIPE signal Denis V. Lunev
@ 2014-12-31 13:06 ` Denis V. Lunev
2014-12-31 13:06 ` [Qemu-devel] [PATCH 8/8] guest agent: add guest-exec and guest-exec-status interfaces " Denis V. Lunev
` (2 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Denis V. Lunev @ 2014-12-31 13:06 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, Simon Zolin, Michael Roth
From: Simon Zolin <szolin@parallels.com>
Create anonymous pipe that can be passed as a stdin/stdout/stderr handle to
a child process spawned using forthcoming guest-file-exec command. Working
with a pipe is done using the existing interfaces guest-file-*.
Signed-off-by: Simon Zolin <szolin@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qga/commands-win32.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 74 insertions(+), 4 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 8fb29fc..4160c2b 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -37,6 +37,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 +85,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 +98,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 +146,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,9 +157,70 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode,
return fd;
}
+/* Create an anonymous pipe.
+ * Emulate non-blocking UNIX behavior using PIPE_NOWAIT flag: ReadFile()
+ * will fail with ERROR_NO_DATA; WriteFile() will return 0 bytes written. */
int64_t qmp_guest_pipe_open(const char *mode, Error **errp)
{
- error_set(errp, QERR_UNSUPPORTED);
+ HANDLE fd[2];
+ int this_end, other_end;
+ DWORD pipemode;
+ int64_t handle;
+ SECURITY_ATTRIBUTES sa = {
+ sizeof(SECURITY_ATTRIBUTES), NULL, 0
+ };
+
+ slog("guest-pipe-open called");
+
+ if ((mode[0] != 'r' && mode[0] != 'w') || mode[1] != '\0') {
+ error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "Only \"r\" or \"w\" are the valid modes to open a pipe");
+ return -1;
+ }
+
+ if (!CreatePipe(&fd[0], &fd[1], &sa, 0)) {
+ error_set_errno(errp, GetLastError(), QERR_QGA_COMMAND_FAILED,
+ "CreatePipe() failed");
+ return -1;
+ }
+
+ this_end = (mode[0] == 'w');
+ other_end = !this_end;
+
+ pipemode = PIPE_READMODE_BYTE | PIPE_NOWAIT;
+ if (!SetNamedPipeHandleState(fd[this_end], &pipemode, NULL, NULL)) {
+ error_set_errno(errp, GetLastError(), QERR_QGA_COMMAND_FAILED,
+ "SetNamedPipeHandleState() failed");
+ goto fail;
+ }
+
+ handle = guest_file_handle_add(fd[this_end], fd[other_end], errp);
+ if (handle == -1) {
+ goto fail;
+ }
+
+ slog("guest-pipe-open: handle: %" PRId64, handle);
+
+ return handle;
+
+fail:
+ CloseHandle(fd[0]);
+ CloseHandle(fd[1]);
+
+ return -1;
+}
+
+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;
}
@@ -168,6 +232,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_errno(errp, GetLastError(), "failed to close pipe handle");
+ return;
+ }
+
ret = CloseHandle(gfh->fh);
if (!ret) {
error_setg_errno(errp, GetLastError(), "failed close handle");
@@ -707,7 +777,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
"guest-suspend-hybrid", "guest-network-get-interfaces",
"guest-get-vcpus", "guest-set-vcpus",
"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] 24+ messages in thread* [Qemu-devel] [PATCH 8/8] guest agent: add guest-exec and guest-exec-status interfaces on Windows
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
` (6 preceding siblings ...)
2014-12-31 13:06 ` [Qemu-devel] [PATCH 7/8] guest agent: add guest-pipe-open command on Windows Denis V. Lunev
@ 2014-12-31 13:06 ` Denis V. Lunev
2015-01-09 17:06 ` [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Michael Roth
2015-01-27 13:52 ` Denis V. Lunev
9 siblings, 0 replies; 24+ messages in thread
From: Denis V. Lunev @ 2014-12-31 13:06 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, Simon Zolin, Michael Roth
From: Simon Zolin <szolin@parallels.com>
guest-exec command executes a new process on a guest machine. Command-line
arguments, environment, stdin/stdout/stderr handles can be passed to a new
process.
guest-exec-status command gets the status of process executed by
'guest-exec'.
Signed-off-by: Simon Zolin <szolin@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qga/commands-win32.c | 356 +++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 346 insertions(+), 10 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 4160c2b..85c300b 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -224,9 +224,10 @@ static int guest_pipe_close_other_end(GuestFileHandle *gfh)
return 0;
}
+static bool guest_exec_file_busy(GuestFileHandle *gfh);
+
void qmp_guest_file_close(int64_t handle, Error **errp)
{
- bool ret;
GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
slog("guest-file-close called, handle: %" PRId64, handle);
if (gfh == NULL) {
@@ -238,9 +239,19 @@ void qmp_guest_file_close(int64_t handle, Error **errp)
return;
}
- ret = CloseHandle(gfh->fh);
- if (!ret) {
- error_setg_errno(errp, GetLastError(), "failed close handle");
+ if (gfh->fh != INVALID_HANDLE_VALUE) {
+ if (!CloseHandle(gfh->fh)) {
+ error_setg_errno(errp, GetLastError(), "failed to close handle");
+ return;
+ }
+
+ gfh->fh = INVALID_HANDLE_VALUE;
+ }
+
+ if (guest_exec_file_busy(gfh)) {
+ error_setg_errno(errp, GetLastError(),
+ "can't close handle %" PRId64 ", "
+ "because it's used by guest-exec", handle);
return;
}
@@ -458,10 +469,252 @@ 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;
+}
+
+/* Return TRUE if the specified file is used in guest-exec command.
+ * We must not free the memory associated with GuestFileHandle* until
+ * guest-exec-status is called. */
+static bool guest_exec_file_busy(GuestFileHandle *gfh)
+{
+ GuestExecInfo *gei;
+
+ QTAILQ_FOREACH(gei, &guest_exec_state.processes, next) {
+
+ if (gei->gfh_stdin == gfh ||
+ gei->gfh_stdout == gfh ||
+ gei->gfh_stderr == gfh) {
+
+ return true;
+ }
+ }
+
+ return false;
+}
+
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_errno(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, char **argv_str,
+ Error **errp)
+{
+ const strList *it;
+ bool with_spaces;
+ size_t cap = 0;
+ WCHAR *wargs;
+ char *pargv;
+
+ *wpath = NULL;
+
+ 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, (*argv_str + 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, (*argv_str + 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 mbtowc_failed;
+ }
+
+ cap = strlen(path) + 1;
+ *wpath = g_malloc(cap * sizeof(WCHAR));
+ if (utf8_to_ucs2(*wpath, cap, path, -1) == 0) {
+ goto mbtowc_failed;
+ }
+
+ return wargs;
+
+mbtowc_failed:
+ error_setg_errno(errp, GetLastError(), "MultiByteToWideChar() failed");
+ g_free(*argv_str);
+ g_free(wargs);
+ if (*wpath != NULL) {
+ g_free(*wpath);
+ }
+ return NULL;
+}
+
+/* Prepare environment string for CreateProcess(). */
+static char *guest_exec_get_env(strList *env, Error **errp)
+{
+ const strList *it;
+ size_t cap = 0;
+ char *env_str, *s;
+
+ for (it = env; it != NULL; it = it->next) {
+ cap += strlen(it->value) + 1;
+ }
+ cap++;
+
+ env_str = g_malloc(cap);
+ s = env_str;
+
+ for (it = env; it != NULL; it = it->next) {
+
+ pstrcpy(s, (env_str + cap) - s, it->value);
+ s += strlen(it->value);
+
+ *s++ = '\0';
+ }
+ *s = '\0';
+
+ return env_str;
+}
+
+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;
}
int64_t qmp_guest_exec(const char *path,
@@ -472,8 +725,91 @@ int64_t 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;
+ BOOL b;
+ PROCESS_INFORMATION info;
+ STARTUPINFOW si;
+ char *argv_str, *env_str = NULL;
+ WCHAR *wpath, *wargs;
+ GuestFileHandle *gfh_stdin = NULL, *gfh_stdout = NULL, *gfh_stderr = NULL;
+
+ wargs = guest_exec_get_args(path, &wpath, has_params ? params : NULL,
+ &argv_str, errp);
+ if (wargs == NULL) {
+ return -1;
+ }
+ slog("guest-exec called: %s", argv_str);
+ g_free(argv_str);
+
+ env_str = guest_exec_get_env(has_env ? env : NULL, errp);
+ if (env_str == NULL) {
+ return -1;
+ }
+
+ 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;
+ }
+ }
+
+ memset(&si, 0, sizeof(STARTUPINFOW));
+ si.cb = sizeof(STARTUPINFOW);
+ si.hStdInput = INVALID_HANDLE_VALUE;
+ si.hStdOutput = INVALID_HANDLE_VALUE;
+ si.hStdError = INVALID_HANDLE_VALUE;
+
+ 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*/,
+ DETACHED_PROCESS,
+ env_str, NULL /*inherited current dir*/, &si, &info);
+ if (!b) {
+ error_setg_errno(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;
+
+done:
+ g_free(wpath);
+ g_free(wargs);
+ g_free(env_str);
+ return pid;
}
GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
@@ -777,8 +1113,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
"guest-suspend-hybrid", "guest-network-get-interfaces",
"guest-get-vcpus", "guest-set-vcpus",
"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) {
@@ -806,4 +1141,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] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
` (7 preceding siblings ...)
2014-12-31 13:06 ` [Qemu-devel] [PATCH 8/8] guest agent: add guest-exec and guest-exec-status interfaces " Denis V. Lunev
@ 2015-01-09 17:06 ` Michael Roth
2015-01-09 17:10 ` Eric Blake
2015-01-09 18:09 ` Denis V. Lunev
2015-01-27 13:52 ` Denis V. Lunev
9 siblings, 2 replies; 24+ messages in thread
From: Michael Roth @ 2015-01-09 17:06 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel, Semen Zolin
Quoting Denis V. Lunev (2014-12-31 07:06:46)
> hese patches for guest-agent add the functionality to execute commands on
> a guest UNIX machine.
Hi Denis,
Glad to see these getting picked up. I did at some point hack up a rewrite
of the original code though which has some elements might be worth considering
incorporating into your patchset.
The main one is the use of g_spawn_async_with_pipes(), which wraps fork/exec
vs. CreateProcess() and allows for more shared code between posix/win32.
It also creates the stdin/out/err FDs for you, which I suppose we could
do ourselves manually here as well, but it also raises the question of
whether guest-pipe-open is really necessary. In my code I ended up dropping
it since I can't imagine a use-case outside of guest-exec, but in doing so
also I dropped the ability to pipe one process into another...
But thinking about it more I think you can still pipe one process into
another by dup2()'ing the stdin FD returned by g_spawn_async_with_pipes()
to whatever stdout/stderr you wish to specify from a previous process.
The other one worth considering is allowing cmdline to simply be a string,
to parse it into arguments using g_shell_parse_argv(), which should also
be cross-platform.
If you do things that the core exec code ends up looking something like
this:
static GuestExecInfo *guest_exec_spawn(const char *cmdline, bool interactive,
Error **errp)
{
GSpawnFlags default_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD;
gboolean ret;
GPid gpid;
gchar **argv;
gint argc;
GError *gerr = NULL;
gint fd_in = -1, fd_out = -1, fd_err = -1;
ret = g_shell_parse_argv(cmdline, &argc, &argv, &gerr);
if (!ret || gerr) {
error_setg(errp, "failed to parse command: %s, %s", cmdline,
gerr->message);
return NULL;
}
ret = g_spawn_async_with_pipes(NULL, argv, NULL,
default_flags, NULL, NULL, &gpid,
interactive ? &fd_in : NULL, &fd_out, &fd_err, &gerr);
if (gerr) {
error_setg(errp, "failed to execute command: %s, %s", cmdline,
gerr->message);
return NULL;
}
if (!ret) {
error_setg(errp, "failed to execute command");
return NULL;
}
return guest_exec_info_new(gpid, cmdline, fd_in, fd_out, fd_err);
}
GuestExecResponse *qmp_guest_exec_async(const char *cmdline,
bool has_interactive,
bool interactive,
Error **errp)
{
GuestExecResponse *ger;
GuestExecInfo *gei;
int32_t handle;
gei = guest_exec_spawn(cmdline, has_interactive && interactive, errp);
if (error_is_set(errp)) {
return NULL;
}
print_gei(gei);
ger = g_new0(GuestExecResponse, 1);
if (has_interactive && interactive) {
ger->has_handle_stdin = true;
ger->handle_stdin =
guest_file_handle_add_fd(gei->fd_in, "a", errp);
if (error_is_set(errp)) {
return NULL;
}
}
ger->has_handle_stdout = true;
ger->handle_stdout =
guest_file_handle_add_fd(gei->fd_out, "r", errp);
if (error_is_set(errp)) {
return NULL;
}
ger->has_handle_stderr = true;
ger->handle_stderr =
guest_file_handle_add_fd(gei->fd_err, "r", errp);
if (error_is_set(errp)) {
return NULL;
}
handle = guest_exec_info_register(gei);
ger->status = qmp_guest_exec_status(handle, false, false, false, 0, errp);
if (error_is_set(errp)) {
return NULL;
}
return ger;
}
Where GuestExecResponse takes the place of the original PID return, since
we now need to fetch the stdin/stdout/stderr handles as well. In my code
it was defined as:
{ 'type': 'GuestExecResponse',
'data': { 'status': 'GuestExecStatus',
'*handle-stdin': 'int', '*handle-stdout': 'int',
'*handle-stderr': 'int' } }
Sorry for not just posting the patchset somewhere, the code was initially lost
in an accidental wipe of /home so I currently only have vim backup files to
piece it together from atm.
>
> 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 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: Semen Zolin <szolin@parallels.com>
> Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
2015-01-09 17:06 ` [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Michael Roth
@ 2015-01-09 17:10 ` Eric Blake
2015-01-09 18:09 ` Denis V. Lunev
1 sibling, 0 replies; 24+ messages in thread
From: Eric Blake @ 2015-01-09 17:10 UTC (permalink / raw)
To: Michael Roth, Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel, Semen Zolin
[-- Attachment #1: Type: text/plain, Size: 544 bytes --]
On 01/09/2015 10:06 AM, Michael Roth wrote:
> The other one worth considering is allowing cmdline to simply be a string,
> to parse it into arguments using g_shell_parse_argv(), which should also
> be cross-platform.
Please no. Passing an unparsed string and asking for platform-specific
rules to reparse it is dangerous, compared to requiring the user to pass
an array of literal arguments that need no further reparsing.
--
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] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
2015-01-09 17:06 ` [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Michael Roth
2015-01-09 17:10 ` Eric Blake
@ 2015-01-09 18:09 ` Denis V. Lunev
2015-01-09 19:29 ` Michael Roth
1 sibling, 1 reply; 24+ messages in thread
From: Denis V. Lunev @ 2015-01-09 18:09 UTC (permalink / raw)
To: Michael Roth; +Cc: Olga Krishtal, qemu-devel, Semen Zolin
On 09/01/15 20:06, Michael Roth wrote:
> Quoting Denis V. Lunev (2014-12-31 07:06:46)
>> hese patches for guest-agent add the functionality to execute commands on
>> a guest UNIX machine.
> Hi Denis,
>
> Glad to see these getting picked up. I did at some point hack up a rewrite
> of the original code though which has some elements might be worth considering
> incorporating into your patchset.
>
> The main one is the use of g_spawn_async_with_pipes(), which wraps fork/exec
> vs. CreateProcess() and allows for more shared code between posix/win32.
>
> It also creates the stdin/out/err FDs for you, which I suppose we could
> do ourselves manually here as well, but it also raises the question of
> whether guest-pipe-open is really necessary. In my code I ended up dropping
> it since I can't imagine a use-case outside of guest-exec, but in doing so
> also I dropped the ability to pipe one process into another...
>
> But thinking about it more I think you can still pipe one process into
> another by dup2()'ing the stdin FD returned by g_spawn_async_with_pipes()
> to whatever stdout/stderr you wish to specify from a previous process.
I do not think that this will be ever used. IMHO nobody will bind
processes in such a way. In the worst case the user will exec
something like 'sh -c "process1 | process2"'. This is better and
shorter.
> The other one worth considering is allowing cmdline to simply be a string,
> to parse it into arguments using g_shell_parse_argv(), which should also
> be cross-platform.
>
> If you do things that the core exec code ends up looking something like
> this:
>
> static GuestExecInfo *guest_exec_spawn(const char *cmdline, bool interactive,
> Error **errp)
> {
> GSpawnFlags default_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD;
> gboolean ret;
> GPid gpid;
> gchar **argv;
> gint argc;
> GError *gerr = NULL;
> gint fd_in = -1, fd_out = -1, fd_err = -1;
>
> ret = g_shell_parse_argv(cmdline, &argc, &argv, &gerr);
> if (!ret || gerr) {
> error_setg(errp, "failed to parse command: %s, %s", cmdline,
> gerr->message);
> return NULL;
> }
>
> ret = g_spawn_async_with_pipes(NULL, argv, NULL,
> default_flags, NULL, NULL, &gpid,
> interactive ? &fd_in : NULL, &fd_out, &fd_err, &gerr);
> if (gerr) {
> error_setg(errp, "failed to execute command: %s, %s", cmdline,
> gerr->message);
> return NULL;
> }
> if (!ret) {
> error_setg(errp, "failed to execute command");
> return NULL;
> }
>
> return guest_exec_info_new(gpid, cmdline, fd_in, fd_out, fd_err);
> }
>
> GuestExecResponse *qmp_guest_exec_async(const char *cmdline,
> bool has_interactive,
> bool interactive,
> Error **errp)
> {
> GuestExecResponse *ger;
> GuestExecInfo *gei;
> int32_t handle;
>
> gei = guest_exec_spawn(cmdline, has_interactive && interactive, errp);
> if (error_is_set(errp)) {
> return NULL;
> }
>
> print_gei(gei);
> ger = g_new0(GuestExecResponse, 1);
>
> if (has_interactive && interactive) {
> ger->has_handle_stdin = true;
> ger->handle_stdin =
> guest_file_handle_add_fd(gei->fd_in, "a", errp);
> if (error_is_set(errp)) {
> return NULL;
> }
> }
>
> ger->has_handle_stdout = true;
> ger->handle_stdout =
> guest_file_handle_add_fd(gei->fd_out, "r", errp);
> if (error_is_set(errp)) {
> return NULL;
> }
>
> ger->has_handle_stderr = true;
> ger->handle_stderr =
> guest_file_handle_add_fd(gei->fd_err, "r", errp);
> if (error_is_set(errp)) {
> return NULL;
> }
>
> handle = guest_exec_info_register(gei);
> ger->status = qmp_guest_exec_status(handle, false, false, false, 0, errp);
> if (error_is_set(errp)) {
> return NULL;
> }
>
> return ger;
> }
>
> Where GuestExecResponse takes the place of the original PID return, since
> we now need to fetch the stdin/stdout/stderr handles as well. In my code
> it was defined as:
>
> { 'type': 'GuestExecResponse',
> 'data': { 'status': 'GuestExecStatus',
> '*handle-stdin': 'int', '*handle-stdout': 'int',
> '*handle-stderr': 'int' } }
>
> Sorry for not just posting the patchset somewhere, the code was initially lost
> in an accidental wipe of /home so I currently only have vim backup files to
> piece it together from atm.
Frankly speaking this exact interface is not that
convenient for a real life products. The necessity
to poll exec status and to poll input/output
pipes is really boring and really affects the
performance.
Actually there are 2 major scenarios for this:
1. "Enter inside VM", i.e. the user obtains shell
inside VM and works in VM from host f.e. to setup
network
2. Execute command inside VM and collect output
in host.
For both case the proposed interface with guest
pipes is not convenient, in order to obtain interactive
shell in guest we should poll frequently (100 times
in seconds to avoid lag) and in my experience
end-users likes to run a lot of automation scripts
using this channel.
Thus we have used virtserial as a real transport for
case 1) and it is working seamlessly. This means
that we open virtserial in guest for input and output
and connect to Unix socket in host with shell application.
Poll of exec-status is necessary evil, but it does not affect
interactivity and could be done once in a second.
I would be good to have some notifications from
guest that some events are pending (input/output
data or exec status is available). But I do not know
at the moment how to implement this. At least I have
not invested resources into this as I would like
to hear some opinion from the community first.
This patchset is made mostly to initiate the discussion.
Michael, could you spend a bit of time looking
into patches 1 and 2 of the patchset. They have been
implemented and reviewed by us and could be
merged separately in advance. They provides
functional equivalent of already existing Linux (Posix)
functionality.
As for your approach, I would tend to agree with
Eric. It is not safe.
Regards,
Den
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
2015-01-09 18:09 ` Denis V. Lunev
@ 2015-01-09 19:29 ` Michael Roth
2015-01-13 10:13 ` Denis V. Lunev
0 siblings, 1 reply; 24+ messages in thread
From: Michael Roth @ 2015-01-09 19:29 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel, Semen Zolin
Quoting Denis V. Lunev (2015-01-09 12:09:10)
> On 09/01/15 20:06, Michael Roth wrote:
> > Quoting Denis V. Lunev (2014-12-31 07:06:46)
> >> hese patches for guest-agent add the functionality to execute commands on
> >> a guest UNIX machine.
> > Hi Denis,
> >
> > Glad to see these getting picked up. I did at some point hack up a rewrite
> > of the original code though which has some elements might be worth considering
> > incorporating into your patchset.
> >
> > The main one is the use of g_spawn_async_with_pipes(), which wraps fork/exec
> > vs. CreateProcess() and allows for more shared code between posix/win32.
> >
> > It also creates the stdin/out/err FDs for you, which I suppose we could
> > do ourselves manually here as well, but it also raises the question of
> > whether guest-pipe-open is really necessary. In my code I ended up dropping
> > it since I can't imagine a use-case outside of guest-exec, but in doing so
> > also I dropped the ability to pipe one process into another...
> >
> > But thinking about it more I think you can still pipe one process into
> > another by dup2()'ing the stdin FD returned by g_spawn_async_with_pipes()
> > to whatever stdout/stderr you wish to specify from a previous process.
> I do not think that this will be ever used. IMHO nobody will bind
> processes in such a way. In the worst case the user will exec
> something like 'sh -c "process1 | process2"'. This is better and
> shorter.
Yah, that was ultimately my reason for dropping the use-case, and just
having interactive/non-interactive rather than explicit control over
input/output pipes. But it's the one thing that havng guest-pipe-open
potentially worthwhile, so I think there's a good cause to drop that
interface and let guest-exec pass us the FDs if we agree it isn't
worth supporting.
>
> > The other one worth considering is allowing cmdline to simply be a string,
> > to parse it into arguments using g_shell_parse_argv(), which should also
> > be cross-platform.
> >
> > If you do things that the core exec code ends up looking something like
> > this:
> >
> > static GuestExecInfo *guest_exec_spawn(const char *cmdline, bool interactive,
> > Error **errp)
> > {
> > GSpawnFlags default_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD;
> > gboolean ret;
> > GPid gpid;
> > gchar **argv;
> > gint argc;
> > GError *gerr = NULL;
> > gint fd_in = -1, fd_out = -1, fd_err = -1;
> >
> > ret = g_shell_parse_argv(cmdline, &argc, &argv, &gerr);
> > if (!ret || gerr) {
> > error_setg(errp, "failed to parse command: %s, %s", cmdline,
> > gerr->message);
> > return NULL;
> > }
> >
> > ret = g_spawn_async_with_pipes(NULL, argv, NULL,
> > default_flags, NULL, NULL, &gpid,
> > interactive ? &fd_in : NULL, &fd_out, &fd_err, &gerr);
> > if (gerr) {
> > error_setg(errp, "failed to execute command: %s, %s", cmdline,
> > gerr->message);
> > return NULL;
> > }
> > if (!ret) {
> > error_setg(errp, "failed to execute command");
> > return NULL;
> > }
> >
> > return guest_exec_info_new(gpid, cmdline, fd_in, fd_out, fd_err);
> > }
> >
> > GuestExecResponse *qmp_guest_exec_async(const char *cmdline,
> > bool has_interactive,
> > bool interactive,
> > Error **errp)
> > {
> > GuestExecResponse *ger;
> > GuestExecInfo *gei;
> > int32_t handle;
> >
> > gei = guest_exec_spawn(cmdline, has_interactive && interactive, errp);
> > if (error_is_set(errp)) {
> > return NULL;
> > }
> >
> > print_gei(gei);
> > ger = g_new0(GuestExecResponse, 1);
> >
> > if (has_interactive && interactive) {
> > ger->has_handle_stdin = true;
> > ger->handle_stdin =
> > guest_file_handle_add_fd(gei->fd_in, "a", errp);
> > if (error_is_set(errp)) {
> > return NULL;
> > }
> > }
> >
> > ger->has_handle_stdout = true;
> > ger->handle_stdout =
> > guest_file_handle_add_fd(gei->fd_out, "r", errp);
> > if (error_is_set(errp)) {
> > return NULL;
> > }
> >
> > ger->has_handle_stderr = true;
> > ger->handle_stderr =
> > guest_file_handle_add_fd(gei->fd_err, "r", errp);
> > if (error_is_set(errp)) {
> > return NULL;
> > }
> >
> > handle = guest_exec_info_register(gei);
> > ger->status = qmp_guest_exec_status(handle, false, false, false, 0, errp);
> > if (error_is_set(errp)) {
> > return NULL;
> > }
> >
> > return ger;
> > }
> >
> > Where GuestExecResponse takes the place of the original PID return, since
> > we now need to fetch the stdin/stdout/stderr handles as well. In my code
> > it was defined as:
> >
> > { 'type': 'GuestExecResponse',
> > 'data': { 'status': 'GuestExecStatus',
> > '*handle-stdin': 'int', '*handle-stdout': 'int',
> > '*handle-stderr': 'int' } }
> >
> > Sorry for not just posting the patchset somewhere, the code was initially lost
> > in an accidental wipe of /home so I currently only have vim backup files to
> > piece it together from atm.
> Frankly speaking this exact interface is not that
> convenient for a real life products. The necessity
> to poll exec status and to poll input/output
> pipes is really boring and really affects the
> performance.
>
> Actually there are 2 major scenarios for this:
> 1. "Enter inside VM", i.e. the user obtains shell
> inside VM and works in VM from host f.e. to setup
> network
> 2. Execute command inside VM and collect output
> in host.
Ultimately I ended up with 2 interfaces, guest-exec-async,
which is implemented something like above, and guest-exec,
which simplifies the common case by executing synchronously
and simply allowing a timeout to be specified as an
argument. base64-encoded stdout/stderr buffer is subsequently
returned, and limited in size to 1M (user can redirect to
file in guest as an alternative). I think this handles
the vast amount of use-cases for 2), but for processes
that generate lots of output writing to a filesystem can
be problematic. And for that use case I don't think
polling is particularly an issue, performance wise.
so I have a hard time rationalizing away the need for a
guest-exec-async at some point, even if we don't support
leveraging it for interactive shells.
guest->host events would be an interesting way to optimize
it though, but I'm okay with making polling necessary to
read from or reap a process, and adding that as a cue to
make things more efficient later while maintaining
compatibility with existing users/interfaces.
The pipes are indeed tedious, which is why the added setup
of using guest-pipe-open was dropped in my implementation.
You still have to deal with reading/writing/closed to FDs
returned by guest-exec-async, but that's the core use-case
for that sort of interface I think.
>
> For both case the proposed interface with guest
> pipes is not convenient, in order to obtain interactive
> shell in guest we should poll frequently (100 times
> in seconds to avoid lag) and in my experience
> end-users likes to run a lot of automation scripts
> using this channel.
>
> Thus we have used virtserial as a real transport for
> case 1) and it is working seamlessly. This means
> that we open virtserial in guest for input and output
> and connect to Unix socket in host with shell application.
> Poll of exec-status is necessary evil, but it does not affect
> interactivity and could be done once in a second.
>
> I would be good to have some notifications from
> guest that some events are pending (input/output
> data or exec status is available). But I do not know
> at the moment how to implement this. At least I have
> not invested resources into this as I would like
> to hear some opinion from the community first.
> This patchset is made mostly to initiate the discussion.
>
> Michael, could you spend a bit of time looking
> into patches 1 and 2 of the patchset. They have been
> implemented and reviewed by us and could be
> merged separately in advance. They provides
> functional equivalent of already existing Linux (Posix)
> functionality.
Absolutely!
>
> As for your approach, I would tend to agree with
> Eric. It is not safe.
I hadn't fully considered the matter of safety. I know shell
operators are santized/unsupported by the interface, but I do
suppose even basic command-line parsing can still be ambiguous
from one program to the next so perhaps we should avoid it on
that basis at the least.
>
> Regards,
> Den
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
2015-01-09 19:29 ` Michael Roth
@ 2015-01-13 10:13 ` Denis V. Lunev
2015-02-03 20:24 ` Michael Roth
0 siblings, 1 reply; 24+ messages in thread
From: Denis V. Lunev @ 2015-01-13 10:13 UTC (permalink / raw)
To: Michael Roth; +Cc: Olga Krishtal, qemu-devel, Semen Zolin
On 09/01/15 22:29, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-01-09 12:09:10)
>> On 09/01/15 20:06, Michael Roth wrote:
>>> Quoting Denis V. Lunev (2014-12-31 07:06:46)
>>>> hese patches for guest-agent add the functionality to execute commands on
>>>> a guest UNIX machine.
>>> Hi Denis,
>>>
>>> Glad to see these getting picked up. I did at some point hack up a rewrite
>>> of the original code though which has some elements might be worth considering
>>> incorporating into your patchset.
>>>
>>> The main one is the use of g_spawn_async_with_pipes(), which wraps fork/exec
>>> vs. CreateProcess() and allows for more shared code between posix/win32.
>>>
>>> It also creates the stdin/out/err FDs for you, which I suppose we could
>>> do ourselves manually here as well, but it also raises the question of
>>> whether guest-pipe-open is really necessary. In my code I ended up dropping
>>> it since I can't imagine a use-case outside of guest-exec, but in doing so
>>> also I dropped the ability to pipe one process into another...
>>>
>>> But thinking about it more I think you can still pipe one process into
>>> another by dup2()'ing the stdin FD returned by g_spawn_async_with_pipes()
>>> to whatever stdout/stderr you wish to specify from a previous process.
>> I do not think that this will be ever used. IMHO nobody will bind
>> processes in such a way. In the worst case the user will exec
>> something like 'sh -c "process1 | process2"'. This is better and
>> shorter.
> Yah, that was ultimately my reason for dropping the use-case, and just
> having interactive/non-interactive rather than explicit control over
> input/output pipes. But it's the one thing that havng guest-pipe-open
> potentially worthwhile, so I think there's a good cause to drop that
> interface and let guest-exec pass us the FDs if we agree it isn't
> worth supporting.
>
>>> The other one worth considering is allowing cmdline to simply be a string,
>>> to parse it into arguments using g_shell_parse_argv(), which should also
>>> be cross-platform.
>>>
>>> If you do things that the core exec code ends up looking something like
>>> this:
>>>
>>> static GuestExecInfo *guest_exec_spawn(const char *cmdline, bool interactive,
>>> Error **errp)
>>> {
>>> GSpawnFlags default_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD;
>>> gboolean ret;
>>> GPid gpid;
>>> gchar **argv;
>>> gint argc;
>>> GError *gerr = NULL;
>>> gint fd_in = -1, fd_out = -1, fd_err = -1;
>>>
>>> ret = g_shell_parse_argv(cmdline, &argc, &argv, &gerr);
>>> if (!ret || gerr) {
>>> error_setg(errp, "failed to parse command: %s, %s", cmdline,
>>> gerr->message);
>>> return NULL;
>>> }
>>>
>>> ret = g_spawn_async_with_pipes(NULL, argv, NULL,
>>> default_flags, NULL, NULL, &gpid,
>>> interactive ? &fd_in : NULL, &fd_out, &fd_err, &gerr);
>>> if (gerr) {
>>> error_setg(errp, "failed to execute command: %s, %s", cmdline,
>>> gerr->message);
>>> return NULL;
>>> }
>>> if (!ret) {
>>> error_setg(errp, "failed to execute command");
>>> return NULL;
>>> }
>>>
>>> return guest_exec_info_new(gpid, cmdline, fd_in, fd_out, fd_err);
>>> }
>>>
>>> GuestExecResponse *qmp_guest_exec_async(const char *cmdline,
>>> bool has_interactive,
>>> bool interactive,
>>> Error **errp)
>>> {
>>> GuestExecResponse *ger;
>>> GuestExecInfo *gei;
>>> int32_t handle;
>>>
>>> gei = guest_exec_spawn(cmdline, has_interactive && interactive, errp);
>>> if (error_is_set(errp)) {
>>> return NULL;
>>> }
>>>
>>> print_gei(gei);
>>> ger = g_new0(GuestExecResponse, 1);
>>>
>>> if (has_interactive && interactive) {
>>> ger->has_handle_stdin = true;
>>> ger->handle_stdin =
>>> guest_file_handle_add_fd(gei->fd_in, "a", errp);
>>> if (error_is_set(errp)) {
>>> return NULL;
>>> }
>>> }
>>>
>>> ger->has_handle_stdout = true;
>>> ger->handle_stdout =
>>> guest_file_handle_add_fd(gei->fd_out, "r", errp);
>>> if (error_is_set(errp)) {
>>> return NULL;
>>> }
>>>
>>> ger->has_handle_stderr = true;
>>> ger->handle_stderr =
>>> guest_file_handle_add_fd(gei->fd_err, "r", errp);
>>> if (error_is_set(errp)) {
>>> return NULL;
>>> }
>>>
>>> handle = guest_exec_info_register(gei);
>>> ger->status = qmp_guest_exec_status(handle, false, false, false, 0, errp);
>>> if (error_is_set(errp)) {
>>> return NULL;
>>> }
>>>
>>> return ger;
>>> }
>>>
>>> Where GuestExecResponse takes the place of the original PID return, since
>>> we now need to fetch the stdin/stdout/stderr handles as well. In my code
>>> it was defined as:
>>>
>>> { 'type': 'GuestExecResponse',
>>> 'data': { 'status': 'GuestExecStatus',
>>> '*handle-stdin': 'int', '*handle-stdout': 'int',
>>> '*handle-stderr': 'int' } }
>>>
>>> Sorry for not just posting the patchset somewhere, the code was initially lost
>>> in an accidental wipe of /home so I currently only have vim backup files to
>>> piece it together from atm.
>> Frankly speaking this exact interface is not that
>> convenient for a real life products. The necessity
>> to poll exec status and to poll input/output
>> pipes is really boring and really affects the
>> performance.
>>
>> Actually there are 2 major scenarios for this:
>> 1. "Enter inside VM", i.e. the user obtains shell
>> inside VM and works in VM from host f.e. to setup
>> network
>> 2. Execute command inside VM and collect output
>> in host.
> Ultimately I ended up with 2 interfaces, guest-exec-async,
> which is implemented something like above, and guest-exec,
> which simplifies the common case by executing synchronously
> and simply allowing a timeout to be specified as an
> argument. base64-encoded stdout/stderr buffer is subsequently
> returned, and limited in size to 1M (user can redirect to
> file in guest as an alternative). I think this handles
> the vast amount of use-cases for 2), but for processes
> that generate lots of output writing to a filesystem can
> be problematic. And for that use case I don't think
> polling is particularly an issue, performance wise.
> so I have a hard time rationalizing away the need for a
> guest-exec-async at some point, even if we don't support
> leveraging it for interactive shells.
>
> guest->host events would be an interesting way to optimize
> it though, but I'm okay with making polling necessary to
> read from or reap a process, and adding that as a cue to
> make things more efficient later while maintaining
> compatibility with existing users/interfaces.
>
> The pipes are indeed tedious, which is why the added setup
> of using guest-pipe-open was dropped in my implementation.
> You still have to deal with reading/writing/closed to FDs
> returned by guest-exec-async, but that's the core use-case
> for that sort of interface I think.
>
>> For both case the proposed interface with guest
>> pipes is not convenient, in order to obtain interactive
>> shell in guest we should poll frequently (100 times
>> in seconds to avoid lag) and in my experience
>> end-users likes to run a lot of automation scripts
>> using this channel.
>>
>> Thus we have used virtserial as a real transport for
>> case 1) and it is working seamlessly. This means
>> that we open virtserial in guest for input and output
>> and connect to Unix socket in host with shell application.
>> Poll of exec-status is necessary evil, but it does not affect
>> interactivity and could be done once in a second.
>>
>> I would be good to have some notifications from
>> guest that some events are pending (input/output
>> data or exec status is available). But I do not know
>> at the moment how to implement this. At least I have
>> not invested resources into this as I would like
>> to hear some opinion from the community first.
>> This patchset is made mostly to initiate the discussion.
>>
>> Michael, could you spend a bit of time looking
>> into patches 1 and 2 of the patchset. They have been
>> implemented and reviewed by us and could be
>> merged separately in advance. They provides
>> functional equivalent of already existing Linux (Posix)
>> functionality.
> Absolutely!
>
>> As for your approach, I would tend to agree with
>> Eric. It is not safe.
> I hadn't fully considered the matter of safety. I know shell
> operators are santized/unsupported by the interface, but I do
> suppose even basic command-line parsing can still be ambiguous
> from one program to the next so perhaps we should avoid it on
> that basis at the least.
>
>> Regards,
>> Den
OK, Michael, I am a little bit confused at the moment.
What will be the next step and who is waiting whom?
I think that the major architectural argument from
your side is pointed out by Eric and should not be taken
into account.
The matter regarding "sync & async" exec types is
still questionable and could be omitted at the moment.
We will be able to extend the interface later. Guest
notifications is a more interesting thing but it is
much more difficult to implement.
Therefore I think that the ball is on your side and
we are waiting for a review from your side.
Regards,
Den
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
2015-01-13 10:13 ` Denis V. Lunev
@ 2015-02-03 20:24 ` Michael Roth
2015-02-03 21:31 ` Denis V. Lunev
0 siblings, 1 reply; 24+ messages in thread
From: Michael Roth @ 2015-02-03 20:24 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Olga Krishtal, qemu-devel, Semen Zolin
Quoting Denis V. Lunev (2015-01-13 04:13:03)
> On 09/01/15 22:29, Michael Roth wrote:
> > Quoting Denis V. Lunev (2015-01-09 12:09:10)
> >> On 09/01/15 20:06, Michael Roth wrote:
> >>> Quoting Denis V. Lunev (2014-12-31 07:06:46)
> >>>> hese patches for guest-agent add the functionality to execute commands on
> >>>> a guest UNIX machine.
> >>> Hi Denis,
> >>>
> >>> Glad to see these getting picked up. I did at some point hack up a rewrite
> >>> of the original code though which has some elements might be worth considering
> >>> incorporating into your patchset.
> >>>
> >>> The main one is the use of g_spawn_async_with_pipes(), which wraps fork/exec
> >>> vs. CreateProcess() and allows for more shared code between posix/win32.
> >>>
> >>> It also creates the stdin/out/err FDs for you, which I suppose we could
> >>> do ourselves manually here as well, but it also raises the question of
> >>> whether guest-pipe-open is really necessary. In my code I ended up dropping
> >>> it since I can't imagine a use-case outside of guest-exec, but in doing so
> >>> also I dropped the ability to pipe one process into another...
> >>>
> >>> But thinking about it more I think you can still pipe one process into
> >>> another by dup2()'ing the stdin FD returned by g_spawn_async_with_pipes()
> >>> to whatever stdout/stderr you wish to specify from a previous process.
> >> I do not think that this will be ever used. IMHO nobody will bind
> >> processes in such a way. In the worst case the user will exec
> >> something like 'sh -c "process1 | process2"'. This is better and
> >> shorter.
> > Yah, that was ultimately my reason for dropping the use-case, and just
> > having interactive/non-interactive rather than explicit control over
> > input/output pipes. But it's the one thing that havng guest-pipe-open
> > potentially worthwhile, so I think there's a good cause to drop that
> > interface and let guest-exec pass us the FDs if we agree it isn't
> > worth supporting.
> >
> >>> The other one worth considering is allowing cmdline to simply be a string,
> >>> to parse it into arguments using g_shell_parse_argv(), which should also
> >>> be cross-platform.
> >>>
> >>> If you do things that the core exec code ends up looking something like
> >>> this:
> >>>
> >>> static GuestExecInfo *guest_exec_spawn(const char *cmdline, bool interactive,
> >>> Error **errp)
> >>> {
> >>> GSpawnFlags default_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD;
> >>> gboolean ret;
> >>> GPid gpid;
> >>> gchar **argv;
> >>> gint argc;
> >>> GError *gerr = NULL;
> >>> gint fd_in = -1, fd_out = -1, fd_err = -1;
> >>>
> >>> ret = g_shell_parse_argv(cmdline, &argc, &argv, &gerr);
> >>> if (!ret || gerr) {
> >>> error_setg(errp, "failed to parse command: %s, %s", cmdline,
> >>> gerr->message);
> >>> return NULL;
> >>> }
> >>>
> >>> ret = g_spawn_async_with_pipes(NULL, argv, NULL,
> >>> default_flags, NULL, NULL, &gpid,
> >>> interactive ? &fd_in : NULL, &fd_out, &fd_err, &gerr);
> >>> if (gerr) {
> >>> error_setg(errp, "failed to execute command: %s, %s", cmdline,
> >>> gerr->message);
> >>> return NULL;
> >>> }
> >>> if (!ret) {
> >>> error_setg(errp, "failed to execute command");
> >>> return NULL;
> >>> }
> >>>
> >>> return guest_exec_info_new(gpid, cmdline, fd_in, fd_out, fd_err);
> >>> }
> >>>
> >>> GuestExecResponse *qmp_guest_exec_async(const char *cmdline,
> >>> bool has_interactive,
> >>> bool interactive,
> >>> Error **errp)
> >>> {
> >>> GuestExecResponse *ger;
> >>> GuestExecInfo *gei;
> >>> int32_t handle;
> >>>
> >>> gei = guest_exec_spawn(cmdline, has_interactive && interactive, errp);
> >>> if (error_is_set(errp)) {
> >>> return NULL;
> >>> }
> >>>
> >>> print_gei(gei);
> >>> ger = g_new0(GuestExecResponse, 1);
> >>>
> >>> if (has_interactive && interactive) {
> >>> ger->has_handle_stdin = true;
> >>> ger->handle_stdin =
> >>> guest_file_handle_add_fd(gei->fd_in, "a", errp);
> >>> if (error_is_set(errp)) {
> >>> return NULL;
> >>> }
> >>> }
> >>>
> >>> ger->has_handle_stdout = true;
> >>> ger->handle_stdout =
> >>> guest_file_handle_add_fd(gei->fd_out, "r", errp);
> >>> if (error_is_set(errp)) {
> >>> return NULL;
> >>> }
> >>>
> >>> ger->has_handle_stderr = true;
> >>> ger->handle_stderr =
> >>> guest_file_handle_add_fd(gei->fd_err, "r", errp);
> >>> if (error_is_set(errp)) {
> >>> return NULL;
> >>> }
> >>>
> >>> handle = guest_exec_info_register(gei);
> >>> ger->status = qmp_guest_exec_status(handle, false, false, false, 0, errp);
> >>> if (error_is_set(errp)) {
> >>> return NULL;
> >>> }
> >>>
> >>> return ger;
> >>> }
> >>>
> >>> Where GuestExecResponse takes the place of the original PID return, since
> >>> we now need to fetch the stdin/stdout/stderr handles as well. In my code
> >>> it was defined as:
> >>>
> >>> { 'type': 'GuestExecResponse',
> >>> 'data': { 'status': 'GuestExecStatus',
> >>> '*handle-stdin': 'int', '*handle-stdout': 'int',
> >>> '*handle-stderr': 'int' } }
> >>>
> >>> Sorry for not just posting the patchset somewhere, the code was initially lost
> >>> in an accidental wipe of /home so I currently only have vim backup files to
> >>> piece it together from atm.
> >> Frankly speaking this exact interface is not that
> >> convenient for a real life products. The necessity
> >> to poll exec status and to poll input/output
> >> pipes is really boring and really affects the
> >> performance.
> >>
> >> Actually there are 2 major scenarios for this:
> >> 1. "Enter inside VM", i.e. the user obtains shell
> >> inside VM and works in VM from host f.e. to setup
> >> network
> >> 2. Execute command inside VM and collect output
> >> in host.
> > Ultimately I ended up with 2 interfaces, guest-exec-async,
> > which is implemented something like above, and guest-exec,
> > which simplifies the common case by executing synchronously
> > and simply allowing a timeout to be specified as an
> > argument. base64-encoded stdout/stderr buffer is subsequently
> > returned, and limited in size to 1M (user can redirect to
> > file in guest as an alternative). I think this handles
> > the vast amount of use-cases for 2), but for processes
> > that generate lots of output writing to a filesystem can
> > be problematic. And for that use case I don't think
> > polling is particularly an issue, performance wise.
> > so I have a hard time rationalizing away the need for a
> > guest-exec-async at some point, even if we don't support
> > leveraging it for interactive shells.
> >
> > guest->host events would be an interesting way to optimize
> > it though, but I'm okay with making polling necessary to
> > read from or reap a process, and adding that as a cue to
> > make things more efficient later while maintaining
> > compatibility with existing users/interfaces.
> >
> > The pipes are indeed tedious, which is why the added setup
> > of using guest-pipe-open was dropped in my implementation.
> > You still have to deal with reading/writing/closed to FDs
> > returned by guest-exec-async, but that's the core use-case
> > for that sort of interface I think.
> >
> >> For both case the proposed interface with guest
> >> pipes is not convenient, in order to obtain interactive
> >> shell in guest we should poll frequently (100 times
> >> in seconds to avoid lag) and in my experience
> >> end-users likes to run a lot of automation scripts
> >> using this channel.
> >>
> >> Thus we have used virtserial as a real transport for
> >> case 1) and it is working seamlessly. This means
> >> that we open virtserial in guest for input and output
> >> and connect to Unix socket in host with shell application.
> >> Poll of exec-status is necessary evil, but it does not affect
> >> interactivity and could be done once in a second.
> >>
> >> I would be good to have some notifications from
> >> guest that some events are pending (input/output
> >> data or exec status is available). But I do not know
> >> at the moment how to implement this. At least I have
> >> not invested resources into this as I would like
> >> to hear some opinion from the community first.
> >> This patchset is made mostly to initiate the discussion.
> >>
> >> Michael, could you spend a bit of time looking
> >> into patches 1 and 2 of the patchset. They have been
> >> implemented and reviewed by us and could be
> >> merged separately in advance. They provides
> >> functional equivalent of already existing Linux (Posix)
> >> functionality.
> > Absolutely!
> >
> >> As for your approach, I would tend to agree with
> >> Eric. It is not safe.
> > I hadn't fully considered the matter of safety. I know shell
> > operators are santized/unsupported by the interface, but I do
> > suppose even basic command-line parsing can still be ambiguous
> > from one program to the next so perhaps we should avoid it on
> > that basis at the least.
> >
> >> Regards,
> >> Den
> OK, Michael, I am a little bit confused at the moment.
> What will be the next step and who is waiting whom?
>
> I think that the major architectural argument from
> your side is pointed out by Eric and should not be taken
> into account.
Sorry for the delay. The main outstanding issue I have from
an architectural standpoint is whether we should consider
using g_spawn_sync/g_spawn_async to avoid the need to
maintain OS-specific variants of the exec support. I realize
that's not a good argument in the face of real/working code,
and I've been working on getting my g_spawn*()-based
implementation cleaned up so we can make a decision based
on that.
If you're still targetting 2.3 (soft-freeze is Feb 16th)
for guest-exec let me know and I can try to hurry things
along on my end, but for now I'm assuming the guest-exec
functionality is something we can pursue for 2.4.
I have been testing the w32 guest-file-* patches and they
seem to be in good shape for 2.3, so please repost those
as a separate patchset and I'll work to get them into the
next QGA pull.
>
> The matter regarding "sync & async" exec types is
> still questionable and could be omitted at the moment.
> We will be able to extend the interface later. Guest
> notifications is a more interesting thing but it is
> much more difficult to implement.
>
> Therefore I think that the ball is on your side and
> we are waiting for a review from your side.
>
> Regards,
> Den
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
2015-02-03 20:24 ` Michael Roth
@ 2015-02-03 21:31 ` Denis V. Lunev
0 siblings, 0 replies; 24+ messages in thread
From: Denis V. Lunev @ 2015-02-03 21:31 UTC (permalink / raw)
To: Michael Roth; +Cc: Olga Krishtal, qemu-devel, Semen Zolin
On 03/02/15 23:24, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-01-13 04:13:03)
>> On 09/01/15 22:29, Michael Roth wrote:
>>> Quoting Denis V. Lunev (2015-01-09 12:09:10)
>>>> On 09/01/15 20:06, Michael Roth wrote:
>>>>> Quoting Denis V. Lunev (2014-12-31 07:06:46)
>>>>>> hese patches for guest-agent add the functionality to execute commands on
>>>>>> a guest UNIX machine.
>>>>> Hi Denis,
>>>>>
>>>>> Glad to see these getting picked up. I did at some point hack up a rewrite
>>>>> of the original code though which has some elements might be worth considering
>>>>> incorporating into your patchset.
>>>>>
>>>>> The main one is the use of g_spawn_async_with_pipes(), which wraps fork/exec
>>>>> vs. CreateProcess() and allows for more shared code between posix/win32.
>>>>>
>>>>> It also creates the stdin/out/err FDs for you, which I suppose we could
>>>>> do ourselves manually here as well, but it also raises the question of
>>>>> whether guest-pipe-open is really necessary. In my code I ended up dropping
>>>>> it since I can't imagine a use-case outside of guest-exec, but in doing so
>>>>> also I dropped the ability to pipe one process into another...
>>>>>
>>>>> But thinking about it more I think you can still pipe one process into
>>>>> another by dup2()'ing the stdin FD returned by g_spawn_async_with_pipes()
>>>>> to whatever stdout/stderr you wish to specify from a previous process.
>>>> I do not think that this will be ever used. IMHO nobody will bind
>>>> processes in such a way. In the worst case the user will exec
>>>> something like 'sh -c "process1 | process2"'. This is better and
>>>> shorter.
>>> Yah, that was ultimately my reason for dropping the use-case, and just
>>> having interactive/non-interactive rather than explicit control over
>>> input/output pipes. But it's the one thing that havng guest-pipe-open
>>> potentially worthwhile, so I think there's a good cause to drop that
>>> interface and let guest-exec pass us the FDs if we agree it isn't
>>> worth supporting.
>>>
>>>>> The other one worth considering is allowing cmdline to simply be a string,
>>>>> to parse it into arguments using g_shell_parse_argv(), which should also
>>>>> be cross-platform.
>>>>>
>>>>> If you do things that the core exec code ends up looking something like
>>>>> this:
>>>>>
>>>>> static GuestExecInfo *guest_exec_spawn(const char *cmdline, bool interactive,
>>>>> Error **errp)
>>>>> {
>>>>> GSpawnFlags default_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD;
>>>>> gboolean ret;
>>>>> GPid gpid;
>>>>> gchar **argv;
>>>>> gint argc;
>>>>> GError *gerr = NULL;
>>>>> gint fd_in = -1, fd_out = -1, fd_err = -1;
>>>>>
>>>>> ret = g_shell_parse_argv(cmdline, &argc, &argv, &gerr);
>>>>> if (!ret || gerr) {
>>>>> error_setg(errp, "failed to parse command: %s, %s", cmdline,
>>>>> gerr->message);
>>>>> return NULL;
>>>>> }
>>>>>
>>>>> ret = g_spawn_async_with_pipes(NULL, argv, NULL,
>>>>> default_flags, NULL, NULL, &gpid,
>>>>> interactive ? &fd_in : NULL, &fd_out, &fd_err, &gerr);
>>>>> if (gerr) {
>>>>> error_setg(errp, "failed to execute command: %s, %s", cmdline,
>>>>> gerr->message);
>>>>> return NULL;
>>>>> }
>>>>> if (!ret) {
>>>>> error_setg(errp, "failed to execute command");
>>>>> return NULL;
>>>>> }
>>>>>
>>>>> return guest_exec_info_new(gpid, cmdline, fd_in, fd_out, fd_err);
>>>>> }
>>>>>
>>>>> GuestExecResponse *qmp_guest_exec_async(const char *cmdline,
>>>>> bool has_interactive,
>>>>> bool interactive,
>>>>> Error **errp)
>>>>> {
>>>>> GuestExecResponse *ger;
>>>>> GuestExecInfo *gei;
>>>>> int32_t handle;
>>>>>
>>>>> gei = guest_exec_spawn(cmdline, has_interactive && interactive, errp);
>>>>> if (error_is_set(errp)) {
>>>>> return NULL;
>>>>> }
>>>>>
>>>>> print_gei(gei);
>>>>> ger = g_new0(GuestExecResponse, 1);
>>>>>
>>>>> if (has_interactive && interactive) {
>>>>> ger->has_handle_stdin = true;
>>>>> ger->handle_stdin =
>>>>> guest_file_handle_add_fd(gei->fd_in, "a", errp);
>>>>> if (error_is_set(errp)) {
>>>>> return NULL;
>>>>> }
>>>>> }
>>>>>
>>>>> ger->has_handle_stdout = true;
>>>>> ger->handle_stdout =
>>>>> guest_file_handle_add_fd(gei->fd_out, "r", errp);
>>>>> if (error_is_set(errp)) {
>>>>> return NULL;
>>>>> }
>>>>>
>>>>> ger->has_handle_stderr = true;
>>>>> ger->handle_stderr =
>>>>> guest_file_handle_add_fd(gei->fd_err, "r", errp);
>>>>> if (error_is_set(errp)) {
>>>>> return NULL;
>>>>> }
>>>>>
>>>>> handle = guest_exec_info_register(gei);
>>>>> ger->status = qmp_guest_exec_status(handle, false, false, false, 0, errp);
>>>>> if (error_is_set(errp)) {
>>>>> return NULL;
>>>>> }
>>>>>
>>>>> return ger;
>>>>> }
>>>>>
>>>>> Where GuestExecResponse takes the place of the original PID return, since
>>>>> we now need to fetch the stdin/stdout/stderr handles as well. In my code
>>>>> it was defined as:
>>>>>
>>>>> { 'type': 'GuestExecResponse',
>>>>> 'data': { 'status': 'GuestExecStatus',
>>>>> '*handle-stdin': 'int', '*handle-stdout': 'int',
>>>>> '*handle-stderr': 'int' } }
>>>>>
>>>>> Sorry for not just posting the patchset somewhere, the code was initially lost
>>>>> in an accidental wipe of /home so I currently only have vim backup files to
>>>>> piece it together from atm.
>>>> Frankly speaking this exact interface is not that
>>>> convenient for a real life products. The necessity
>>>> to poll exec status and to poll input/output
>>>> pipes is really boring and really affects the
>>>> performance.
>>>>
>>>> Actually there are 2 major scenarios for this:
>>>> 1. "Enter inside VM", i.e. the user obtains shell
>>>> inside VM and works in VM from host f.e. to setup
>>>> network
>>>> 2. Execute command inside VM and collect output
>>>> in host.
>>> Ultimately I ended up with 2 interfaces, guest-exec-async,
>>> which is implemented something like above, and guest-exec,
>>> which simplifies the common case by executing synchronously
>>> and simply allowing a timeout to be specified as an
>>> argument. base64-encoded stdout/stderr buffer is subsequently
>>> returned, and limited in size to 1M (user can redirect to
>>> file in guest as an alternative). I think this handles
>>> the vast amount of use-cases for 2), but for processes
>>> that generate lots of output writing to a filesystem can
>>> be problematic. And for that use case I don't think
>>> polling is particularly an issue, performance wise.
>>> so I have a hard time rationalizing away the need for a
>>> guest-exec-async at some point, even if we don't support
>>> leveraging it for interactive shells.
>>>
>>> guest->host events would be an interesting way to optimize
>>> it though, but I'm okay with making polling necessary to
>>> read from or reap a process, and adding that as a cue to
>>> make things more efficient later while maintaining
>>> compatibility with existing users/interfaces.
>>>
>>> The pipes are indeed tedious, which is why the added setup
>>> of using guest-pipe-open was dropped in my implementation.
>>> You still have to deal with reading/writing/closed to FDs
>>> returned by guest-exec-async, but that's the core use-case
>>> for that sort of interface I think.
>>>
>>>> For both case the proposed interface with guest
>>>> pipes is not convenient, in order to obtain interactive
>>>> shell in guest we should poll frequently (100 times
>>>> in seconds to avoid lag) and in my experience
>>>> end-users likes to run a lot of automation scripts
>>>> using this channel.
>>>>
>>>> Thus we have used virtserial as a real transport for
>>>> case 1) and it is working seamlessly. This means
>>>> that we open virtserial in guest for input and output
>>>> and connect to Unix socket in host with shell application.
>>>> Poll of exec-status is necessary evil, but it does not affect
>>>> interactivity and could be done once in a second.
>>>>
>>>> I would be good to have some notifications from
>>>> guest that some events are pending (input/output
>>>> data or exec status is available). But I do not know
>>>> at the moment how to implement this. At least I have
>>>> not invested resources into this as I would like
>>>> to hear some opinion from the community first.
>>>> This patchset is made mostly to initiate the discussion.
>>>>
>>>> Michael, could you spend a bit of time looking
>>>> into patches 1 and 2 of the patchset. They have been
>>>> implemented and reviewed by us and could be
>>>> merged separately in advance. They provides
>>>> functional equivalent of already existing Linux (Posix)
>>>> functionality.
>>> Absolutely!
>>>
>>>> As for your approach, I would tend to agree with
>>>> Eric. It is not safe.
>>> I hadn't fully considered the matter of safety. I know shell
>>> operators are santized/unsupported by the interface, but I do
>>> suppose even basic command-line parsing can still be ambiguous
>>> from one program to the next so perhaps we should avoid it on
>>> that basis at the least.
>>>
>>>> Regards,
>>>> Den
>> OK, Michael, I am a little bit confused at the moment.
>> What will be the next step and who is waiting whom?
>>
>> I think that the major architectural argument from
>> your side is pointed out by Eric and should not be taken
>> into account.
> Sorry for the delay. The main outstanding issue I have from
> an architectural standpoint is whether we should consider
> using g_spawn_sync/g_spawn_async to avoid the need to
> maintain OS-specific variants of the exec support. I realize
> that's not a good argument in the face of real/working code,
> and I've been working on getting my g_spawn*()-based
> implementation cleaned up so we can make a decision based
> on that.
>
> If you're still targetting 2.3 (soft-freeze is Feb 16th)
> for guest-exec let me know and I can try to hurry things
> along on my end, but for now I'm assuming the guest-exec
> functionality is something we can pursue for 2.4.
actually we are not bound 2.3 or 2.4 and there is enough
time to make code consistent. But I think that this functionality
is important because it should reach distros and this takes
additional time.
IMHO it would be better to merge something working and
after that reshape it in next release with a better implementation.
> I have been testing the w32 guest-file-* patches and they
> seem to be in good shape for 2.3, so please repost those
> as a separate patchset and I'll work to get them into the
> next QGA pull.
ok
>> The matter regarding "sync & async" exec types is
>> still questionable and could be omitted at the moment.
>> We will be able to extend the interface later. Guest
>> notifications is a more interesting thing but it is
>> much more difficult to implement.
>>
>> Therefore I think that the ball is on your side and
>> we are waiting for a review from your side.
>>
>> Regards,
>> Den
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux
2014-12-31 13:06 [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Denis V. Lunev
` (8 preceding siblings ...)
2015-01-09 17:06 ` [Qemu-devel] [PATCH v2 0/8] qemu: guest agent: implement guest-exec command for Linux Michael Roth
@ 2015-01-27 13:52 ` Denis V. Lunev
9 siblings, 0 replies; 24+ messages in thread
From: Denis V. Lunev @ 2015-01-27 13:52 UTC (permalink / raw)
Cc: Olga Krishtal, qemu-devel, Semen Zolin
On 31/12/14 16:06, Denis V. Lunev wrote:
> hese patches for guest-agent add the functionality to execute commands on
> a guest UNIX machine.
>
> 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 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: Semen Zolin <szolin@parallels.com>
> Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
>
ping
^ permalink raw reply [flat|nested] 24+ messages in thread