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
