Re: [PATCH v2 22/22] qga: centralize logic for disabling/enabling commands

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

Hello Daniel,

This cleanup seems like a good idea,

On Thu, 13 Jun 2024 18:44, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>It is confusing having many different pieces of code enabling and
>disabling commands, and it is not clear that they all have the same
>semantics, especially wrt prioritization of the block/allow lists.
>The code attempted to prevent the user from setting both the block
>and allow lists concurrently, however, the logic was flawed as it
>checked settings in the configuration file  separately from the
>command line arguments. Thus it was possible to set a block list
>in the config file and an allow list via a command line argument.
>The --dump-conf option also creates a configuration file with both
>keys present, even if unset, which means it is creating a config
>that cannot actually be loaded again.
>
>Centralizing the code in a single method "ga_apply_command_filters"
>will provide a strong guarantee of consistency and clarify the
>intended behaviour. With this there is no compelling technical
>reason to prevent concurrent setting of both the allow and block
>lists, so this flawed restriction is removed.
>
>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>---
> docs/interop/qemu-ga.rst |  14 +++++
> qga/commands-posix.c     |   6 --
> qga/commands-win32.c     |   6 --
> qga/main.c               | 128 +++++++++++++++++----------------------
> 4 files changed, 70 insertions(+), 84 deletions(-)
>
>diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst
>index e42b370319..e35dcaf0e7 100644
>--- a/docs/interop/qemu-ga.rst
>+++ b/docs/interop/qemu-ga.rst
>@@ -28,6 +28,20 @@ configuration options on the command line. For the same key, the last
> option wins, but the lists accumulate (see below for configuration
> file format).
> 
>+If an allowed RPCs list is defined in the configuration, then all
>+RPCs will be blocked by default, except for the allowed list.
>+
>+If a blocked RPCs list is defined in the configuration, then all
>+RPCs will be allowed by default, except for the blocked list.
>+
>+If both allowed and blocked RPCs lists are defined in the configuration,
>+then all RPCs will be blocked by default, and then allowed list will
>+be applied, followed by the blocked list.

Nit: Missing an article here

-then all RPCs will be blocked by default, and then allowed list will
+then all RPCs will be blocked by default, then the allowed list will


