Re: [Qemu-devel] [PATCH v2 08/10] migration: No need to return the size of the cache

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) 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 <address@hidden>
>> ---
>>  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 / address@hidden / Manchester, UK