Reorder the code so the assertion of block occurs before it is
used in the subsequent lines.
Signed-off-by: Sergei Heifetz <heifetz@yandex-team.com>
---
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);
-
newsize = TARGET_PAGE_ALIGN(newsize);
newsize = REAL_HOST_PAGE_ALIGN(newsize);
--
2.34.1Le 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 <heifetz@yandex-team.com>
> ---
> 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
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 <heifetz@yandex-team.com>
>> ---
>> 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.
On Sat, 7 Feb 2026 at 10:24, Sergei Heifetz <heifetz@yandex-team.com> 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 <heifetz@yandex-team.com>
> >> ---
> >> 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
On 2/7/26 22:24, Peter Maydell wrote:
> On Sat, 7 Feb 2026 at 10:24, Sergei Heifetz <heifetz@yandex-team.com> 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 <heifetz@yandex-team.com>
>>>> ---
>>>> 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.
On Sat, 7 Feb 2026, Sergei Heifetz 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 <heifetz@yandex-team.com>
>>> ---
>>> 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.
What does the const qualifier have to do with dereferencing block when
it's NULL? You'd just need to split the line as:
int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
{
const ram_addr_t oldsize;
const ram_addr_t unaligned_size = newsize;
assert(block);
oldsize = block->used_length;
How is that not obvious? The commit message also does not say why do you
want to change this. How did you come up with this patch, what issue does
it fix when block is NULL here?
Regards,
BALATON Zoltan
On 2/7/26 16:16, BALATON Zoltan wrote:
> On Sat, 7 Feb 2026, Sergei Heifetz 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 <heifetz@yandex-team.com>
>>>> ---
>>>> 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.
>
> What does the const qualifier have to do with dereferencing block when
> it's NULL? You'd just need to split the line as:
>
> int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
> {
> const ram_addr_t oldsize;
> const ram_addr_t unaligned_size = newsize;
>
> assert(block);
> oldsize = block->used_length;
This code won't compile because you are trying to assign a new value to
a const variable. That's precisely why you would need to remove the
const qualifier from oldsize in order to keep the assert.
> How is that not obvious? The commit message also does not say why do
> you want to change this. How did you come up with this patch, what
> issue does it fix when block is NULL here?
The original code contains a useless assert that comes after
dereferencing block. The patch fixes that.
On Fri, 6 Feb 2026 at 13:33, Sergei Heifetz <heifetz@yandex-team.com> wrote:
>
>
>
> 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);
>
>
>
> Hello, Laurent. Please note that I didn't touch these lines.
Laurent's comment is not about those two lines, but about
the ones above, that you don't quote in your reply. (Laurent
is following the usual mailing list style of interleaved commentary,
so his remark follows the part of your email that it is in
response to.)
> @@ -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);
> -
You've moved a code line (the assert) above the declaration lines, so
the function used to follow this coding style rule, but now it doesn't.
thanks
-- PMM
© 2016 - 2026 Red Hat, Inc.