* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> > * Juan Quintela (quint...@redhat.com) wrote:
> >> After the previous commits, we make sure that the value passed is
> >> right, or we just drop an error. So now we return if there is one
> >> error or we have setup correctly the value passed.
> >>
> >> Signed-off-by: Juan Quintela <quint...@redhat.com>
> >> ---
> >> migration/migration.c | 6 ++----
> >> migration/ram.c | 8 +++-----
> >> migration/ram.h | 2 +-
> >> 3 files changed, 6 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 3feffb5e26..f3d4503ce2 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -1371,14 +1371,12 @@ void qmp_migrate_cancel(Error **errp)
> >> void qmp_migrate_set_cache_size(int64_t value, Error **errp)
> >> {
> >> MigrationState *s = migrate_get_current();
> >> - int64_t new_size;
> >>
> >> - new_size = xbzrle_cache_resize(value, errp);
> >> - if (new_size < 0) {
> >> + if (xbzrle_cache_resize(value, errp) < 0) {
> >
> > That's not consistent with the function below; it returns
> > '0 or negative' for error; this check only tests for negative
> > as an error.
>
> int xbzrle_cache_resize(int64_t new_size, Error **errp)
> {
> PageCache *new_cache;
> int64_t ret = 0;
>
> /* Check for truncation */
> if (new_size != (size_t)new_size) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> "exceeding address space");
> return -1;
> }
>
> if (new_size == migrate_xbzrle_cache_size()) {
> /* nothing to do */
> return new_size;
> }
>
> XBZRLE_cache_lock();
>
> if (XBZRLE.cache != NULL) {
> new_cache = cache_init(new_size, TARGET_PAGE_SIZE, errp);
> if (!new_cache) {
> ret = -1;
> goto out;
> }
>
> cache_fini(XBZRLE.cache);
> XBZRLE.cache = new_cache;
> }
> out:
> XBZRLE_cache_unlock();
> return ret;
> }
>
>
> That is the function that we end having.
> -1 means error
> 0 means success (yes, I should have changed the return new_size to
> return 0).
>
> I *think* it is ok. Notice that this is after we have changed all users
> to know that it returns an error or set the value that it has been
> passed.
There's two problems with that function (as shown):
a) The comment (which isn't shown) is:
+ * Returns the 0 or negative in case of error.
so the comment says 0 means error which doesn't match your check
b) In the case of XBZRLE.cache == NULL; you end up returning 0 - that
feels like an error?
Dave
> Later, Juan.
>
>
> >
> > Dave
> >
> >> return;
> >> }
> >>
> >> - s->xbzrle_cache_size = new_size;
> >> + s->xbzrle_cache_size = value;
> >> }
> >>
> >> int64_t qmp_query_migrate_cache_size(Error **errp)
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index c84f22d759..ed4d3c6295 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -111,15 +111,15 @@ static void XBZRLE_cache_unlock(void)
> >> * migration may be using the cache and might finish during this call,
> >> * hence changes to the cache are protected by XBZRLE.lock().
> >> *
> >> - * Returns the new_size or negative in case of error.
> >> + * Returns the 0 or negative in case of error.
> >> *
> >> * @new_size: new cache size
> >> * @errp: set *errp if the check failed, with reason
> >> */
> >> -int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
> >> +int xbzrle_cache_resize(int64_t new_size, Error **errp)
> >> {
> >> PageCache *new_cache;
> >> - int64_t ret;
> >> + int64_t ret = 0;
> >>
> >> /* Check for truncation */
> >> if (new_size != (size_t)new_size) {
> >> @@ -152,8 +152,6 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error
> >> **errp)
> >> cache_fini(XBZRLE.cache);
> >> XBZRLE.cache = new_cache;
> >> }
> >> -
> >> - ret = new_size;
> >> out:
> >> XBZRLE_cache_unlock();
> >> return ret;
> >> diff --git a/migration/ram.h b/migration/ram.h
> >> index 511b3dc582..c8ae382b5b 100644
> >> --- a/migration/ram.h
> >> +++ b/migration/ram.h
> >> @@ -35,7 +35,7 @@
> >> extern MigrationStats ram_counters;
> >> extern XBZRLECacheStats xbzrle_counters;
> >>
> >> -int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
> >> +int xbzrle_cache_resize(int64_t new_size, Error **errp);
> >> uint64_t ram_bytes_remaining(void);
> >> uint64_t ram_bytes_total(void);
> >>
> >> --
> >> 2.13.6
> >>
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
