"Dr. David Alan Gilbert" <[email protected]> writes:
> * Markus Armbruster ([email protected]) 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 <[email protected]>
>> ---
>> 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.
