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

["Daniel P. Berrangé" <berrange@redhat.com>]

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