| Message ID | 20201113065236.2644169-5-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Fixes and cleanups aroung migrate-set-parameters | expand |
* 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> 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> 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
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;
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(-)