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

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

[Marc-André Lureau]

Hi,

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

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 seperate function
  qga: rename 'path' to 'device_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 --dump-conf option
  qga: add an optionnal qemu-ga.conf system configuration
  qga: start a man page

 Makefile             |  14 +-
 qemu-doc.texi        |   6 +
 qemu-ga.texi         | 135 +++++++++++++++++++
 qga/main.c           | 357 ++++++++++++++++++++++++++++++++++++---------------
 qga/qapi-schema.json |   2 +-
 5 files changed, 409 insertions(+), 105 deletions(-)
 create mode 100644 qemu-ga.texi

-- 
2.4.3

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

[Marc-André Lureau]

---
 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 b446dc7..fbf983c 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -755,7 +755,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 02/12] qga: use exit() when parsing options

[Marc-André Lureau]

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

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

diff --git a/qga/main.c b/qga/main.c
index 23cde01..af93992 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 03/12] qga: move string split in seperate function

[Marc-André Lureau]

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

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

diff --git a/qga/main.c b/qga/main.c
index af93992..0c455f8 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 04/12] qga: rename 'path' to 'device_path'

[Marc-André Lureau]

'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@gmail.com>
---
 qga/main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 0c455f8..1c81575 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, *device_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;
+            device_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(device_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, device_path)) {
         g_critical("failed to initialize guest agent channel");
         goto out_bad;
     }
-- 
2.4.3

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

[Marc-André Lureau]

A following patch will return allocated string.

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

diff --git a/qga/main.c b/qga/main.c
index 1c81575..b776d16 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, *device_path = NULL;
-    const char *log_filepath = NULL;
-    const char *pid_filepath;
+    char *method = NULL, *device_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':
-            device_path = optarg;
+            device_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(device_path, log_filepath,
-                                       fixed_state_dir)) {
+                if (ga_install_service(device_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(device_path);
+#ifdef CONFIG_FSFREEZE
+    g_free(fsfreeze_hook);
+#endif
+
     return EXIT_FAILURE;
 }
-- 
2.4.3

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

[Marc-André Lureau]

Move option parsing out of giant main().

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

diff --git a/qga/main.c b/qga/main.c
index b776d16..b965f61 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -941,19 +941,25 @@ 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, *device_path = NULL;
-    char *log_filepath = NULL;
-    char *pid_filepath = NULL;
+static char *device_path;
+static char *method;
+static char *log_filepath;
+static char *pid_filepath;
 #ifdef CONFIG_FSFREEZE
-    char *fsfreeze_hook = NULL;
+static char *fsfreeze_hook;
 #endif
-    char *state_dir = NULL;
+static char *state_dir;
 #ifdef _WIN32
-    const char *service = NULL;
+static const char *service;
 #endif
+static GList *blacklist;
+static int daemonize;
+static GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
+
+static void option_parse(int argc, char **argv)
+{
+    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,14 +979,7 @@ 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);
-
-    init_dfl_pathnames();
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
         switch (ch) {
         case 'm':
@@ -1058,6 +1057,16 @@ int main(int argc, char **argv)
             exit(EXIT_FAILURE);
         }
     }
+}
+
+int main(int argc, char **argv)
+{
+    GAState *s;
+
+    module_call_init(MODULE_INIT_QAPI);
+
+    init_dfl_pathnames();
+    option_parse(argc, argv);
 
     if (pid_filepath == NULL) {
         pid_filepath = g_strdup(dfl_pathnames.pidfile);
-- 
2.4.3

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

[Marc-André Lureau]

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@gmail.com>
---
 qga/main.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index b965f61..5575637 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;
@@ -1076,6 +1059,23 @@ int main(int argc, char **argv)
         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 08/12] qga: move agent run in a seperate function

[Marc-André Lureau]

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

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

diff --git a/qga/main.c b/qga/main.c
index 5575637..aaf0e10 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1042,39 +1042,13 @@ static void option_parse(int argc, char **argv)
     }
 }
 
