Hi
On Tue, Jun 4, 2024 at 5:51 PM Daniel P. Berrangé <berra...@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é <berra...@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
>
>
>
--
Marc-André Lureau
