Patchwork savevm: set right return value for qemu_file_rate_limit

login
register
mail settings
Submitter Lei Li
Date July 21, 2013, 12:56 p.m.
Message ID <1374411399-17535-1-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/260520/
State New
Headers show

Comments

Lei Li - July 21, 2013, 12:56 p.m.
Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate
limiting to QEMUFile, but set the return value for qemu_file_rate_limit
to 1 in the case of qemu_file_get_error. It is wrong and should be negative
compared to the original function buffered_rate_limit and the current logic
in ram_save_iterate. As qemu_file_rate_limit is called manually to determine
if it has to exit, add the defination of the meaning of the return values
as well.


Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 savevm.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
Lei Li - July 24, 2013, 3:48 p.m.
In the logic of ram_save_iterate(), it checks the ret of qemu_file_rate_limit(f)
by ret < 0 if there has been an error. But now it will never return negative value
because qemu_file_rate_limit() return 1 if qemu_file_get_error().

Also the original implementation of qemu_file_rate_limit() as function
buffered_rate_limit() did return negative for this situation.
  

On 07/21/2013 08:56 PM, Lei Li wrote:
> Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate
> limiting to QEMUFile, but set the return value for qemu_file_rate_limit
> to 1 in the case of qemu_file_get_error. It is wrong and should be negative
> compared to the original function buffered_rate_limit and the current logic
> in ram_save_iterate. As qemu_file_rate_limit is called manually to determine
> if it has to exit, add the defination of the meaning of the return values
> as well.
>
>
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>   savevm.c |   14 ++++++++++++--
>   1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index e0491e7..f406790 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -904,10 +904,20 @@ int64_t qemu_ftell(QEMUFile *f)
>       return f->pos;
>   }
>
> +/*
> + * The meaning of the return values is:
> + *   0: We can continue sending
> + *   1: Time to stop
> + *   negative: There has been an error
> + */
> +
>   int qemu_file_rate_limit(QEMUFile *f)
>   {
> -    if (qemu_file_get_error(f)) {
> -        return 1;
> +    int ret;
> +
> +    ret = qemu_file_get_error(f);
> +    if (ret) {
> +        return ret;
>       }
>       if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) {
>           return 1;
Lei Li - Aug. 5, 2013, 8:59 a.m.
PING? any comments?

On 07/24/2013 11:48 PM, Lei Li wrote:
> In the logic of ram_save_iterate(), it checks the ret of 
> qemu_file_rate_limit(f)
> by ret < 0 if there has been an error. But now it will never return 
> negative value
> because qemu_file_rate_limit() return 1 if qemu_file_get_error().
>
> Also the original implementation of qemu_file_rate_limit() as function
> buffered_rate_limit() did return negative for this situation.
>
>
> On 07/21/2013 08:56 PM, Lei Li wrote:
>> Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate
>> limiting to QEMUFile, but set the return value for qemu_file_rate_limit
>> to 1 in the case of qemu_file_get_error. It is wrong and should be 
>> negative
>> compared to the original function buffered_rate_limit and the current 
>> logic
>> in ram_save_iterate. As qemu_file_rate_limit is called manually to 
>> determine
>> if it has to exit, add the defination of the meaning of the return 
>> values
>> as well.
>>
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   savevm.c |   14 ++++++++++++--
>>   1 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index e0491e7..f406790 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -904,10 +904,20 @@ int64_t qemu_ftell(QEMUFile *f)
>>       return f->pos;
>>   }
>>
>> +/*
>> + * The meaning of the return values is:
>> + *   0: We can continue sending
>> + *   1: Time to stop
>> + *   negative: There has been an error
>> + */
>> +
>>   int qemu_file_rate_limit(QEMUFile *f)
>>   {
>> -    if (qemu_file_get_error(f)) {
>> -        return 1;
>> +    int ret;
>> +
>> +    ret = qemu_file_get_error(f);
>> +    if (ret) {
>> +        return ret;
>>       }
>>       if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) {
>>           return 1;
>
>

Patch

diff --git a/savevm.c b/savevm.c
index e0491e7..f406790 100644
--- a/savevm.c
+++ b/savevm.c
@@ -904,10 +904,20 @@  int64_t qemu_ftell(QEMUFile *f)
     return f->pos;
 }
 
+/*
+ * The meaning of the return values is:
+ *   0: We can continue sending
+ *   1: Time to stop
+ *   negative: There has been an error
+ */
+
 int qemu_file_rate_limit(QEMUFile *f)
 {
-    if (qemu_file_get_error(f)) {
-        return 1;
+    int ret;
+
+    ret = qemu_file_get_error(f);
+    if (ret) {
+        return ret;
     }
     if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) {
         return 1;