-int main(int argc, char **argv)
+static int run_agent(GAState *s)
 {
-    GAState *s;
-
-    module_call_init(MODULE_INIT_QAPI);
-
-    init_dfl_pathnames();
-    option_parse(argc, argv);
-
-    if (pid_filepath == NULL) {
-        pid_filepath = g_strdup(dfl_pathnames.pidfile);
-    }
-
-    if (state_dir == NULL) {
-        state_dir = g_strdup(dfl_pathnames.state_dir);
-    }
-
-    if (method == NULL) {
-        method = g_strdup("virtio-serial");
-    }
+    ga_state = s;
 
-    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;
-        }
-    }
+    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
@@ -1090,20 +1064,6 @@ int main(int argc, char **argv)
     }
 #endif
 
-    s = g_malloc0(sizeof(GAState));
-    s->log_level = log_level;
-    s->log_file = stderr;
-#ifdef CONFIG_FSFREEZE
-    s->fsfreeze_hook = 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);
-    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
@@ -1154,7 +1114,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;
         }
@@ -1165,7 +1125,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;
     }
 
     blacklist = ga_command_blacklist_init(blacklist);
@@ -1185,14 +1145,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, method, device_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);
@@ -1206,15 +1166,65 @@ 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 (daemonize) {
-        unlink(pid_filepath);
+int main(int argc, char **argv)
+{
+    int ret = EXIT_SUCCESS;
+    GAState *s = g_new0(GAState, 1);
+
+    module_call_init(MODULE_INIT_QAPI);
+
+    init_dfl_pathnames();
+    option_parse(argc, argv);
+
+    if (pid_filepath == NULL) {
+        pid_filepath = g_strdup(dfl_pathnames.pidfile);
+    }
+
+    if (state_dir == NULL) {
+        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");
+            ret = EXIT_FAILURE;
+            goto end;
+        }
+    }
+
+    s->log_level = log_level;
+    s->log_file = stderr;
+#ifdef CONFIG_FSFREEZE
+    s->fsfreeze_hook = fsfreeze_hook;
+#endif
+    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
+                                                 state_dir);
+    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
+    s->frozen = false;
+
+    ret = run_agent(s);
+
+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 (daemonize) {
         unlink(pid_filepath);
     }
@@ -1227,6 +1237,7 @@ out_bad:
 #ifdef CONFIG_FSFREEZE
     g_free(fsfreeze_hook);
 #endif
+    g_free(s);
 
-    return EXIT_FAILURE;
+    return ret;
 }
-- 
2.4.3

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

[Marc-André Lureau]

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

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

diff --git a/qga/main.c b/qga/main.c
index aaf0e10..bd87050 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;
 };
 
@@ -1224,6 +1224,8 @@ end:
     if (s->channel) {
         ga_channel_free(s->channel);
     }
+    g_free(s->pstate_filepath);
+    g_free(s->state_filepath_isfrozen);
 
     if (daemonize) {
         unlink(pid_filepath);
-- 
2.4.3

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

[Marc-André Lureau]

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@gmail.com>
---
 qga/main.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index bd87050..f6dbb3e 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -215,6 +215,7 @@ static void usage(const char *cmd)
 #endif
 "  -b, --blacklist   comma-separated list of RPCs to disable (no spaces, \"?\"\n"
 "                    to list available RPCs)\n"
+"  -D, --dump-conf   dump the configuration and exit\n"
 "  -h, --help        display this help and exit\n"
 "\n"
 "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
@@ -904,6 +905,21 @@ static void ga_print_cmd(QmpCommand *cmd, void *opaque)
     printf("%s\n", qmp_command_name(cmd));
 }
 
+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 GList *split_list(gchar *str, const gchar separator)
 {
     GList *list = NULL;
@@ -936,9 +952,28 @@ static char *state_dir;
 static const char *service;
 #endif
 static GList *blacklist;
-static int daemonize;
+static int daemonize, dumpconf;
 static GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
 
+static void dump_config(void)
+{
+    gchar *bl = list_join(blacklist, ',');
+
+    printf("[general]\n");
+    printf("daemonize = %d\n", daemonize);
+    printf("pidfile = %s\n", pid_filepath);
+    if (log_filepath) {
+        printf("logfile = %s\n", log_filepath);
+    }
+    printf("verbose = %d\n", log_level == G_LOG_LEVEL_MASK);
+    printf("method = %s\n", method);
+    printf("path = %s\n", device_path);
+    printf("statedir = %s\n", state_dir);
+    printf("blacklist = %s\n", bl);
+
+    g_free(bl);
+}
+
 static void option_parse(int argc, char **argv)
 {
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
@@ -946,6 +981,7 @@ static void option_parse(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
@@ -1031,6 +1067,9 @@ static void option_parse(int argc, char **argv)
             }
             break;
 #endif
+        case 'D':
+            dumpconf = 1;
+            break;
         case 'h':
             usage(argv[0]);
             exit(EXIT_SUCCESS);
@@ -1205,6 +1244,11 @@ int main(int argc, char **argv)
         }
     }
 
+    if (dumpconf) {
+        dump_config();
+        goto end;
+    }
+
     s->log_level = log_level;
     s->log_file = stderr;
 #ifdef CONFIG_FSFREEZE
-- 
2.4.3

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

[Marc-André Lureau]

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 distro-specific init keys/files)

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

diff --git a/qga/main.c b/qga/main.c
index f6dbb3e..9026c9a 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;
@@ -951,6 +952,7 @@ static char *state_dir;
 #ifdef _WIN32
 static const char *service;
 #endif
+static char *bliststr;
 static GList *blacklist;
 static int daemonize, dumpconf;
 static GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
@@ -959,6 +961,7 @@ static void dump_config(void)
 {
     gchar *bl = list_join(blacklist, ',');
 
+    printf("### Read from configuration file: %s ###\n", QGA_CONF_DEFAULT);
     printf("[general]\n");
     printf("daemonize = %d\n", daemonize);
     printf("pidfile = %s\n", pid_filepath);
@@ -974,6 +977,71 @@ static void dump_config(void)
     g_free(bl);
 }
 
+static void load_system_config(void)
+{
+    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)) {
+        daemonize =
+            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
+        method =
+            g_key_file_get_string(keyfile, "general", "method", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
+        device_path =
+            g_key_file_get_string(keyfile, "general", "path", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
+        log_filepath =
+            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
+        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)) {
+        fsfreeze_hook =
+            g_key_file_get_string(keyfile,
+                                  "general", "fsfreeze-hook", &gerr);
+    }
+#endif
+    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
+        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 */
+        log_level = G_LOG_LEVEL_MASK;
+    }
+    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
+        bliststr =
+            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
+        blacklist = g_list_concat(blacklist, split_list(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 option_parse(int argc, char **argv)
 {
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
@@ -1216,6 +1284,7 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_QAPI);
 
     init_dfl_pathnames();
+    load_system_config();
     option_parse(argc, argv);
 
     if (pid_filepath == NULL) {
@@ -1283,6 +1352,7 @@ end:
 #ifdef CONFIG_FSFREEZE
     g_free(fsfreeze_hook);
 #endif
+    g_free(bliststr);
     g_free(s);
 
     return ret;
-- 
2.4.3

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

[Marc-André Lureau]

Add a simple man page for the qemu agent.

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

diff --git a/Makefile b/Makefile
index c9be643..45b1a12 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 0125bc7..aa3d165 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..a5e8002
--- /dev/null
+++ b/qemu-ga.texi
@@ -0,0 +1,135 @@
+@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.
+
+@itemize
+@item
+get information from the guest
+@item
+set the guest's system time
+@item
+read/write a file
+@item
+sync an freeze the filesystems
+@item
+suspend the guest
+@item
+reconfigugre 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.
+
+@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 00/12] qemu-ga: add a configuration file

[Marc-André Lureau]

Hi

ping!

End of freeze is coming, it would be useful to have some feedback for
this series :)

thanks

-- 
Marc-André Lureau

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

[Eric Blake]

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

On 07/01/2015 05:47 AM, Marc-André Lureau wrote:
> ---
>  qga/qapi-schema.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

As a doc change, it's trivial enough for inclusion if we wanted it, but
now we're late enough that it's probably also fine to wait for 2.5.

> 
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b446dc7..fbf983c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -755,7 +755,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
> 

-- 
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 02/12] qga: use exit() when parsing options

[Eric Blake]

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

On 07/01/2015 05:47 AM, Marc-André Lureau wrote:
> The option parsing is going to be moved to a seperate function,

s/seperate/separate/

> use exit() consistantly.

s/consistantly/consistently/
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>  qga/main.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 03/12] qga: move string split in seperate function

[Michael Roth]

Quoting Marc-André Lureau (2015-07-01 06:47:38)
> The function is going to be reused in a later patch.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.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 af93992..0c455f8 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
> 

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

[Michael Roth]

Quoting Marc-André Lureau (2015-07-01 06:47:39)
> '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@gmail.com>

I think I'd prefer something like 'channel_path' since we support
sockets as well. Looks good otherwise.

> ---
>  qga/main.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 0c455f8..1c81575 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, *device_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;
> +            device_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(device_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, device_path)) {
>          g_critical("failed to initialize guest agent channel");
>          goto out_bad;
>      }
> -- 
> 2.4.3
> 

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

[Michael Roth]

Quoting Marc-André Lureau (2015-07-01 06:47:42)
> 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@gmail.com>

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

> ---
>  qga/main.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index b965f61..5575637 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;
> @@ -1076,6 +1059,23 @@ int main(int argc, char **argv)
>          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
> 

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

[Michael Roth]

Quoting Marc-André Lureau (2015-07-01 06:47:41)
> Move option parsing out of giant main().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>  qga/main.c | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index b776d16..b965f61 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -941,19 +941,25 @@ 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, *device_path = NULL;
> -    char *log_filepath = NULL;
> -    char *pid_filepath = NULL;
> +static char *device_path;
> +static char *method;
> +static char *log_filepath;
> +static char *pid_filepath;

Since we want to pass these around as a representation of the
configuration state, I'd rather we package them into a GAConfig
structure or something of the sort that and pass it around as arguments
rather than as globals. Between parse/load_config/load_defaults it's
becoming a little difficult to keep track of where all these values are
being modified.

Otherwise, looks good, and makes for a nice cleanup.

>  #ifdef CONFIG_FSFREEZE
> -    char *fsfreeze_hook = NULL;
> +static char *fsfreeze_hook;
>  #endif
> -    char *state_dir = NULL;
> +static char *state_dir;
>  #ifdef _WIN32
> -    const char *service = NULL;
> +static const char *service;
>  #endif
> +static GList *blacklist;
> +static int daemonize;
> +static GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> +
> +static void option_parse(int argc, char **argv)
> +{
> +    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,14 +979,7 @@ 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);
> -
> -    init_dfl_pathnames();
>      while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
>          switch (ch) {
>          case 'm':
> @@ -1058,6 +1057,16 @@ int main(int argc, char **argv)
>              exit(EXIT_FAILURE);
>          }
>      }
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    GAState *s;
> +
> +    module_call_init(MODULE_INIT_QAPI);
> +
> +    init_dfl_pathnames();
> +    option_parse(argc, argv);
> 
>      if (pid_filepath == NULL) {
>          pid_filepath = g_strdup(dfl_pathnames.pidfile);
> -- 
> 2.4.3
> 

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

[Michael Roth]

Quoting Marc-André Lureau (2015-07-01 06:47:43)
> Once the options are populated, move the running state to
> a run_agent() function.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>  qga/main.c | 123 +++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 67 insertions(+), 56 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 5575637..aaf0e10 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1042,39 +1042,13 @@ static void option_parse(int argc, char **argv)
>      }
>  }
> 
> -int main(int argc, char **argv)
> +static int run_agent(GAState *s)
>  {
> -    GAState *s;
> -
> -    module_call_init(MODULE_INIT_QAPI);
> -
> -    init_dfl_pathnames();
> -    option_parse(argc, argv);
> -
> -    if (pid_filepath == NULL) {
> -        pid_filepath = g_strdup(dfl_pathnames.pidfile);
> -    }
> -
> -    if (state_dir == NULL) {
> -        state_dir = g_strdup(dfl_pathnames.state_dir);
> -    }
> -
> -    if (method == NULL) {
> -        method = g_strdup("virtio-serial");
> -    }
> +    ga_state = s;
> 
> -    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;
> -        }
> -    }
> +    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
> @@ -1090,20 +1064,6 @@ int main(int argc, char **argv)
>      }
>  #endif
> 
> -    s = g_malloc0(sizeof(GAState));
> -    s->log_level = log_level;
> -    s->log_file = stderr;
> -#ifdef CONFIG_FSFREEZE
> -    s->fsfreeze_hook = 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);
> -    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
> @@ -1154,7 +1114,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;
>          }
> @@ -1165,7 +1125,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;
>      }
> 
>      blacklist = ga_command_blacklist_init(blacklist);
> @@ -1185,14 +1145,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, method, device_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);
> @@ -1206,15 +1166,65 @@ 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 (daemonize) {
> -        unlink(pid_filepath);
> +int main(int argc, char **argv)
> +{
> +    int ret = EXIT_SUCCESS;
> +    GAState *s = g_new0(GAState, 1);
> +
> +    module_call_init(MODULE_INIT_QAPI);
> +
> +    init_dfl_pathnames();
> +    option_parse(argc, argv);
> +
> +    if (pid_filepath == NULL) {
> +        pid_filepath = g_strdup(dfl_pathnames.pidfile);
> +    }
> +
> +    if (state_dir == NULL) {
> +        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");
> +            ret = EXIT_FAILURE;
> +            goto end;
> +        }
> +    }
> +
> +    s->log_level = log_level;
> +    s->log_file = stderr;
> +#ifdef CONFIG_FSFREEZE
> +    s->fsfreeze_hook = fsfreeze_hook;
> +#endif
> +    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> +                                                 state_dir);
> +    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
> +    s->frozen = false;

