| Message ID | 20240613154406.1365469-17-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga: clean up command source locations and conditionals | expand |
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 > >
On 3/7/24 12:01, Manos Pitsidianakis wrote: > 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(-) >> +static void ga_apply_command_filters(GAState *state) > > Nit: inline? No, consensus is today's compilers are smart enough to notice inlining, developers shouldn't worry about this anymore. >> +{ >> + qmp_for_each_command(&ga_commands, ga_apply_command_filters_iter, >> state); >> +}
On Wed, Jul 03, 2024 at 01:01:11PM +0300, Manos Pitsidianakis wrote: > 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/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 That would imply each check does an early return. When I add in the following series, I have further checks going in this method which rely on the fallthrough for overrides, which works better as it is written here. With regards, Daniel
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. + +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; } -/* 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)) { 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) +{ + 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; }
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(-)