Re: [PATCH 4/6] migration: Check xbzrle-cache-size more carefully

[Markus Armbruster]

"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.