[Qemu-devel] [PATCH v2 00/12] qemu-ga: add a configuration file

[Qemu-devel] [PATCH v2 00/12] qemu-ga: add a configuration file

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

The following patches for the qemu agent add support for an optionnal
configuration file, and a man page.

Since v1:
- spelling fixes
- change device_path to channel_path
- moving config to GAConfig struct
- do check_is_frozen() during main
- use g_key_file_to_data() for the dump

This is related to this RFE:
https://bugzilla.redhat.com/show_bug.cgi?id=1101556

Marc-André Lureau (12):
  qga: misc spelling
  qga: use exit() when parsing options
  qga: move string split in separate function
  qga: rename 'path' to 'channel_path'
  qga: copy argument strings
  qga: move option parsing to seperate function
  qga: fill default options in main()
  qga: move agent run in a seperate function
  qga: free a bit more
  qga: add an optionnal qemu-ga.conf system configuration
  qga: add --dump-conf option
  qga: start a man page

 Makefile             |  14 +-
 qemu-doc.texi        |   6 +
 qemu-ga.texi         | 136 +++++++++++++++
 qga/main.c           | 464 ++++++++++++++++++++++++++++++++++++---------------
 qga/qapi-schema.json |   2 +-
 5 files changed, 481 insertions(+), 141 deletions(-)
 create mode 100644 qemu-ga.texi

-- 
2.4.3

[Qemu-devel] [PATCH v2 01/12] qga: misc spelling

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/qapi-schema.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 18e3cc3..6b0bd16 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -793,7 +793,7 @@
 # scheme. Refer to the documentation of the guest operating system
 # in question to determine what is supported.
 #
-# Note all guest operating systems will support use of the
+# Not all guest operating systems will support use of the
 # @crypted flag, as they may require the clear-text password
 #
 # The @password parameter must always be base64 encoded before
-- 
2.4.3

[Qemu-devel] [PATCH v2 02/12] qga: use exit() when parsing options

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The option parsing is going to be moved to a separate function,
use exit() consistently.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qga/main.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 791982e..10bb2f7 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -992,14 +992,14 @@ int main(int argc, char **argv)
             break;
         case 'V':
             printf("QEMU Guest Agent %s\n", QEMU_VERSION);
-            return 0;
+            exit(EXIT_SUCCESS);
         case 'd':
             daemonize = 1;
             break;
         case 'b': {
             if (is_help_option(optarg)) {
                 qmp_for_each_command(ga_print_cmd, NULL);
-                return 0;
+                exit(EXIT_SUCCESS);
             }
             for (j = 0, i = 0, len = strlen(optarg); i < len; i++) {
                 if (optarg[i] == ',') {
@@ -1027,36 +1027,36 @@ int main(int argc, char **argv)
                                   NULL :
                                   state_dir;
                 if (ga_install_vss_provider()) {
-                    return EXIT_FAILURE;
+                    exit(EXIT_FAILURE);
                 }
                 if (ga_install_service(path, log_filepath, fixed_state_dir)) {
-                    return EXIT_FAILURE;
+                    exit(EXIT_FAILURE);
                 }
-                return 0;
+                exit(EXIT_SUCCESS);
             } else if (strcmp(service, "uninstall") == 0) {
                 ga_uninstall_vss_provider();
-                return ga_uninstall_service();
+                exit(ga_uninstall_service());
             } else if (strcmp(service, "vss-install") == 0) {
                 if (ga_install_vss_provider()) {
-                    return EXIT_FAILURE;
+                    exit(EXIT_FAILURE);
                 }
-                return EXIT_SUCCESS;
+                exit(EXIT_SUCCESS);
             } else if (strcmp(service, "vss-uninstall") == 0) {
                 ga_uninstall_vss_provider();
-                return EXIT_SUCCESS;
+                exit(EXIT_SUCCESS);
             } else {
                 printf("Unknown service command.\n");
-                return EXIT_FAILURE;
+                exit(EXIT_FAILURE);
             }
             break;
 #endif
         case 'h':
             usage(argv[0]);
-            return 0;
+            exit(EXIT_SUCCESS);
         case '?':
             g_print("Unknown option, try '%s --help' for more information.\n",
                     argv[0]);
-            return EXIT_FAILURE;
+            exit(EXIT_FAILURE);
         }
     }
 
-- 
2.4.3

[Qemu-devel] [PATCH v2 03/12] qga: move string split in separate function

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The function is going to be reused in a later patch.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 10bb2f7..e75022c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -921,6 +921,26 @@ static void ga_print_cmd(QmpCommand *cmd, void *opaque)
     printf("%s\n", qmp_command_name(cmd));
 }
 
+static GList *split_list(gchar *str, const gchar separator)
+{
+    GList *list = NULL;
+    int i, j, len;
+
+    for (j = 0, i = 0, len = strlen(str); i < len; i++) {
+        if (str[i] == separator) {
+            str[i] = 0;
+            list = g_list_append(list, &str[j]);
+            j = i + 1;
+        }
+    }
+
+    if (j < i) {
+        list = g_list_append(list, &str[j]);
+    }
+
+    return list;
+}
+
 int main(int argc, char **argv)
 {
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
@@ -953,7 +973,7 @@ int main(int argc, char **argv)
         { "statedir", 1, NULL, 't' },
         { NULL, 0, NULL, 0 }
     };
-    int opt_ind = 0, ch, daemonize = 0, i, j, len;
+    int opt_ind = 0, ch, daemonize = 0;
     GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
     GList *blacklist = NULL;
     GAState *s;
@@ -1001,16 +1021,7 @@ int main(int argc, char **argv)
                 qmp_for_each_command(ga_print_cmd, NULL);
                 exit(EXIT_SUCCESS);
             }
-            for (j = 0, i = 0, len = strlen(optarg); i < len; i++) {
-                if (optarg[i] == ',') {
-                    optarg[i] = 0;
-                    blacklist = g_list_append(blacklist, &optarg[j]);
-                    j = i + 1;
-                }
-            }
-            if (j < i) {
-                blacklist = g_list_append(blacklist, &optarg[j]);
-            }
+            blacklist = g_list_concat(blacklist, split_list(optarg, ','));
             break;
         }
 #ifdef _WIN32
-- 
2.4.3

[Qemu-devel] [PATCH v2 04/12] qga: rename 'path' to 'channel_path'

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

