diff mbox series

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

Message ID 20230208093600.242665-4-het.gala@nutanix.com
State New
Headers show
Series migration: Modified 'migrate' QAPI command for migration | expand

Commit Message

Het Gala Feb. 8, 2023, 9:35 a.m. UTC
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(-)

Comments

Daniel P. Berrangé Feb. 9, 2023, 12:05 p.m. UTC | #1
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-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index ef25bc8929..a9f65ded7a 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -32,6 +32,101 @@
>  #include "sysemu/runstate.h"
>  #include "ui/qemu-spice.h"
>  
> +static bool
> +migrate_channel_from_qdict(MigrateChannel **channel,
> +                           const QDict *qdict, Error **errp)
> +{
> +    Error *err = NULL;
> +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
> +    const char *transport_str = qdict_get_try_str(qdict, "transport");
> +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
> +    const char *inet_host = qdict_get_try_str(qdict, "host");
> +    const char *inet_port = qdict_get_try_str(qdict, "port");
> +    const char *unix_path = qdict_get_try_str(qdict, "path");
> +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
> +    const char *vsock_port = qdict_get_try_str(qdict, "port");
> +    const char *fd = qdict_get_try_str(qdict, "str");
> +    QList *exec = qdict_get_qlist(qdict, "exec");
> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateChannelType channel_type;
> +    MigrateTransport transport;
> +    MigrateAddress *addr = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new(SocketAddress, 1);
> +    SocketAddressType type;
> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> +
> +    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
> +                                   channeltype, -1, &err);
> +    if (channel_type < 0) {
> +        goto end;
> +    }
> +
> +    transport = qapi_enum_parse(&MigrateTransport_lookup,
> +                                transport_str, -1, &err);
> +    if (transport < 0) {
> +        goto end;
> +    }
> +
> +    type = qapi_enum_parse(&SocketAddressType_lookup,
> +                           socketaddr_type, -1, &err);
> +    if (type < 0) {
> +        goto end;
> +    }
> +
> +    addr->transport = transport;
> +
> +    switch (transport) {
> +    case MIGRATE_TRANSPORT_SOCKET:
> +        saddr->type = type;
> +
> +        switch (type) {
> +        case SOCKET_ADDRESS_TYPE_INET:
> +            saddr->u.inet.host = (char *)inet_host;
> +            saddr->u.inet.port = (char *)inet_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_UNIX:
> +            saddr->u.q_unix.path = (char *)unix_path;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_VSOCK:
> +            saddr->u.vsock.cid = (char *)vsock_cid;
> +            saddr->u.vsock.port = (char *)vsock_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_FD:
> +            saddr->u.fd.str = (char *)fd;
> +            break;
> +        default:
> +            error_setg(errp, "%s: Unknown socket type %d",
> +                       __func__, saddr->type);
> +            break;
> +        }
> +
> +        addr->u.socket.socket_type = saddr;
> +        break;
> +    case MIGRATE_TRANSPORT_EXEC:
> +        addr->u.exec.data = (strList *)exec;
> +         break;
> +    case MIGRATE_TRANSPORT_RDMA:
> +        isock->host = (char *)inet_host;
> +        isock->port = (char *)inet_port;
> +        addr->u.rdma.data = isock;
> +        break;
> +    default:
> +        error_setg(errp, "%s: Unknown migrate transport type %d",
> +                   __func__, addr->transport);
> +        break;
> +    }
> +
> +    val->channeltype = channel_type;
> +    val->addr = addr;
> +    *channel = val;
> +    return true;
> +
> +end:
> +    error_propagate(errp, err);
> +    return false;
> +}
> +
> +
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  {
>      MigrationInfo *info;
> @@ -701,8 +796,16 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>      const char *uri = qdict_get_str(qdict, "uri");
>      Error *err = NULL;
>  
> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
> +    MigrateChannel *channel = g_new0(MigrateChannel, 1);
> +
> +    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
> +        error_setg(&err, "error in retrieving channel from qdict");
> +        return;
> +    }
> +
> +    qmp_migrate(uri, channel, !!blk, blk, !!inc, inc,
>                  false, false, true, resume, &err);
> +    qapi_free_MigrateChannel(channel);
>      if (hmp_handle_error(mon, err)) {
>          return;
>      }
> 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");

> +                          "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.

