Re: [PATCH v2 3/3] replay: do not build if TCG is not available

[Claudio Fontana]

Hi Paolo,

On 10/13/20 12:29 AM, Paolo Bonzini wrote:
> On 12/10/20 23:45, Claudio Fontana wrote:
>> +    ctx = blk_get_aio_context(blk);
>> +    if (!replay_bh_schedule_oneshot_event(ctx, error_callback_bh, acb)) {
>> +        /* regular case without replay */
>> +        aio_bh_schedule_oneshot(ctx, error_callback_bh, acb);
>> +    }
> 
> Why can't the stub just call aio_bh_schedule_oneshot?  

Absolutely, it can, I just considered the option and dropped it in the end.

> This makes the
> API even more complicated.

In my view not really, the API just returns a boolean that tells you if the 
event was consumed or not.

> 
> I think you are doing this in order to avoid link errors in tools, but

Not really, I am preferring this alternative not for linking reasons, but to 
make it better,

I just see the problem of putting actual functional code in stubs, that is 
something that violates least-surprise principle for me.
Any change to actual production code dealing with events needs changes in .. 
stubs?

Also factoring in this choice is trying to obviate to the fact that the replay 
code funnels all (many) QEMU events
into itself, so any investigation of the code needs to involve the replay 
framework, whether replay is enabled or not,
and whether replay is even built-in or not, and I think this is not a good idea.

I also considered just wrapping every event "hook" code around the codebase 
around an if (replay_events_enabled()) or if (tcg_enabled),
but this lead to code duplication, where the same code would be repeated, once 
inside replay-events when replay is disabled,
and once in the general code when replay is not compiled in.

Changing the API to return bool is_event_consumed just seemed the best option 
right now to me.


> it's not necessary. you can have more than one stub file:
> 
> - replay/replay-stub.c for functions needed by emulators (added with
> "if_false:", it also includes the monitor commands);
> 
> - stubs/replay.c for functions needed by tools (including
> replay_bh_schedule_oneshot_event which is currently in
> stubs/replay-user.c for some reason).
> 
> Paolo
> 

I understand this is a temporary measure anyway, waiting for better isolation 
of the replay framework inside tcg, and maybe a better way to graft its hooks 
to events in the rest of the code,
in a way that is less invasive and more automated (maybe something similar to 
trace?)

If people feel strongly that this is a wrong step, we can do the alternative 
and put production code inside the stubs, but it just seems wrong.

Thanks!

Ciao,

Claudio