| Message ID | 20190424004700.12766-2-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cleanup savevm | expand |
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 (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
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;
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(-)