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

[Qemu-devel] [PATCH v3 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
Since v2:
- fix compilation in intermediate patch
- remove some extra space in intermediate patch
- add some missing Reviewed-by tags

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 separate function
  qga: fill default options in main()
  qga: move agent run in a separate 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 v3 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz>
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 v3 01/12] qga: misc spelling

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Denis V. Lunev <den@openvz>
s/den@openvz/den@openvz.org/

> 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

[Qemu-devel] [PATCH v3 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>
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

[Qemu-devel] [PATCH v3 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

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

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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)
> +{
it would be much better to declare this helper as "split_list(const 
gchar *str, ...
and make temporary copy of parameter inside. Without this the function
as NASTY side-effect and trashes the string passed in.

Also it is rather reinvents strtok wheel.

you can also use g_strsplit at your convenience, but may be this is overkill

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

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

[Marc-André Lureau]

Hi

On Wed, Aug 26, 2015 at 7:55 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
> it would be much better to declare this helper as "split_list(const gchar
> *str, ...
> and make temporary copy of parameter inside. Without this the function
> as NASTY side-effect and trashes the string passed in.
> Also it is rather reinvents strtok wheel.

Yep, notice I didn't change the code, merely moved it.

> you can also use g_strsplit at your convenience, but may be this is overkill

But it doesn't return a glist. So while I agree with you in general,
that wasn't my goal to change this code, but just to be able to reuse
it.

thanks

-- 
Marc-André Lureau

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

[Denis V. Lunev]

On 08/26/2015 09:07 PM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Aug 26, 2015 at 7:55 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
>> it would be much better to declare this helper as "split_list(const gchar
>> *str, ...
>> and make temporary copy of parameter inside. Without this the function
>> as NASTY side-effect and trashes the string passed in.
>> Also it is rather reinvents strtok wheel.
> Yep, notice I didn't change the code, merely moved it.
I think that this side effect is visible if the code remains in place
and becomes invisible since you move it to the function.
This could create problem if somebody will reuse this call.


>> you can also use g_strsplit at your convenience, but may be this is overkill
> But it doesn't return a glist. So while I agree with you in general,
> that wasn't my goal to change this code, but just to be able to reuse
> it.
>
> thanks
>

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

[Marc-André Lureau]

Hi

On Wed, Aug 26, 2015 at 8:23 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
> I think that this side effect is visible if the code remains in place
> and becomes invisible since you move it to the function.
> This could create problem if somebody will reuse this call.

what about replacing it with:

static GList *split_list(gchar *str, const gchar *delim)
{
    GList *list = NULL;
    int i;
    gchar **strv;

    strv = g_strsplit(str, delim, -1);
    for (i = 0; strv[i]; i++) {
        list = g_list_prepend(list, strv[i]);
    }
    g_free(strv);

    return list;
}

would that work for you?

the list must then be g_list_free_full()

-- 
Marc-André Lureau

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

[Marc-André Lureau]

Hi

On Wed, Aug 26, 2015 at 8:30 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> the list must then be g_list_free_full()


Actually, the ga_command_blacklist_init() doesn't dup the string, so
it will have to do it too..tbh, I don't mind either way.

-- 
Marc-André Lureau

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

[Denis V. Lunev]

On 08/26/2015 09:30 PM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Aug 26, 2015 at 8:23 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
>> I think that this side effect is visible if the code remains in place
>> and becomes invisible since you move it to the function.
>> This could create problem if somebody will reuse this call.
> what about replacing it with:
>
> static GList *split_list(gchar *str, const gchar *delim)
> {
>      GList *list = NULL;
>      int i;
>      gchar **strv;
>
>      strv = g_strsplit(str, delim, -1);
>      for (i = 0; strv[i]; i++) {
>          list = g_list_prepend(list, strv[i]);
>      }
>      g_free(strv);
>
>      return list;
> }
>
> would that work for you?
yep! and you could declare it with 'const gchar *str'

> the list must then be g_list_free_full()
>

[Qemu-devel] [PATCH v3 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>
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 v3 04/12] qga: rename 'path' to 'channel_path'

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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;
>       }
Reviewed-by: Denis V. Lunev <den@openvz.org>

[Qemu-devel] [PATCH v3 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>
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 v3 05/12] qga: copy argument strings

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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)) {

here the original behaviour is silently changed

>                       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;
>   }
I think that patch in this form is overkill. IMHO it would be shorter
to change initialization sequence clearing pid_filepath and
state_dir assignment and not play with g_free/g_strdup
These bits will not make next patch shorter. It would be the same.
The description could be clear with this approach

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