'path' is already a global function, rename the variable since it's
going to be in global scope in a later patch.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index e75022c..ede5306 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -944,7 +944,7 @@ static GList *split_list(gchar *str, const gchar separator)
 int main(int argc, char **argv)
 {
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
-    const char *method = NULL, *path = NULL;
+    const char *method = NULL, *channel_path = NULL;
     const char *log_filepath = NULL;
     const char *pid_filepath;
 #ifdef CONFIG_FSFREEZE
@@ -990,7 +990,7 @@ int main(int argc, char **argv)
             method = optarg;
             break;
         case 'p':
-            path = optarg;
+            channel_path = optarg;
             break;
         case 'l':
             log_filepath = optarg;
@@ -1040,7 +1040,8 @@ int main(int argc, char **argv)
                 if (ga_install_vss_provider()) {
                     exit(EXIT_FAILURE);
                 }
-                if (ga_install_service(path, log_filepath, fixed_state_dir)) {
+                if (ga_install_service(channel_path, log_filepath,
+                                       fixed_state_dir)) {
                     exit(EXIT_FAILURE);
                 }
                 exit(EXIT_SUCCESS);
@@ -1185,7 +1186,7 @@ int main(int argc, char **argv)
 #endif
 
     s->main_loop = g_main_loop_new(NULL, false);
-    if (!channel_init(ga_state, method, path)) {
+    if (!channel_init(ga_state, method, channel_path)) {
         g_critical("failed to initialize guest agent channel");
         goto out_bad;
     }
-- 
2.4.3

[Qemu-devel] [PATCH v2 05/12] qga: copy argument strings

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

A following patch will return allocated string.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 57 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index ede5306..83b7804 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -944,13 +944,13 @@ static GList *split_list(gchar *str, const gchar separator)
 int main(int argc, char **argv)
 {
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
-    const char *method = NULL, *channel_path = NULL;
-    const char *log_filepath = NULL;
-    const char *pid_filepath;
+    char *method = NULL, *channel_path = NULL;
+    char *log_filepath = NULL;
+    char *pid_filepath = NULL;
 #ifdef CONFIG_FSFREEZE
-    const char *fsfreeze_hook = NULL;
+    char *fsfreeze_hook = NULL;
 #endif
-    const char *state_dir;
+    char *state_dir = NULL;
 #ifdef _WIN32
     const char *service = NULL;
 #endif
@@ -981,31 +981,28 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_QAPI);
 
     init_dfl_pathnames();
-    pid_filepath = dfl_pathnames.pidfile;
-    state_dir = dfl_pathnames.state_dir;
-
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
         switch (ch) {
         case 'm':
-            method = optarg;
+            method = g_strdup(optarg);
             break;
         case 'p':
-            channel_path = optarg;
+            channel_path = g_strdup(optarg);
             break;
         case 'l':
-            log_filepath = optarg;
+            log_filepath = g_strdup(optarg);
             break;
         case 'f':
-            pid_filepath = optarg;
+            pid_filepath = g_strdup(optarg);
             break;
 #ifdef CONFIG_FSFREEZE
         case 'F':
-            fsfreeze_hook = optarg ? optarg : QGA_FSFREEZE_HOOK_DEFAULT;
+            fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT);
             break;
 #endif
         case 't':
-             state_dir = optarg;
-             break;
+            state_dir = g_strdup(optarg);
+            break;
         case 'v':
             /* enable all log levels */
             log_level = G_LOG_LEVEL_MASK;
@@ -1028,20 +1025,10 @@ int main(int argc, char **argv)
         case 's':
             service = optarg;
             if (strcmp(service, "install") == 0) {
-                const char *fixed_state_dir;
-
-                /* If the user passed the "-t" option, we save that state dir
-                 * in the service. Otherwise we let the service fetch the state
-                 * dir from the environment when it starts.
-                 */
-                fixed_state_dir = (state_dir == dfl_pathnames.state_dir) ?
-                                  NULL :
-                                  state_dir;
                 if (ga_install_vss_provider()) {
                     exit(EXIT_FAILURE);
                 }
-                if (ga_install_service(channel_path, log_filepath,
-                                       fixed_state_dir)) {
+                if (ga_install_service(channel_path, log_filepath, state_dir)) {
                     exit(EXIT_FAILURE);
                 }
                 exit(EXIT_SUCCESS);
@@ -1072,6 +1059,14 @@ int main(int argc, char **argv)
         }
     }
 
+    if (pid_filepath == NULL) {
+        pid_filepath = g_strdup(dfl_pathnames.pidfile);
+    }
+
+    if (state_dir == NULL) {
+        state_dir = g_strdup(dfl_pathnames.state_dir);
+    }
+
 #ifdef _WIN32
     /* On win32 the state directory is application specific (be it the default
      * or a user override). We got past the command line parsing; let's create
@@ -1214,5 +1209,15 @@ out_bad:
     if (daemonize) {
         unlink(pid_filepath);
     }
+
+    g_free(method);
+    g_free(log_filepath);
+    g_free(pid_filepath);
+    g_free(state_dir);
+    g_free(channel_path);
+#ifdef CONFIG_FSFREEZE
+    g_free(fsfreeze_hook);
+#endif
+
     return EXIT_FAILURE;
 }
-- 
2.4.3

[Qemu-devel] [PATCH v2 06/12] qga: move option parsing to seperate function

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Move option parsing out of giant main().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 165 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 96 insertions(+), 69 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 83b7804..a8dda38 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -941,19 +941,28 @@ static GList *split_list(gchar *str, const gchar separator)
     return list;
 }
 
-int main(int argc, char **argv)
-{
-    const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
-    char *method = NULL, *channel_path = NULL;
-    char *log_filepath = NULL;
-    char *pid_filepath = NULL;
+typedef struct GAConfig {
+    char *channel_path;
+    char *method;
+    char *log_filepath;
+    char *pid_filepath;
 #ifdef CONFIG_FSFREEZE
-    char *fsfreeze_hook = NULL;
+    char *fsfreeze_hook;
 #endif
-    char *state_dir = NULL;
+    char *state_dir;
 #ifdef _WIN32
-    const char *service = NULL;
+    const char *service;
 #endif
+    GList *blacklist;
+    int daemonize;
+    GLogLevelFlags log_level;
+} GAConfig;
+
+static GAConfig *config_parse(int argc, char **argv)
+{
+    GAConfig *config = g_new0(GAConfig, 1);
+    const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
+    int opt_ind = 0, ch;
     const struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
@@ -973,74 +982,71 @@ int main(int argc, char **argv)
         { "statedir", 1, NULL, 't' },
         { NULL, 0, NULL, 0 }
     };
-    int opt_ind = 0, ch, daemonize = 0;
-    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
-    GList *blacklist = NULL;
-    GAState *s;
 
-    module_call_init(MODULE_INIT_QAPI);
+    config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
 
-    init_dfl_pathnames();
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
         switch (ch) {
         case 'm':
-            method = g_strdup(optarg);
+            config->method = g_strdup(optarg);
             break;
         case 'p':
-            channel_path = g_strdup(optarg);
+            config->channel_path = g_strdup(optarg);
             break;
         case 'l':
-            log_filepath = g_strdup(optarg);
+            config->log_filepath = g_strdup(optarg);
             break;
         case 'f':
-            pid_filepath = g_strdup(optarg);
+            config->pid_filepath = g_strdup(optarg);
             break;
 #ifdef CONFIG_FSFREEZE
         case 'F':
-            fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT);
+            config->fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT);
             break;
 #endif
         case 't':
-            state_dir = g_strdup(optarg);
+            config->state_dir = g_strdup(optarg);
             break;
         case 'v':
             /* enable all log levels */
-            log_level = G_LOG_LEVEL_MASK;
+            config->log_level = G_LOG_LEVEL_MASK;
             break;
         case 'V':
             printf("QEMU Guest Agent %s\n", QEMU_VERSION);
             exit(EXIT_SUCCESS);
         case 'd':
-            daemonize = 1;
+            config->daemonize = 1;
             break;
         case 'b': {
             if (is_help_option(optarg)) {
                 qmp_for_each_command(ga_print_cmd, NULL);
                 exit(EXIT_SUCCESS);
             }
-            blacklist = g_list_concat(blacklist, split_list(optarg, ','));
+            config->blacklist = g_list_concat(config->blacklist,
+                                             split_list(optarg, ','));
             break;
         }
 #ifdef _WIN32
         case 's':
-            service = optarg;
-            if (strcmp(service, "install") == 0) {
+            config->service = optarg;
+            if (strcmp(config->service, "install") == 0) {
                 if (ga_install_vss_provider()) {
                     exit(EXIT_FAILURE);
                 }
-                if (ga_install_service(channel_path, log_filepath, state_dir)) {
+                if (ga_install_service(config->channel_path,
+                                       config->log_filepath, config->state_dir)) {
                     exit(EXIT_FAILURE);
                 }
                 exit(EXIT_SUCCESS);
-            } else if (strcmp(service, "uninstall") == 0) {
+            } else if (strcmp(config->service, "uninstall") == 0) {
                 ga_uninstall_vss_provider();
                 exit(ga_uninstall_service());
-            } else if (strcmp(service, "vss-install") == 0) {
+            } else if (strcmp(config->service, "vss-install") == 0) {
                 if (ga_install_vss_provider()) {
                     exit(EXIT_FAILURE);
                 }
                 exit(EXIT_SUCCESS);
-            } else if (strcmp(service, "vss-uninstall") == 0) {
+            } else if (strcmp(config->service, "vss-uninstall") == 0) {
                 ga_uninstall_vss_provider();
                 exit(EXIT_SUCCESS);
             } else {
@@ -1059,12 +1065,39 @@ int main(int argc, char **argv)
         }
     }
 
-    if (pid_filepath == NULL) {
-        pid_filepath = g_strdup(dfl_pathnames.pidfile);
+    return config;
+}
+
+static void config_free(GAConfig *config)
+{
+    g_free(config->method);
+    g_free(config->log_filepath);
+    g_free(config->pid_filepath);
+    g_free(config->state_dir);
+    g_free(config->channel_path);
+#ifdef CONFIG_FSFREEZE
+    g_free(config->fsfreeze_hook);
+#endif
+    g_free(config);
+}
+
+ int main(int argc, char **argv)
+{
+    GAState *s;
+    GAConfig *config;
+
+    module_call_init(MODULE_INIT_QAPI);
+
+    init_dfl_pathnames();
+
+    config = config_parse(argc, argv);
+
+    if (config->pid_filepath == NULL) {
+        config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
     }
 
-    if (state_dir == NULL) {
-        state_dir = g_strdup(dfl_pathnames.state_dir);
+    if (config->state_dir == NULL) {
+        config->state_dir = g_strdup(dfl_pathnames.state_dir);
     }
 
 #ifdef _WIN32
@@ -1074,25 +1107,25 @@ int main(int argc, char **argv)
      * error later on, we won't try to clean up the directory, it is considered
      * persistent.
      */
-    if (g_mkdir_with_parents(state_dir, S_IRWXU) == -1) {
+    if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
         g_critical("unable to create (an ancestor of) the state directory"
-                   " '%s': %s", state_dir, strerror(errno));
+                   " '%s': %s", config->state_dir, strerror(errno));
         return EXIT_FAILURE;
     }
 #endif
 
     s = g_malloc0(sizeof(GAState));
-    s->log_level = log_level;
+    s->log_level = config->log_level;
     s->log_file = stderr;
 #ifdef CONFIG_FSFREEZE
-    s->fsfreeze_hook = fsfreeze_hook;
+    s->fsfreeze_hook = config->fsfreeze_hook;
 #endif
     g_log_set_default_handler(ga_log, s);
     g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
     ga_enable_logging(s);
     s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
-                                                 state_dir);
-    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
+                                                 config->state_dir);
+    s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir);
     s->frozen = false;
 
 #ifndef _WIN32
@@ -1125,23 +1158,23 @@ int main(int argc, char **argv)
 #endif
 
     if (ga_is_frozen(s)) {
-        if (daemonize) {
+        if (config->daemonize) {
             /* delay opening/locking of pidfile till filesystems are unfrozen */
-            s->deferred_options.pid_filepath = pid_filepath;
+            s->deferred_options.pid_filepath = config->pid_filepath;
             become_daemon(NULL);
         }
-        if (log_filepath) {
+        if (config->log_filepath) {
             /* delay opening the log file till filesystems are unfrozen */
-            s->deferred_options.log_filepath = log_filepath;
+            s->deferred_options.log_filepath = config->log_filepath;
         }
         ga_disable_logging(s);
         qmp_for_each_command(ga_disable_non_whitelisted, NULL);
     } else {
-        if (daemonize) {
-            become_daemon(pid_filepath);
+        if (config->daemonize) {
+            become_daemon(config->pid_filepath);
         }
-        if (log_filepath) {
-            FILE *log_file = ga_open_logfile(log_filepath);
+        if (config->log_filepath) {
+            FILE *log_file = ga_open_logfile(config->log_filepath);
             if (!log_file) {
                 g_critical("unable to open specified log file: %s",
                            strerror(errno));
@@ -1159,14 +1192,15 @@ int main(int argc, char **argv)
         goto out_bad;
     }
 
-    blacklist = ga_command_blacklist_init(blacklist);
-    if (blacklist) {
-        s->blacklist = blacklist;
+    config->blacklist = ga_command_blacklist_init(config->blacklist);
+    if (config->blacklist) {
+        GList *l = config->blacklist;
+        s->blacklist = config->blacklist;
         do {
-            g_debug("disabling command: %s", (char *)blacklist->data);
-            qmp_disable_command(blacklist->data);
-            blacklist = g_list_next(blacklist);
-        } while (blacklist);
+            g_debug("disabling command: %s", (char *)l->data);
+            qmp_disable_command(l->data);
+            l = g_list_next(l);
+        } while (l);
     }
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
@@ -1181,14 +1215,14 @@ int main(int argc, char **argv)
 #endif
 
     s->main_loop = g_main_loop_new(NULL, false);
-    if (!channel_init(ga_state, method, channel_path)) {
+    if (!channel_init(ga_state, config->method, config->channel_path)) {
         g_critical("failed to initialize guest agent channel");
         goto out_bad;
     }
 #ifndef _WIN32
     g_main_loop_run(ga_state->main_loop);
 #else
-    if (daemonize) {
+    if (config->daemonize) {
         SERVICE_TABLE_ENTRY service_table[] = {
             { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
         StartServiceCtrlDispatcher(service_table);
@@ -1200,24 +1234,17 @@ int main(int argc, char **argv)
     ga_command_state_cleanup_all(ga_state->command_state);
     ga_channel_free(ga_state->channel);
 
-    if (daemonize) {
-        unlink(pid_filepath);
+    if (config->daemonize) {
+        unlink(config->pid_filepath);
     }
     return 0;
 
 out_bad:
-    if (daemonize) {
-        unlink(pid_filepath);
+    if (config->daemonize) {
+        unlink(config->pid_filepath);
     }
 
-    g_free(method);
-    g_free(log_filepath);
-    g_free(pid_filepath);
-    g_free(state_dir);
-    g_free(channel_path);
-#ifdef CONFIG_FSFREEZE
-    g_free(fsfreeze_hook);
-#endif
+    config_free(config);
 
     return EXIT_FAILURE;
 }
-- 
2.4.3

[Qemu-devel] [PATCH v2 07/12] qga: fill default options in main()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Fill all default options during main(). This is a preparation patch
to allow to dump the configuration.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index a8dda38..979ce5c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -658,23 +658,6 @@ static gboolean channel_init(GAState *s, const gchar *method, const gchar *path)
 {
     GAChannelMethod channel_method;
 
-    if (method == NULL) {
-        method = "virtio-serial";
-    }
-
-    if (path == NULL) {
-        if (strcmp(method, "virtio-serial") == 0 ) {
-            /* try the default path for the virtio-serial port */
-            path = QGA_VIRTIO_PATH_DEFAULT;
-        } else if (strcmp(method, "isa-serial") == 0){
-            /* try the default path for the serial port - COM1 */
-            path = QGA_SERIAL_PATH_DEFAULT;
-        } else {
-            g_critical("must specify a path for this channel");
-            return false;
-        }
-    }
-
     if (strcmp(method, "virtio-serial") == 0) {
         s->virtio = true; /* virtio requires special handling in some cases */
         channel_method = GA_CHANNEL_VIRTIO_SERIAL;
@@ -1081,7 +1064,7 @@ static void config_free(GAConfig *config)
     g_free(config);
 }
 
- int main(int argc, char **argv)
+int main(int argc, char **argv)
 {
     GAState *s;
     GAConfig *config;
@@ -1100,6 +1083,23 @@ static void config_free(GAConfig *config)
         config->state_dir = g_strdup(dfl_pathnames.state_dir);
     }
 
+    if (method == NULL) {
+        method = g_strdup("virtio-serial");
+    }
+
+    if (device_path == NULL) {
+        if (strcmp(method, "virtio-serial") == 0) {
+            /* try the default path for the virtio-serial port */
+            device_path = g_strdup(QGA_VIRTIO_PATH_DEFAULT);
+        } else if (strcmp(method, "isa-serial") == 0) {
+            /* try the default path for the serial port - COM1 */
+            device_path = g_strdup(QGA_SERIAL_PATH_DEFAULT);
+        } else {
+            g_critical("must specify a path for this channel");
+            goto out_bad;
+        }
+    }
+
 #ifdef _WIN32
     /* On win32 the state directory is application specific (be it the default
      * or a user override). We got past the command line parsing; let's create
-- 
2.4.3

[Qemu-devel] [PATCH v2 08/12] qga: move agent run in a seperate function

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Once the options are populated, move the running state to
a run_agent() function.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 164 +++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 89 insertions(+), 75 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 979ce5c..118847c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1064,70 +1064,8 @@ static void config_free(GAConfig *config)
     g_free(config);
 }
 
-int main(int argc, char **argv)
+static bool check_is_frozen(GAState *s)
 {
-    GAState *s;
-    GAConfig *config;
-
-    module_call_init(MODULE_INIT_QAPI);
-
-    init_dfl_pathnames();
-
-    config = config_parse(argc, argv);
-
-    if (config->pid_filepath == NULL) {
-        config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
-    }
-
-    if (config->state_dir == NULL) {
-        config->state_dir = g_strdup(dfl_pathnames.state_dir);
-    }
-
-    if (method == NULL) {
-        method = g_strdup("virtio-serial");
-    }
-
-    if (device_path == NULL) {
-        if (strcmp(method, "virtio-serial") == 0) {
-            /* try the default path for the virtio-serial port */
-            device_path = g_strdup(QGA_VIRTIO_PATH_DEFAULT);
-        } else if (strcmp(method, "isa-serial") == 0) {
-            /* try the default path for the serial port - COM1 */
-            device_path = g_strdup(QGA_SERIAL_PATH_DEFAULT);
-        } else {
-            g_critical("must specify a path for this channel");
-            goto out_bad;
-        }
-    }
-
-#ifdef _WIN32
-    /* On win32 the state directory is application specific (be it the default
-     * or a user override). We got past the command line parsing; let's create
-     * the directory (with any intermediate directories). If we run into an
-     * error later on, we won't try to clean up the directory, it is considered
-     * persistent.
-     */
-    if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
-        g_critical("unable to create (an ancestor of) the state directory"
-                   " '%s': %s", config->state_dir, strerror(errno));
-        return EXIT_FAILURE;
-    }
-#endif
-
-    s = g_malloc0(sizeof(GAState));
-    s->log_level = config->log_level;
-    s->log_file = stderr;
-#ifdef CONFIG_FSFREEZE
-    s->fsfreeze_hook = config->fsfreeze_hook;
-#endif
-    g_log_set_default_handler(ga_log, s);
-    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
-    ga_enable_logging(s);
-    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
-                                                 config->state_dir);
-    s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir);
-    s->frozen = false;
-
 #ifndef _WIN32
     /* check if a previous instance of qemu-ga exited with filesystems' state
      * marked as frozen. this could be a stale value (a non-qemu-ga process
@@ -1153,7 +1091,31 @@ int main(int argc, char **argv)
                   " guest-fsfreeze-thaw is issued, or filesystems are"
                   " manually unfrozen and the file %s is removed",
                   s->state_filepath_isfrozen);
-        s->frozen = true;
+        return true;
+    }
+#endif
+    return false;
+}
+
+static int run_agent(GAState *s, GAConfig *config)
+{
+    ga_state = s;
+
+    g_log_set_default_handler(ga_log, s);
+    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
+    ga_enable_logging(s);
+
+#ifdef _WIN32
+    /* On win32 the state directory is application specific (be it the default
+     * or a user override). We got past the command line parsing; let's create
+     * the directory (with any intermediate directories). If we run into an
+     * error later on, we won't try to clean up the directory, it is considered
+     * persistent.
+     */
+    if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
+        g_critical("unable to create (an ancestor of) the state directory"
+                   " '%s': %s", config->state_dir, strerror(errno));
+        return EXIT_FAILURE;
     }
 #endif
 
@@ -1178,7 +1140,7 @@ int main(int argc, char **argv)
             if (!log_file) {
                 g_critical("unable to open specified log file: %s",
                            strerror(errno));
-                goto out_bad;
+                return EXIT_FAILURE;
             }
             s->log_file = log_file;
         }
@@ -1189,7 +1151,7 @@ int main(int argc, char **argv)
                                s->pstate_filepath,
                                ga_is_frozen(s))) {
         g_critical("failed to load persistent state");
-        goto out_bad;
+        return EXIT_FAILURE;
     }
 
     config->blacklist = ga_command_blacklist_init(config->blacklist);
@@ -1210,14 +1172,14 @@ int main(int argc, char **argv)
 #ifndef _WIN32
     if (!register_signal_handlers()) {
         g_critical("failed to register signal handlers");
-        goto out_bad;
+        return EXIT_FAILURE;
     }
 #endif
 
     s->main_loop = g_main_loop_new(NULL, false);
     if (!channel_init(ga_state, config->method, config->channel_path)) {
         g_critical("failed to initialize guest agent channel");
-        goto out_bad;
+        return EXIT_FAILURE;
     }
 #ifndef _WIN32
     g_main_loop_run(ga_state->main_loop);
@@ -1231,20 +1193,72 @@ int main(int argc, char **argv)
     }
 #endif
 
-    ga_command_state_cleanup_all(ga_state->command_state);
-    ga_channel_free(ga_state->channel);
+    return EXIT_SUCCESS;
+}
 
-    if (config->daemonize) {
-        unlink(config->pid_filepath);
+int main(int argc, char **argv)
+{
+    int ret = EXIT_SUCCESS;
+    GAState *s = g_new0(GAState, 1);
+    GAConfig *config;
+
+    module_call_init(MODULE_INIT_QAPI);
+
+    init_dfl_pathnames();
+
+    config = config_parse(argc, argv);
+
+    if (config->pid_filepath == NULL) {
+        config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
+    }
+
+    if (config->state_dir == NULL) {
+        config->state_dir = g_strdup(dfl_pathnames.state_dir);
+    }
+
+    if (config->method == NULL) {
+        config->method = g_strdup("virtio-serial");
+    }
+
+    if (config->channel_path == NULL) {
+        if (strcmp(config->method, "virtio-serial") == 0) {
+            /* try the default path for the virtio-serial port */
+            config->channel_path = g_strdup(QGA_VIRTIO_PATH_DEFAULT);
+        } else if (strcmp(config->method, "isa-serial") == 0) {
+            /* try the default path for the serial port - COM1 */
+            config->channel_path = g_strdup(QGA_SERIAL_PATH_DEFAULT);
+        } else {
+            g_critical("must specify a path for this channel");
+            ret = EXIT_FAILURE;
+            goto end;
+        }
+    }
+
+    s->log_level = config->log_level;
+    s->log_file = stderr;
+#ifdef CONFIG_FSFREEZE
+    s->fsfreeze_hook = config->fsfreeze_hook;
+#endif
+    s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir);
+    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
+                                                 config->state_dir);
+    s->frozen = check_is_frozen(s);
+
+    ret = run_agent(s, config);
+
+end:
+    if (s->command_state) {
+        ga_command_state_cleanup_all(s->command_state);
+    }
+    if (s->channel) {
+        ga_channel_free(s->channel);
     }
-    return 0;
 
-out_bad:
     if (config->daemonize) {
         unlink(config->pid_filepath);
     }
 
     config_free(config);
 
-    return EXIT_FAILURE;
+    return ret;
 }
-- 
2.4.3

[Qemu-devel] [PATCH v2 09/12] qga: free a bit more

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Now that main() has a single exit point, we can free a few
more allocations.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 118847c..58f2fc7 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -82,7 +82,7 @@ struct GAState {
     bool delimit_response;
     bool frozen;
     GList *blacklist;
-    const char *state_filepath_isfrozen;
+    char *state_filepath_isfrozen;
     struct {
         const char *log_filepath;
         const char *pid_filepath;
@@ -90,7 +90,7 @@ struct GAState {
 #ifdef CONFIG_FSFREEZE
     const char *fsfreeze_hook;
 #endif
-    const gchar *pstate_filepath;
+    gchar *pstate_filepath;
     GAPersistentState pstate;
 };
 
@@ -1253,6 +1253,8 @@ end:
     if (s->channel) {
         ga_channel_free(s->channel);
     }
+    g_free(s->pstate_filepath);
+    g_free(s->state_filepath_isfrozen);
 
     if (config->daemonize) {
         unlink(config->pid_filepath);
-- 
2.4.3

[Qemu-devel] [PATCH v2 10/12] qga: add an optionnal qemu-ga.conf system configuration

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Learn to configure the agent with a system configuration.

This may simplify command-line handling, especially when the blacklist
is long.

Among the other benefits, this may standardize the configuration of a
init service (instead of distro-specific init keys/files)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 7 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 58f2fc7..154975c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -56,6 +56,7 @@
 #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
 #endif
 #define QGA_SENTINEL_BYTE 0xFF
+#define QGA_CONF_DEFAULT CONFIG_QEMU_CONFDIR G_DIR_SEPARATOR_S "qemu-ga.conf"
 
 static struct {
     const char *state_dir;
@@ -936,14 +937,80 @@ typedef struct GAConfig {
 #ifdef _WIN32
     const char *service;
 #endif
+    gchar *bliststr;
     GList *blacklist;
     int daemonize;
     GLogLevelFlags log_level;
 } GAConfig;
 
-static GAConfig *config_parse(int argc, char **argv)
+static void config_load(GAConfig *config)
+{
+    GError *gerr = NULL;
+    GKeyFile *keyfile;
+
+    /* read system config */
+    keyfile = g_key_file_new();
+    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
+        goto end;
+    }
+    if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
+        config->daemonize =
+            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
+        config->method =
+            g_key_file_get_string(keyfile, "general", "method", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
+        config->channel_path =
+            g_key_file_get_string(keyfile, "general", "path", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
+        config->log_filepath =
+            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
+        config->pid_filepath =
+            g_key_file_get_string(keyfile, "general", "pidfile", &gerr);
+    }
+#ifdef CONFIG_FSFREEZE
+    if (g_key_file_has_key(keyfile, "general", "fsfreeze-hook", NULL)) {
+        config->fsfreeze_hook =
+            g_key_file_get_string(keyfile,
+                                  "general", "fsfreeze-hook", &gerr);
+    }
+#endif
+    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
+        config->state_dir =
+            g_key_file_get_string(keyfile, "general", "statedir", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "verbose", NULL) &&
+        g_key_file_get_boolean(keyfile, "general", "verbose", &gerr)) {
+        /* enable all log levels */
+        config->log_level = G_LOG_LEVEL_MASK;
+    }
+    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
+        config->bliststr =
+            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
+        config->blacklist = g_list_concat(config->blacklist,
+                                          split_list(config->bliststr, ','));
+    }
+
+end:
+    if (keyfile) {
+        g_key_file_free(keyfile);
+    }
+    if (gerr &&
+        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
+        g_critical("error loading configuration from path: %s, %s",
+                   QGA_CONF_DEFAULT, gerr->message);
+        exit(EXIT_FAILURE);
+    }
+    g_clear_error(&gerr);
+}
+
+static void config_parse(GAConfig *config, int argc, char **argv)
 {
-    GAConfig *config = g_new0(GAConfig, 1);
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
     int opt_ind = 0, ch;
     const struct option lopt[] = {
@@ -1047,8 +1114,6 @@ static GAConfig *config_parse(int argc, char **argv)
             exit(EXIT_FAILURE);
         }
     }
-
-    return config;
 }
 
 static void config_free(GAConfig *config)
@@ -1058,6 +1123,7 @@ static void config_free(GAConfig *config)
     g_free(config->pid_filepath);
     g_free(config->state_dir);
     g_free(config->channel_path);
+    g_free(config->bliststr);
 #ifdef CONFIG_FSFREEZE
     g_free(config->fsfreeze_hook);
 #endif
@@ -1200,13 +1266,13 @@ int main(int argc, char **argv)
 {
     int ret = EXIT_SUCCESS;
     GAState *s = g_new0(GAState, 1);
-    GAConfig *config;
+    GAConfig *config = g_new0(GAConfig, 1);
 
     module_call_init(MODULE_INIT_QAPI);
 
     init_dfl_pathnames();
-
-    config = config_parse(argc, argv);
+    config_load(config);
+    config_parse(config, argc, argv);
 
     if (config->pid_filepath == NULL) {
         config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
-- 
2.4.3

[Qemu-devel] [PATCH v2 11/12] qga: add --dump-conf option

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This new option allows to review the agent configuration,
and ease the task of writing a configuration file.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/qga/main.c b/qga/main.c
index 154975c..a47310b 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -941,6 +941,7 @@ typedef struct GAConfig {
     GList *blacklist;
     int daemonize;
     GLogLevelFlags log_level;
+    int dumpconf;
 } GAConfig;
 
 static void config_load(GAConfig *config)
@@ -1009,6 +1010,58 @@ end:
     g_clear_error(&gerr);
 }
 
+static gchar *list_join(GList *list, const gchar separator)
+{
+    GString *str = g_string_new("");
+
+    while (list) {
+        str = g_string_append(str, (gchar *)list->data);
+        list = g_list_next(list);
+        if (list) {
+            str = g_string_append_c(str, separator);
+        }
+    }
+
+    return g_string_free(str, FALSE);
+}
+
+static void config_dump(GAConfig *config)
+{
+    GError *error = NULL;
+    GKeyFile *keyfile;
+    gchar *tmp;
+
+    keyfile = g_key_file_new();
+    g_key_file_set_boolean(keyfile, "general", "daemon", config->daemonize);
+    g_key_file_set_string(keyfile, "general", "method", config->method);
+    g_key_file_set_string(keyfile, "general", "path", config->channel_path);
+    if (config->log_filepath) {
+        g_key_file_set_string(keyfile, "general", "logfile",
+                              config->log_filepath);
+    }
+    g_key_file_set_string(keyfile, "general", "pidfile", config->pid_filepath);
+#ifdef CONFIG_FSFREEZE
+    if (config->fsfreeze_hook) {
+        g_key_file_set_string(keyfile, "general", "fsfreeze-hook",
+                              config->fsfreeze_hook);
+    }
+#endif
+    g_key_file_set_string(keyfile, "general", "statedir", config->state_dir);
+    g_key_file_set_boolean(keyfile, "general", "verbose",
+                           config->log_level == G_LOG_LEVEL_MASK);
+    tmp = list_join(config->blacklist, ',');
+    g_key_file_set_string(keyfile, "general", "blacklist", tmp);
+    g_free(tmp);
+
+    tmp = g_key_file_to_data(keyfile, NULL, &error);
+    printf("%s", tmp);
+
+    g_free(tmp);
+    if (keyfile) {
+        g_key_file_free(keyfile);
+    }
+}
+
 static void config_parse(GAConfig *config, int argc, char **argv)
 {
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
@@ -1016,6 +1069,7 @@ static void config_parse(GAConfig *config, int argc, char **argv)
     const struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
+        { "dump-conf", 0, NULL, 'D' },
         { "logfile", 1, NULL, 'l' },
         { "pidfile", 1, NULL, 'f' },
 #ifdef CONFIG_FSFREEZE
@@ -1067,6 +1121,9 @@ static void config_parse(GAConfig *config, int argc, char **argv)
         case 'd':
             config->daemonize = 1;
             break;
+        case 'D':
+            config->dumpconf = 1;
+            break;
         case 'b': {
             if (is_help_option(optarg)) {
                 qmp_for_each_command(ga_print_cmd, NULL);
@@ -1310,6 +1367,11 @@ int main(int argc, char **argv)
                                                  config->state_dir);
     s->frozen = check_is_frozen(s);
 
+    if (config->dumpconf) {
+        config_dump(config);
+        goto end;
+    }
+
     ret = run_agent(s, config);
 
 end:
-- 
2.4.3

[Qemu-devel] [PATCH v2 12/12] qga: start a man page

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add a simple man page for the qemu agent.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 Makefile      |  14 +++++-
 qemu-doc.texi |   6 +++
 qemu-ga.texi  | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+), 2 deletions(-)
 create mode 100644 qemu-ga.texi

diff --git a/Makefile b/Makefile
index 340d9c8..67d44b8 100644
--- a/Makefile
+++ b/Makefile
@@ -88,7 +88,8 @@ LIBS+=-lz $(LIBS_TOOLS)
 HELPERS-$(CONFIG_LINUX) = qemu-bridge-helper$(EXESUF)
 
 ifdef BUILD_DOCS
-DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8 qmp-commands.txt
+DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8 qemu-ga.8
+DOCS+=qmp-commands.txt
 ifdef CONFIG_LINUX
 DOCS+=kvm_stat.1
 endif
@@ -400,6 +401,9 @@ ifneq ($(TOOLS),)
 	$(INSTALL_DIR) "$(DESTDIR)$(mandir)/man8"
 	$(INSTALL_DATA) qemu-nbd.8 "$(DESTDIR)$(mandir)/man8"
 endif
+ifneq (,$(findstring qemu-ga,$(TOOLS)))
+	$(INSTALL_DATA) qemu-ga.8 "$(DESTDIR)$(mandir)/man8"
+endif
 endif
 ifdef CONFIG_VIRTFS
 	$(INSTALL_DIR) "$(DESTDIR)$(mandir)/man1"
@@ -538,6 +542,12 @@ qemu-nbd.8: qemu-nbd.texi
 	  $(POD2MAN) --section=8 --center=" " --release=" " qemu-nbd.pod > $@, \
 	  "  GEN   $@")
 
+qemu-ga.8: qemu-ga.texi
+	$(call quiet-command, \
+	  perl -Ww -- $(SRC_PATH)/scripts/texi2pod.pl $< qemu-ga.pod && \
+	  $(POD2MAN) --section=8 --center=" " --release=" " qemu-ga.pod > $@, \
+	  "  GEN   $@")
+
 kvm_stat.1: scripts/kvm/kvm_stat.texi
 	$(call quiet-command, \
 	  perl -Ww -- $(SRC_PATH)/scripts/texi2pod.pl $< kvm_stat.pod && \
@@ -551,7 +561,7 @@ pdf: qemu-doc.pdf qemu-tech.pdf
 
 qemu-doc.dvi qemu-doc.html qemu-doc.info qemu-doc.pdf: \
 	qemu-img.texi qemu-nbd.texi qemu-options.texi \
-	qemu-monitor.texi qemu-img-cmds.texi
+	qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi
 
 ifdef CONFIG_WIN32
 
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 94af8c0..84d17d1 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -412,6 +412,7 @@ snapshots.
 * vm_snapshots::              VM snapshots
 * qemu_img_invocation::       qemu-img Invocation
 * qemu_nbd_invocation::       qemu-nbd Invocation
+* qemu_ga_invocation::        qemu-ga Invocation
 * disk_images_formats::       Disk image file formats
 * host_drives::               Using host drives
 * disk_images_fat_images::    Virtual FAT disk images
@@ -505,6 +506,11 @@ state is not saved or restored properly (in particular USB).
 
 @include qemu-nbd.texi
 
+@node qemu_ga_invocation
+@subsection @code{qemu-ga} Invocation
+
+@include qemu-ga.texi
+
 @node disk_images_formats
 @subsection Disk image file formats
 
diff --git a/qemu-ga.texi b/qemu-ga.texi
new file mode 100644
index 0000000..7d4a628
--- /dev/null
+++ b/qemu-ga.texi
@@ -0,0 +1,136 @@
+@example
+@c man begin SYNOPSIS
+usage: qemu-ga [-m <method> -p <path>] [OPTION]...
+@c man end
+@end example
+
+@c man begin DESCRIPTION
+
+The QEMU Guest Agent is a deamon that allows the host to perform
+various operations in the guest, such as:
+
+@itemize
+@item
+get information from the guest
+@item
+set the guest's system time
+@item
+read/write a file
+@item
+sync and freeze the filesystems
+@item
+suspend the guest
+@item
+reconfigure guest local processors
+@item
+set user's password
+@item
+...
+@end itemize
+
+qemu-ga will read a system configuration file on startup (located at
+q@file{/etc/qemu/qemu-ga.conf} by default), then parse remaining
+configuration options on the command line. For the same key, the last
+option wins, but the lists accumulate (see below for configuration
+file format).
+
+@c man end
+
+@c man begin OPTIONS
+@table @option
+@item -m, --method=@var{method}
+  Transport method: one of @samp{unix-listen}, @samp{virtio-serial}, or
+  @samp{isa-serial} (@samp{virtio-serial} is the default).
+
+@item -p, --path=@var{path}
+  Device/socket path (the default for virtio-serial is:
+  @samp{/dev/virtio-ports/org.qemu.guest_agent.0},
+  the default for isa-serial is: @samp{/dev/ttyS0})
+
+@item -l, --logfile=@var{path}
+  Set log file path, logs to stderr by default.
+
+@item -f, --pidfile=@var{path}
+  Specify pid file (default is @samp{/var/run/qemu-ga.pid}).
+
+@item -F, --fsfreeze-hook=@var{path}
+  Enable fsfreeze hook. Accepts an optional argument that specifies
+  script to run on freeze/thaw. Script will be called with
+  'freeze'/'thaw' arguments accordingly.  (default is
+  @samp{/etc/qemu/fsfreeze-hook}) If using -F with an argument, do
+  not follow -F with a space. (for example:
+  @samp{-F/var/run/fsfreezehook.sh})
+
+@item -t, --statedir=@var{path}
+  Specify the directory to store state information (absolute paths only,
+  default is @samp{/var/run}).
+
+@item -v, --verbose
+  Log extra debugging information.
+
+@item -V, --version
+  Print version information and exit.
+
+@item -d, --daemon
+  Daemonize after startup (detach from terminal).
+
+@item -b, --blacklist=@var{list}
+  Comma-separated list of RPCs to disable (no spaces, @samp{?} to list
+  available RPCs).
+
+@item -D, --dump-conf
+  Dump the configuration in a format compatible with @file{qemu-ga.conf}
+  and exit.
+
+@item -h, --help
+  Display this help and exit.
+@end table
+
+@c man end
+
+@c man begin FILES
+
+The syntax of the @file{qemu-ga.conf} configuration file follows the
+Desktop Entry Specification, here is a quick summary: it consists of
+groups of key-value pairs, interspersed with comments.
+
+@example
+# qemu-ga configuration sample
+[general]
+daemonize = 0
+pidfile = /var/run/qemu-ga.pid
+verbose = 0
+method = virtio-serial
+path = /dev/virtio-ports/org.qemu.guest_agent.0
+statedir = /var/run
+@end example
+
+The list of keys follows the command line options:
+@table @option
+@item daemon= boolean
+@item method= string
+@item path= string
+@item logfile= string
+@item pidfile= string
+@item fsfreeze-hook= string
+@item statedir= string
+@item verbose= boolean
+@item blacklist= string list
+@end table
+
+@c man end
+
+@ignore
+
+@setfilename qemu-ga
+@settitle QEMU Guest Agent
+
+@c man begin AUTHOR
+Michael Roth <mdroth@linux.vnet.ibm.com>
+@c man end
+
+@c man begin SEEALSO
+qemu(1)
+@c man end
+
+@end ignore
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 01/12] qga: misc spelling

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:13)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/qapi-schema.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 18e3cc3..6b0bd16 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -793,7 +793,7 @@
>  # scheme. Refer to the documentation of the guest operating system
>  # in question to determine what is supported.
>  #
> -# Note all guest operating systems will support use of the
> +# Not all guest operating systems will support use of the
>  # @crypted flag, as they may require the clear-text password
>  #
>  # The @password parameter must always be base64 encoded before
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 02/12] qga: use exit() when parsing options

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:14)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The option parsing is going to be moved to a separate function,
> use exit() consistently.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/main.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 791982e..10bb2f7 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -992,14 +992,14 @@ int main(int argc, char **argv)
>              break;
>          case 'V':
>              printf("QEMU Guest Agent %s\n", QEMU_VERSION);
> -            return 0;
> +            exit(EXIT_SUCCESS);
>          case 'd':
>              daemonize = 1;
>              break;
>          case 'b': {
>              if (is_help_option(optarg)) {
>                  qmp_for_each_command(ga_print_cmd, NULL);
> -                return 0;
> +                exit(EXIT_SUCCESS);
>              }
>              for (j = 0, i = 0, len = strlen(optarg); i < len; i++) {
>                  if (optarg[i] == ',') {
> @@ -1027,36 +1027,36 @@ int main(int argc, char **argv)
>                                    NULL :
>                                    state_dir;
>                  if (ga_install_vss_provider()) {
> -                    return EXIT_FAILURE;
> +                    exit(EXIT_FAILURE);
>                  }
>                  if (ga_install_service(path, log_filepath, fixed_state_dir)) {
> -                    return EXIT_FAILURE;
> +                    exit(EXIT_FAILURE);
>                  }
> -                return 0;
> +                exit(EXIT_SUCCESS);
>              } else if (strcmp(service, "uninstall") == 0) {
>                  ga_uninstall_vss_provider();
> -                return ga_uninstall_service();
> +                exit(ga_uninstall_service());
>              } else if (strcmp(service, "vss-install") == 0) {
>                  if (ga_install_vss_provider()) {
> -                    return EXIT_FAILURE;
> +                    exit(EXIT_FAILURE);
>                  }
> -                return EXIT_SUCCESS;
> +                exit(EXIT_SUCCESS);
>              } else if (strcmp(service, "vss-uninstall") == 0) {
>                  ga_uninstall_vss_provider();
> -                return EXIT_SUCCESS;
> +                exit(EXIT_SUCCESS);
>              } else {
>                  printf("Unknown service command.\n");
> -                return EXIT_FAILURE;
> +                exit(EXIT_FAILURE);
>              }
>              break;
>  #endif
>          case 'h':
>              usage(argv[0]);
> -            return 0;
> +            exit(EXIT_SUCCESS);
>          case '?':
>              g_print("Unknown option, try '%s --help' for more information.\n",
>                      argv[0]);
> -            return EXIT_FAILURE;
> +            exit(EXIT_FAILURE);
>          }
>      }
> 
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 04/12] qga: rename 'path' to 'channel_path'

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:16)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 'path' is already a global function, rename the variable since it's
> going to be in global scope in a later patch.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/main.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index e75022c..ede5306 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -944,7 +944,7 @@ static GList *split_list(gchar *str, const gchar separator)
>  int main(int argc, char **argv)
>  {
>      const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> -    const char *method = NULL, *path = NULL;
> +    const char *method = NULL, *channel_path = NULL;
>      const char *log_filepath = NULL;
>      const char *pid_filepath;
>  #ifdef CONFIG_FSFREEZE
> @@ -990,7 +990,7 @@ int main(int argc, char **argv)
>              method = optarg;
>              break;
>          case 'p':
> -            path = optarg;
> +            channel_path = optarg;
>              break;
>          case 'l':
>              log_filepath = optarg;
> @@ -1040,7 +1040,8 @@ int main(int argc, char **argv)
>                  if (ga_install_vss_provider()) {
>                      exit(EXIT_FAILURE);
>                  }
> -                if (ga_install_service(path, log_filepath, fixed_state_dir)) {
> +                if (ga_install_service(channel_path, log_filepath,
> +                                       fixed_state_dir)) {
>                      exit(EXIT_FAILURE);
>                  }
>                  exit(EXIT_SUCCESS);
> @@ -1185,7 +1186,7 @@ int main(int argc, char **argv)
>  #endif
> 
>      s->main_loop = g_main_loop_new(NULL, false);
> -    if (!channel_init(ga_state, method, path)) {
> +    if (!channel_init(ga_state, method, channel_path)) {
>          g_critical("failed to initialize guest agent channel");
>          goto out_bad;
>      }
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 01/12] qga: misc spelling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On 08/25/2015 04:10 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/qapi-schema.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This already had Reviewed-by in version 1:
http://thread.gmane.org/gmane.comp.emulators.qemu/347998/focus=347997

It's best to edit your commit message and manually add in that notation,
when sending a v2, so that reviewers know that nothing changed since
last time they approved it:
http://wiki.qemu.org/Contribute/SubmitAPatch

[and while trying to write this email, I ran into an "Internal Server
Error" when trying to view
https://lists.gnu.org/archive/html/qemu-devel/2015-07/threads.html; I
don't know if the problem is transient, or if the webmasters at
nongnu.org know what is going on, but it would be a shame if our
archives can't be restored to working order]

-- 
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 --]

Re: [Qemu-devel] [PATCH v2 05/12] qga: copy argument strings

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:17)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> A following patch will return allocated string.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/main.c | 57 +++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index ede5306..83b7804 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -944,13 +944,13 @@ static GList *split_list(gchar *str, const gchar separator)
>  int main(int argc, char **argv)
>  {
>      const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> -    const char *method = NULL, *channel_path = NULL;
> -    const char *log_filepath = NULL;
> -    const char *pid_filepath;
> +    char *method = NULL, *channel_path = NULL;
> +    char *log_filepath = NULL;
> +    char *pid_filepath = NULL;
>  #ifdef CONFIG_FSFREEZE
> -    const char *fsfreeze_hook = NULL;
> +    char *fsfreeze_hook = NULL;
>  #endif
> -    const char *state_dir;
> +    char *state_dir = NULL;
>  #ifdef _WIN32
>      const char *service = NULL;
>  #endif
> @@ -981,31 +981,28 @@ int main(int argc, char **argv)
>      module_call_init(MODULE_INIT_QAPI);
> 
>      init_dfl_pathnames();
> -    pid_filepath = dfl_pathnames.pidfile;
> -    state_dir = dfl_pathnames.state_dir;
> -
>      while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
>          switch (ch) {
>          case 'm':
> -            method = optarg;
> +            method = g_strdup(optarg);
>              break;
>          case 'p':
> -            channel_path = optarg;
> +            channel_path = g_strdup(optarg);
>              break;
>          case 'l':
> -            log_filepath = optarg;
> +            log_filepath = g_strdup(optarg);
>              break;
>          case 'f':
> -            pid_filepath = optarg;
> +            pid_filepath = g_strdup(optarg);
>              break;
>  #ifdef CONFIG_FSFREEZE
>          case 'F':
> -            fsfreeze_hook = optarg ? optarg : QGA_FSFREEZE_HOOK_DEFAULT;
> +            fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT);
>              break;
>  #endif
>          case 't':
> -             state_dir = optarg;
> -             break;
> +            state_dir = g_strdup(optarg);
> +            break;
>          case 'v':
>              /* enable all log levels */
>              log_level = G_LOG_LEVEL_MASK;
> @@ -1028,20 +1025,10 @@ int main(int argc, char **argv)
>          case 's':
>              service = optarg;
>              if (strcmp(service, "install") == 0) {
> -                const char *fixed_state_dir;
> -
> -                /* If the user passed the "-t" option, we save that state dir
> -                 * in the service. Otherwise we let the service fetch the state
> -                 * dir from the environment when it starts.
> -                 */
> -                fixed_state_dir = (state_dir == dfl_pathnames.state_dir) ?
> -                                  NULL :
> -                                  state_dir;
>                  if (ga_install_vss_provider()) {
>                      exit(EXIT_FAILURE);
>                  }
> -                if (ga_install_service(channel_path, log_filepath,
> -                                       fixed_state_dir)) {
> +                if (ga_install_service(channel_path, log_filepath, state_dir)) {
>                      exit(EXIT_FAILURE);
>                  }
>                  exit(EXIT_SUCCESS);
> @@ -1072,6 +1059,14 @@ int main(int argc, char **argv)
>          }
>      }
> 
> +    if (pid_filepath == NULL) {
> +        pid_filepath = g_strdup(dfl_pathnames.pidfile);
> +    }
> +
> +    if (state_dir == NULL) {
> +        state_dir = g_strdup(dfl_pathnames.state_dir);
> +    }
> +
>  #ifdef _WIN32
>      /* On win32 the state directory is application specific (be it the default
>       * or a user override). We got past the command line parsing; let's create
> @@ -1214,5 +1209,15 @@ out_bad:
>      if (daemonize) {
>          unlink(pid_filepath);
>      }
> +
> +    g_free(method);
> +    g_free(log_filepath);
> +    g_free(pid_filepath);
> +    g_free(state_dir);
> +    g_free(channel_path);
> +#ifdef CONFIG_FSFREEZE
> +    g_free(fsfreeze_hook);
> +#endif
> +
>      return EXIT_FAILURE;
>  }
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 01/12] qga: misc spelling

[Marc-André Lureau]

Hi

On Wed, Aug 26, 2015 at 12:30 AM, Eric Blake <eblake@redhat.com> wrote:
> It's best to edit your commit message and manually add in that notation,
> when sending a v2, so that reviewers know that nothing changed since
> last time they approved it:
> http://wiki.qemu.org/Contribute/SubmitAPatch


Weird, I thought I did it (see other patches), anyway, I'll added them now.

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 06/12] qga: move option parsing to seperate function

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:18)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Move option parsing out of giant main().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/main.c | 165 +++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 96 insertions(+), 69 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 83b7804..a8dda38 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -941,19 +941,28 @@ static GList *split_list(gchar *str, const gchar separator)
>      return list;
>  }
> 
> -int main(int argc, char **argv)
> -{
> -    const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> -    char *method = NULL, *channel_path = NULL;
> -    char *log_filepath = NULL;
> -    char *pid_filepath = NULL;
> +typedef struct GAConfig {
> +    char *channel_path;
> +    char *method;
> +    char *log_filepath;
> +    char *pid_filepath;
>  #ifdef CONFIG_FSFREEZE
> -    char *fsfreeze_hook = NULL;
> +    char *fsfreeze_hook;
>  #endif
> -    char *state_dir = NULL;
> +    char *state_dir;
>  #ifdef _WIN32
> -    const char *service = NULL;
> +    const char *service;
>  #endif
> +    GList *blacklist;
> +    int daemonize;
> +    GLogLevelFlags log_level;
> +} GAConfig;
> +
> +static GAConfig *config_parse(int argc, char **argv)
> +{
> +    GAConfig *config = g_new0(GAConfig, 1);
> +    const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
> +    int opt_ind = 0, ch;
>      const struct option lopt[] = {
>          { "help", 0, NULL, 'h' },
>          { "version", 0, NULL, 'V' },
> @@ -973,74 +982,71 @@ int main(int argc, char **argv)
>          { "statedir", 1, NULL, 't' },
>          { NULL, 0, NULL, 0 }
>      };
> -    int opt_ind = 0, ch, daemonize = 0;
> -    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> -    GList *blacklist = NULL;
> -    GAState *s;
> 
> -    module_call_init(MODULE_INIT_QAPI);
> +    config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> 
> -    init_dfl_pathnames();
>      while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
>          switch (ch) {
>          case 'm':
> -            method = g_strdup(optarg);
> +            config->method = g_strdup(optarg);
>              break;
>          case 'p':
> -            channel_path = g_strdup(optarg);
> +            config->channel_path = g_strdup(optarg);
>              break;
>          case 'l':
> -            log_filepath = g_strdup(optarg);
> +            config->log_filepath = g_strdup(optarg);
>              break;
>          case 'f':
> -            pid_filepath = g_strdup(optarg);
> +            config->pid_filepath = g_strdup(optarg);
>              break;
>  #ifdef CONFIG_FSFREEZE
>          case 'F':
> -            fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT);
> +            config->fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT);
>              break;
>  #endif
>          case 't':
> -            state_dir = g_strdup(optarg);
> +            config->state_dir = g_strdup(optarg);
>              break;
>          case 'v':
>              /* enable all log levels */
> -            log_level = G_LOG_LEVEL_MASK;
> +            config->log_level = G_LOG_LEVEL_MASK;
>              break;
>          case 'V':
>              printf("QEMU Guest Agent %s\n", QEMU_VERSION);
>              exit(EXIT_SUCCESS);
>          case 'd':
> -            daemonize = 1;
> +            config->daemonize = 1;
>              break;
>          case 'b': {
>              if (is_help_option(optarg)) {
>                  qmp_for_each_command(ga_print_cmd, NULL);
>                  exit(EXIT_SUCCESS);
>              }
> -            blacklist = g_list_concat(blacklist, split_list(optarg, ','));
> +            config->blacklist = g_list_concat(config->blacklist,
> +                                             split_list(optarg, ','));
>              break;
>          }
>  #ifdef _WIN32
>          case 's':
> -            service = optarg;
> -            if (strcmp(service, "install") == 0) {
> +            config->service = optarg;
> +            if (strcmp(config->service, "install") == 0) {
>                  if (ga_install_vss_provider()) {
>                      exit(EXIT_FAILURE);
>                  }
> -                if (ga_install_service(channel_path, log_filepath, state_dir)) {
> +                if (ga_install_service(config->channel_path,
> +                                       config->log_filepath, config->state_dir)) {
>                      exit(EXIT_FAILURE);
>                  }
>                  exit(EXIT_SUCCESS);
> -            } else if (strcmp(service, "uninstall") == 0) {
> +            } else if (strcmp(config->service, "uninstall") == 0) {
>                  ga_uninstall_vss_provider();
>                  exit(ga_uninstall_service());
> -            } else if (strcmp(service, "vss-install") == 0) {
> +            } else if (strcmp(config->service, "vss-install") == 0) {
>                  if (ga_install_vss_provider()) {
>                      exit(EXIT_FAILURE);
>                  }
>                  exit(EXIT_SUCCESS);
> -            } else if (strcmp(service, "vss-uninstall") == 0) {
> +            } else if (strcmp(config->service, "vss-uninstall") == 0) {
>                  ga_uninstall_vss_provider();
>                  exit(EXIT_SUCCESS);
>              } else {
> @@ -1059,12 +1065,39 @@ int main(int argc, char **argv)
>          }
>      }
> 
> -    if (pid_filepath == NULL) {
> -        pid_filepath = g_strdup(dfl_pathnames.pidfile);
> +    return config;
> +}
> +
> +static void config_free(GAConfig *config)
> +{
> +    g_free(config->method);
> +    g_free(config->log_filepath);
> +    g_free(config->pid_filepath);
> +    g_free(config->state_dir);
> +    g_free(config->channel_path);
> +#ifdef CONFIG_FSFREEZE
> +    g_free(config->fsfreeze_hook);
> +#endif
> +    g_free(config);
> +}
> +
> + int main(int argc, char **argv)
> +{
> +    GAState *s;
> +    GAConfig *config;
> +
> +    module_call_init(MODULE_INIT_QAPI);
> +
> +    init_dfl_pathnames();
> +
> +    config = config_parse(argc, argv);
> +
> +    if (config->pid_filepath == NULL) {
> +        config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
>      }
> 
> -    if (state_dir == NULL) {
> -        state_dir = g_strdup(dfl_pathnames.state_dir);
> +    if (config->state_dir == NULL) {
> +        config->state_dir = g_strdup(dfl_pathnames.state_dir);
>      }
> 
>  #ifdef _WIN32
> @@ -1074,25 +1107,25 @@ int main(int argc, char **argv)
>       * error later on, we won't try to clean up the directory, it is considered
>       * persistent.
>       */
> -    if (g_mkdir_with_parents(state_dir, S_IRWXU) == -1) {
> +    if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
>          g_critical("unable to create (an ancestor of) the state directory"
> -                   " '%s': %s", state_dir, strerror(errno));
> +                   " '%s': %s", config->state_dir, strerror(errno));
>          return EXIT_FAILURE;
>      }
>  #endif
> 
>      s = g_malloc0(sizeof(GAState));
> -    s->log_level = log_level;
> +    s->log_level = config->log_level;
>      s->log_file = stderr;
>  #ifdef CONFIG_FSFREEZE
> -    s->fsfreeze_hook = fsfreeze_hook;
> +    s->fsfreeze_hook = config->fsfreeze_hook;
>  #endif
>      g_log_set_default_handler(ga_log, s);
>      g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>      ga_enable_logging(s);
>      s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> -                                                 state_dir);
> -    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
> +                                                 config->state_dir);
> +    s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir);
>      s->frozen = false;
> 
>  #ifndef _WIN32
> @@ -1125,23 +1158,23 @@ int main(int argc, char **argv)
>  #endif
> 
>      if (ga_is_frozen(s)) {
> -        if (daemonize) {
> +        if (config->daemonize) {
>              /* delay opening/locking of pidfile till filesystems are unfrozen */
> -            s->deferred_options.pid_filepath = pid_filepath;
> +            s->deferred_options.pid_filepath = config->pid_filepath;
>              become_daemon(NULL);
>          }
> -        if (log_filepath) {
> +        if (config->log_filepath) {
>              /* delay opening the log file till filesystems are unfrozen */
> -            s->deferred_options.log_filepath = log_filepath;
> +            s->deferred_options.log_filepath = config->log_filepath;
>          }
>          ga_disable_logging(s);
>          qmp_for_each_command(ga_disable_non_whitelisted, NULL);
>      } else {
> -        if (daemonize) {
> -            become_daemon(pid_filepath);
> +        if (config->daemonize) {
> +            become_daemon(config->pid_filepath);
>          }
> -        if (log_filepath) {
> -            FILE *log_file = ga_open_logfile(log_filepath);
> +        if (config->log_filepath) {
> +            FILE *log_file = ga_open_logfile(config->log_filepath);
>              if (!log_file) {
>                  g_critical("unable to open specified log file: %s",
>                             strerror(errno));
> @@ -1159,14 +1192,15 @@ int main(int argc, char **argv)
>          goto out_bad;
>      }
> 
> -    blacklist = ga_command_blacklist_init(blacklist);
> -    if (blacklist) {
> -        s->blacklist = blacklist;
> +    config->blacklist = ga_command_blacklist_init(config->blacklist);
> +    if (config->blacklist) {
> +        GList *l = config->blacklist;
> +        s->blacklist = config->blacklist;
>          do {
> -            g_debug("disabling command: %s", (char *)blacklist->data);
> -            qmp_disable_command(blacklist->data);
> -            blacklist = g_list_next(blacklist);
> -        } while (blacklist);
> +            g_debug("disabling command: %s", (char *)l->data);
> +            qmp_disable_command(l->data);
> +            l = g_list_next(l);
> +        } while (l);
>      }
>      s->command_state = ga_command_state_new();
>      ga_command_state_init(s, s->command_state);
> @@ -1181,14 +1215,14 @@ int main(int argc, char **argv)
>  #endif
> 
>      s->main_loop = g_main_loop_new(NULL, false);
> -    if (!channel_init(ga_state, method, channel_path)) {
> +    if (!channel_init(ga_state, config->method, config->channel_path)) {
>          g_critical("failed to initialize guest agent channel");
>          goto out_bad;
>      }
>  #ifndef _WIN32
>      g_main_loop_run(ga_state->main_loop);
>  #else
> -    if (daemonize) {
> +    if (config->daemonize) {
>          SERVICE_TABLE_ENTRY service_table[] = {
>              { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
>          StartServiceCtrlDispatcher(service_table);
> @@ -1200,24 +1234,17 @@ int main(int argc, char **argv)
>      ga_command_state_cleanup_all(ga_state->command_state);
>      ga_channel_free(ga_state->channel);
> 
> -    if (daemonize) {
> -        unlink(pid_filepath);
> +    if (config->daemonize) {
> +        unlink(config->pid_filepath);
>      }
>      return 0;
> 
>  out_bad:
> -    if (daemonize) {
> -        unlink(pid_filepath);
> +    if (config->daemonize) {
> +        unlink(config->pid_filepath);
>      }
> 
> -    g_free(method);
> -    g_free(log_filepath);
> -    g_free(pid_filepath);
> -    g_free(state_dir);
> -    g_free(channel_path);
> -#ifdef CONFIG_FSFREEZE
> -    g_free(fsfreeze_hook);
> -#endif
> +    config_free(config);
> 
>      return EXIT_FAILURE;
>  }
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 07/12] qga: fill default options in main()

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:19)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Fill all default options during main(). This is a preparation patch
> to allow to dump the configuration.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qga/main.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index a8dda38..979ce5c 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -658,23 +658,6 @@ static gboolean channel_init(GAState *s, const gchar *method, const gchar *path)
>  {
>      GAChannelMethod channel_method;
> 
> -    if (method == NULL) {
> -        method = "virtio-serial";
> -    }
> -
> -    if (path == NULL) {
> -        if (strcmp(method, "virtio-serial") == 0 ) {
> -            /* try the default path for the virtio-serial port */
> -            path = QGA_VIRTIO_PATH_DEFAULT;
> -        } else if (strcmp(method, "isa-serial") == 0){
> -            /* try the default path for the serial port - COM1 */
> -            path = QGA_SERIAL_PATH_DEFAULT;
> -        } else {
> -            g_critical("must specify a path for this channel");
> -            return false;
> -        }
> -    }
> -
>      if (strcmp(method, "virtio-serial") == 0) {
>          s->virtio = true; /* virtio requires special handling in some cases */
>          channel_method = GA_CHANNEL_VIRTIO_SERIAL;
> @@ -1081,7 +1064,7 @@ static void config_free(GAConfig *config)
>      g_free(config);
>  }
> 
> - int main(int argc, char **argv)
> +int main(int argc, char **argv)
>  {
>      GAState *s;
>      GAConfig *config;
> @@ -1100,6 +1083,23 @@ static void config_free(GAConfig *config)
>          config->state_dir = g_strdup(dfl_pathnames.state_dir);
>      }
> 
> +    if (method == NULL) {
> +        method = g_strdup("virtio-serial");
> +    }
> +
> +    if (device_path == NULL) {
> +        if (strcmp(method, "virtio-serial") == 0) {
> +            /* try the default path for the virtio-serial port */
> +            device_path = g_strdup(QGA_VIRTIO_PATH_DEFAULT);
> +        } else if (strcmp(method, "isa-serial") == 0) {
> +            /* try the default path for the serial port - COM1 */
> +            device_path = g_strdup(QGA_SERIAL_PATH_DEFAULT);
> +        } else {
> +            g_critical("must specify a path for this channel");
> +            goto out_bad;
> +        }
> +    }
> +

I think this needs to be config->method and config->device_path. It gets
fixed in the next patch, but looks like it'll break build bisect.

Since that requires a respin anyway: there's an extra space in
'int main()' that gets added in a prior patch then removed here, so
might as well fix that up while we're there.

>  #ifdef _WIN32
>      /* On win32 the state directory is application specific (be it the default
>       * or a user override). We got past the command line parsing; let's create
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 08/12] qga: move agent run in a seperate function

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:20)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Once the options are populated, move the running state to
> a run_agent() function.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/main.c | 164 +++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 89 insertions(+), 75 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 979ce5c..118847c 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1064,70 +1064,8 @@ static void config_free(GAConfig *config)
>      g_free(config);
>  }
> 
> -int main(int argc, char **argv)
> +static bool check_is_frozen(GAState *s)
>  {
> -    GAState *s;
> -    GAConfig *config;
> -
> -    module_call_init(MODULE_INIT_QAPI);
> -
> -    init_dfl_pathnames();
> -
> -    config = config_parse(argc, argv);
> -
> -    if (config->pid_filepath == NULL) {
> -        config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
> -    }
> -
> -    if (config->state_dir == NULL) {
> -        config->state_dir = g_strdup(dfl_pathnames.state_dir);
> -    }
> -
> -    if (method == NULL) {
> -        method = g_strdup("virtio-serial");
> -    }
> -
> -    if (device_path == NULL) {
> -        if (strcmp(method, "virtio-serial") == 0) {
> -            /* try the default path for the virtio-serial port */
> -            device_path = g_strdup(QGA_VIRTIO_PATH_DEFAULT);
> -        } else if (strcmp(method, "isa-serial") == 0) {
> -            /* try the default path for the serial port - COM1 */
> -            device_path = g_strdup(QGA_SERIAL_PATH_DEFAULT);
> -        } else {
> -            g_critical("must specify a path for this channel");
> -            goto out_bad;
> -        }
> -    }
> -
> -#ifdef _WIN32
> -    /* On win32 the state directory is application specific (be it the default
> -     * or a user override). We got past the command line parsing; let's create
> -     * the directory (with any intermediate directories). If we run into an
> -     * error later on, we won't try to clean up the directory, it is considered
> -     * persistent.
> -     */
> -    if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
> -        g_critical("unable to create (an ancestor of) the state directory"
> -                   " '%s': %s", config->state_dir, strerror(errno));
> -        return EXIT_FAILURE;
> -    }
> -#endif
> -
> -    s = g_malloc0(sizeof(GAState));
> -    s->log_level = config->log_level;
> -    s->log_file = stderr;
> -#ifdef CONFIG_FSFREEZE
> -    s->fsfreeze_hook = config->fsfreeze_hook;
> -#endif
> -    g_log_set_default_handler(ga_log, s);
> -    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> -    ga_enable_logging(s);
> -    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> -                                                 config->state_dir);
> -    s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir);
> -    s->frozen = false;
> -
>  #ifndef _WIN32
>      /* check if a previous instance of qemu-ga exited with filesystems' state
>       * marked as frozen. this could be a stale value (a non-qemu-ga process
> @@ -1153,7 +1091,31 @@ int main(int argc, char **argv)
>                    " guest-fsfreeze-thaw is issued, or filesystems are"
>                    " manually unfrozen and the file %s is removed",
>                    s->state_filepath_isfrozen);
> -        s->frozen = true;
> +        return true;
> +    }
> +#endif
> +    return false;
> +}
> +
> +static int run_agent(GAState *s, GAConfig *config)
> +{
> +    ga_state = s;
> +
> +    g_log_set_default_handler(ga_log, s);
> +    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> +    ga_enable_logging(s);
> +
> +#ifdef _WIN32
> +    /* On win32 the state directory is application specific (be it the default
> +     * or a user override). We got past the command line parsing; let's create
> +     * the directory (with any intermediate directories). If we run into an
> +     * error later on, we won't try to clean up the directory, it is considered
> +     * persistent.
> +     */
> +    if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
> +        g_critical("unable to create (an ancestor of) the state directory"
> +                   " '%s': %s", config->state_dir, strerror(errno));
> +        return EXIT_FAILURE;
>      }
>  #endif
> 
> @@ -1178,7 +1140,7 @@ int main(int argc, char **argv)
>              if (!log_file) {
>                  g_critical("unable to open specified log file: %s",
>                             strerror(errno));
> -                goto out_bad;
> +                return EXIT_FAILURE;
>              }
>              s->log_file = log_file;
>          }
> @@ -1189,7 +1151,7 @@ int main(int argc, char **argv)
>                                 s->pstate_filepath,
>                                 ga_is_frozen(s))) {
>          g_critical("failed to load persistent state");
> -        goto out_bad;
> +        return EXIT_FAILURE;
>      }
> 
>      config->blacklist = ga_command_blacklist_init(config->blacklist);
> @@ -1210,14 +1172,14 @@ int main(int argc, char **argv)
>  #ifndef _WIN32
>      if (!register_signal_handlers()) {
>          g_critical("failed to register signal handlers");
> -        goto out_bad;
> +        return EXIT_FAILURE;
>      }
>  #endif
> 
>      s->main_loop = g_main_loop_new(NULL, false);
>      if (!channel_init(ga_state, config->method, config->channel_path)) {
>          g_critical("failed to initialize guest agent channel");
> -        goto out_bad;
> +        return EXIT_FAILURE;
>      }
>  #ifndef _WIN32
>      g_main_loop_run(ga_state->main_loop);
> @@ -1231,20 +1193,72 @@ int main(int argc, char **argv)
>      }
>  #endif
> 
> -    ga_command_state_cleanup_all(ga_state->command_state);
> -    ga_channel_free(ga_state->channel);
> +    return EXIT_SUCCESS;
> +}
> 
> -    if (config->daemonize) {
> -        unlink(config->pid_filepath);
> +int main(int argc, char **argv)
> +{
> +    int ret = EXIT_SUCCESS;
> +    GAState *s = g_new0(GAState, 1);
> +    GAConfig *config;
> +
> +    module_call_init(MODULE_INIT_QAPI);
> +
> +    init_dfl_pathnames();
> +
> +    config = config_parse(argc, argv);
> +
> +    if (config->pid_filepath == NULL) {
> +        config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
> +    }
> +
> +    if (config->state_dir == NULL) {
> +        config->state_dir = g_strdup(dfl_pathnames.state_dir);
> +    }
> +
> +    if (config->method == NULL) {
> +        config->method = g_strdup("virtio-serial");
> +    }
> +
> +    if (config->channel_path == NULL) {
> +        if (strcmp(config->method, "virtio-serial") == 0) {
> +            /* try the default path for the virtio-serial port */
> +            config->channel_path = g_strdup(QGA_VIRTIO_PATH_DEFAULT);
> +        } else if (strcmp(config->method, "isa-serial") == 0) {
> +            /* try the default path for the serial port - COM1 */
> +            config->channel_path = g_strdup(QGA_SERIAL_PATH_DEFAULT);
> +        } else {
> +            g_critical("must specify a path for this channel");
> +            ret = EXIT_FAILURE;
> +            goto end;
> +        }
> +    }
> +
> +    s->log_level = config->log_level;
> +    s->log_file = stderr;
> +#ifdef CONFIG_FSFREEZE
> +    s->fsfreeze_hook = config->fsfreeze_hook;
> +#endif
> +    s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir);
> +    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> +                                                 config->state_dir);
> +    s->frozen = check_is_frozen(s);
> +
> +    ret = run_agent(s, config);
> +
> +end:
> +    if (s->command_state) {
> +        ga_command_state_cleanup_all(s->command_state);
> +    }
> +    if (s->channel) {
> +        ga_channel_free(s->channel);
>      }
> -    return 0;
> 
> -out_bad:
>      if (config->daemonize) {
>          unlink(config->pid_filepath);
>      }
> 
>      config_free(config);
> 
> -    return EXIT_FAILURE;
> +    return ret;
>  }
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 07/12] qga: fill default options in main()

