migrate-set-parameters passes the size to xbzrle_cache_resize().
xbzrle_cache_resize() checks it fits into size_t before it passes it
on to cache_init(). cache_init() checks it is a power of two no
smaller than @page_size.
cache_init() is also called from xbzrle_init(), bypassing
xbzrle_cache_resize()'s check.
Drop xbzrle_cache_resize()'s check, and check more carefully in
cache_init().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
migration/page_cache.c | 15 ++++-----------
migration/ram.c | 7 -------
2 files changed, 4 insertions(+), 18 deletions(-)
diff --git a/migration/page_cache.c b/migration/page_cache.c
index b384f265fb..e07f4ad1dc 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -41,17 +41,10 @@ struct PageCache {
PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
{
int64_t i;
- size_t num_pages = new_size / page_size;
+ uint64_t num_pages = new_size / page_size;
PageCache *cache;
- if (new_size < page_size) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
- "is smaller than one target page size");
- return NULL;
- }
-
- /* round down to the nearest power of 2 */
- if (!is_power_of_2(num_pages)) {
+ if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
"is not a power of two number of pages");
return NULL;
@@ -71,8 +64,8 @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
trace_migration_pagecache_init(cache->max_num_items);
/* We prefer not to abort if there is no memory */
- cache->page_cache = g_try_malloc((cache->max_num_items) *
- sizeof(*cache->page_cache));
+ cache->page_cache = g_try_malloc_n(cache->max_num_items,
+ sizeof(*cache->page_cache));
if (!cache->page_cache) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
"Failed to allocate page cache");
diff --git a/migration/ram.c b/migration/ram.c
index a84425d04f..d632ae694c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -131,13 +131,6 @@ int xbzrle_cache_resize(uint64_t new_size, Error **errp)
PageCache *new_cache;
int64_t ret = 0;
- /* Check for truncation */
- if (new_size != (size_t)new_size) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
- "exceeding address space");
- return -1;
- }
-
if (new_size == migrate_xbzrle_cache_size()) {
/* nothing to do */
return 0;
--
2.26.2
* Markus Armbruster (armbru@redhat.com) wrote:
> migrate-set-parameters passes the size to xbzrle_cache_resize().
> xbzrle_cache_resize() checks it fits into size_t before it passes it
> on to cache_init(). cache_init() checks it is a power of two no
> smaller than @page_size.
>
> cache_init() is also called from xbzrle_init(), bypassing
> xbzrle_cache_resize()'s check.
>
> Drop xbzrle_cache_resize()'s check, and check more carefully in
> cache_init().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> migration/page_cache.c | 15 ++++-----------
> migration/ram.c | 7 -------
> 2 files changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index b384f265fb..e07f4ad1dc 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -41,17 +41,10 @@ struct PageCache {
> PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
> {
> int64_t i;
> - size_t num_pages = new_size / page_size;
> + uint64_t num_pages = new_size / page_size;
> PageCache *cache;
>
> - if (new_size < page_size) {
> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> - "is smaller than one target page size");
> - return NULL;
> - }
> -
> - /* round down to the nearest power of 2 */
> - if (!is_power_of_2(num_pages)) {
> + if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> "is not a power of two number of pages");
That error message is now wrong since it includes a whole bunch of
reasons.
Also, the comparison is now on the divided num_pages, it's not that
obvious to me that checking the num_pages makes sense in acomparison to
checking the actual cache size.
(Arguably the check should also happen in migrate_params_test_apply)
Dave
> return NULL;
> @@ -71,8 +64,8 @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
> trace_migration_pagecache_init(cache->max_num_items);
>
> /* We prefer not to abort if there is no memory */
> - cache->page_cache = g_try_malloc((cache->max_num_items) *
> - sizeof(*cache->page_cache));
> + cache->page_cache = g_try_malloc_n(cache->max_num_items,
> + sizeof(*cache->page_cache));
> if (!cache->page_cache) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> "Failed to allocate page cache");
> diff --git a/migration/ram.c b/migration/ram.c
> index a84425d04f..d632ae694c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -131,13 +131,6 @@ int xbzrle_cache_resize(uint64_t new_size, Error **errp)
> PageCache *new_cache;
> int64_t ret = 0;
>
> - /* Check for truncation */
> - if (new_size != (size_t)new_size) {
> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> - "exceeding address space");
> - return -1;
> - }
> -
> if (new_size == migrate_xbzrle_cache_size()) {
> /* nothing to do */
> return 0;
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> migrate-set-parameters passes the size to xbzrle_cache_resize().
>> xbzrle_cache_resize() checks it fits into size_t before it passes it
>> on to cache_init(). cache_init() checks it is a power of two no
>> smaller than @page_size.
>>
>> cache_init() is also called from xbzrle_init(), bypassing
>> xbzrle_cache_resize()'s check.
>>
>> Drop xbzrle_cache_resize()'s check, and check more carefully in
>> cache_init().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> migration/page_cache.c | 15 ++++-----------
>> migration/ram.c | 7 -------
>> 2 files changed, 4 insertions(+), 18 deletions(-)
>>
>> diff --git a/migration/page_cache.c b/migration/page_cache.c
>> index b384f265fb..e07f4ad1dc 100644
>> --- a/migration/page_cache.c
>> +++ b/migration/page_cache.c
>> @@ -41,17 +41,10 @@ struct PageCache {
>> PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>> {
>> int64_t i;
>> - size_t num_pages = new_size / page_size;
>> + uint64_t num_pages = new_size / page_size;
>> PageCache *cache;
>>
>> - if (new_size < page_size) {
>> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> - "is smaller than one target page size");
>> - return NULL;
>> - }
>> -
>> - /* round down to the nearest power of 2 */
>> - if (!is_power_of_2(num_pages)) {
>> + if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
>> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> "is not a power of two number of pages");
>
> That error message is now wrong since it includes a whole bunch of
> reasons.
We could argue about "wrong", but I readily concedede it needs
improvement:
Parameter 'cache size' expects is not a power of two number of pages
is crap. I fixed similar crap in my "[PATCH 00/10] Chipping away at
qerror.h", but missed this one.
What about
Parameter 'xbzrle-cache-size' expects a power of two larger than $page_size
?
> Also, the comparison is now on the divided num_pages, it's not that
> obvious to me that checking the num_pages makes sense in acomparison to
> checking the actual cache size.
Would you accept
if (!is_power_of_2(new_size)
|| !num_pages || num_pages != (size_t)num_pages) {
?
If not, please propose something you like better.
> (Arguably the check should also happen in migrate_params_test_apply)
Feels like one bridge too far for this patch.
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> migrate-set-parameters passes the size to xbzrle_cache_resize().
> >> xbzrle_cache_resize() checks it fits into size_t before it passes it
> >> on to cache_init(). cache_init() checks it is a power of two no
> >> smaller than @page_size.
> >>
> >> cache_init() is also called from xbzrle_init(), bypassing
> >> xbzrle_cache_resize()'s check.
> >>
> >> Drop xbzrle_cache_resize()'s check, and check more carefully in
> >> cache_init().
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >> migration/page_cache.c | 15 ++++-----------
> >> migration/ram.c | 7 -------
> >> 2 files changed, 4 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/migration/page_cache.c b/migration/page_cache.c
> >> index b384f265fb..e07f4ad1dc 100644
> >> --- a/migration/page_cache.c
> >> +++ b/migration/page_cache.c
> >> @@ -41,17 +41,10 @@ struct PageCache {
> >> PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
> >> {
> >> int64_t i;
> >> - size_t num_pages = new_size / page_size;
> >> + uint64_t num_pages = new_size / page_size;
> >> PageCache *cache;
> >>
> >> - if (new_size < page_size) {
> >> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> >> - "is smaller than one target page size");
> >> - return NULL;
> >> - }
> >> -
> >> - /* round down to the nearest power of 2 */
> >> - if (!is_power_of_2(num_pages)) {
> >> + if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
> >> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> >> "is not a power of two number of pages");
> >
> > That error message is now wrong since it includes a whole bunch of
> > reasons.
>
> We could argue about "wrong", but I readily concedede it needs
> improvement:
>
> Parameter 'cache size' expects is not a power of two number of pages
>
> is crap. I fixed similar crap in my "[PATCH 00/10] Chipping away at
> qerror.h"
The wording may be crap, but it does at least talk about the correct
problem.
> but missed this one.
>
> What about
>
> Parameter 'xbzrle-cache-size' expects a power of two larger than $page_size
Yes, although you've also put a too-large check in ther=e with that
size_t cast.
> ?
>
> > Also, the comparison is now on the divided num_pages, it's not that
> > obvious to me that checking the num_pages makes sense in acomparison to
> > checking the actual cache size.
>
> Would you accept
>
> if (!is_power_of_2(new_size)
> || !num_pages || num_pages != (size_t)num_pages) {
Well, why is it not || new_size != (size_t)new_size like in the
original?
> ?
>
> If not, please propose something you like better.
>
> > (Arguably the check should also happen in migrate_params_test_apply)
>
> Feels like one bridge too far for this patch.
Sure.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>>
>> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> migrate-set-parameters passes the size to xbzrle_cache_resize().
>> >> xbzrle_cache_resize() checks it fits into size_t before it passes it
>> >> on to cache_init(). cache_init() checks it is a power of two no
>> >> smaller than @page_size.
>> >>
>> >> cache_init() is also called from xbzrle_init(), bypassing
>> >> xbzrle_cache_resize()'s check.
>> >>
>> >> Drop xbzrle_cache_resize()'s check, and check more carefully in
>> >> cache_init().
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >> migration/page_cache.c | 15 ++++-----------
>> >> migration/ram.c | 7 -------
>> >> 2 files changed, 4 insertions(+), 18 deletions(-)
>> >>
>> >> diff --git a/migration/page_cache.c b/migration/page_cache.c
>> >> index b384f265fb..e07f4ad1dc 100644
>> >> --- a/migration/page_cache.c
>> >> +++ b/migration/page_cache.c
>> >> @@ -41,17 +41,10 @@ struct PageCache {
>> >> PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>> >> {
>> >> int64_t i;
>> >> - size_t num_pages = new_size / page_size;
>> >> + uint64_t num_pages = new_size / page_size;
>> >> PageCache *cache;
>> >>
>> >> - if (new_size < page_size) {
>> >> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> >> - "is smaller than one target page size");
>> >> - return NULL;
>> >> - }
>> >> -
>> >> - /* round down to the nearest power of 2 */
>> >> - if (!is_power_of_2(num_pages)) {
>> >> + if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
>> >> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> >> "is not a power of two number of pages");
>> >
>> > That error message is now wrong since it includes a whole bunch of
>> > reasons.
>>
>> We could argue about "wrong", but I readily concedede it needs
>> improvement:
>>
>> Parameter 'cache size' expects is not a power of two number of pages
>>
>> is crap. I fixed similar crap in my "[PATCH 00/10] Chipping away at
>> qerror.h"
>
> The wording may be crap, but it does at least talk about the correct
> problem.
>
>> but missed this one.
>>
>> What about
>>
>> Parameter 'xbzrle-cache-size' expects a power of two larger than $page_size
>
> Yes, although you've also put a too-large check in ther=e with that
> size_t cast.
I'm not sure I understand what you want.
Personally, I hate it when a computer tells me "your value doesn't
satisfy X", and when I adjust my value, it tells me "now it does, but it
also has to satify Y, so there!" Compute, just tell me straight what
you want!
I hasten to add there are exceptions, say when "X" or "Y" are so
complicated that "X and Y" becomes bad UI.
That said, this is just a small drive-by cleanup. I'm happy to adjust
it to your liking, I'm happy to drop it, just tell me what you want :)
>> ?
>>
>> > Also, the comparison is now on the divided num_pages, it's not that
>> > obvious to me that checking the num_pages makes sense in acomparison to
>> > checking the actual cache size.
>>
>> Would you accept
>>
>> if (!is_power_of_2(new_size)
>> || !num_pages || num_pages != (size_t)num_pages) {
>
> Well, why is it not || new_size != (size_t)new_size like in the
> original?
Because nothing in this function actually depends on new_size fitting
into size_t.
My num_pages != (size_t)num_pages guards
cache->max_num_items = num_pages;
>> ?
>>
>> If not, please propose something you like better.
>>
>> > (Arguably the check should also happen in migrate_params_test_apply)
>>
>> Feels like one bridge too far for this patch.
>
> Sure.
>
> Dave
© 2016 - 2026 Red Hat, Inc.