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

[Dr. David Alan Gilbert]

* 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 / Manchester, UK