It's hard to draw the line between what should be in main() as opposed
to run_agent(), but we have some s->frozen initialization happening
here, and then the statefile-based setting s->frozen values being set
in run_agent(), so at least in this case we should move those back to
main(). That means we probabably have to move the checks for the
statefile existence/creation back to main() as well since s->frozen
depends on it.

I think that's the right place anyway, makes sense to load/init the
persistent state file in the same location we handle the config file.

> +
> +    ret = run_agent(s);
> +
> +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 (daemonize) {
>          unlink(pid_filepath);
>      }
> @@ -1227,6 +1237,7 @@ out_bad:
>  #ifdef CONFIG_FSFREEZE
>      g_free(fsfreeze_hook);
>  #endif
> +    g_free(s);
> 
> -    return EXIT_FAILURE;
> +    return ret;
>  }
> -- 
> 2.4.3
> 

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

[Michael Roth]

Quoting Marc-André Lureau (2015-07-01 06:47:45)
> 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@gmail.com>
> ---
>  qga/main.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index bd87050..f6dbb3e 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -215,6 +215,7 @@ static void usage(const char *cmd)
>  #endif
>  "  -b, --blacklist   comma-separated list of RPCs to disable (no spaces, \"?\"\n"
>  "                    to list available RPCs)\n"
> +"  -D, --dump-conf   dump the configuration and exit\n"
>  "  -h, --help        display this help and exit\n"
>  "\n"
>  "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
> @@ -904,6 +905,21 @@ static void ga_print_cmd(QmpCommand *cmd, void *opaque)
>      printf("%s\n", qmp_command_name(cmd));
>  }
> 
> +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 GList *split_list(gchar *str, const gchar separator)
>  {
>      GList *list = NULL;
> @@ -936,9 +952,28 @@ static char *state_dir;
>  static const char *service;
>  #endif
>  static GList *blacklist;
> -static int daemonize;
> +static int daemonize, dumpconf;
>  static GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> 
> +static void dump_config(void)
> +{
> +    gchar *bl = list_join(blacklist, ',');
> +
> +    printf("[general]\n");
> +    printf("daemonize = %d\n", daemonize);
> +    printf("pidfile = %s\n", pid_filepath);
> +    if (log_filepath) {
> +        printf("logfile = %s\n", log_filepath);
> +    }
> +    printf("verbose = %d\n", log_level == G_LOG_LEVEL_MASK);
> +    printf("method = %s\n", method);
> +    printf("path = %s\n", device_path);
> +    printf("statedir = %s\n", state_dir);
> +    printf("blacklist = %s\n", bl);

I think we're missing fsfreeze_hook option here.

To me it seems cleaner to actually create the GKeyFile from current
options, then let GLib do all the work of generation a config file
we can spit out (g_key_file_to_data() should do it i think).

That, paired with the idea of having a GAConfig structure to
encapulate all the config options, might warrant restructuring
things a bit so that we have a
gkeyfile_to_gaconfig()/gkeyfile_from_gaconfig() pair to use for
reading/dumping configs while keeping all the options in an
easily trackable place.

> +
> +    g_free(bl);
> +}
> +
>  static void option_parse(int argc, char **argv)
>  {
>      const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
> @@ -946,6 +981,7 @@ static void option_parse(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
> @@ -1031,6 +1067,9 @@ static void option_parse(int argc, char **argv)
>              }
>              break;
>  #endif
> +        case 'D':
> +            dumpconf = 1;
> +            break;
>          case 'h':
>              usage(argv[0]);
>              exit(EXIT_SUCCESS);
> @@ -1205,6 +1244,11 @@ int main(int argc, char **argv)
>          }
>      }
> 
> +    if (dumpconf) {
> +        dump_config();
> +        goto end;
> +    }
> +
>      s->log_level = log_level;
>      s->log_file = stderr;
>  #ifdef CONFIG_FSFREEZE
> -- 
> 2.4.3
> 

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