[Marc-André Lureau]

Hi

----- Original Message -----
> On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> > 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)) {
> 
> here the original behaviour is silently changed
> 
> >                       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;
> >   }
> I think that patch in this form is overkill. IMHO it would be shorter
> to change initialization sequence clearing pid_filepath and
> state_dir assignment and not play with g_free/g_strdup

Sorry, I don't understand. do you mean it shouldn't use strdup/free at all? that's not possible without leaking in the further patches that return allocated strings.

> These bits will not make next patch shorter. It would be the same.
> The description could be clear with this approach

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

[Denis V. Lunev]

On 08/26/2015 09:17 PM, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
>>> 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)) {
>> here the original behaviour is silently changed
>>
I was wrong here, pls ignore..


>>>                        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;
>>>    }
>> I think that patch in this form is overkill. IMHO it would be shorter
>> to change initialization sequence clearing pid_filepath and
>> state_dir assignment and not play with g_free/g_strdup
> Sorry, I don't understand. do you mean it shouldn't use strdup/free at all? that's not possible without leaking in the further patches that return allocated strings.
>
>> These bits will not make next patch shorter. It would be the same.
>> The description could be clear with this approach
>
lets consider this patch. You have done 2 things:
- changed initialisation order and dropped nasty temporary variables
- introduced alloc/free code

But in the next patch each line with alloc/free code
will be changed due to variable rename and
moving to the separate function (free), which
IMHO means that this preparatory step is unnecessary,
you will make almost same changes in the next
patch

Thus the sum of changes in this patch/next patch
would be less with uncovering initialization
change details, which are missed in the patch
description

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

[Marc-André Lureau]

On Wed, Aug 26, 2015 at 8:27 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
> lets consider this patch. You have done 2 things:
> - changed initialisation order and dropped nasty temporary variables
> - introduced alloc/free code
>
> But in the next patch each line with alloc/free code
> will be changed due to variable rename and
> moving to the separate function (free), which
> IMHO means that this preparatory step is unnecessary,
> you will make almost same changes in the next
> patch

to me this patch is a preparatory cleanup, having variable initialized
to NULL, string allocated, and free. The nice side effect is that we
can get rid of the weird fixed_state_dir stuff.

> Thus the sum of changes in this patch/next patch
> would be less with uncovering initialization
> change details, which are missed in the patch
> description

I agree I can complete the commit description, but I think it's best
to keep this preparatory cleanup.

-- 
Marc-André Lureau

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

[Denis V. Lunev]

On 08/26/2015 09:41 PM, Marc-André Lureau wrote:
> On Wed, Aug 26, 2015 at 8:27 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
>> lets consider this patch. You have done 2 things:
>> - changed initialisation order and dropped nasty temporary variables
>> - introduced alloc/free code
>>
>> But in the next patch each line with alloc/free code
>> will be changed due to variable rename and
>> moving to the separate function (free), which
>> IMHO means that this preparatory step is unnecessary,
>> you will make almost same changes in the next
>> patch
> to me this patch is a preparatory cleanup, having variable initialized
> to NULL, string allocated, and free. The nice side effect is that we
> can get rid of the weird fixed_state_dir stuff.
>
>> Thus the sum of changes in this patch/next patch
>> would be less with uncovering initialization
>> change details, which are missed in the patch
>> description
> I agree I can complete the commit description, but I think it's best
> to keep this preparatory cleanup.
>
yep. This is good to me.