>+
>+While filesystems are frozen, all except for a designated safe set
>+of RPCs will blocked, regardless of what the general configuration
>+declares.
>+
> Options
> -------
> 
>diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>index f4104f2760..578d29f228 100644
>--- a/qga/commands-posix.c
>+++ b/qga/commands-posix.c
>@@ -1136,12 +1136,6 @@ error:
> 
> #endif /* HAVE_GETIFADDRS */
> 
>-/* add unsupported commands to the list of blocked RPCs */
>-GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
>-{
>-    return blockedrpcs;
>-}
>-
> /* register init/cleanup routines for stateful command groups */
> void ga_command_state_init(GAState *s, GACommandState *cs)
> {
>diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>index 5866cc2e3c..61b36da469 100644
>--- a/qga/commands-win32.c
>+++ b/qga/commands-win32.c
>@@ -1958,12 +1958,6 @@ done:
>     g_free(rawpasswddata);
> }
> 
>-/* add unsupported commands to the list of blocked RPCs */
>-GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
>-{
>-    return blockedrpcs;
>-}
>-
> /* register init/cleanup routines for stateful command groups */
> void ga_command_state_init(GAState *s, GACommandState *cs)
> {
>diff --git a/qga/main.c b/qga/main.c
>index f68a32bf7b..72c16fead8 100644
>--- a/qga/main.c
>+++ b/qga/main.c
>@@ -419,60 +419,79 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
>     return strcmp(str1, str2);
> }
> 
>-/* disable commands that aren't safe for fsfreeze */
>-static void ga_disable_not_allowed_freeze(const QmpCommand *cmd, void *opaque)
>+static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
> {
>-    bool allowed = false;
>     int i = 0;
>+    GAConfig *config = state->config;
>     const char *name = qmp_command_name(cmd);
>+    /* Fallback policy is allow everything */
>+    bool allowed = true;
> 
>-    while (ga_freeze_allowlist[i] != NULL) {
>-        if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
>+    if (config->allowedrpcs) {
>+        /*
>+         * If an allow-list is given, this changes the fallback
>+         * policy to deny everything
>+         */
>+        allowed = false;
>+
>+        if (g_list_find_custom(config->allowedrpcs, name, ga_strcmp) != NULL) {
>             allowed = true;
>         }
>-        i++;
>     }
>-    if (!allowed) {
>-        g_debug("disabling command: %s", name);
>-        qmp_disable_command(&ga_commands, name, "the agent is in frozen state");
>-    }
>-}
> 
>-/* [re-]enable all commands, except those explicitly blocked by user */
>-static void ga_enable_non_blocked(const QmpCommand *cmd, void *opaque)
>-{
>-    GAState *s = opaque;
>-    GList *blockedrpcs = s->blockedrpcs;
>-    GList *allowedrpcs = s->allowedrpcs;
>-    const char *name = qmp_command_name(cmd);
>-
>-    if (g_list_find_custom(blockedrpcs, name, ga_strcmp) == NULL) {
>-        if (qmp_command_is_enabled(cmd)) {
>-            return;
>+    /*
>+     * If both allowedrpcs and blockedrpcs are set, the blocked
>+     * list will take priority
>+     */
>+    if (config->blockedrpcs) {
>+        if (g_list_find_custom(config->blockedrpcs, name, ga_strcmp) != NULL) {
>+            allowed = false;
>         }
>+    }
> 
>-        if (allowedrpcs &&
>-            g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
>-            return;
>-        }
>+    /*
>+     * If frozen, this filtering must take priority over
>+     * absolutely everything
>+     */
>+    if (state->frozen) {
>+        allowed = false;
> 
>-        g_debug("enabling command: %s", name);
>-        qmp_enable_command(&ga_commands, name);
>+        while (ga_freeze_allowlist[i] != NULL) {
>+            if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
>+                allowed = true;
>+            }
>+            i++;
>+        }
>     }
>+
>+    return allowed;
> }

IUUC, we can check by priority here: first check if (state->frozen), 
then blockedrpcs, then allowedrpcs and then return a default fallback 
value allowed = config->blockedrpcs != NULL && config->allowedrpcs != 
NULL

This way the function will sort of document what is written on the docs 
as well.


> 
>-/* disable commands that aren't allowed */
>-static void ga_disable_not_allowed(const QmpCommand *cmd, void *opaque)
>+static void ga_apply_command_filters_iter(const QmpCommand *cmd, void *opaque)
> {
>-    GList *allowedrpcs = opaque;
>+    GAState *state = opaque;
>+    bool want = ga_command_is_allowed(cmd, state);
>+    bool have = qmp_command_is_enabled(cmd);
>     const char *name = qmp_command_name(cmd);
> 
>-    if (g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
>+    if (want == have) {
>+        return;
>+    }
>+
>+    if (qmp_command_is_enabled(cmd)) {

Nit:

  if (have) {

Since it's already declared.

>         g_debug("disabling command: %s", name);
>         qmp_disable_command(&ga_commands, name, "the command is not allowed");
>+    } else {
>+        g_debug("enabling command: %s", name);
>+        qmp_enable_command(&ga_commands, name);
>     }
> }
> 
>+static void ga_apply_command_filters(GAState *state)

Nit: inline?

>+{
>+    qmp_for_each_command(&ga_commands, ga_apply_command_filters_iter, state);
>+}
>+
> static bool ga_create_file(const char *path)
> {
>     int fd = open(path, O_CREAT | O_WRONLY, S_IWUSR | S_IRUSR);
>@@ -505,15 +524,14 @@ void ga_set_frozen(GAState *s)
>     if (ga_is_frozen(s)) {
>         return;
>     }
>-    /* disable all forbidden (for frozen state) commands */
>-    qmp_for_each_command(&ga_commands, ga_disable_not_allowed_freeze, NULL);
>     g_warning("disabling logging due to filesystem freeze");
>-    ga_disable_logging(s);
>     s->frozen = true;
>     if (!ga_create_file(s->state_filepath_isfrozen)) {
>         g_warning("unable to create %s, fsfreeze may not function properly",
>                   s->state_filepath_isfrozen);
>     }
>+    ga_apply_command_filters(s);
>+    ga_disable_logging(s);
> }
> 
> void ga_unset_frozen(GAState *s)
>@@ -545,12 +563,12 @@ void ga_unset_frozen(GAState *s)
>     }
> 
>     /* enable all disabled, non-blocked and allowed commands */
>-    qmp_for_each_command(&ga_commands, ga_enable_non_blocked, s);
>     s->frozen = false;
>     if (!ga_delete_file(s->state_filepath_isfrozen)) {
>         g_warning("unable to delete %s, fsfreeze may not function properly",
>                   s->state_filepath_isfrozen);
>     }
>+    ga_apply_command_filters(s);
> }
> 
> #ifdef CONFIG_FSFREEZE
>@@ -1082,13 +1100,6 @@ static void config_load(GAConfig *config, const char *confpath, bool required)
>                                           split_list(config->aliststr, ","));
>     }
> 
>-    if (g_key_file_has_key(keyfile, "general", "block-rpcs", NULL) &&
>-        g_key_file_has_key(keyfile, "general", "allow-rpcs", NULL)) {
>-        g_critical("wrong config, using 'block-rpcs' and 'allow-rpcs' keys at"
>-                   " the same time is not allowed");
>-        exit(EXIT_FAILURE);
>-    }
>-
> end:
>     g_key_file_free(keyfile);
>     if (gerr && (required ||
>@@ -1168,7 +1179,6 @@ static void config_parse(GAConfig *config, int argc, char **argv)
> {
>     const char *sopt = "hVvdc:m:p:l:f:F::b:a:s:t:Dr";
>     int opt_ind = 0, ch;
>-    bool block_rpcs = false, allow_rpcs = false;
>     const struct option lopt[] = {
>         { "help", 0, NULL, 'h' },
>         { "version", 0, NULL, 'V' },
>@@ -1264,7 +1274,6 @@ static void config_parse(GAConfig *config, int argc, char **argv)
>             }
>             config->blockedrpcs = g_list_concat(config->blockedrpcs,
>                                                 split_list(optarg, ","));
>-            block_rpcs = true;
>             break;
>         }
>         case 'a': {
>@@ -1274,7 +1283,6 @@ static void config_parse(GAConfig *config, int argc, char **argv)
>             }
>             config->allowedrpcs = g_list_concat(config->allowedrpcs,
>                                                 split_list(optarg, ","));
>-            allow_rpcs = true;
>             break;
>         }
> #ifdef _WIN32
>@@ -1315,12 +1323,6 @@ static void config_parse(GAConfig *config, int argc, char **argv)
>             exit(EXIT_FAILURE);
>         }
>     }
>-
>-    if (block_rpcs && allow_rpcs) {
>-        g_critical("wrong commandline, using --block-rpcs and --allow-rpcs at the"
>-                   " same time is not allowed");
>-        exit(EXIT_FAILURE);
>-    }
> }
> 
> static void config_free(GAConfig *config)
>@@ -1431,7 +1433,6 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
>             s->deferred_options.log_filepath = config->log_filepath;
>         }
>         ga_disable_logging(s);
>-        qmp_for_each_command(&ga_commands, ga_disable_not_allowed_freeze, NULL);
>     } else {
>         if (config->daemonize) {
>             become_daemon(config->pid_filepath);
>@@ -1455,25 +1456,6 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
>         return NULL;
>     }
> 
>-    if (config->allowedrpcs) {
>-        qmp_for_each_command(&ga_commands, ga_disable_not_allowed, config->allowedrpcs);
>-        s->allowedrpcs = config->allowedrpcs;
>-    }
>-
>-    /*
>-     * Some commands can be blocked due to system limitation.
>-     * Initialize blockedrpcs list even if allowedrpcs specified.
>-     */
>-    config->blockedrpcs = ga_command_init_blockedrpcs(config->blockedrpcs);
>-    if (config->blockedrpcs) {
>-        GList *l = config->blockedrpcs;
>-        s->blockedrpcs = config->blockedrpcs;
>-        do {
>-            g_debug("disabling command: %s", (char *)l->data);
>-            qmp_disable_command(&ga_commands, l->data, NULL);
>-            l = g_list_next(l);
>-        } while (l);
>-    }
>     s->command_state = ga_command_state_new();
>     ga_command_state_init(s, s->command_state);
>     ga_command_state_init_all(s->command_state);
>@@ -1499,6 +1481,8 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
>     }
> #endif
> 
>+    ga_apply_command_filters(s);
>+
>     ga_state = s;
>     return s;
> }
>-- 
>2.45.1
>
>