qemu-ga patch queue

---
 Makefile  | 14 ++++++++------
 configure |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

Makefile +8 -6

index e370876..cbf252d 100644
@@ -298,18 +298,15 @@ $(qapi-modules) $(SRC_PATH)/scripts/qapi-introspect.py $(qapi-py)
 QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
 $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN)
 
-# we require QGA_VSS_PROVIDER files to be built alongside qemu-ga
-# executable since they are shipped together, but we don't want to actually
-# link against them
-qemu-ga$(EXESUF): $(qga-obj-y) libqemuutil.a libqemustub.a $(QGA_VSS_PROVIDER)
-	$(call LINK, $(filter-out $(QGA_VSS_PROVIDER), $^))
+qemu-ga$(EXESUF): $(qga-obj-y) libqemuutil.a libqemustub.a
+	$(call LINK, $^)
 
 ifdef QEMU_GA_MSI_ENABLED
 QEMU_GA_MSI=qemu-ga-$(ARCH).msi
 
 msi: $(QEMU_GA_MSI)
 
-$(QEMU_GA_MSI): qemu-ga.exe
+$(QEMU_GA_MSI): qemu-ga.exe $(QGA_VSS_PROVIDER)
 
 $(QEMU_GA_MSI): config-host.mak

@@ -321,6 +318,11 @@ msi:
 	@echo "MSI build not configured or dependency resolution failed (reconfigure with --enable-guest-agent-msi option)"
 endif
 
+ifneq ($(EXESUF),)
+.PHONY: qemu-ga
+qemu-ga: qemu-ga$(EXESUF) $(QGA_VSS_PROVIDER) $(QEMU_GA_MSI)
+endif
+
 clean:
 # avoid old build problems by removing potentially incorrect old files
 	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h

configure +1 -1

index f08327e..cc62c38 100755
@@ -4441,7 +4441,7 @@ fi
 
 if [ "$guest_agent" != "no" ]; then
   if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
-      tools="qemu-ga\$(EXESUF) $tools"
+      tools="qemu-ga $tools"
       guest_agent=yes
   elif [ "$guest_agent" != yes ]; then
       guest_agent=no

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #2 lore raw

PATCH 2/12 qga: Use g_new() & friends where that makes obvious sense

From: Markus Armbruster <[email protected]>

g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors.

This commit only touches allocations with size arguments of the form sizeof(T). Same Coccinelle semantic patch as in commit b45c03f.

Signed-off-by Markus Armbruster <[email protected]>

Reviewed-by Eric Blake <[email protected]>

---
 qga/channel-posix.c             |  2 +-
 qga/channel-win32.c             |  2 +-
 qga/commands-posix.c            | 10 +++++-----
 qga/commands-win32.c            | 10 +++++-----
 qga/commands.c                  |  6 +++---
 qga/guest-agent-command-state.c |  4 ++--
 6 files changed, 17 insertions(+), 17 deletions(-)

qga/channel-posix.c +1 -1

index 8aad4fe..61aa3cb 100644
@@ -249,7 +249,7 @@ GIOStatus ga_channel_read(GAChannel *c, gchar *buf, gsize size, gsize *count)
 GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
                           GAChannelCallback cb, gpointer opaque)
 {
-    GAChannel *c = g_malloc0(sizeof(GAChannel));
+    GAChannel *c = g_new0(GAChannel, 1);
     c->event_cb = cb;
     c->user_data = opaque;

qga/channel-win32.c +1 -1

index 04fa5e4..215b15a 100644
@@ -322,7 +322,7 @@ static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
 GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
                           GAChannelCallback cb, gpointer opaque)
 {
-    GAChannel *c = g_malloc0(sizeof(GAChannel));
+    GAChannel *c = g_new0(GAChannel, 1);
     SECURITY_ATTRIBUTES sec_attrs;
 
     if (!ga_channel_open(c, method, path)) {

qga/commands-posix.c +5 -5

index b03c316..a932809 100644
@@ -235,7 +235,7 @@ static int64_t guest_file_handle_add(FILE *fh, Error **errp)
         return -1;
     }
 
-    gfh = g_malloc0(sizeof(GuestFileHandle));
+    gfh = g_new0(GuestFileHandle, 1);
     gfh->id = handle;
     gfh->fh = fh;
     QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);

@@ -488,7 +488,7 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
         slog("guest-file-read failed, handle: %" PRId64, handle);
     } else {
         buf[read_count] = 0;
-        read_data = g_malloc0(sizeof(GuestFileRead));
+        read_data = g_new0(GuestFileRead, 1);
         read_data->count = read_count;
         read_data->eof = feof(fh);
         if (read_count) {

@@ -533,7 +533,7 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
         error_setg_errno(errp, errno, "failed to write to file");
         slog("guest-file-write failed, handle: %" PRId64, handle);
     } else {
-        write_data = g_malloc0(sizeof(GuestFileWrite));
+        write_data = g_new0(GuestFileWrite, 1);
         write_data->count = write_count;
         write_data->eof = feof(fh);
     }

@@ -678,7 +678,7 @@ static void build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
             continue;
         }
 