With regards,
Daniel
Het Gala Feb. 9, 2023, 1:38 p.m. UTC | #2
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-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index ef25bc8929..a9f65ded7a 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -32,6 +32,101 @@
>>   #include "sysemu/runstate.h"
>>   #include "ui/qemu-spice.h"
>>   
>> +static bool
>> +migrate_channel_from_qdict(MigrateChannel **channel,
>> +                           const QDict *qdict, Error **errp)
>> +{
>> +    Error *err = NULL;
>> +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
>> +    const char *transport_str = qdict_get_try_str(qdict, "transport");
>> +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
>> +    const char *inet_host = qdict_get_try_str(qdict, "host");
>> +    const char *inet_port = qdict_get_try_str(qdict, "port");
>> +    const char *unix_path = qdict_get_try_str(qdict, "path");
>> +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
>> +    const char *vsock_port = qdict_get_try_str(qdict, "port");
>> +    const char *fd = qdict_get_try_str(qdict, "str");
>> +    QList *exec = qdict_get_qlist(qdict, "exec");
>> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>> +    MigrateChannelType channel_type;
>> +    MigrateTransport transport;
>> +    MigrateAddress *addr = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr = g_new(SocketAddress, 1);
>> +    SocketAddressType type;
>> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>> +
>> +    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
>> +                                   channeltype, -1, &err);
>> +    if (channel_type < 0) {
>> +        goto end;
>> +    }
>> +
>> +    transport = qapi_enum_parse(&MigrateTransport_lookup,
>> +                                transport_str, -1, &err);
>> +    if (transport < 0) {
>> +        goto end;
>> +    }
>> +
>> +    type = qapi_enum_parse(&SocketAddressType_lookup,
>> +                           socketaddr_type, -1, &err);
>> +    if (type < 0) {
>> +        goto end;
>> +    }
>> +
>> +    addr->transport = transport;
>> +
>> +    switch (transport) {
>> +    case MIGRATE_TRANSPORT_SOCKET:
>> +        saddr->type = type;
>> +
>> +        switch (type) {
>> +        case SOCKET_ADDRESS_TYPE_INET:
>> +            saddr->u.inet.host = (char *)inet_host;
>> +            saddr->u.inet.port = (char *)inet_port;
>> +            break;
>> +        case SOCKET_ADDRESS_TYPE_UNIX:
>> +            saddr->u.q_unix.path = (char *)unix_path;
>> +            break;
>> +        case SOCKET_ADDRESS_TYPE_VSOCK:
>> +            saddr->u.vsock.cid = (char *)vsock_cid;
>> +            saddr->u.vsock.port = (char *)vsock_port;
>> +            break;
>> +        case SOCKET_ADDRESS_TYPE_FD:
>> +            saddr->u.fd.str = (char *)fd;
>> +            break;
>> +        default:
>> +            error_setg(errp, "%s: Unknown socket type %d",
>> +                       __func__, saddr->type);
>> +            break;
>> +        }
>> +
>> +        addr->u.socket.socket_type = saddr;
>> +        break;
>> +    case MIGRATE_TRANSPORT_EXEC:
>> +        addr->u.exec.data = (strList *)exec;
>> +         break;
>> +    case MIGRATE_TRANSPORT_RDMA:
>> +        isock->host = (char *)inet_host;
>> +        isock->port = (char *)inet_port;
>> +        addr->u.rdma.data = isock;
>> +        break;
>> +    default:
>> +        error_setg(errp, "%s: Unknown migrate transport type %d",
>> +                   __func__, addr->transport);
>> +        break;
>> +    }
>> +
>> +    val->channeltype = channel_type;
>> +    val->addr = addr;
>> +    *channel = val;
>> +    return true;
>> +
>> +end:
>> +    error_propagate(errp, err);
>> +    return false;
>> +}
>> +
>> +
>>   void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>>   {
>>       MigrationInfo *info;
>> @@ -701,8 +796,16 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>       const char *uri = qdict_get_str(qdict, "uri");
>>       Error *err = NULL;
>>   
>> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
>> +    MigrateChannel *channel = g_new0(MigrateChannel, 1);
>> +
>> +    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
>> +        error_setg(&err, "error in retrieving channel from qdict");
>> +        return;
>> +    }
>> +
>> +    qmp_migrate(uri, channel, !!blk, blk, !!inc, inc,
>>                   false, false, true, resume, &err);
>> +    qapi_free_MigrateChannel(channel);
>>       if (hmp_handle_error(mon, err)) {
>>           return;
>>       }
>> 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.

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 ?
> With regards,
> Daniel
Regards,
Het Gala
Daniel P. Berrangé Feb. 9, 2023, 2 p.m. UTC | #3
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
Het Gala Feb. 10, 2023, 6:44 a.m. UTC | #4
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
diff mbox series