[Marc-André Lureau]

Hi

On Wed, Aug 26, 2015 at 12:46 AM, Michael Roth
<mdroth@linux.vnet.ibm.com> wrote:
> I think this needs to be config->method and config->device_path. It gets
> fixed in the next patch, but looks like it'll break build bisect.

indeed

> Since that requires a respin anyway: there's an extra space in
> 'int main()' that gets added in a prior patch then removed here, so
> might as well fix that up while we're there.

fixed

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 10/12] qga: add an optionnal qemu-ga.conf system configuration

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:22)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Learn to configure the agent with a system configuration.
> 
> This may simplify command-line handling, especially when the blacklist
> is long.
> 
> Among the other benefits, this may standardize the configuration of a
> init service (instead of distro-specific init keys/files)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 73 insertions(+), 7 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 58f2fc7..154975c 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -56,6 +56,7 @@
>  #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
>  #endif
>  #define QGA_SENTINEL_BYTE 0xFF
> +#define QGA_CONF_DEFAULT CONFIG_QEMU_CONFDIR G_DIR_SEPARATOR_S "qemu-ga.conf"

We should probably do this in init_dfl_pathnames() for consistency.

> 
>  static struct {
>      const char *state_dir;
> @@ -936,14 +937,80 @@ typedef struct GAConfig {
>  #ifdef _WIN32
>      const char *service;
>  #endif
> +    gchar *bliststr;
>      GList *blacklist;
>      int daemonize;
>      GLogLevelFlags log_level;
>  } GAConfig;
> 
> -static GAConfig *config_parse(int argc, char **argv)
> +static void config_load(GAConfig *config)
> +{
> +    GError *gerr = NULL;
> +    GKeyFile *keyfile;
> +
> +    /* read system config */
> +    keyfile = g_key_file_new();
> +    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
> +        goto end;
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
> +        config->daemonize =
> +            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
> +        config->method =
> +            g_key_file_get_string(keyfile, "general", "method", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
> +        config->channel_path =
> +            g_key_file_get_string(keyfile, "general", "path", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
> +        config->log_filepath =
> +            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
> +        config->pid_filepath =
> +            g_key_file_get_string(keyfile, "general", "pidfile", &gerr);
> +    }
> +#ifdef CONFIG_FSFREEZE
> +    if (g_key_file_has_key(keyfile, "general", "fsfreeze-hook", NULL)) {
> +        config->fsfreeze_hook =
> +            g_key_file_get_string(keyfile,
> +                                  "general", "fsfreeze-hook", &gerr);
> +    }
> +#endif
> +    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
> +        config->state_dir =
> +            g_key_file_get_string(keyfile, "general", "statedir", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "verbose", NULL) &&
> +        g_key_file_get_boolean(keyfile, "general", "verbose", &gerr)) {
> +        /* enable all log levels */
> +        config->log_level = G_LOG_LEVEL_MASK;
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
> +        config->bliststr =
> +            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
> +        config->blacklist = g_list_concat(config->blacklist,
> +                                          split_list(config->bliststr, ','));
> +    }
> +
> +end:
> +    if (keyfile) {
> +        g_key_file_free(keyfile);
> +    }
> +    if (gerr &&
> +        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
> +        g_critical("error loading configuration from path: %s, %s",
> +                   QGA_CONF_DEFAULT, gerr->message);
> +        exit(EXIT_FAILURE);
> +    }
> +    g_clear_error(&gerr);

What about other file errors, like permission issues? Maybe just have
config_load() return bool, and just spit out stringified gerr prior to
return false?

Looks good otherwise.

> +}
> +
> +static void config_parse(GAConfig *config, int argc, char **argv)
>  {
> -    GAConfig *config = g_new0(GAConfig, 1);
>      const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
>      int opt_ind = 0, ch;
>      const struct option lopt[] = {
> @@ -1047,8 +1114,6 @@ static GAConfig *config_parse(int argc, char **argv)
>              exit(EXIT_FAILURE);
>          }
>      }
> -
> -    return config;
>  }
> 
>  static void config_free(GAConfig *config)
> @@ -1058,6 +1123,7 @@ static void config_free(GAConfig *config)
>      g_free(config->pid_filepath);
>      g_free(config->state_dir);
>      g_free(config->channel_path);
> +    g_free(config->bliststr);
>  #ifdef CONFIG_FSFREEZE
>      g_free(config->fsfreeze_hook);
>  #endif
> @@ -1200,13 +1266,13 @@ int main(int argc, char **argv)
>  {
>      int ret = EXIT_SUCCESS;
>      GAState *s = g_new0(GAState, 1);
> -    GAConfig *config;
> +    GAConfig *config = g_new0(GAConfig, 1);
> 
>      module_call_init(MODULE_INIT_QAPI);
> 
>      init_dfl_pathnames();
> -
> -    config = config_parse(argc, argv);
> +    config_load(config);
> +    config_parse(config, argc, argv);
> 
>      if (config->pid_filepath == NULL) {
>          config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 11/12] qga: add --dump-conf option

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:23)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This new option allows to review the agent configuration,
> and ease the task of writing a configuration file.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/main.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 154975c..a47310b 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -941,6 +941,7 @@ typedef struct GAConfig {
>      GList *blacklist;
>      int daemonize;
>      GLogLevelFlags log_level;
> +    int dumpconf;
>  } GAConfig;
> 
>  static void config_load(GAConfig *config)
> @@ -1009,6 +1010,58 @@ end:
>      g_clear_error(&gerr);
>  }
> 
> +static gchar *list_join(GList *list, const gchar separator)
> +{
> +    GString *str = g_string_new("");
> +
> +    while (list) {
> +        str = g_string_append(str, (gchar *)list->data);
> +        list = g_list_next(list);
> +        if (list) {
> +            str = g_string_append_c(str, separator);
> +        }
> +    }
> +
> +    return g_string_free(str, FALSE);
> +}
> +
> +static void config_dump(GAConfig *config)
> +{
> +    GError *error = NULL;
> +    GKeyFile *keyfile;
> +    gchar *tmp;
> +
> +    keyfile = g_key_file_new();
> +    g_key_file_set_boolean(keyfile, "general", "daemon", config->daemonize);
> +    g_key_file_set_string(keyfile, "general", "method", config->method);
> +    g_key_file_set_string(keyfile, "general", "path", config->channel_path);
> +    if (config->log_filepath) {
> +        g_key_file_set_string(keyfile, "general", "logfile",
> +                              config->log_filepath);
> +    }
> +    g_key_file_set_string(keyfile, "general", "pidfile", config->pid_filepath);
> +#ifdef CONFIG_FSFREEZE
> +    if (config->fsfreeze_hook) {
> +        g_key_file_set_string(keyfile, "general", "fsfreeze-hook",
> +                              config->fsfreeze_hook);
> +    }
> +#endif
> +    g_key_file_set_string(keyfile, "general", "statedir", config->state_dir);
> +    g_key_file_set_boolean(keyfile, "general", "verbose",
> +                           config->log_level == G_LOG_LEVEL_MASK);
> +    tmp = list_join(config->blacklist, ',');

Since we have the list_join() helper to convert config->blacklist, it
makes me wonder if there's really any need to add/init config->bliststr
in the previous patch.

This patch looks good though:


Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> +    g_key_file_set_string(keyfile, "general", "blacklist", tmp);
> +    g_free(tmp);
> +
> +    tmp = g_key_file_to_data(keyfile, NULL, &error);
> +    printf("%s", tmp);
> +
> +    g_free(tmp);
> +    if (keyfile) {
> +        g_key_file_free(keyfile);
> +    }
> +}
> +
>  static void config_parse(GAConfig *config, int argc, char **argv)
>  {
>      const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
> @@ -1016,6 +1069,7 @@ static void config_parse(GAConfig *config, int argc, char **argv)
>      const struct option lopt[] = {
>          { "help", 0, NULL, 'h' },
>          { "version", 0, NULL, 'V' },
> +        { "dump-conf", 0, NULL, 'D' },
>          { "logfile", 1, NULL, 'l' },
>          { "pidfile", 1, NULL, 'f' },
>  #ifdef CONFIG_FSFREEZE
> @@ -1067,6 +1121,9 @@ static void config_parse(GAConfig *config, int argc, char **argv)
>          case 'd':
>              config->daemonize = 1;
>              break;
> +        case 'D':
> +            config->dumpconf = 1;
> +            break;
>          case 'b': {
>              if (is_help_option(optarg)) {
>                  qmp_for_each_command(ga_print_cmd, NULL);
> @@ -1310,6 +1367,11 @@ int main(int argc, char **argv)
>                                                   config->state_dir);
>      s->frozen = check_is_frozen(s);
> 
> +    if (config->dumpconf) {
> +        config_dump(config);
> +        goto end;
> +    }
> +
>      ret = run_agent(s, config);
> 
>  end:
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 12/12] qga: start a man page

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:24)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add a simple man page for the qemu agent.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  Makefile      |  14 +++++-
>  qemu-doc.texi |   6 +++
>  qemu-ga.texi  | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 154 insertions(+), 2 deletions(-)
>  create mode 100644 qemu-ga.texi
> 
> diff --git a/Makefile b/Makefile
> index 340d9c8..67d44b8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -88,7 +88,8 @@ LIBS+=-lz $(LIBS_TOOLS)
>  HELPERS-$(CONFIG_LINUX) = qemu-bridge-helper$(EXESUF)
> 
>  ifdef BUILD_DOCS
> -DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8 qmp-commands.txt
> +DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8 qemu-ga.8
> +DOCS+=qmp-commands.txt
>  ifdef CONFIG_LINUX
>  DOCS+=kvm_stat.1
>  endif
> @@ -400,6 +401,9 @@ ifneq ($(TOOLS),)
>         $(INSTALL_DIR) "$(DESTDIR)$(mandir)/man8"
>         $(INSTALL_DATA) qemu-nbd.8 "$(DESTDIR)$(mandir)/man8"
>  endif
> +ifneq (,$(findstring qemu-ga,$(TOOLS)))
> +       $(INSTALL_DATA) qemu-ga.8 "$(DESTDIR)$(mandir)/man8"
> +endif
>  endif
>  ifdef CONFIG_VIRTFS
>         $(INSTALL_DIR) "$(DESTDIR)$(mandir)/man1"
> @@ -538,6 +542,12 @@ qemu-nbd.8: qemu-nbd.texi
>           $(POD2MAN) --section=8 --center=" " --release=" " qemu-nbd.pod > $@, \
>           "  GEN   $@")
> 
> +qemu-ga.8: qemu-ga.texi
> +       $(call quiet-command, \
> +         perl -Ww -- $(SRC_PATH)/scripts/texi2pod.pl $< qemu-ga.pod && \
> +         $(POD2MAN) --section=8 --center=" " --release=" " qemu-ga.pod > $@, \
> +         "  GEN   $@")
> +
>  kvm_stat.1: scripts/kvm/kvm_stat.texi
>         $(call quiet-command, \
>           perl -Ww -- $(SRC_PATH)/scripts/texi2pod.pl $< kvm_stat.pod && \
> @@ -551,7 +561,7 @@ pdf: qemu-doc.pdf qemu-tech.pdf
> 
>  qemu-doc.dvi qemu-doc.html qemu-doc.info qemu-doc.pdf: \
>         qemu-img.texi qemu-nbd.texi qemu-options.texi \
> -       qemu-monitor.texi qemu-img-cmds.texi
> +       qemu-monitor.texi qemu-img-cmds.texi qemu-ga.texi
> 
>  ifdef CONFIG_WIN32
> 
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 94af8c0..84d17d1 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -412,6 +412,7 @@ snapshots.
>  * vm_snapshots::              VM snapshots
>  * qemu_img_invocation::       qemu-img Invocation
>  * qemu_nbd_invocation::       qemu-nbd Invocation
> +* qemu_ga_invocation::        qemu-ga Invocation
>  * disk_images_formats::       Disk image file formats
>  * host_drives::               Using host drives
>  * disk_images_fat_images::    Virtual FAT disk images
> @@ -505,6 +506,11 @@ state is not saved or restored properly (in particular USB).
> 
>  @include qemu-nbd.texi
> 
> +@node qemu_ga_invocation
> +@subsection @code{qemu-ga} Invocation
> +
> +@include qemu-ga.texi
> +
>  @node disk_images_formats
>  @subsection Disk image file formats
> 
> diff --git a/qemu-ga.texi b/qemu-ga.texi
> new file mode 100644
> index 0000000..7d4a628
> --- /dev/null
> +++ b/qemu-ga.texi
> @@ -0,0 +1,136 @@
> +@example
> +@c man begin SYNOPSIS
> +usage: qemu-ga [-m <method> -p <path>] [OPTION]...
> +@c man end
> +@end example
> +
> +@c man begin DESCRIPTION
> +
> +The QEMU Guest Agent is a deamon that allows the host to perform
> +various operations in the guest, such as:
> +
> +@itemize
> +@item
> +get information from the guest
> +@item
> +set the guest's system time
> +@item
> +read/write a file
> +@item
> +sync and freeze the filesystems
> +@item
> +suspend the guest
> +@item
> +reconfigure guest local processors
> +@item
> +set user's password
> +@item
> +...
> +@end itemize
> +
> +qemu-ga will read a system configuration file on startup (located at
> +q@file{/etc/qemu/qemu-ga.conf} by default), then parse remaining
> +configuration options on the command line. For the same key, the last
> +option wins, but the lists accumulate (see below for configuration
> +file format).
> +
> +@c man end
> +
> +@c man begin OPTIONS
> +@table @option
> +@item -m, --method=@var{method}
> +  Transport method: one of @samp{unix-listen}, @samp{virtio-serial}, or
> +  @samp{isa-serial} (@samp{virtio-serial} is the default).
> +
> +@item -p, --path=@var{path}
> +  Device/socket path (the default for virtio-serial is:
> +  @samp{/dev/virtio-ports/org.qemu.guest_agent.0},
> +  the default for isa-serial is: @samp{/dev/ttyS0})
> +
> +@item -l, --logfile=@var{path}
> +  Set log file path, logs to stderr by default.
> +
> +@item -f, --pidfile=@var{path}
> +  Specify pid file (default is @samp{/var/run/qemu-ga.pid}).
> +
> +@item -F, --fsfreeze-hook=@var{path}
> +  Enable fsfreeze hook. Accepts an optional argument that specifies
> +  script to run on freeze/thaw. Script will be called with
> +  'freeze'/'thaw' arguments accordingly.  (default is
> +  @samp{/etc/qemu/fsfreeze-hook}) If using -F with an argument, do
> +  not follow -F with a space. (for example:
> +  @samp{-F/var/run/fsfreezehook.sh})
> +
> +@item -t, --statedir=@var{path}
> +  Specify the directory to store state information (absolute paths only,
> +  default is @samp{/var/run}).
> +
> +@item -v, --verbose
> +  Log extra debugging information.
> +
> +@item -V, --version
> +  Print version information and exit.
> +
> +@item -d, --daemon
> +  Daemonize after startup (detach from terminal).
> +
> +@item -b, --blacklist=@var{list}
> +  Comma-separated list of RPCs to disable (no spaces, @samp{?} to list
> +  available RPCs).
> +
> +@item -D, --dump-conf
> +  Dump the configuration in a format compatible with @file{qemu-ga.conf}
> +  and exit.
> +
> +@item -h, --help
> +  Display this help and exit.
> +@end table
> +
> +@c man end
> +
> +@c man begin FILES
> +
> +The syntax of the @file{qemu-ga.conf} configuration file follows the
> +Desktop Entry Specification, here is a quick summary: it consists of
> +groups of key-value pairs, interspersed with comments.
> +
> +@example
> +# qemu-ga configuration sample
> +[general]
> +daemonize = 0
> +pidfile = /var/run/qemu-ga.pid
> +verbose = 0
> +method = virtio-serial
> +path = /dev/virtio-ports/org.qemu.guest_agent.0
> +statedir = /var/run
> +@end example
> +
> +The list of keys follows the command line options:
> +@table @option
> +@item daemon= boolean
> +@item method= string
> +@item path= string
> +@item logfile= string
> +@item pidfile= string
> +@item fsfreeze-hook= string
> +@item statedir= string
> +@item verbose= boolean
> +@item blacklist= string list
> +@end table
> +
> +@c man end
> +
> +@ignore
> +
> +@setfilename qemu-ga
> +@settitle QEMU Guest Agent
> +
> +@c man begin AUTHOR
> +Michael Roth <mdroth@linux.vnet.ibm.com>
> +@c man end
> +
> +@c man begin SEEALSO
> +qemu(1)
> +@c man end
> +
> +@end ignore
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v2 10/12] qga: add an optionnal qemu-ga.conf system configuration