-        mount = g_malloc0(sizeof(FsMount));
+        mount = g_new0(FsMount, 1);
         mount->dirname = g_strdup(ment->mnt_dir);
         mount->devtype = g_strdup(ment->mnt_type);
         mount->devmajor = devmajor;

@@ -757,7 +757,7 @@ static void build_fs_mount_list(FsMountList *mounts, Error **errp)
             }
         }
 
-        mount = g_malloc0(sizeof(FsMount));
+        mount = g_new0(FsMount, 1);
         mount->dirname = g_strdup(line + dir_s);
         mount->devtype = g_strdup(dash + type_s);
         mount->devmajor = devmajor;

qga/commands-win32.c +5 -5

index 41bdd3f..0481646 100644
@@ -106,7 +106,7 @@ static int64_t guest_file_handle_add(HANDLE fh, Error **errp)
     if (handle < 0) {
         return -1;
     }
-    gfh = g_malloc0(sizeof(GuestFileHandle));
+    gfh = g_new0(GuestFileHandle, 1);
     gfh->id = handle;
     gfh->fh = fh;
     QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);

@@ -298,7 +298,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
         slog("guest-file-read failed, handle %" PRId64, handle);
     } else {
         buf[read_count] = 0;
-        read_data = g_malloc0(sizeof(GuestFileRead));
+        read_data = g_new0(GuestFileRead, 1);
         read_data->count = (size_t)read_count;
         read_data->eof = read_count == 0;

@@ -342,7 +342,7 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
         error_setg_win32(errp, GetLastError(), "failed to write to file");
         slog("guest-file-write-failed, handle: %" PRId64, handle);
     } else {
-        write_data = g_malloc0(sizeof(GuestFileWrite));
+        write_data = g_new0(GuestFileWrite, 1);
         write_data->count = (size_t) write_count;
     }

@@ -865,7 +865,7 @@ static DWORD WINAPI do_suspend(LPVOID opaque)
 void qmp_guest_suspend_disk(Error **errp)
 {
     Error *local_err = NULL;
-    GuestSuspendMode *mode = g_malloc(sizeof(GuestSuspendMode));
+    GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
 
     *mode = GUEST_SUSPEND_MODE_DISK;
     check_suspend_mode(*mode, &local_err);

@@ -881,7 +881,7 @@ void qmp_guest_suspend_disk(Error **errp)
 void qmp_guest_suspend_ram(Error **errp)
 {
     Error *local_err = NULL;
-    GuestSuspendMode *mode = g_malloc(sizeof(GuestSuspendMode));
+    GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
 
     *mode = GUEST_SUSPEND_MODE_RAM;
     check_suspend_mode(*mode, &local_err);

qga/commands.c +3 -3

index 7834967..9a765e8 100644
@@ -51,12 +51,12 @@ static void qmp_command_info(QmpCommand *cmd, void *opaque)
     GuestAgentCommandInfo *cmd_info;
     GuestAgentCommandInfoList *cmd_info_list;
 
-    cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
+    cmd_info = g_new0(GuestAgentCommandInfo, 1);
     cmd_info->name = g_strdup(qmp_command_name(cmd));
     cmd_info->enabled = qmp_command_is_enabled(cmd);
     cmd_info->success_response = qmp_has_success_response(cmd);
 
-    cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList));
+    cmd_info_list = g_new0(GuestAgentCommandInfoList, 1);
     cmd_info_list->value = cmd_info;
     cmd_info_list->next = info->supported_commands;
     info->supported_commands = cmd_info_list;

