On 4/25/2025 8:42 PM, David Hildenbrand wrote:
> On 07.04.25 09:49, Chenyi Qiang wrote:
>> Update ReplayRamDiscard() function to return the result and unify the
>> ReplayRamPopulate() and ReplayRamDiscard() to ReplayStateChange() at
>> the same time due to their identical definitions. This unification
>> simplifies related structures, such as VirtIOMEMReplayData, which makes
>> it more cleaner and maintainable.
>>
>> Signed-off-by: Chenyi Qiang <chenyi.qi...@intel.com>
>> ---
>> Changes in v4:
>> - Modify the commit message. We won't use Replay() operation when
>> doing the attribute change like v3.
>>
>> Changes in v3:
>> - Newly added.
>> ---
>
> [...]
>
>> -typedef int (*ReplayRamPopulate)(MemoryRegionSection *section, void
>> *opaque);
>> -typedef void (*ReplayRamDiscard)(MemoryRegionSection *section, void
>> *opaque);
>> +typedef int (*ReplayStateChange)(MemoryRegionSection *section, void
>> *opaque);
>>
>
> But it's not a state change.
>
> ReplayRamState maybe?
OK. Will rename it to ReplayRamDiscardState as mentioned in another
thread. Thanks.
>
> [...]
>> /*
>> diff --git a/system/memory.c b/system/memory.c
>> index 62d6b410f0..b5ab729e13 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -2147,7 +2147,7 @@ bool ram_discard_manager_is_populated(const
>> RamDiscardManager *rdm,
>> int ram_discard_manager_replay_populated(const RamDiscardManager
>> *rdm,
>> MemoryRegionSection *section,
>> - ReplayRamPopulate replay_fn,
>> + ReplayStateChange replay_fn,
>> void *opaque)
>> {
>> RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_GET_CLASS(rdm);
>> @@ -2156,15 +2156,15 @@ int ram_discard_manager_replay_populated(const
>> RamDiscardManager *rdm,
>> return rdmc->replay_populated(rdm, section, replay_fn, opaque);
>> }
>> -void ram_discard_manager_replay_discarded(const RamDiscardManager
>> *rdm,
>> - MemoryRegionSection *section,
>> - ReplayRamDiscard replay_fn,
>> - void *opaque)
>> +int ram_discard_manager_replay_discarded(const RamDiscardManager *rdm,
>> + MemoryRegionSection *section,
>> + ReplayStateChange replay_fn,
>> + void *opaque)
>> {
>> RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_GET_CLASS(rdm);
>> g_assert(rdmc->replay_discarded);
>> - rdmc->replay_discarded(rdm, section, replay_fn, opaque);
>> + return rdmc->replay_discarded(rdm, section, replay_fn, opaque);
>> }
>
> The idea was that ram_discard_manager_replay_discarded() would never be
> able to fail. But I don't think this really matters, because the
> function is provided by the caller, that can just always return 0 --
> like we do in dirty_bitmap_clear_section() now.
>
> So yeah, this looks fine to me, given that we don't call it a "state
> change" when we are merely replaying a selected state.
>
