Current call flow of save_snapshot is:

  save_snapshot
    migration_is_blocked
      qemu_savevm_state
        migration_is_blocked

Since qemu_savevm_state is only called in save_snapshot, this means
migration_is_blocked has been already checked.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/savevm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 92af2471cd..2eea604624 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1412,10 +1412,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
         return -EINVAL;
     }
 
-    if (migration_is_blocked(errp)) {
-        return -EINVAL;
-    }
-
     if (migrate_use_block()) {
         error_setg(errp, "Block migration and snapshots are incompatible");
         return -EINVAL;
-- 
2.19.1

On 4/23/19 9:46 PM, Wei Yang wrote:
> Current call flow of save_snapshot is:
>
>    save_snapshot
>      migration_is_blocked
>        qemu_savevm_state
>          migration_is_blocked
>
> Since qemu_savevm_state is only called in save_snapshot, this means
> migration_is_blocked has been already checked.

I think it would be a nice touch to add a comment in qemu_savevm_state,
saying that the function must be called with migration_is_blocked()
context. Just in case someone else in the future ends up re-using the
function.


Other than that, +1 for less code duplication.


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>




>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>   migration/savevm.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 92af2471cd..2eea604624 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1412,10 +1412,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>           return -EINVAL;
>       }
>   
> -    if (migration_is_blocked(errp)) {
> -        return -EINVAL;
> -    }
> -
>       if (migrate_use_block()) {
>           error_setg(errp, "Block migration and snapshots are incompatible");
>           return -EINVAL;

On Thu, Apr 25, 2019 at 04:20:57PM -0300, Daniel Henrique Barboza wrote:
>
>
>On 4/23/19 9:46 PM, Wei Yang wrote:
>> Current call flow of save_snapshot is:
>> 
>>    save_snapshot
>>      migration_is_blocked
>>        qemu_savevm_state
>>          migration_is_blocked
>> 
>> Since qemu_savevm_state is only called in save_snapshot, this means
>> migration_is_blocked has been already checked.
>
>I think it would be a nice touch to add a comment in qemu_savevm_state,
>saying that the function must be called with migration_is_blocked()
>context. Just in case someone else in the future ends up re-using the
>function.
>

That's reasonable, let me add a comment for qemu_savevm_state().

>
>Other than that, +1 for less code duplication.
>
>
>Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>
>
>
>
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>   migration/savevm.c | 4 ----
>>   1 file changed, 4 deletions(-)
>> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 92af2471cd..2eea604624 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1412,10 +1412,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>>           return -EINVAL;
>>       }
>> -    if (migration_is_blocked(errp)) {
>> -        return -EINVAL;
>> -    }
>> -
>>       if (migrate_use_block()) {
>>           error_setg(errp, "Block migration and snapshots are incompatible");
>>           return -EINVAL;

-- 
Wei Yang
Help you, Help me

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Current call flow of save_snapshot is:
> 
>   save_snapshot
>     migration_is_blocked
>       qemu_savevm_state
>         migration_is_blocked
> 
> Since qemu_savevm_state is only called in save_snapshot, this means
> migration_is_blocked has been already checked.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/savevm.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 92af2471cd..2eea604624 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1412,10 +1412,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>          return -EINVAL;
>      }
>  
> -    if (migration_is_blocked(errp)) {
> -        return -EINVAL;
> -    }
> -
>      if (migrate_use_block()) {
>          error_setg(errp, "Block migration and snapshots are incompatible");
>          return -EINVAL;
> -- 
> 2.19.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Current call flow of save_snapshot is:
> 
>   save_snapshot
>     migration_is_blocked
>       qemu_savevm_state
>         migration_is_blocked
> 
> Since qemu_savevm_state is only called in save_snapshot, this means
> migration_is_blocked has been already checked.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Queued 1,3,4

> ---
>  migration/savevm.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 92af2471cd..2eea604624 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1412,10 +1412,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>          return -EINVAL;
>      }
>  
> -    if (migration_is_blocked(errp)) {
> -        return -EINVAL;
> -    }
> -
>      if (migrate_use_block()) {
>          error_setg(errp, "Block migration and snapshots are incompatible");
>          return -EINVAL;
> -- 
> 2.19.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK