| Message ID | 20230706102937.82490-2-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: trivialities | expand |
Laszlo Ersek <lersek@redhat.com> wrote: > It cuts back on those awkward, duplicated !(has_resume && resume) > expressions. > > Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration) > Cc: Leonardo Bras <leobras@redhat.com> (reviewer:Migration) > Cc: Peter Xu <peterx@redhat.com> (reviewer:Migration) > Cc: qemu-trivial@nongnu.org > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2018404 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Thanks. queued.
06.07.2023 13:29, Laszlo Ersek пишет: > It cuts back on those awkward, duplicated !(has_resume && resume) > expressions. > > Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration) > Cc: Leonardo Bras <leobras@redhat.com> (reviewer:Migration) > Cc: Peter Xu <peterx@redhat.com> (reviewer:Migration) > Cc: qemu-trivial@nongnu.org > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2018404 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > migration/migration.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 096e8191d15c..a60a5acee533 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1637,6 +1637,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > bool has_inc, bool inc, bool has_detach, bool detach, > bool has_resume, bool resume, Error **errp) > { > + bool resume_requested; > Error *local_err = NULL; > MigrationState *s = migrate_get_current(); > const char *p = NULL; > @@ -1646,13 +1647,14 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > return; > } > > + resume_requested = has_resume && resume; Dunno if it's worth it or cleaner, but it can be reduced to if (!has_resume) resume = false; and checking for only resume below this point. In other words, there's no need for an additional local var. All other params (has_inc & inc, has_detach_detach etc) are like this too. Anyway, Reviewed-by: Michael Tokarev <mjt@tls.msk.ru> /mjt
On 7/6/23 15:28, Michael Tokarev wrote: > 06.07.2023 13:29, Laszlo Ersek пишет: >> It cuts back on those awkward, duplicated !(has_resume && resume) >> expressions. >> >> Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration) >> Cc: Leonardo Bras <leobras@redhat.com> (reviewer:Migration) >> Cc: Peter Xu <peterx@redhat.com> (reviewer:Migration) >> Cc: qemu-trivial@nongnu.org >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2018404 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> migration/migration.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 096e8191d15c..a60a5acee533 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -1637,6 +1637,7 @@ void qmp_migrate(const char *uri, bool has_blk, >> bool blk, >> bool has_inc, bool inc, bool has_detach, bool detach, >> bool has_resume, bool resume, Error **errp) >> { >> + bool resume_requested; >> Error *local_err = NULL; >> MigrationState *s = migrate_get_current(); >> const char *p = NULL; >> @@ -1646,13 +1647,14 @@ void qmp_migrate(const char *uri, bool >> has_blk, bool blk, >> return; >> } >> + resume_requested = has_resume && resume; > > Dunno if it's worth it or cleaner, but it can be reduced to > > if (!has_resume) resume = false; > > and checking for only resume below this point. > In other words, there's no need for an additional local var. I vehemently disagree with overwriting (input) parameters. One situation where that practice is a disaster is single-stepping through the function in an interactive debugger. You won't see the actual argument the function was originally called with. I know it's sometimes comfortable to just reuse a "count" input paramater as a loop index that runs to zero -- I resist that too, it's a trap (for the same reason), IMO. > > All other params (has_inc & inc, has_detach_detach etc) are like this > too. > > Anyway, > > Reviewed-by: Michael Tokarev <mjt@tls.msk.ru> Thanks! Laszlo
On 6/7/23 12:29, Laszlo Ersek wrote: > It cuts back on those awkward, duplicated !(has_resume && resume) > expressions. > > Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration) > Cc: Leonardo Bras <leobras@redhat.com> (reviewer:Migration) > Cc: Peter Xu <peterx@redhat.com> (reviewer:Migration) > Cc: qemu-trivial@nongnu.org > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2018404 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > migration/migration.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/migration/migration.c b/migration/migration.c index 096e8191d15c..a60a5acee533 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1637,6 +1637,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, bool has_inc, bool inc, bool has_detach, bool detach, bool has_resume, bool resume, Error **errp) { + bool resume_requested; Error *local_err = NULL; MigrationState *s = migrate_get_current(); const char *p = NULL; @@ -1646,13 +1647,14 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, return; } + resume_requested = has_resume && resume; if (!migrate_prepare(s, has_blk && blk, has_inc && inc, - has_resume && resume, errp)) { + resume_requested, errp)) { /* Error detected, put into errp */ return; } - if (!(has_resume && resume)) { + if (!resume_requested) { if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) { return; } @@ -1671,7 +1673,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, } else if (strstart(uri, "fd:", &p)) { fd_start_outgoing_migration(s, p, &local_err); } else { - if (!(has_resume && resume)) { + if (!resume_requested) { yank_unregister_instance(MIGRATION_YANK_INSTANCE); } error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri", @@ -1683,7 +1685,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, } if (local_err) { - if (!(has_resume && resume)) { + if (!resume_requested) { yank_unregister_instance(MIGRATION_YANK_INSTANCE); } migrate_fd_error(s, local_err);
It cuts back on those awkward, duplicated !(has_resume && resume) expressions. Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration) Cc: Leonardo Bras <leobras@redhat.com> (reviewer:Migration) Cc: Peter Xu <peterx@redhat.com> (reviewer:Migration) Cc: qemu-trivial@nongnu.org Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2018404 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- migration/migration.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)