On 4/9/2025 1:43 PM, Alexey Kardashevskiy wrote:
>
>
> On 7/4/25 17: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.
>> ---
>> hw/virtio/virtio-mem.c | 20 ++++++++++----------
>> include/exec/memory.h | 31 ++++++++++++++++---------------
>> migration/ram.c | 5 +++--
>> system/memory.c | 12 ++++++------
>> 4 files changed, 35 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
>> index d0d3a0240f..1a88d649cb 100644
>> --- a/hw/virtio/virtio-mem.c
>> +++ b/hw/virtio/virtio-mem.c
>> @@ -1733,7 +1733,7 @@ static bool virtio_mem_rdm_is_populated(const
>> RamDiscardManager *rdm,
>> }
>> struct VirtIOMEMReplayData {
>> - void *fn;
>> + ReplayStateChange fn;
>
>
> s/ReplayStateChange/ReplayRamStateChange/
>
> Just "State" is way too generic imho.
LGTM.
>
>
>> void *opaque;
>> };
>> @@ -1741,12 +1741,12 @@ static int
>> virtio_mem_rdm_replay_populated_cb(MemoryRegionSection *s, void *arg)
>> {
>> struct VirtIOMEMReplayData *data = arg;
>> - return ((ReplayRamPopulate)data->fn)(s, data->opaque);
>> + return data->fn(s, data->opaque);
>> }
>> static int virtio_mem_rdm_replay_populated(const RamDiscardManager
>> *rdm,
>> MemoryRegionSection *s,
>> - ReplayRamPopulate replay_fn,
>> + ReplayStateChange replay_fn,
>> void *opaque)
>> {
>> const VirtIOMEM *vmem = VIRTIO_MEM(rdm);
>> @@ -1765,14 +1765,14 @@ static int
>> virtio_mem_rdm_replay_discarded_cb(MemoryRegionSection *s,
>> {
>> struct VirtIOMEMReplayData *data = arg;
>> - ((ReplayRamDiscard)data->fn)(s, data->opaque);
>> + data->fn(s, data->opaque);
>> return 0;
>
> return data->fn(s, data->opaque); ?
>
> Or a comment why we ignore the return result? Thanks,
You are right, since we have made ReplayRamDiscard() return the results,
it is doable to use "return data->fn(s, data->opaque)" directly. Thanks.
>
>> }
>> -static void virtio_mem_rdm_replay_discarded(const RamDiscardManager
>> *rdm,
>> - MemoryRegionSection *s,
>> - ReplayRamDiscard replay_fn,
>> - void *opaque)
>> +static int virtio_mem_rdm_replay_discarded(const RamDiscardManager *rdm,
>> + MemoryRegionSection *s,
>> + ReplayStateChange replay_fn,
>> + void *opaque)
>> {
>> const VirtIOMEM *vmem = VIRTIO_MEM(rdm);
>> struct VirtIOMEMReplayData data = {
>> @@ -1781,8 +1781,8 @@ static void
>> virtio_mem_rdm_replay_discarded(const RamDiscardManager *rdm,
>> };
>> g_assert(s->mr == &vmem->memdev->mr);
>> - virtio_mem_for_each_unplugged_section(vmem, s, &data,
>> -
>> virtio_mem_rdm_replay_discarded_cb);
>> + return virtio_mem_for_each_unplugged_section(vmem, s, &data,
>> +
>> virtio_mem_rdm_replay_discarded_cb);
>
>
> a nit: "WARNING: line over 80 characters" - I have no idea what is the
> best thing to do here though.
It is not easy to adjust the line. Then I think we can keep it.
>
>
>