[Michael Roth]

Quoting Marc-André Lureau (2015-07-01 06:47:47)
> Add a simple man page for the qemu agent.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>  Makefile      |  14 +++++-
>  qemu-doc.texi |   6 +++
>  qemu-ga.texi  | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 153 insertions(+), 2 deletions(-)
>  create mode 100644 qemu-ga.texi
> 
> diff --git a/Makefile b/Makefile
> index c9be643..45b1a12 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 0125bc7..aa3d165 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..a5e8002
> --- /dev/null
> +++ b/qemu-ga.texi
> @@ -0,0 +1,135 @@
> +@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.

Maybe:

 "various operations in the guest, such as:"

Makes it clearer it's not an exhaustive list.

> +
> +@itemize
> +@item
> +get information from the guest
> +@item
> +set the guest's system time
> +@item
> +read/write a file
> +@item
> +sync an freeze the filesystems

*and freeze

> +@item
> +suspend the guest
> +@item
> +reconfigugre guest local processors

*reconfigure

> +@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

, then parse remaining

> +configuration options on the command line. For the same key, the last
> +option wins, but the lists accumulate.

Maybe an added:

"(see below for configuration file format)"

would be useful. Might lose less thorough readers (such as myself) here
(initially I assumed there wouldn't be examples below and starting
writing a comment about it).

> +
> +@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})

I'm not sure if it's possible, but would be nice if we could use the
#defines from QGA to grab these values. Would help keep things in sync.
Not a huge deal if there's no simple method.

> +
> +@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 01/12] qga: misc spelling

[Denis V. Lunev]

On 07/01/2015 02:47 PM, Marc-André Lureau wrote:
> ---
>   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 b446dc7..fbf983c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -755,7 +755,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
Reviewed-by: Denis V. Lunev <den@openvz.org>

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

[Denis V. Lunev]

On 07/01/2015 02:47 PM, Marc-André Lureau wrote:
> The option parsing is going to be moved to a seperate function,
> use exit() consistantly.
s/seperate/separate/
s/consistantly/consistently/

> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>   qga/main.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 23cde01..af93992 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);
>           }
>       }
>   
Fairly speaking the approach is not very clean as
it can be. exit() in the middle if the program is
almost the worst thing to be done as it
introduces various leaks. Anyway, pls conside
these words as world of old slowpoke. Technically
the patch is correct thus

