[RFC,for-2.3,4/6] qga: implement qmp_guest_set_memory_blocks() for Linux with sysfs

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 qga/commands-posix.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

Quoting zhanghailiang (2014-12-06 00:59:17)
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  qga/commands-posix.c | 42 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index d3f7d4f..1010e86 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2059,6 +2059,34 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>      error_propagate(errp, local_err);
>      return NULL;
>  }
> +
> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
> +                                    Error **errp)
> +{
> +    int64_t processed;
> +    Error *local_err = NULL;
> +
> +    processed = 0;
> +    while (mem_blks != NULL) {
> +        transfer_memory_block(mem_blks->value, false, &local_err);
> +        if (local_err) {
> +            break;
> +        }

The documentation claims to maximize the number of blocks we set in the event
of an error, which would suggest we continue processing the remainder of the
list rather than breaking here.

> +        ++processed;
> +        mem_blks = mem_blks->next;
> +    }
> +
> +    if (local_err) {
> +        if (processed == 0) {
> +            error_propagate(errp, local_err);
> +        } else {
> +            error_free(local_err);
> +        }

Really think a list of error/status would make it easier for management to make
sense of things.

> +    }
> +
> +    return processed;
> +}
> +
>  #else /* defined(__linux__) */
> 
>  void qmp_guest_suspend_disk(Error **errp)
> @@ -2100,6 +2128,13 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>      return NULL;
>  }
> 
> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
> +                                    Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;
> +}
> +
>  #endif
> 
>  #if !defined(CONFIG_FSFREEZE)
> @@ -2186,13 +2221,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      return blacklist;
>  }
> 
> -int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
> -                                    Error **errp)
> -{
> -    error_set(errp, QERR_UNSUPPORTED);
> -    return -1;
> -}
> -

Same comment here as with prior patch

>  int64_t qmp_guest_get_memory_block_size(Error **errp)
>  {
>      error_set(errp, QERR_UNSUPPORTED);
> -- 
> 1.7.12.4

On 2014/12/22 5:23, Michael Roth wrote:
> Quoting zhanghailiang (2014-12-06 00:59:17)
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   qga/commands-posix.c | 42 +++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 35 insertions(+), 7 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index d3f7d4f..1010e86 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2059,6 +2059,34 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>>       error_propagate(errp, local_err);
>>       return NULL;
>>   }
>> +
>> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>> +                                    Error **errp)
>> +{
>> +    int64_t processed;
>> +    Error *local_err = NULL;
>> +
>> +    processed = 0;
>> +    while (mem_blks != NULL) {
>> +        transfer_memory_block(mem_blks->value, false, &local_err);
>> +        if (local_err) {
>> +            break;
>> +        }
>
> The documentation claims to maximize the number of blocks we set in the event
> of an error, which would suggest we continue processing the remainder of the
> list rather than breaking here.
>

OK, one more thing, should we fix like that for set-vcpus ?

>> +        ++processed;
>> +        mem_blks = mem_blks->next;
>> +    }
>> +
>> +    if (local_err) {
>> +        if (processed == 0) {
>> +            error_propagate(errp, local_err);
>> +        } else {
>> +            error_free(local_err);
>> +        }
>
> Really think a list of error/status would make it easier for management to make
> sense of things.
>

Agreed :)

>> +    }
>> +
>> +    return processed;
>> +}
>> +
>>   #else /* defined(__linux__) */
>>
>>   void qmp_guest_suspend_disk(Error **errp)
>> @@ -2100,6 +2128,13 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>>       return NULL;
>>   }
>>
>> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>> +                                    Error **errp)
>> +{
>> +    error_set(errp, QERR_UNSUPPORTED);
>> +    return -1;
>> +}
>> +
>>   #endif
>>
>>   #if !defined(CONFIG_FSFREEZE)
>> @@ -2186,13 +2221,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>>       return blacklist;
>>   }
>>
>> -int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>> -                                    Error **errp)
>> -{
>> -    error_set(errp, QERR_UNSUPPORTED);
>> -    return -1;
>> -}
>> -
>
> Same comment here as with prior patch
>

OK, thanks.

>>   int64_t qmp_guest_get_memory_block_size(Error **errp)
>>   {
>>       error_set(errp, QERR_UNSUPPORTED);
>> --
>> 1.7.12.4
>
>
> .
>