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