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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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);
>> +}