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

[Daniel P . Berrangé]

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
-- 
|: 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 :|