Patch

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index ef25bc8929..a9f65ded7a 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -32,6 +32,101 @@ 
 #include "sysemu/runstate.h"
 #include "ui/qemu-spice.h"
 
+static bool
+migrate_channel_from_qdict(MigrateChannel **channel,
+                           const QDict *qdict, Error **errp)
+{
+    Error *err = NULL;
+    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
+    const char *transport_str = qdict_get_try_str(qdict, "transport");
+    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
+    const char *inet_host = qdict_get_try_str(qdict, "host");
+    const char *inet_port = qdict_get_try_str(qdict, "port");
+    const char *unix_path = qdict_get_try_str(qdict, "path");
+    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
+    const char *vsock_port = qdict_get_try_str(qdict, "port");
+    const char *fd = qdict_get_try_str(qdict, "str");
+    QList *exec = qdict_get_qlist(qdict, "exec");
+    MigrateChannel *val = g_new0(MigrateChannel, 1);
+    MigrateChannelType channel_type;
+    MigrateTransport transport;
+    MigrateAddress *addr = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new(SocketAddress, 1);
+    SocketAddressType type;
+    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
+
+    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
+                                   channeltype, -1, &err);
+    if (channel_type < 0) {
+        goto end;
+    }
+
+    transport = qapi_enum_parse(&MigrateTransport_lookup,
+                                transport_str, -1, &err);
+    if (transport < 0) {
+        goto end;
+    }
+
+    type = qapi_enum_parse(&SocketAddressType_lookup,
+                           socketaddr_type, -1, &err);
+    if (type < 0) {
+        goto end;
+    }
+
+    addr->transport = transport;
+
+    switch (transport) {
+    case MIGRATE_TRANSPORT_SOCKET:
+        saddr->type = type;
+
+        switch (type) {
+        case SOCKET_ADDRESS_TYPE_INET:
+            saddr->u.inet.host = (char *)inet_host;
+            saddr->u.inet.port = (char *)inet_port;
+            break;
+        case SOCKET_ADDRESS_TYPE_UNIX:
+            saddr->u.q_unix.path = (char *)unix_path;
+            break;
+        case SOCKET_ADDRESS_TYPE_VSOCK:
+            saddr->u.vsock.cid = (char *)vsock_cid;
+            saddr->u.vsock.port = (char *)vsock_port;
+            break;
+        case SOCKET_ADDRESS_TYPE_FD:
+            saddr->u.fd.str = (char *)fd;
+            break;
+        default:
+            error_setg(errp, "%s: Unknown socket type %d",
+                       __func__, saddr->type);
+            break;
+        }
+
+        addr->u.socket.socket_type = saddr;
+        break;
+    case MIGRATE_TRANSPORT_EXEC:
+        addr->u.exec.data = (strList *)exec;
+         break;
+    case MIGRATE_TRANSPORT_RDMA:
+        isock->host = (char *)inet_host;
+        isock->port = (char *)inet_port;
+        addr->u.rdma.data = isock;
+        break;
+    default:
+        error_setg(errp, "%s: Unknown migrate transport type %d",
+                   __func__, addr->transport);
+        break;
+    }
+
+    val->channeltype = channel_type;
+    val->addr = addr;
+    *channel = val;
+    return true;
+
+end:
+    error_propagate(errp, err);
+    return false;
+}
+
+
 void hmp_info_migrate(Monitor *mon, const QDict *qdict)
 {
     MigrationInfo *info;
@@ -701,8 +796,16 @@  void hmp_migrate(Monitor *mon, const QDict *qdict)
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
 
-    qmp_migrate(uri, !!blk, blk, !!inc, inc,
+    MigrateChannel *channel = g_new0(MigrateChannel, 1);
+
+    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
+        error_setg(&err, "error in retrieving channel from qdict");
+        return;
+    }
+
+    qmp_migrate(uri, channel, !!blk, blk, !!inc, inc,
                 false, false, true, resume, &err);
+    qapi_free_MigrateChannel(channel);
     if (hmp_handle_error(mon, err)) {
         return;
     }
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"
+                          "mutually exclusive");
+        return;
+    }
+
     migrate_protocol_allow_multi_channels(false);
     if (strstart(uri, "tcp:", &p) ||
         strstart(uri, "unix:", NULL) ||