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é" <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.
> > 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é <berra...@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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
