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

[Laszlo Ersek <lersek@redhat.com>]

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