@@ -64,7 +64,7 @@ static void qmp_command_info(QmpCommand *cmd, void *opaque)
 
 struct GuestAgentInfo *qmp_guest_info(Error **errp)
 {
-    GuestAgentInfo *info = g_malloc0(sizeof(GuestAgentInfo));
+    GuestAgentInfo *info = g_new0(GuestAgentInfo, 1);
 
     info->version = g_strdup(QEMU_VERSION);
     qmp_for_each_command(qmp_command_info, info);

qga/guest-agent-command-state.c +2 -2

index 969da23..128c549 100644
@@ -27,7 +27,7 @@ void ga_command_state_add(GACommandState *cs,
                           void (*init)(void),
                           void (*cleanup)(void))
 {
-    GACommandGroup *cg = g_malloc0(sizeof(GACommandGroup));
+    GACommandGroup *cg = g_new0(GACommandGroup, 1);
     cg->init = init;
     cg->cleanup = cleanup;
     cs->groups = g_slist_append(cs->groups, cg);

@@ -67,7 +67,7 @@ void ga_command_state_cleanup_all(GACommandState *cs)
 
 GACommandState *ga_command_state_new(void)
 {
-    GACommandState *cs = g_malloc0(sizeof(GACommandState));
+    GACommandState *cs = g_new0(GACommandState, 1);
     cs->groups = NULL;
     return cs;
 }

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #3 lore raw

PATCH 3/12 qga: add QGA_CONF environment variable

From: Marc-André Lureau <[email protected]>

Having a environment variable allows to override default configuration path, useful for testing. Note that this can't easily be an argument, since loading config is done before parsing the arguments.

Signed-off-by Marc-André Lureau <[email protected]>

---
 qga/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

qga/main.c +2 -1

index d8e063a..18e1e1d 100644
@@ -945,10 +945,11 @@ static void config_load(GAConfig *config)
 {
     GError *gerr = NULL;
     GKeyFile *keyfile;
+    const char *conf = g_getenv("QGA_CONF") ?: QGA_CONF_DEFAULT;
 
     /* read system config */
     keyfile = g_key_file_new();
-    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
+    if (!g_key_file_load_from_file(keyfile, conf, 0, &gerr)) {
         goto end;
     }
     if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #4 lore raw

PATCH 4/12 qga: do not override configuration verbosity

From: Marc-André Lureau <[email protected]>

Move the default verbosity settings before loading the configuration file, or it will overwrite it. Found thanks to writing qga tests :)

Signed-off-by Marc-André Lureau <[email protected]>

---
 qga/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

qga/main.c +2 -2

index 18e1e1d..aa6a063 100644
@@ -1083,8 +1083,6 @@ static void config_parse(GAConfig *config, int argc, char **argv)
         { NULL, 0, NULL, 0 }
     };
 
-    config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
-
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
         switch (ch) {
         case 'm':

@@ -1332,6 +1330,8 @@ int main(int argc, char **argv)
     GAState *s = g_new0(GAState, 1);
     GAConfig *config = g_new0(GAConfig, 1);
 
+    config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
+
     module_call_init(MODULE_INIT_QAPI);
 
     init_dfl_pathnames();

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #5 lore raw

PATCH 5/12 qtest: add a few fd-level qmp helpers

From: Marc-André Lureau <[email protected]>

Add a few functions to interact with qmp via a simple fd.

Signed-off-by Marc-André Lureau <[email protected]>

---
 tests/libqtest.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 tests/libqtest.h |  7 +++++++
 2 files changed, 48 insertions(+), 4 deletions(-)

tests/libqtest.c +41 -4

index 2a396ba..b6d700c 100644
@@ -356,7 +356,7 @@ static void qmp_response(JSONMessageParser *parser, QList *tokens)
     qmp->response = (QDict *)obj;
 }
 
-QDict *qtest_qmp_receive(QTestState *s)
+QDict *qmp_fd_receive(int fd)
 {
     QMPResponseParser qmp;
     bool log = getenv("QTEST_LOG") != NULL;

@@ -367,7 +367,7 @@ QDict *qtest_qmp_receive(QTestState *s)
         ssize_t len;
         char c;
 
-        len = read(s->qmp_fd, &c, 1);
+        len = read(fd, &c, 1);
         if (len == -1 && errno == EINTR) {
             continue;
         }

@@ -387,12 +387,17 @@ QDict *qtest_qmp_receive(QTestState *s)
     return qmp.response;
 }
 
+QDict *qtest_qmp_receive(QTestState *s)
+{
+    return qmp_fd_receive(s->qmp_fd);
+}
+
 /**
  * Allow users to send a message without waiting for the reply,
  * in the case that they choose to discard all replies up until
  * a particular EVENT is received.
  */
-void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap)
+void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
 {
     va_list ap_copy;
     QObject *qobj;

@@ -416,13 +421,25 @@ void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap)
             fprintf(stderr, "%s", str);
         }
         /* Send QMP request */
-        socket_send(s->qmp_fd, str, size);
+        socket_send(fd, str, size);
 
         QDECREF(qstr);
         qobject_decref(qobj);
     }
 }
 
+void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap)
+{
+    qmp_fd_sendv(s->qmp_fd, fmt, ap);
+}
+
+QDict *qmp_fdv(int fd, const char *fmt, va_list ap)
+{
+    qmp_fd_sendv(fd, fmt, ap);
+
+    return qmp_fd_receive(fd);
+}
+
 QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
 {
     qtest_async_qmpv(s, fmt, ap);

@@ -431,6 +448,26 @@ QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
     return qtest_qmp_receive(s);
 }
 
