[PATCH 0/2] migration: trivialities

[PATCH 0/2] migration: trivialities

[Laszlo Ersek]

SSIA; originally for RHBZ#2018404 (hence the links in the commit
messages).

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

Laszlo Ersek (2):
  migration: factor out "resume_requested" in qmp_migrate()
  migration: unexport migrate_fd_error()

 migration/migration.h |  1 -
 migration/migration.c | 12 +++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)


base-commit: 2a6ae69154542caa91dd17c40fd3f5ffbec300de

[PATCH 1/2] migration: factor out "resume_requested" in qmp_migrate()

[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;
     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);

[PATCH 2/2] migration: unexport migrate_fd_error()

[Laszlo Ersek]

The only migrate_fd_error() call sites are in "migration/migration.c",
which is also where we define migrate_fd_error(). Make the function
static, and remove its declaration from "migration/migration.h".

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.h | 1 -
 migration/migration.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index a80b22b703cd..b7c8b67542b8 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -466,7 +466,6 @@ bool  migration_has_all_channels(void);
 uint64_t migrate_max_downtime(void);
 
 void migrate_set_error(MigrationState *s, const Error *error);
-void migrate_fd_error(MigrationState *s, const Error *error);
 
 void migrate_fd_connect(MigrationState *s, Error *error_in);
 
diff --git a/migration/migration.c b/migration/migration.c
index a60a5acee533..91bba630a828 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1220,7 +1220,7 @@ static void migrate_error_free(MigrationState *s)
     }
 }
 
-void migrate_fd_error(MigrationState *s, const Error *error)
+static void migrate_fd_error(MigrationState *s, const Error *error)
 {
     trace_migrate_fd_error(error_get_pretty(error));
     assert(s->to_dst_file == NULL);

Re: [PATCH 1/2] migration: factor out "resume_requested" in qmp_migrate()

[Juan Quintela]

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.

Re: [PATCH 2/2] migration: unexport migrate_fd_error()

[Juan Quintela]

Laszlo Ersek <lersek@redhat.com> wrote:
> The only migrate_fd_error() call sites are in "migration/migration.c",
> which is also where we define migrate_fd_error(). Make the function
> static, and remove its declaration from "migration/migration.h".
>
> 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>

queued.

Re: [PATCH 0/2] migration: trivialities

[Peter Xu]

On Thu, Jul 06, 2023 at 12:29:35PM +0200, Laszlo Ersek wrote:
> SSIA; originally for RHBZ#2018404 (hence the links in the commit
> messages).
> 
> 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
> 
> Laszlo Ersek (2):
>   migration: factor out "resume_requested" in qmp_migrate()
>   migration: unexport migrate_fd_error()

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 1/2] migration: factor out "resume_requested" in qmp_migrate()

[Michael Tokarev]

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

Re: [PATCH 1/2] migration: factor out "resume_requested" in qmp_migrate()

[Laszlo Ersek]

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

Re: [PATCH 1/2] migration: factor out "resume_requested" in qmp_migrate()

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 2/2] migration: unexport migrate_fd_error()

[Philippe Mathieu-Daudé]

On 6/7/23 12:29, Laszlo Ersek wrote:
> The only migrate_fd_error() call sites are in "migration/migration.c",
> which is also where we define migrate_fd_error(). Make the function
> static, and remove its declaration from "migration/migration.h".
> 
> 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.h | 1 -
>   migration/migration.c | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/2] migration: trivialities

[Michael Tokarev]

06.07.2023 13:29, Laszlo Ersek wrote:
> SSIA; originally for RHBZ#2018404 (hence the links in the commit
> messages).
> 
> 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
> 
> Laszlo Ersek (2):
>    migration: factor out "resume_requested" in qmp_migrate()
>    migration: unexport migrate_fd_error()
> 
>   migration/migration.h |  1 -
>   migration/migration.c | 12 +++++++-----
>   2 files changed, 7 insertions(+), 6 deletions(-)

Applied to trivial-patches tree, thanks!

/mjt