Re: [PATCH 3/4] system/physmem.c: reorder usage and assertion of block

On 2/7/26 22:24, Peter Maydell wrote:

> On Sat, 7 Feb 2026 at 10:24, Sergei Heifetz <[email protected]> wrote:
>> On 2/4/26 17:53, Laurent Vivier wrote:
>>> Le 04/02/2026 à 08:57, Sergei Heifetz a écrit :
>>>> Reorder the code so the assertion of block occurs before it is
>>>> used in the subsequent lines.
>>>>
>>>> Signed-off-by: Sergei Heifetz <[email protected]>
>>>> ---
>>>>    system/physmem.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/system/physmem.c b/system/physmem.c
>>>> index b0311f4531..4308e02940 100644
>>>> --- a/system/physmem.c
>>>> +++ b/system/physmem.c
>>>> @@ -2054,11 +2054,11 @@ static int memory_try_enable_merging(void
>>>> *addr, size_t len)
>>>>     */
>>>>    int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
>>>>    {
>>>> +    assert(block);
>>>> +
>>>>        const ram_addr_t oldsize = block->used_length;
>>>>        const ram_addr_t unaligned_size = newsize;
>>>>    -    assert(block);
>>>> -
>>> According to coding style (docs/devel/style.rst):
>>>
>>> Mixed declarations (interleaving statements and declarations within
>>> blocks) are generally not allowed.
>>>
>>>>        newsize = TARGET_PAGE_ALIGN(newsize);
>>>>        newsize = REAL_HOST_PAGE_ALIGN(newsize);
>>> Thanks,
>>> Laurent
>> Should I remove the assertion altogether, then? I think the const
>> qualifier on oldsize is more useful than the assertion.
> I think personally I would drop the assertion. This falls into a
> category I think of as "assertion doesn't buy us anything".
> If block is NULL we're going to crash immediately when we
> dereference it, so the assert doesn't help to turn a hard-to-debug
> bug into an easy-to-debug bug (e.g. by catching incorrect values
> long before they're used) or to turn a nasty kind of failure
> into a safer one (e.g. by turning an array overrun into
> an assertion).
>
> And looking more closely, we call this function in only
> two places:
>   * migration/ram.c, where we've already dereferenced block
>     in the code just before the call
>   * system/memory.c, which asserts mr->ram_block before passing it in
>
> So I think the assert in this function isn't buying us anything,
> and the simplest thing is to delete it.
>
> thanks
> -- PMM
Thanks a lot for your detailed feedback. I completely agree with your 
points and will resubmit the patch without the assert.