Reviewed-by: Denis V. Lunev <den@openvz.org>

except typos above.

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

[Marc-André Lureau]

On Tue, Aug 25, 2015 at 7:43 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Quoting Marc-André Lureau (2015-07-01 06:47:47)
>> Add a simple man page for the qemu agent.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
>> ---
>>  Makefile      |  14 +++++-
>>  qemu-doc.texi |   6 +++
>>  qemu-ga.texi  | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 153 insertions(+), 2 deletions(-)
>>  create mode 100644 qemu-ga.texi
>>
>> diff --git a/Makefile b/Makefile
>> index c9be643..45b1a12 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 0125bc7..aa3d165 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..a5e8002
>> --- /dev/null
>> +++ b/qemu-ga.texi
>> @@ -0,0 +1,135 @@
>> +@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.
>
> Maybe:
>
>  "various operations in the guest, such as:"
>
> Makes it clearer it's not an exhaustive list.

ok

>
>> +
>> +@itemize
>> +@item
>> +get information from the guest
>> +@item
>> +set the guest's system time
>> +@item
>> +read/write a file
>> +@item
>> +sync an freeze the filesystems
>
> *and freeze

yep

>
>> +@item
>> +suspend the guest
>> +@item
>> +reconfigugre guest local processors
>
> *reconfigure
>

yep

>> +@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
>
> , then parse remaining
>
>> +configuration options on the command line. For the same key, the last
>> +option wins, but the lists accumulate.
>
> Maybe an added:
>
> "(see below for configuration file format)"

ok

>
> would be useful. Might lose less thorough readers (such as myself) here
> (initially I assumed there wouldn't be examples below and starting
> writing a comment about it).
>
>> +
>> +@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})
>
> I'm not sure if it's possible, but would be nice if we could use the
> #defines from QGA to grab these values. Would help keep things in sync.
> Not a huge deal if there's no simple method.

looks like it is a bit tricky, I left this

thanks

>
>> +
>> +@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
>>
>



-- 
Marc-André Lureau

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

[Marc-André Lureau]

On Tue, Aug 25, 2015 at 7:11 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> I think we're missing fsfreeze_hook option here.
>

good catch

> To me it seems cleaner to actually create the GKeyFile from current
> options, then let GLib do all the work of generation a config file
> we can spit out (g_key_file_to_data() should do it i think).

I don't mind, why not.

> That, paired with the idea of having a GAConfig structure to
> encapulate all the config options, might warrant restructuring
> things a bit so that we have a
> gkeyfile_to_gaconfig()/gkeyfile_from_gaconfig() pair to use for
> reading/dumping configs while keeping all the options in an
> easily trackable place.




-- 
Marc-André Lureau

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

[Marc-André Lureau]

On Tue, Aug 25, 2015 at 6:51 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> It's hard to draw the line between what should be in main() as opposed
> to run_agent(), but we have some s->frozen initialization happening
> here, and then the statefile-based setting s->frozen values being set
> in run_agent(), so at least in this case we should move those back to
> main(). That means we probabably have to move the checks for the
> statefile existence/creation back to main() as well since s->frozen
> depends on it.

ok, I moved some frozen initialization in a check_is_frozen() function
called from main.

> I think that's the right place anyway, makes sense to load/init the
> persistent state file in the same location we handle the config file.


thanks

-- 
Marc-André Lureau

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

[Marc-André Lureau]

hi

On Tue, Aug 25, 2015 at 6:24 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Since we want to pass these around as a representation of the
> configuration state, I'd rather we package them into a GAConfig
> structure or something of the sort that and pass it around as arguments
> rather than as globals. Between parse/load_config/load_defaults it's
> becoming a little difficult to keep track of where all these values are
> being modified.


done, thanks

-- 
Marc-André Lureau

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

[Marc-André Lureau]

hi

On Tue, Aug 25, 2015 at 5:55 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Quoting Marc-André Lureau (2015-07-01 06:47:39)
>> '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@gmail.com>
>
> I think I'd prefer something like 'channel_path' since we support
> sockets as well. Looks good otherwise.


ok, done

-- 
Marc-André Lureau