* 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.
> ?
>
> > 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 / dgilb...@redhat.com / Manchester, UK
