Re: [PATCH v2 3/6] migration: HMP side changes for modified 'migrate' QAPI design

[Het Gala <het.gala@nutanix.com>]

On 09/02/23 7:30 pm, Daniel P. Berrangé wrote:
> On Thu, Feb 09, 2023 at 07:08:13PM +0530, Het Gala wrote:
>> On 09/02/23 5:35 pm, Daniel P. Berrangé wrote:
>>> On Wed, Feb 08, 2023 at 09:35:57AM +0000, Het Gala wrote:
>>>> hmp_migrate() stores modified QAPI 'migrate' arguments from qdict
>>>> into well defined MigrateChannel struct with help of
>>>> migrate_channel_from_qdict().
>>>> hmp_migrate() also accepts uri string as modified QAPI a 'migrate'
>>>> argument (for backward compatibility).
>>>>
>>>> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>>>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>>> ---
>>>>    migration/migration-hmp-cmds.c | 105 ++++++++++++++++++++++++++++++++-
>>>>    migration/migration.c          |  15 ++++-
>>>>    2 files changed, 116 insertions(+), 4 deletions(-)
>>>>
>
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 7a14aa98d8..f6dd8dbb03 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -2463,9 +2463,9 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>>>>        return true;
>>>>    }
>>>> -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)
>>>> +void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>>>> +                 bool blk, bool has_inc, bool inc, bool has_detach,
>>>> +                 bool detach, bool has_resume, bool resume, Error **errp)
>>>>    {
>>>>        Error *local_err = NULL;
>>>>        MigrationState *s = migrate_get_current();
>>>> @@ -2483,6 +2483,15 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>            }
>>>>        }
>>>> +    /*
>>>> +     * Having preliminary checks for uri and channel
>>>> +     */
>>>> +    if (uri && channel) {
>>>> +        error_setg(errp, "uri and channels options should be"
>>> s/should be/are/, also best to quote parameter names, eg
>>>
>>>       error_setg(errp,
>>>                  "'uri' and 'channels' options are mutually exclusive");
>> Ack.
>>>> +                          "mutually exclusive");
>>>> +        return;
>>>> +    }
>>>> +
>>> This change for qmp_migrate will need to be in patch 2.
>>>
>>> QEMU needs to compile & pass tests successfully after each individual
>>> patch. Currently it'll fail on patch 2, because the code generator
>>> wil emit the new qmp_migrate API declaration signature, but the change
>>> to the implementation signature is here in patch 3.
>> Yes Daniel, it will fail on patch 2. My understanding was that, even if
>> sometimes individual patches dont compile properly, the final series of all
>> the patches should be compiled properly. But I understand your point.
> No, unfortunately we require the strict behaviour, where *every* individual
> commit must compile and pass unit tests.
>
> The reason for this is that when chasing regression bugs, it is common for
> developers to use 'git bisect' to test compilation across a range of
> releases. 'git bisect' will land on completely arbitrary commits, so it
> is critical that every QEMU commit in the repo must compile and pass
> tests. It isn't sufficient for just the end of the series to compile.
>
>> I have a small concern here Daniel, if you could help me resolve it?
>> - There is a similar issue in patch 4. Where some function parameters are to
>> be changed. But that function responds to both source and destination side
>> changes. So though patch 4 contains all the source side changes, it does not
>> take into account destination side changes and it does not compile entirely.
>> And after destination side changes are inside patch 6, the dependencies are
>> resolved. How is it possible to compile individual patches in this case,
>> because then each patch should also have some significant meaning to all the
>> changes. So, in that way, source side changes in one commit and destination
>> side changes in another commit makes more sense right ?
> If there is code that is shared between src + dst, that may put constraints
> on how you split up the patches.
>
> Possibly a split like this may avoid having dependancy problems:
>
>    * Patch intoduces the 'MigrateAddress' struct and related child
>      objects, but *not* the MigrateChannel struct.
>      
>    * Patch introduces code that can parse a 'uri' and spit out a
>      'MigrateAddress' struct.
>      
>    * Patch converts internal socket backend to accept MigrateAddress,
>      with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
>      
>    * Patch converts internal exec backend to accept MigrateAddress
>      with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
>      
>    * Patch converts internal rdma backend to accept MigrateAddress
>      with 'migrate/migrate_incoming' impl convert from uri -> MigrateAddress
>      
>    * Patch converts 'migrate' command to accept MigrateChannel param
>      directly
>    
>    * Patch converts 'migrate_incoming' command to accept MigrateChannel
>      param directly.
Thankyou Daniel, seems to be a great idea. I will try to restructure the 
patches in a manner that every patch will compile and pass unit tests 
individually.
> With regards,
> Daniel
Regards,
Het Gala