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

[Daniel P . Berrangé]

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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|