"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
>>
>> > * Markus Armbruster (arm...@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 <arm...@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