+QDict *qmp_fd(int fd, const char *fmt, ...)
+{
+    va_list ap;
+    QDict *response;
+
+    va_start(ap, fmt);
+    response = qmp_fdv(fd, fmt, ap);
+    va_end(ap);
+    return response;
+}
+
+void qmp_fd_send(int fd, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    qmp_fd_sendv(fd, fmt, ap);
+    va_end(ap);
+}
+
 QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
 {
     va_list ap;

tests/libqtest.h +7 -0

index 55bccbf..9818ef7 100644
@@ -851,4 +851,11 @@ static inline int64_t clock_set(int64_t val)
  */
 bool qtest_big_endian(void);
 
+
+QDict *qmp_fd_receive(int fd);
+void qmp_fd_sendv(int fd, const char *fmt, va_list ap);
+void qmp_fd_send(int fd, const char *fmt, ...);
+QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
+QDict *qmp_fd(int fd, const char *fmt, ...);
+
 #endif

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #6 lore raw

PATCH 6/12 glib-compat: add 2.38/2.40/2.46 asserts

From: Marc-André Lureau <[email protected]>

Those are mostly useful for writing tests.

Signed-off-by Marc-André Lureau <[email protected]>

Reviewed-by Claudio Fontana <[email protected]>

---
 include/glib-compat.h | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

include/glib-compat.h +61 -0

index 318e000..fb25f43 100644
@@ -165,4 +165,65 @@ static inline GThread *g_thread_new(const char *name,
 #define CompatGCond GCond
 #endif /* glib 2.31 */
 
+#ifndef g_assert_true
+#define g_assert_true(expr)                                                    \
+    do {                                                                       \
+        if (G_LIKELY(expr)) {                                                  \
+        } else {                                                               \
+            g_assertion_message(G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC,   \
+                                "'" #expr "' should be TRUE");                 \
+        }                                                                      \
+    } while (0)
+#endif
+
+#ifndef g_assert_false
+#define g_assert_false(expr)                                                   \
+    do {                                                                       \
+        if (G_LIKELY(!(expr))) {                                               \
+        } else {                                                               \
+            g_assertion_message(G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC,   \
+                                "'" #expr "' should be FALSE");                \
+        }                                                                      \
+    } while (0)
+#endif
+
+#ifndef g_assert_null
+#define g_assert_null(expr)                                                    \
+    do {                                                                       \
+        if (G_LIKELY((expr) == NULL)) {                                        \
+        } else {                                                               \
+            g_assertion_message(G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC,   \
+                                "'" #expr "' should be NULL");                 \
+        }                                                                      \
+    } while (0)
+#endif
+
+#ifndef g_assert_nonnull
+#define g_assert_nonnull(expr)                                                 \
+    do {                                                                       \
+        if (G_LIKELY((expr) != NULL)) {                                        \
+        } else {                                                               \
+            g_assertion_message(G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC,   \
+                                "'" #expr "' should not be NULL");             \
+        }                                                                      \
+    } while (0)
+#endif
+
+#ifndef g_assert_cmpmem
+#define g_assert_cmpmem(m1, l1, m2, l2)                                        \
+    do {                                                                       \
+        gconstpointer __m1 = m1, __m2 = m2;                                    \
+        int __l1 = l1, __l2 = l2;                                              \
+        if (__l1 != __l2) {                                                    \
+            g_assertion_message_cmpnum(                                        \
+                G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC,                   \
+                #l1 " (len(" #m1 ")) == " #l2 " (len(" #m2 "))", __l1, "==",   \
+                __l2, 'i');                                                    \
+        } else if (memcmp(__m1, __m2, __l1) != 0) {                            \
+            g_assertion_message(G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC,   \
+                                "assertion failed (" #m1 " == " #m2 ")");      \
+        }                                                                      \
+    } while (0)
+#endif
+
 #endif

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #7 lore raw

PATCH 7/12 tests: add a local test for guest agent

From: Marc-André Lureau <[email protected]>

Add some local guest agent tests, as it is better than nothing, only when CONFIG_POSIX (using unix sockets).

With the QGA_TEST_SIDE_EFFECTING environment variable, it will include tests with side effects, such as freezing/thawing the FS or changing the time.

(a better test would involve a managed VM (or container), but it might be better to leave that off to autotest/avocado)

Signed-off-by Marc-André Lureau <[email protected]>

* use mkdtemp() in placeof g_mkdtemp() for glib 2.22 compat
* drop redundant/conflicting compat defines for g_assert_{true,false}, since glib-compat has them now.

---
 tests/Makefile   |   3 +
 tests/test-qga.c | 774 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 777 insertions(+)
 create mode 100644 tests/test-qga.c

tests/Makefile +3 -0

index 209eca9..bf218c9 100644
@@ -80,6 +80,7 @@ check-unit-$(CONFIG_GNUTLS_HASH) += tests/test-crypto-hash$(EXESUF)
 check-unit-y += tests/test-crypto-cipher$(EXESUF)
 check-unit-$(CONFIG_GNUTLS) += tests/test-crypto-tlscredsx509$(EXESUF)
 check-unit-$(CONFIG_GNUTLS) += tests/test-crypto-tlssession$(EXESUF)
+check-unit-$(CONFIG_POSIX) += tests/test-qga$(EXESUF)
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh

@@ -539,6 +540,8 @@ endif
 qtest-obj-y = tests/libqtest.o $(test-util-obj-y)
 $(check-qtest-y): $(qtest-obj-y)
 
+tests/test-qga: tests/test-qga.o $(qtest-obj-y)
+
 .PHONY: check-help
 check-help:
 	@echo "Regression testing targets:"

tests/test-qga.c new file +774 -0

774 lines not on this page · raw message

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #8 lore raw

PATCH 8/12 qga: drop guest_file_init helper and replace it with static initializers

From: "Denis V. Lunev" <[email protected]>

This just makes code shorter and better.

Signed-off-by Yuri Pudgorodskiy <[email protected]>

---
 qga/commands-posix.c | 10 +++-------
 qga/commands-win32.c | 10 +++-------
 2 files changed, 6 insertions(+), 14 deletions(-)

qga/commands-posix.c +3 -7

index a932809..03b0f27 100644
@@ -223,7 +223,9 @@ typedef struct GuestFileHandle {
 
 static struct {
     QTAILQ_HEAD(, GuestFileHandle) filehandles;
-} guest_file_state;
+} guest_file_state = {
+    .filehandles = QTAILQ_HEAD_INITIALIZER(guest_file_state.filehandles),
+};
 
 static int64_t guest_file_handle_add(FILE *fh, Error **errp)
 {

@@ -586,11 +588,6 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
     }
 }
 
-static void guest_file_init(void)
-{
-    QTAILQ_INIT(&guest_file_state.filehandles);
-}
-
 /* linux-specific implementations. avoid this if at all possible. */
 #if defined(__linux__)

@@ -2486,5 +2483,4 @@ void ga_command_state_init(GAState *s, GACommandState *cs)
 #if defined(CONFIG_FSFREEZE)
     ga_command_state_add(cs, NULL, guest_fsfreeze_cleanup);
 #endif
-    ga_command_state_add(cs, guest_file_init, NULL);
 }

qga/commands-win32.c +3 -7

index 0481646..d9de23b 100644
@@ -55,7 +55,9 @@ typedef struct GuestFileHandle {
 
 static struct {
     QTAILQ_HEAD(, GuestFileHandle) filehandles;
-} guest_file_state;
+} guest_file_state = {
+    .filehandles = QTAILQ_HEAD_INITIALIZER(guest_file_state.filehandles),
+};
 
 
 typedef struct OpenFlags {

@@ -390,11 +392,6 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
     }
 }
 
-static void guest_file_init(void)
-{
-    QTAILQ_INIT(&guest_file_state.filehandles);
-}
-
 #ifdef CONFIG_QGA_NTDDSCSI
 
 static STORAGE_BUS_TYPE win2qemu[] = {

@@ -1330,5 +1327,4 @@ 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);
 }

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #9 lore raw

PATCH 9/12 qga: guest exec functionality

From: Yuri Pudgorodskiy <[email protected]>

Guest-exec rewriten in platform-independant style with glib spawn.

Child process is spawn asynchroneously and exit status can later be picked up by guest-exec-status command.

stdin/stdout/stderr of the child now is redirected to /dev/null Later we will add ability to specify stdin in guest-exec command and to get collected stdout/stderr with guest-exec-status.

Signed-off-by Yuri Pudgorodskiy <[email protected]>

CC Michael Roth <[email protected]>

*use g_new0 in place of g_malloc for GuestExec struct

---
 qga/commands.c       | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json |  62 +++++++++++++++++
 2 files changed, 250 insertions(+)

qga/commands.c +188 -0

index 9a765e8..32646dc 100644
@@ -70,3 +70,191 @@ struct GuestAgentInfo *qmp_guest_info(Error **errp)
     qmp_for_each_command(qmp_command_info, info);
     return info;
 }
+
+struct GuestExecInfo {
+    GPid pid;
+    gint status;
+    bool finished;
+    QTAILQ_ENTRY(GuestExecInfo) next;
+};
+typedef struct GuestExecInfo GuestExecInfo;
+
+static struct {
+    QTAILQ_HEAD(, GuestExecInfo) processes;
+} guest_exec_state = {
+    .processes = QTAILQ_HEAD_INITIALIZER(guest_exec_state.processes),
+};
+
+static GuestExecInfo *guest_exec_info_add(GPid pid)
+{
+    GuestExecInfo *gei;
+
+    gei = g_new0(GuestExecInfo, 1);
+    gei->pid = pid;
+    QTAILQ_INSERT_TAIL(&guest_exec_state.processes, gei, next);
+
+    return gei;
+}
+
+static GuestExecInfo *guest_exec_info_find(GPid pid)
+{
+    GuestExecInfo *gei;
+
+    QTAILQ_FOREACH(gei, &guest_exec_state.processes, next) {
+        if (gei->pid == pid) {
+            return gei;
+        }
+    }
+
+    return NULL;
+}
+
+GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
+{
+    GuestExecInfo *gei;
+    GuestExecStatus *ges;
+
+    slog("guest-exec-status called, pid: %u", (uint32_t)pid);
+
+    gei = guest_exec_info_find((GPid)pid);
+    if (gei == NULL) {
+        error_setg(err, QERR_INVALID_PARAMETER, "pid");
+        return NULL;
+    }
+
+    ges = g_new0(GuestExecStatus, 1);
+    ges->exited = gei->finished;
+
+    if (gei->finished) {
+        /* Glib has no portable way to parse exit status.
+         * On UNIX, we can get either exit code from normal termination
+         * or signal number.
+         * On Windows, it is either the same exit code or the exception
+         * value for an unhandled exception that caused the process
+         * to terminate.
+         * See MSDN for GetExitCodeProcess() and ntstatus.h for possible
+         * well-known codes, e.g. C0000005 ACCESS_DENIED - analog of SIGSEGV
+         * References:
+         *   https://msdn.microsoft.com/en-us/library/windows/desktop/ms683189(v=vs.85).aspx
+         *   https://msdn.microsoft.com/en-us/library/aa260331(v=vs.60).aspx
+         */
+#ifdef G_OS_WIN32
+        /* Additionally WIN32 does not provide any additional information
+         * on whetherthe child exited or terminated via signal.
+         * We use this simple range check to distingish application exit code
+         * (usually value less then 256) and unhandled exception code with
+         * ntstatus (always value greater then 0xC0000005). */
+        if ((uint32_t)gei->status < 0xC0000000U) {
+            ges->has_exitcode = true;
+            ges->exitcode = gei->status;
+        } else {
+            ges->has_signal = true;
+            ges->signal = gei->status;
+        }
+#else
+        if (WIFEXITED(gei->status)) {
+            ges->has_exitcode = true;
+            ges->exitcode = WEXITSTATUS(gei->status);
+        } else if (WIFSIGNALED(gei->status)) {
+            ges->has_signal = true;
+            ges->signal = WTERMSIG(gei->status);
+        }
+#endif
+        QTAILQ_REMOVE(&guest_exec_state.processes, gei, next);
+        g_free(gei);
+    }
+
+    return ges;
+}
+
+/* Get environment variables or arguments array for execve(). */
+static char **guest_exec_get_args(const strList *entry, bool log)
+{
+    const strList *it;
+    int count = 1, i = 0;  /* reserve for NULL terminator */
+    char **args;
+    char *str; /* for logging array of arguments */
+    size_t str_size = 1;
+
+    for (it = entry; it != NULL; it = it->next) {
+        count++;
+        str_size += 1 + strlen(it->value);
+    }
+
+    str = g_malloc(str_size);
+    *str = 0;
+    args = g_malloc(count * sizeof(char *));
+    for (it = entry; it != NULL; it = it->next) {
+        args[i++] = it->value;
+        pstrcat(str, str_size, it->value);
+        if (it->next) {
+            pstrcat(str, str_size, " ");
+        }
+    }
+    args[i] = NULL;
+
+    if (log) {
+        slog("guest-exec called: \"%s\"", str);
+    }
+    g_free(str);
+
+    return args;
+}
+
+static void guest_exec_child_watch(GPid pid, gint status, gpointer data)
+{
+    GuestExecInfo *gei = (GuestExecInfo *)data;
+
+    g_debug("guest_exec_child_watch called, pid: %u, status: %u",
+            (uint32_t)pid, (uint32_t)status);
+
+    gei->status = status;
+    gei->finished = true;
+
+    g_spawn_close_pid(pid);
+}
+
+GuestExec *qmp_guest_exec(const char *path,
+                       bool has_arg, strList *arg,
+                       bool has_env, strList *env,
+                       bool has_inp_data, const char *inp_data,
+                       bool has_capture_output, bool capture_output,
+                       Error **err)
+{
+    GPid pid;
+    GuestExec *ge = NULL;
+    GuestExecInfo *gei;
+    char **argv, **envp;
+    strList arglist;
+    gboolean ret;
+    GError *gerr = NULL;
+
+    arglist.value = (char *)path;
+    arglist.next = has_arg ? arg : NULL;
+
+    argv = guest_exec_get_args(&arglist, true);
+    envp = guest_exec_get_args(has_env ? env : NULL, false);
+
+    ret = g_spawn_async_with_pipes(NULL, argv, envp,
+            G_SPAWN_SEARCH_PATH |
+            G_SPAWN_DO_NOT_REAP_CHILD |
+            G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL,
+            NULL, NULL, &pid, NULL, NULL, NULL, &gerr);
+    if (!ret) {
+        error_setg(err, QERR_QGA_COMMAND_FAILED, gerr->message);
+        g_error_free(gerr);
+        goto done;
+    }
+
+    ge = g_new0(GuestExec, 1);
+    ge->pid = (int64_t)pid;
+
+    gei = guest_exec_info_add(pid);
+    g_child_watch_add(pid, guest_exec_child_watch, gei);
+
+done:
+    g_free(argv);
+    g_free(envp);
+
+    return ge;
+}

qga/qapi-schema.json +62 -0

65 lines not on this page · raw message

Signature · 1 line

1.9.1

Eric Blake

2015-10-14 22:39 UTC #10 lore raw

Re: [Qemu-devel] [PATCH 09/12] qga: guest exec functionality

Quoted 3 lines · Michael Roth

From: Yuri Pudgorodskiy <[email protected]>
Guest-exec rewriten in platform-independant style with glib spawn.

s/rewriten/rewritten/
s/independant/independent/

Quoted 2 lines

Child process is spawn asynchroneously and exit status can later

s/asynchroneously/asynchronously/

Quoted 13 lines

be picked up by guest-exec-status command.
stdin/stdout/stderr of the child now is redirected to /dev/null Later we will add ability to specify stdin in guest-exec command and to get collected stdout/stderr with guest-exec-status.
Signed-off-by Yuri Pudgorodskiy <[email protected]>
Signed-off-by Denis V. Lunev <[email protected]>
CC Michael Roth <[email protected]>
*use g_new0 in place of g_malloc for GuestExec struct
Signed-off-by Michael Roth <[email protected]>
---

qga/qapi-schema.json line 983

Quoted 9 lines

qga/qapi-schema.json +9 -0

+##
+# @guest-exec:
+#
+# Execute a command in the guest
+#
+# @path: path or executable name to execute
+# @arg: #optional argument list to pass to executable
+# @env: #optional environment variables to pass to executable
+# @inp-data: #optional data to be passed to process stdin (base64 encoded)

Should this be 'input-data' instead of abbreviating?

qga/qapi-schema.json line 985

Quoted 2 lines

qga/qapi-schema.json +2 -0

+# @capture-output: #optional bool flags to enable capture of
+#                  stdout/stderr of running process

Might be worth mentioning that the default is false.

qga/qapi-schema.json line 994

Quoted 9 lines

qga/qapi-schema.json +9 -0

+#
+# Returns: PID on success.
+#
+# Since: 2.5
+##
+{ 'command': 'guest-exec',
+  'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
+               '*inp-data': 'str', '*capture-output': 'bool' },
+  'returns': 'GuestExec' }

Are there any restrictions on how elements of env must behave, such as requiring an '='? If so, does that mean we need any more structure than a raw 'str', such as {'name':'str', 'value':'str'}? Or is that overkill?

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Michael Roth

author

2015-10-15 03:48 UTC #11 lore raw

Re: [Qemu-devel] [PATCH 09/12] qga: guest exec functionality

Quoting Eric Blake (2015-10-14 17:38:56)

Quoted 43 lines · Michael Roth

Quoted 3 lines
From: Yuri Pudgorodskiy <[email protected]>
Guest-exec rewriten in platform-independant style with glib spawn.
s/rewriten/rewritten/
s/independant/independent/
Quoted 2 lines
Child process is spawn asynchroneously and exit status can later
s/asynchroneously/asynchronously/
Quoted 13 lines
be picked up by guest-exec-status command.
stdin/stdout/stderr of the child now is redirected to /dev/null Later we will add ability to specify stdin in guest-exec command and to get collected stdout/stderr with guest-exec-status.
Signed-off-by Yuri Pudgorodskiy <[email protected]>
Signed-off-by Denis V. Lunev <[email protected]>
CC Michael Roth <[email protected]>
*use g_new0 in place of g_malloc for GuestExec struct
Signed-off-by Michael Roth <[email protected]>
---
qga/qapi-schema.json line 983
Quoted 9 lines
qga/qapi-schema.json +9 -0
+##
+# @guest-exec:
+#
+# Execute a command in the guest
+#
+# @path: path or executable name to execute
+# @arg: #optional argument list to pass to executable
+# @env: #optional environment variables to pass to executable
+# @inp-data: #optional data to be passed to process stdin (base64 encoded)
Should this be 'input-data' instead of abbreviating?
qga/qapi-schema.json line 985
Quoted 2 lines
qga/qapi-schema.json +2 -0
+# @capture-output: #optional bool flags to enable capture of
+#                  stdout/stderr of running process
Might be worth mentioning that the default is false.

Quoted 13 lines

qga/qapi-schema.json line 994
Quoted 9 lines
qga/qapi-schema.json +9 -0
+#
+# Returns: PID on success.
+#
+# Since: 2.5
+##
+{ 'command': 'guest-exec',
+  'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
+               '*inp-data': 'str', '*capture-output': 'bool' },
+  'returns': 'GuestExec' }
Are there any restrictions on how elements of env must behave, such as requiring an '='? If so, does that mean we need any more structure than a raw 'str', such as {'name':'str', 'value':'str'}? Or is that overkill?