[Qemu-devel] [PATCH v3 06/12] qga: move option parsing to separate 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>
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..8c4a075 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 v3 06/12] qga: move option parsing to separate function

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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..8c4a075 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;
>   }
Reviewed-by: Denis V. Lunev <den@openvz.org>

[Qemu-devel] [PATCH v3 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 | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 8c4a075..80f51fe 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;
@@ -1100,6 +1083,23 @@ int main(int argc, char **argv)
         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");
+            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 v3 07/12] qga: fill default options in main()

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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 | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 8c4a075..80f51fe 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;
> @@ -1100,6 +1083,23 @@ int main(int argc, char **argv)
>           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");
> +            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
Reviewed-by: Denis V. Lunev <den@openvz.org>

[Qemu-devel] [PATCH v3 08/12] qga: move agent run in a separate 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>
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 80f51fe..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 (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");
-            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 v3 08/12] qga: move agent run in a separate function

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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 80f51fe..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 (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");
> -            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;
>   }
Reviewed-by: Denis V. Lunev <den@openvz.org>

[Qemu-devel] [PATCH v3 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

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

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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);
Reviewed-by: Denis V. Lunev <den@openvz.org>

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 7 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 58f2fc7..9193043 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; /* blacklist may point to this string */
     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

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

[Eric Blake]

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

On 08/26/2015 04:05 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 

s/optionnal/optional/ in the subject line

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

s/a init/an init/

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

-- 
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 v3 10/12] qga: add an optionnal qemu-ga.conf system configuration

[Marc-André Lureau]

Hi

----- Original Message -----
> On 08/26/2015 04:05 AM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> 
> s/optionnal/optional/ in the subject line
> 
> > 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)
> 
> s/a init/an init/
> 

thanks, fixed

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

[Michael Roth]

Quoting Marc-André Lureau (2015-08-26 10:11:30)
> 
> Hi
> 
> ----- Original Message -----
> > On 08/26/2015 04:05 AM, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > 
> > s/optionnal/optional/ in the subject line
> > 
> > > 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)
> > 
> > s/a init/an init/

I have it fixed up in my staging tree so no need for a v4 (assuming I don't hit
any other snags).

> > 
> 
> thanks, fixed
> 

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

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>   qga/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 73 insertions(+), 7 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 58f2fc7..9193043 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; /* blacklist may point to this string */
>       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);
sequential calling of config_load/config_parse
means memory leak in config_parse if the
option is present in the file and overwritten
during parsing.

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

[Marc-André Lureau]

Hi

On Wed, Aug 26, 2015 at 8:41 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
> sequential calling of config_load/config_parse
> means memory leak in config_parse if the
> option is present in the file and overwritten
> during parsing.


true, I'll add some g_free() calls.

-- 
Marc-André Lureau

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/qga/main.c b/qga/main.c
index 9193043..72dc366 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

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

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>   qga/main.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
>
> diff --git a/qga/main.c b/qga/main.c
> index 9193043..72dc366 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();
keyfile == NULL either means error and you should
immediately return as I am not quite sure that calls
below will survive

Thus the check below for keyfile == NULL is extra
and could be dropped. If these functions are OK
with NULL then free will be also fine too and check
is not needed too.

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

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

[Marc-André Lureau]

----- Original Message -----
> On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> > 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>
> > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >   qga/main.c | 62
> >   ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 62 insertions(+)
> >
> > diff --git a/qga/main.c b/qga/main.c
> > index 9193043..72dc366 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();
> keyfile == NULL either means error and you should
> immediately return as I am not quite sure that calls
> below will survive

let's add an assert() there.

> Thus the check below for keyfile == NULL is extra
> and could be dropped. If these functions are OK
> with NULL then free will be also fine too and check
> is not needed too.

ok

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

[Qemu-devel] [PATCH v3 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>
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 v3 12/12] qga: start a man page

[Denis V. Lunev]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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:
s/deamon/daemon/


Guys, I am not a native speaker and it would be fine if
somebody will pay attention for the text.