[Marc-André Lureau]

Hi

On Wed, Aug 26, 2015 at 1:09 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> We should probably do this in init_dfl_pathnames() for consistency.

QGA_FSFREEZE_HOOK_DEFAULT is also initialized here

in init_dfl_pathnames(), it depends on the value of
get_local_state_pathname() while here it's static.

>>
>>  static struct {
>>      const char *state_dir;
>> @@ -936,14 +937,80 @@ typedef struct GAConfig {
>>  #ifdef _WIN32
>>      const char *service;
>>  #endif
>> +    gchar *bliststr;
>>      GList *blacklist;
>>      int daemonize;
>>      GLogLevelFlags log_level;
>>  } GAConfig;
>>
>> -static GAConfig *config_parse(int argc, char **argv)
>> +static void config_load(GAConfig *config)
>> +{
>> +    GError *gerr = NULL;
>> +    GKeyFile *keyfile;
>> +
>> +    /* read system config */
>> +    keyfile = g_key_file_new();
>> +    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
>> +        goto end;
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
>> +        config->daemonize =
>> +            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
>> +        config->method =
>> +            g_key_file_get_string(keyfile, "general", "method", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
>> +        config->channel_path =
>> +            g_key_file_get_string(keyfile, "general", "path", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
>> +        config->log_filepath =
>> +            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
>> +        config->pid_filepath =
>> +            g_key_file_get_string(keyfile, "general", "pidfile", &gerr);
>> +    }
>> +#ifdef CONFIG_FSFREEZE
>> +    if (g_key_file_has_key(keyfile, "general", "fsfreeze-hook", NULL)) {
>> +        config->fsfreeze_hook =
>> +            g_key_file_get_string(keyfile,
>> +                                  "general", "fsfreeze-hook", &gerr);
>> +    }
>> +#endif
>> +    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
>> +        config->state_dir =
>> +            g_key_file_get_string(keyfile, "general", "statedir", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "verbose", NULL) &&
>> +        g_key_file_get_boolean(keyfile, "general", "verbose", &gerr)) {
>> +        /* enable all log levels */
>> +        config->log_level = G_LOG_LEVEL_MASK;
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
>> +        config->bliststr =
>> +            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
>> +        config->blacklist = g_list_concat(config->blacklist,
>> +                                          split_list(config->bliststr, ','));
>> +    }
>> +
>> +end:
>> +    if (keyfile) {
>> +        g_key_file_free(keyfile);
>> +    }
>> +    if (gerr &&
>> +        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
>> +        g_critical("error loading configuration from path: %s, %s",
>> +                   QGA_CONF_DEFAULT, gerr->message);
>> +        exit(EXIT_FAILURE);
>> +    }
>> +    g_clear_error(&gerr);
>
> What about other file errors, like permission issues? Maybe just have
> config_load() return bool, and just spit out stringified gerr prior to
> return false?

all errors have a g_critical(), except for the case of file missing.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 10/12] qga: add an optionnal qemu-ga.conf system configuration

[Michael Roth]

Quoting Marc-André Lureau (2015-08-25 18:18:16)
> Hi
> 
> On Wed, Aug 26, 2015 at 1:09 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > We should probably do this in init_dfl_pathnames() for consistency.
> 
> QGA_FSFREEZE_HOOK_DEFAULT is also initialized here
> 
> in init_dfl_pathnames(), it depends on the value of
> get_local_state_pathname() while here it's static.

Ahh, yah I missed that. It's fine where it's at then.

> 
> >>
> >>  static struct {
> >>      const char *state_dir;
> >> @@ -936,14 +937,80 @@ typedef struct GAConfig {
> >>  #ifdef _WIN32
> >>      const char *service;
> >>  #endif
> >> +    gchar *bliststr;
> >>      GList *blacklist;
> >>      int daemonize;
> >>      GLogLevelFlags log_level;
> >>  } GAConfig;
> >>
> >> -static GAConfig *config_parse(int argc, char **argv)
> >> +static void config_load(GAConfig *config)
> >> +{
> >> +    GError *gerr = NULL;
> >> +    GKeyFile *keyfile;
> >> +
> >> +    /* read system config */
> >> +    keyfile = g_key_file_new();
> >> +    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
> >> +        goto end;
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
> >> +        config->daemonize =
> >> +            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
> >> +        config->method =
> >> +            g_key_file_get_string(keyfile, "general", "method", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
> >> +        config->channel_path =
> >> +            g_key_file_get_string(keyfile, "general", "path", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
> >> +        config->log_filepath =
> >> +            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
> >> +        config->pid_filepath =
> >> +            g_key_file_get_string(keyfile, "general", "pidfile", &gerr);
> >> +    }
> >> +#ifdef CONFIG_FSFREEZE
> >> +    if (g_key_file_has_key(keyfile, "general", "fsfreeze-hook", NULL)) {
> >> +        config->fsfreeze_hook =
> >> +            g_key_file_get_string(keyfile,
> >> +                                  "general", "fsfreeze-hook", &gerr);
> >> +    }
> >> +#endif
> >> +    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
> >> +        config->state_dir =
> >> +            g_key_file_get_string(keyfile, "general", "statedir", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "verbose", NULL) &&
> >> +        g_key_file_get_boolean(keyfile, "general", "verbose", &gerr)) {
> >> +        /* enable all log levels */
> >> +        config->log_level = G_LOG_LEVEL_MASK;
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
> >> +        config->bliststr =
> >> +            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
> >> +        config->blacklist = g_list_concat(config->blacklist,
> >> +                                          split_list(config->bliststr, ','));
> >> +    }
> >> +
> >> +end:
> >> +    if (keyfile) {
> >> +        g_key_file_free(keyfile);
> >> +    }
> >> +    if (gerr &&
> >> +        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
> >> +        g_critical("error loading configuration from path: %s, %s",
> >> +                   QGA_CONF_DEFAULT, gerr->message);
> >> +        exit(EXIT_FAILURE);
> >> +    }
> >> +    g_clear_error(&gerr);
> >
> > What about other file errors, like permission issues? Maybe just have
> > config_load() return bool, and just spit out stringified gerr prior to
> > return false?
> 
> all errors have a g_critical(), except for the case of file missing.

Argh, missed the '!'. Looks good then:

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> 
> 
> -- 
> Marc-André Lureau
> 

Re: [Qemu-devel] [PATCH v2 11/12] qga: add --dump-conf option

[Marc-André Lureau]

Hi

On Wed, Aug 26, 2015 at 1:13 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Since we have the list_join() helper to convert config->blacklist, it
> makes me wonder if there's really any need to add/init config->bliststr
> in the previous patch.

It is just to keep the string in memory. split_list() doesn't dup the
given string, it merely changes it and add pointers to the list.

I'll add a comment about it in the code.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 10/12] qga: add an optionnal qemu-ga.conf system configuration

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Learn to configure the agent with a system configuration.
>
> This may simplify command-line handling, especially when the blacklist
> is long.
>
> Among the other benefits, this may standardize the configuration of a
> init service (instead of distro-specific init keys/files)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

This uses GLib's Key-value file parser.  Note that we have our own .ini
parser qemu_config_parse().  It predates our use of GLib.  Having two
different parsers risks inconsistency.  Since qga is already using
GLib's, using it some more there is better than adding a use of our own
parser, so no objection to your patch on that ground.

Replacing qemu_config_parse()'s parsing guts by GLib probably won't save
code, but it could be nice for consistency.  Well outside this patch's
scope, of course.