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