I think it's overkill, since glib's g_spawn_*() functions expect it as a string list in the same way as it does for command parameters. As a result we have common parsing code to feed both. Documenting env as something other than this will only result in extra code to turn it back into the same format we have currently.

Have all other suggestions pushed to qga tree and will send a v2 Thursday morning CST:
https://github.com/mdroth/qemu/commits/qga

Quoted 5 lines

Signature · 3 lines

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Michael Roth

author

2015-10-14 20:08 UTC #12 lore raw

PATCH 10/12 qga: handle possible SIGPIPE in guest-file-write

From: "Denis V. Lunev" <[email protected]>

qemu-ga should not exit on guest-file-write to pipe without read end but proper error code should be returned. The behavior of the spawned process should be default thus SIGPIPE processing should be reset to default after fork() but before exec().

Signed-off-by Yuri Pudgorodskiy <[email protected]>

---
 qga/commands.c | 18 +++++++++++++++++-
 qga/main.c     |  6 ++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

qga/commands.c +17 -1

index 32646dc..5794c0d 100644
@@ -214,6 +214,22 @@ static void guest_exec_child_watch(GPid pid, gint status, gpointer data)
     g_spawn_close_pid(pid);
 }
 
+/** Reset ignored signals back to default. */
+static void guest_exec_task_setup(gpointer data)
+{
+#if !defined(G_OS_WIN32)
+    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));
+    }
+#endif
+}
+
 GuestExec *qmp_guest_exec(const char *path,
                        bool has_arg, strList *arg,
                        bool has_env, strList *env,

@@ -239,7 +255,7 @@ GuestExec *qmp_guest_exec(const char *path,
             G_SPAWN_SEARCH_PATH |
             G_SPAWN_DO_NOT_REAP_CHILD |
             G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL,
-            NULL, NULL, &pid, NULL, NULL, NULL, &gerr);
+            guest_exec_task_setup, NULL, &pid, NULL, NULL, NULL, &gerr);
     if (!ret) {
         error_setg(err, QERR_QGA_COMMAND_FAILED, gerr->message);
         g_error_free(gerr);

qga/main.c +6 -0

index aa6a063..068169f 100644
@@ -161,6 +161,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;
 }

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #13 lore raw

