| Message ID | 20240604134933.220112-21-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga: clean up command source locations and conditionals | expand |
Hi On Tue, Jun 4, 2024 at 5:51 PM 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. > > Centralizing the code in a single method "ga_apply_command_filters" > will provide a strong guarantee of consistency and clarify the > intended behaviour. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > The clean up is very much welcome and looks correct, but it crashes: Thread 1 "qemu-ga" received signal SIGSEGV, Segmentation fault. 0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800, state=0x555555633710) at ../qga/main.c:430 430 if (config->allowedrpcs) { (gdb) bt #0 0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800, state=0x555555633710) at ../qga/main.c:430 #1 ga_apply_command_filters_iter (cmd=0x555555632800, opaque=0x555555633710) at ../qga/main.c:473 #2 0x000055555559ef81 in qmp_for_each_command (cmds=cmds@entry=0x55555562c2b0 <ga_commands>, fn=fn@entry=0x55555557db30 <ga_apply_command_filters_iter>, opaque=opaque@entry=0x555555633710) at ../qapi/qmp-registry.c:93 #3 0x0000555555571436 in ga_apply_command_filters (state=0x555555633710) at ../qga/main.c:492 #4 initialize_agent (config=0x555555632760, socket_activation=0) at ../qga/main.c:1452 #5 main (argc=<optimized out>, argv=<optimized out>) at ../qga/main.c:1646 (gdb) p state.config $1 = (GAConfig *) 0x0 (meson test fails too) I wonder why s->config is set so late in initialize_agent(). Moving it earlier seems to solve the issue, but reviewing all code paths is tedious.. --- > qga/main.c | 110 ++++++++++++++++++++++++++--------------------------- > 1 file changed, 55 insertions(+), 55 deletions(-) > > diff --git a/qga/main.c b/qga/main.c > index e8f52f0794..c7b7b0a9bc 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 > @@ -1414,7 +1432,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); > @@ -1438,25 +1455,8 @@ 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; > - } > + ga_apply_command_filters(s); > > - /* > - * 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); > -- > 2.45.1 > > >
Hi On Wed, Jun 5, 2024 at 2:37 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Tue, Jun 4, 2024 at 5:51 PM 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. >> >> Centralizing the code in a single method "ga_apply_command_filters" >> will provide a strong guarantee of consistency and clarify the >> intended behaviour. >> >> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >> > > The clean up is very much welcome and looks correct, but it crashes: > > Thread 1 "qemu-ga" received signal SIGSEGV, Segmentation fault. > 0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800, > state=0x555555633710) at ../qga/main.c:430 > 430 if (config->allowedrpcs) { > (gdb) bt > #0 0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800, > state=0x555555633710) at ../qga/main.c:430 > #1 ga_apply_command_filters_iter (cmd=0x555555632800, > opaque=0x555555633710) at ../qga/main.c:473 > #2 0x000055555559ef81 in qmp_for_each_command (cmds=cmds@entry=0x55555562c2b0 > <ga_commands>, fn=fn@entry=0x55555557db30 > <ga_apply_command_filters_iter>, opaque=opaque@entry=0x555555633710) > at ../qapi/qmp-registry.c:93 > #3 0x0000555555571436 in ga_apply_command_filters (state=0x555555633710) > at ../qga/main.c:492 > #4 initialize_agent (config=0x555555632760, socket_activation=0) at > ../qga/main.c:1452 > #5 main (argc=<optimized out>, argv=<optimized out>) at ../qga/main.c:1646 > (gdb) p state.config > $1 = (GAConfig *) 0x0 > > (meson test fails too) > > I wonder why s->config is set so late in initialize_agent(). Moving it > earlier seems to solve the issue, but reviewing all code paths is tedious.. > Actually, there seems to be few ->config users, and they don't check if it's NULL. So I guess it's ok to move it earlier.
On Wed, Jun 05, 2024 at 02:37:24PM +0400, Marc-André Lureau wrote: > Hi > > On Tue, Jun 4, 2024 at 5:51 PM 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. > > > > Centralizing the code in a single method "ga_apply_command_filters" > > will provide a strong guarantee of consistency and clarify the > > intended behaviour. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > > > The clean up is very much welcome and looks correct, but it crashes: > > Thread 1 "qemu-ga" received signal SIGSEGV, Segmentation fault. > 0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800, > state=0x555555633710) at ../qga/main.c:430 > 430 if (config->allowedrpcs) { > (gdb) bt > #0 0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800, > state=0x555555633710) at ../qga/main.c:430 > #1 ga_apply_command_filters_iter (cmd=0x555555632800, > opaque=0x555555633710) at ../qga/main.c:473 > #2 0x000055555559ef81 in qmp_for_each_command (cmds=cmds@entry=0x55555562c2b0 > <ga_commands>, fn=fn@entry=0x55555557db30 <ga_apply_command_filters_iter>, > opaque=opaque@entry=0x555555633710) > at ../qapi/qmp-registry.c:93 > #3 0x0000555555571436 in ga_apply_command_filters (state=0x555555633710) > at ../qga/main.c:492 > #4 initialize_agent (config=0x555555632760, socket_activation=0) at > ../qga/main.c:1452 > #5 main (argc=<optimized out>, argv=<optimized out>) at ../qga/main.c:1646 > (gdb) p state.config > $1 = (GAConfig *) 0x0 > > (meson test fails too) > > I wonder why s->config is set so late in initialize_agent(). Moving it > earlier seems to solve the issue, but reviewing all code paths is tedious.. The ga_apply_command_filters() call can just be moved later, since the only constraint is that is called /before/ we call g_main_loop_run() to start processing I/O With regards, Daniel
diff --git a/qga/main.c b/qga/main.c index e8f52f0794..c7b7b0a9bc 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 @@ -1414,7 +1432,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); @@ -1438,25 +1455,8 @@ 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; - } + ga_apply_command_filters(s); - /* - * 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);
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. Centralizing the code in a single method "ga_apply_command_filters" will provide a strong guarantee of consistency and clarify the intended behaviour. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- qga/main.c | 110 ++++++++++++++++++++++++++--------------------------- 1 file changed, 55 insertions(+), 55 deletions(-)