PATCH 11/12 qga: handle G_IO_STATUS_AGAIN in ga_channel_write_all()

From: Yuri Pudgorodskiy <[email protected]>

glib may return G_IO_STATUS_AGAIN which is actually not an error. Also fixed a bug when on incomplete write buf pointer was not adjusted.

Signed-off-by Yuri Pudgorodskiy <[email protected]>

---
 qga/channel-posix.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

qga/channel-posix.c +11 ~~-12~~

index 61aa3cb..50d9dd3 100644
@@ -217,25 +217,24 @@ GIOStatus ga_channel_write_all(GAChannel *c, const gchar *buf, gsize size)
     GIOStatus status = G_IO_STATUS_NORMAL;
 
     while (size) {
+        g_debug("sending data, count: %d", (int)size);
         status = g_io_channel_write_chars(c->client_channel, buf, size,
                                           &written, &err);
-        g_debug("sending data, count: %d", (int)size);
-        if (err != NULL) {
+        if (status == G_IO_STATUS_NORMAL) {
+            size -= written;
+            buf += written;
+        } else if (status != G_IO_STATUS_AGAIN) {
             g_warning("error writing to channel: %s", err->message);
-            return G_IO_STATUS_ERROR;
-        }
-        if (status != G_IO_STATUS_NORMAL) {
-            break;
+            return status;
         }
-        size -= written;
     }
 
-    if (status == G_IO_STATUS_NORMAL) {
+    do {
         status = g_io_channel_flush(c->client_channel, &err);
-        if (err != NULL) {
-            g_warning("error flushing channel: %s", err->message);
-            return G_IO_STATUS_ERROR;
-        }
+    } while (status == G_IO_STATUS_AGAIN);
+
+    if (status != G_IO_STATUS_NORMAL) {
+        g_warning("error flushing channel: %s", err->message);
     }
 
     return status;

Signature · 1 line

1.9.1

Michael Roth

author

2015-10-14 20:08 UTC #14 lore raw

PATCH 12/12 qga: guest-exec simple stdin/stdout/stderr redirection

From: Yuri Pudgorodskiy <[email protected]>

Implemented with base64-encoded strings in qga json protocol. Glib portable GIOChannel is used for data I/O.

Optinal stdin parameter of guest-exec command is now used as stdin content for spawned subprocess.

If capture-output bool flag is specified, guest-exec redirects out/err file descriptiors internally to pipes and collects subprocess output.

Guest-exe-status is modified to return this collected data to requestor in base64 encoding.

Signed-off-by Yuri Pudgorodskiy <[email protected]>

* switch from 'struct GuestIOExecData' to 'GuestIOExecData'
* s/TRUE/true/g, s/FALSE/false/g for gboolean return values