| Message ID | 20230721144914.170991-7-het.gala@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
On Fri, Jul 21, 2023 at 02:49:31PM +0000, Het Gala wrote: > MigrateChannelList allows to connect accross multiple interfaces. > Add MigrateChannelList struct as argument to migration QAPIs. > > We plan to include multiple channels in future, to connnect > multiple interfaces. Hence, we choose 'MigrateChannelList' > as the new argument over 'MigrateChannel' to make migration > QAPIs future proof. > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > Acked-by: Markus Armbruster <armbru@redhat.com> > --- > migration/migration-hmp-cmds.c | 6 +- > migration/migration.c | 34 ++++++++-- > qapi/migration.json | 109 ++++++++++++++++++++++++++++++++- > softmmu/vl.c | 2 +- > 4 files changed, 139 insertions(+), 12 deletions(-) > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > index 9885d7c9f7..49b150f33f 100644 > --- a/migration/migration-hmp-cmds.c > +++ b/migration/migration-hmp-cmds.c > @@ -424,7 +424,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) > Error *err = NULL; > const char *uri = qdict_get_str(qdict, "uri"); > > - qmp_migrate_incoming(uri, &err); > + qmp_migrate_incoming(uri, false, NULL, &err); > > hmp_handle_error(mon, err); > } > @@ -705,8 +705,8 @@ 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, > - false, false, true, resume, &err); > + qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, > + false, false, true, resume, &err); > if (hmp_handle_error(mon, err)) { > return; > } > diff --git a/migration/migration.c b/migration/migration.c > index f37b388876..bd3a93fc8c 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -466,10 +466,22 @@ static bool migrate_uri_parse(const char *uri, > return true; > } > > -static void qemu_start_incoming_migration(const char *uri, Error **errp) > +static void qemu_start_incoming_migration(const char *uri, bool has_channels, > + MigrationChannelList *channels, > + Error **errp) > { > g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); > > + /* > + * Having preliminary checks for uri and channel > + */ > + if (uri && has_channels) { > + error_setg(errp, "'uri' and 'channels' arguments are mutually " > + "exclusive; exactly one of the two should be present in " > + "'migrate-incoming' qmp command "); > + return; > + } This checks is both are present. Also needs a check if neither are present as that's invalid. > @@ -1694,6 +1708,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > MigrationState *s = migrate_get_current(); > g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); > > + /* > + * Having preliminary checks for uri and channel > + */ > + if (uri && has_channels) { > + error_setg(errp, "'uri' and 'channels' arguments are mutually " > + "exclusive; exactly one of the two should be present in " > + "'migrate' qmp command "); > + return; > + } Same here With regards, Daniel
On Tue, Jul 25, 2023 at 07:34:09PM +0100, Daniel P. Berrangé wrote: > On Fri, Jul 21, 2023 at 02:49:31PM +0000, Het Gala wrote: > > MigrateChannelList allows to connect accross multiple interfaces. > > Add MigrateChannelList struct as argument to migration QAPIs. > > > > We plan to include multiple channels in future, to connnect > > multiple interfaces. Hence, we choose 'MigrateChannelList' > > as the new argument over 'MigrateChannel' to make migration > > QAPIs future proof. > > > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > > Signed-off-by: Het Gala <het.gala@nutanix.com> > > Acked-by: Markus Armbruster <armbru@redhat.com> > > --- > > migration/migration-hmp-cmds.c | 6 +- > > migration/migration.c | 34 ++++++++-- > > qapi/migration.json | 109 ++++++++++++++++++++++++++++++++- > > softmmu/vl.c | 2 +- > > 4 files changed, 139 insertions(+), 12 deletions(-) > > > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > > index 9885d7c9f7..49b150f33f 100644 > > --- a/migration/migration-hmp-cmds.c > > +++ b/migration/migration-hmp-cmds.c > > @@ -424,7 +424,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) > > Error *err = NULL; > > const char *uri = qdict_get_str(qdict, "uri"); > > > > - qmp_migrate_incoming(uri, &err); > > + qmp_migrate_incoming(uri, false, NULL, &err); > > > > hmp_handle_error(mon, err); > > } > > @@ -705,8 +705,8 @@ 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, > > - false, false, true, resume, &err); > > + qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, > > + false, false, true, resume, &err); > > if (hmp_handle_error(mon, err)) { > > return; > > } > > diff --git a/migration/migration.c b/migration/migration.c > > index f37b388876..bd3a93fc8c 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -466,10 +466,22 @@ static bool migrate_uri_parse(const char *uri, > > return true; > > } > > > > -static void qemu_start_incoming_migration(const char *uri, Error **errp) > > +static void qemu_start_incoming_migration(const char *uri, bool has_channels, > > + MigrationChannelList *channels, > > + Error **errp) > > { > > g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); > > > > + /* > > + * Having preliminary checks for uri and channel > > + */ > > + if (uri && has_channels) { > > + error_setg(errp, "'uri' and 'channels' arguments are mutually " > > + "exclusive; exactly one of the two should be present in " > > + "'migrate-incoming' qmp command "); > > + return; > > + } > > This checks is both are present. > > Also needs a check if neither are present as that's invalid. Also it should (temporarily) raise an error if "has_channels" is set, as while we've added the parameter in QAPI, we've not implemented it yet. IOW, raise an error now, and remove the error in a later patch. > > > > @@ -1694,6 +1708,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > > MigrationState *s = migrate_get_current(); > > g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); > > > > + /* > > + * Having preliminary checks for uri and channel > > + */ > > + if (uri && has_channels) { > > + error_setg(errp, "'uri' and 'channels' arguments are mutually " > > + "exclusive; exactly one of the two should be present in " > > + "'migrate' qmp command "); > > + return; > > + } > > Same here > > > 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 :| > > With regards, Daniel
On 26/07/23 12:07 am, Daniel P. Berrangé wrote: > On Tue, Jul 25, 2023 at 07:34:09PM +0100, Daniel P. Berrangé wrote: >> On Fri, Jul 21, 2023 at 02:49:31PM +0000, Het Gala wrote: >>> MigrateChannelList allows to connect accross multiple interfaces. >>> Add MigrateChannelList struct as argument to migration QAPIs. >>> >>> We plan to include multiple channels in future, to connnect >>> multiple interfaces. Hence, we choose 'MigrateChannelList' >>> as the new argument over 'MigrateChannel' to make migration >>> QAPIs future proof. >>> >>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >>> Signed-off-by: Het Gala <het.gala@nutanix.com> >>> Acked-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> migration/migration-hmp-cmds.c | 6 +- >>> migration/migration.c | 34 ++++++++-- >>> qapi/migration.json | 109 ++++++++++++++++++++++++++++++++- >>> softmmu/vl.c | 2 +- >>> 4 files changed, 139 insertions(+), 12 deletions(-) >>> >>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c >>> index 9885d7c9f7..49b150f33f 100644 >>> --- a/migration/migration-hmp-cmds.c >>> +++ b/migration/migration-hmp-cmds.c >>> @@ -424,7 +424,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) >>> Error *err = NULL; >>> const char *uri = qdict_get_str(qdict, "uri"); >>> >>> - qmp_migrate_incoming(uri, &err); >>> + qmp_migrate_incoming(uri, false, NULL, &err); >>> >>> hmp_handle_error(mon, err); >>> } >>> @@ -705,8 +705,8 @@ 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, >>> - false, false, true, resume, &err); >>> + qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, >>> + false, false, true, resume, &err); >>> if (hmp_handle_error(mon, err)) { >>> return; >>> } >>> diff --git a/migration/migration.c b/migration/migration.c >>> index f37b388876..bd3a93fc8c 100644 >>> --- a/migration/migration.c >>> +++ b/migration/migration.c >>> @@ -466,10 +466,22 @@ static bool migrate_uri_parse(const char *uri, >>> return true; >>> } >>> >>> -static void qemu_start_incoming_migration(const char *uri, Error **errp) >>> +static void qemu_start_incoming_migration(const char *uri, bool has_channels, >>> + MigrationChannelList *channels, >>> + Error **errp) >>> { >>> g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); >>> >>> + /* >>> + * Having preliminary checks for uri and channel >>> + */ >>> + if (uri && has_channels) { >>> + error_setg(errp, "'uri' and 'channels' arguments are mutually " >>> + "exclusive; exactly one of the two should be present in " >>> + "'migrate-incoming' qmp command "); >>> + return; >>> + } >> This checks is both are present. >> >> Also needs a check if neither are present as that's invalid. > Also it should (temporarily) raise an error if "has_channels" is > set, as while we've added the parameter in QAPI, we've not > implemented it yet. IOW, raise an error now, and remove the > error in a later patch. Ack. So in total there should be 3 checks right. 1) if 'has_channels' is set, 2) if 'uri' and 'channels' both are present, 3) if 'uri' and 'channels' both are absent. Basically right now only uri should allowed and should atleast be present. I think overall only 1) would be enough and should be checked before 'migration_channels_and_uri_compatible()' and if 'has_channels' is set, just return for now. With this 2) would not be necessary or not come into play in this patch. 3) will be taken care by 'migration_channels_and_uri_compatible()' itself IMO. Let me know if I am missing something here. >> >>> @@ -1694,6 +1708,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >>> MigrationState *s = migrate_get_current(); >>> g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); >>> >>> + /* >>> + * Having preliminary checks for uri and channel >>> + */ >>> + if (uri && has_channels) { >>> + error_setg(errp, "'uri' and 'channels' arguments are mutually " >>> + "exclusive; exactly one of the two should be present in " >>> + "'migrate' qmp command "); >>> + return; >>> + } >> Same here >> >> >> With regards, >> Daniel >> -- >> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__berrange.com&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=AQ1C7WPg2jLYjNXU29Xw7trQcjmB96Yy3-God3-UaIQ&e= -o- https://urldefense.proofpoint.com/v2/url?u=https-3A__www.flickr.com_photos_dberrange&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=aiGUx76ySVL-epTmaFIZUyZbkzeXGedVaXGvFw4xcgo&e= :| >> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__libvirt.org&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=beFwppzRJ_eYlYPZKHlSZpaysLC5AExPh5_inAZBu_k&e= -o- https://urldefense.proofpoint.com/v2/url?u=https-3A__fstop138.berrange.com&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=q6fb1PbChin197qfXnyXa1FpzaC7GecQJOzWO1EUQTo&e= :| >> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__entangle-2Dphoto.org&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=lUJpjpbEMjPFyDvx1GXWR5yrxWfQqTyLI42J5oxavgs&e= -o- https://urldefense.proofpoint.com/v2/url?u=https-3A__www.instagram.com_dberrange&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=u9HElHtI7mdNiWq8I7j1GpnUObfjfopMmpLdvGZRI8k&e= :| >> >> > With regards, > Daniel Regards, Het Gala
Sorry, last reply on this patch was accidently replied only to Daniel. Pasting the reply again so it is received by all the active maintianers here. Apologies for the error 😅 On 26/07/23 12:07 am, Daniel P. Berrangé wrote: > On Tue, Jul 25, 2023 at 07:34:09PM +0100, Daniel P. Berrangé wrote: >> On Fri, Jul 21, 2023 at 02:49:31PM +0000, Het Gala wrote: >>> MigrateChannelList allows to connect accross multiple interfaces. >>> Add MigrateChannelList struct as argument to migration QAPIs. >>> >>> We plan to include multiple channels in future, to connnect >>> multiple interfaces. Hence, we choose 'MigrateChannelList' >>> as the new argument over 'MigrateChannel' to make migration >>> QAPIs future proof. >>> >>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >>> Signed-off-by: Het Gala <het.gala@nutanix.com> >>> Acked-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> migration/migration-hmp-cmds.c | 6 +- >>> migration/migration.c | 34 ++++++++-- >>> qapi/migration.json | 109 ++++++++++++++++++++++++++++++++- >>> softmmu/vl.c | 2 +- >>> 4 files changed, 139 insertions(+), 12 deletions(-) >>> >>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c >>> index 9885d7c9f7..49b150f33f 100644 >>> --- a/migration/migration-hmp-cmds.c >>> +++ b/migration/migration-hmp-cmds.c >>> @@ -424,7 +424,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) >>> Error *err = NULL; >>> const char *uri = qdict_get_str(qdict, "uri"); >>> >>> - qmp_migrate_incoming(uri, &err); >>> + qmp_migrate_incoming(uri, false, NULL, &err); >>> >>> hmp_handle_error(mon, err); >>> } >>> @@ -705,8 +705,8 @@ 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, >>> - false, false, true, resume, &err); >>> + qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, >>> + false, false, true, resume, &err); >>> if (hmp_handle_error(mon, err)) { >>> return; >>> } >>> diff --git a/migration/migration.c b/migration/migration.c >>> index f37b388876..bd3a93fc8c 100644 >>> --- a/migration/migration.c >>> +++ b/migration/migration.c >>> @@ -466,10 +466,22 @@ static bool migrate_uri_parse(const char *uri, >>> return true; >>> } >>> >>> -static void qemu_start_incoming_migration(const char *uri, Error **errp) >>> +static void qemu_start_incoming_migration(const char *uri, bool has_channels, >>> + MigrationChannelList *channels, >>> + Error **errp) >>> { >>> g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); >>> >>> + /* >>> + * Having preliminary checks for uri and channel >>> + */ >>> + if (uri && has_channels) { >>> + error_setg(errp, "'uri' and 'channels' arguments are mutually " >>> + "exclusive; exactly one of the two should be present in " >>> + "'migrate-incoming' qmp command "); >>> + return; >>> + } >> This checks is both are present. >> >> Also needs a check if neither are present as that's invalid. > Also it should (temporarily) raise an error if "has_channels" is > set, as while we've added the parameter in QAPI, we've not > implemented it yet. IOW, raise an error now, and remove the > error in a later patch. Ack. So in total there should be 3 checks right. 1) if 'has_channels' is set, 2) if 'uri' and 'channels' both are present, 3) if 'uri' and 'channels' both are absent. Basically right now only uri should allowed and should atleast be present. I think overall only 1) would be enough and should be checked before 'migration_channels_and_uri_compatible()' and if 'has_channels' is set, just return for now. With this 2) would not be necessary or not come into play in this patch. 3) will be taken care by 'migration_channels_and_uri_compatible()' itself IMO. Let me know if I am missing something here. >> >>> @@ -1694,6 +1708,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >>> MigrationState *s = migrate_get_current(); >>> g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); >>> >>> + /* >>> + * Having preliminary checks for uri and channel >>> + */ >>> + if (uri && has_channels) { >>> + error_setg(errp, "'uri' and 'channels' arguments are mutually " >>> + "exclusive; exactly one of the two should be present in " >>> + "'migrate' qmp command "); >>> + return; >>> + } >> Same here >> >> >> With regards, >> Daniel >> -- >> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__berrange.com&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=AQ1C7WPg2jLYjNXU29Xw7trQcjmB96Yy3-God3-UaIQ&e= -o- https://urldefense.proofpoint.com/v2/url?u=https-3A__www.flickr.com_photos_dberrange&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=aiGUx76ySVL-epTmaFIZUyZbkzeXGedVaXGvFw4xcgo&e= :| >> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__libvirt.org&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=beFwppzRJ_eYlYPZKHlSZpaysLC5AExPh5_inAZBu_k&e= -o- https://urldefense.proofpoint.com/v2/url?u=https-3A__fstop138.berrange.com&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=q6fb1PbChin197qfXnyXa1FpzaC7GecQJOzWO1EUQTo&e= :| >> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__entangle-2Dphoto.org&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=lUJpjpbEMjPFyDvx1GXWR5yrxWfQqTyLI42J5oxavgs&e= -o- https://urldefense.proofpoint.com/v2/url?u=https-3A__www.instagram.com_dberrange&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=i0lmmIrs7N4r3uqLYCdRVXLFaEavt77Ltkec0hIlE4aUQITo9-8povsDHWELv-vE&s=u9HElHtI7mdNiWq8I7j1GpnUObfjfopMmpLdvGZRI8k&e= :| >> >> > With regards, > Daniel Regards, Het Gala
On Wed, Jul 26, 2023 at 01:24:48AM +0530, Het Gala wrote: > Sorry, last reply on this patch was accidently replied only to Daniel. > Pasting the reply again so it is received by all the active maintianers > here. Apologies for the error 😅 > > On 26/07/23 12:07 am, Daniel P. Berrangé wrote: > > On Tue, Jul 25, 2023 at 07:34:09PM +0100, Daniel P. Berrangé wrote: > > > On Fri, Jul 21, 2023 at 02:49:31PM +0000, Het Gala wrote: > > > > MigrateChannelList allows to connect accross multiple interfaces. > > > > Add MigrateChannelList struct as argument to migration QAPIs. > > > > > > > > We plan to include multiple channels in future, to connnect > > > > multiple interfaces. Hence, we choose 'MigrateChannelList' > > > > as the new argument over 'MigrateChannel' to make migration > > > > QAPIs future proof. > > > > > > > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > > > > Signed-off-by: Het Gala <het.gala@nutanix.com> > > > > Acked-by: Markus Armbruster <armbru@redhat.com> > > > > --- > > > > migration/migration-hmp-cmds.c | 6 +- > > > > migration/migration.c | 34 ++++++++-- > > > > qapi/migration.json | 109 ++++++++++++++++++++++++++++++++- > > > > softmmu/vl.c | 2 +- > > > > 4 files changed, 139 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > > > > index 9885d7c9f7..49b150f33f 100644 > > > > --- a/migration/migration-hmp-cmds.c > > > > +++ b/migration/migration-hmp-cmds.c > > > > @@ -424,7 +424,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) > > > > Error *err = NULL; > > > > const char *uri = qdict_get_str(qdict, "uri"); > > > > - qmp_migrate_incoming(uri, &err); > > > > + qmp_migrate_incoming(uri, false, NULL, &err); > > > > hmp_handle_error(mon, err); > > > > } > > > > @@ -705,8 +705,8 @@ 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, > > > > - false, false, true, resume, &err); > > > > + qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, > > > > + false, false, true, resume, &err); > > > > if (hmp_handle_error(mon, err)) { > > > > return; > > > > } > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > index f37b388876..bd3a93fc8c 100644 > > > > --- a/migration/migration.c > > > > +++ b/migration/migration.c > > > > @@ -466,10 +466,22 @@ static bool migrate_uri_parse(const char *uri, > > > > return true; > > > > } > > > > -static void qemu_start_incoming_migration(const char *uri, Error **errp) > > > > +static void qemu_start_incoming_migration(const char *uri, bool has_channels, > > > > + MigrationChannelList *channels, > > > > + Error **errp) > > > > { > > > > g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); > > > > + /* > > > > + * Having preliminary checks for uri and channel > > > > + */ > > > > + if (uri && has_channels) { > > > > + error_setg(errp, "'uri' and 'channels' arguments are mutually " > > > > + "exclusive; exactly one of the two should be present in " > > > > + "'migrate-incoming' qmp command "); > > > > + return; > > > > + } > > > This checks is both are present. > > > > > > Also needs a check if neither are present as that's invalid. > > Also it should (temporarily) raise an error if "has_channels" is > > set, as while we've added the parameter in QAPI, we've not > > implemented it yet. IOW, raise an error now, and remove the > > error in a later patch. > Ack. So in total there should be 3 checks right. 1) if 'has_channels' is > set, 2) if 'uri' and 'channels' both are present, 3) if 'uri' and 'channels' > both are absent. Basically right now only uri should allowed and should > atleast be present. Correct. > I think overall only 1) would be enough and should be checked before > 'migration_channels_and_uri_compatible()' and if 'has_channels' is set, just > return for now. With this 2) would not be necessary or not come into play in > this patch. 3) will be taken care by > 'migration_channels_and_uri_compatible()' itself IMO. I think all the checks should be in this method, as it is the entrypoint for execution of the QMP command and thus where parameter validation should live. Spreading it across methods will make it very easy to open bugs by refactoring code and not realizing the new codepaths don't do the right checks. With regards, Daniel
On 26/07/23 2:05 pm, Daniel P. Berrangé wrote: > On Wed, Jul 26, 2023 at 01:24:48AM +0530, Het Gala wrote: >> Sorry, last reply on this patch was accidently replied only to Daniel. >> Pasting the reply again so it is received by all the active maintianers >> here. Apologies for the error 😅 >> >> On 26/07/23 12:07 am, Daniel P. Berrangé wrote: >>> On Tue, Jul 25, 2023 at 07:34:09PM +0100, Daniel P. Berrangé wrote: >>>> On Fri, Jul 21, 2023 at 02:49:31PM +0000, Het Gala wrote: >>>>> MigrateChannelList allows to connect accross multiple interfaces. >>>>> Add MigrateChannelList struct as argument to migration QAPIs. >>>>> >>>>> We plan to include multiple channels in future, to connnect >>>>> multiple interfaces. Hence, we choose 'MigrateChannelList' >>>>> as the new argument over 'MigrateChannel' to make migration >>>>> QAPIs future proof. >>>>> >>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >>>>> Signed-off-by: Het Gala <het.gala@nutanix.com> >>>>> Acked-by: Markus Armbruster <armbru@redhat.com> >>>>> --- >>>>> migration/migration-hmp-cmds.c | 6 +- >>>>> migration/migration.c | 34 ++++++++-- >>>>> qapi/migration.json | 109 ++++++++++++++++++++++++++++++++- >>>>> softmmu/vl.c | 2 +- >>>>> 4 files changed, 139 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c >>>>> index 9885d7c9f7..49b150f33f 100644 >>>>> --- a/migration/migration-hmp-cmds.c >>>>> +++ b/migration/migration-hmp-cmds.c >>>>> @@ -424,7 +424,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) >>>>> Error *err = NULL; >>>>> const char *uri = qdict_get_str(qdict, "uri"); >>>>> - qmp_migrate_incoming(uri, &err); >>>>> + qmp_migrate_incoming(uri, false, NULL, &err); >>>>> hmp_handle_error(mon, err); >>>>> } >>>>> @@ -705,8 +705,8 @@ 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, >>>>> - false, false, true, resume, &err); >>>>> + qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, >>>>> + false, false, true, resume, &err); >>>>> if (hmp_handle_error(mon, err)) { >>>>> return; >>>>> } >>>>> diff --git a/migration/migration.c b/migration/migration.c >>>>> index f37b388876..bd3a93fc8c 100644 >>>>> --- a/migration/migration.c >>>>> +++ b/migration/migration.c >>>>> @@ -466,10 +466,22 @@ static bool migrate_uri_parse(const char *uri, >>>>> return true; >>>>> } >>>>> -static void qemu_start_incoming_migration(const char *uri, Error **errp) >>>>> +static void qemu_start_incoming_migration(const char *uri, bool has_channels, >>>>> + MigrationChannelList *channels, >>>>> + Error **errp) >>>>> { >>>>> g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); >>>>> + /* >>>>> + * Having preliminary checks for uri and channel >>>>> + */ >>>>> + if (uri && has_channels) { >>>>> + error_setg(errp, "'uri' and 'channels' arguments are mutually " >>>>> + "exclusive; exactly one of the two should be present in " >>>>> + "'migrate-incoming' qmp command "); >>>>> + return; >>>>> + } >>>> This checks is both are present. >>>> >>>> Also needs a check if neither are present as that's invalid. >>> Also it should (temporarily) raise an error if "has_channels" is >>> set, as while we've added the parameter in QAPI, we've not >>> implemented it yet. IOW, raise an error now, and remove the >>> error in a later patch. >> Ack. So in total there should be 3 checks right. 1) if 'has_channels' is >> set, 2) if 'uri' and 'channels' both are present, 3) if 'uri' and 'channels' >> both are absent. Basically right now only uri should allowed and should >> atleast be present. > Correct. Ack. >> I think overall only 1) would be enough and should be checked before >> 'migration_channels_and_uri_compatible()' and if 'has_channels' is set, just >> return for now. With this 2) would not be necessary or not come into play in >> this patch. 3) will be taken care by >> 'migration_channels_and_uri_compatible()' itself IMO. > I think all the checks should be in this method, as it is the entrypoint > for execution of the QMP command and thus where parameter validation > should live. Spreading it across methods will make it very easy to open > bugs by refactoring code and not realizing the new codepaths don't do > the right checks. Okay, I got the concern here. Will add these 3 checks in this patch and later remove 'has_channels' check in the patch where we are implementing MigrateChannel. > With regards, > Daniel Regards, Het Gala
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 9885d7c9f7..49b150f33f 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -424,7 +424,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) Error *err = NULL; const char *uri = qdict_get_str(qdict, "uri"); - qmp_migrate_incoming(uri, &err); + qmp_migrate_incoming(uri, false, NULL, &err); hmp_handle_error(mon, err); } @@ -705,8 +705,8 @@ 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, - false, false, true, resume, &err); + qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, + false, false, true, resume, &err); if (hmp_handle_error(mon, err)) { return; } diff --git a/migration/migration.c b/migration/migration.c index f37b388876..bd3a93fc8c 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -466,10 +466,22 @@ static bool migrate_uri_parse(const char *uri, return true; } -static void qemu_start_incoming_migration(const char *uri, Error **errp) +static void qemu_start_incoming_migration(const char *uri, bool has_channels, + MigrationChannelList *channels, + Error **errp) { g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); + /* + * Having preliminary checks for uri and channel + */ + if (uri && has_channels) { + error_setg(errp, "'uri' and 'channels' arguments are mutually " + "exclusive; exactly one of the two should be present in " + "'migrate-incoming' qmp command "); + return; + } + /* URI is not suitable for migration? */ if (!migration_channels_and_uri_compatible(uri, errp)) { return; @@ -1497,7 +1509,8 @@ void migrate_del_blocker(Error *reason) migration_blockers = g_slist_remove(migration_blockers, reason); } -void qmp_migrate_incoming(const char *uri, Error **errp) +void qmp_migrate_incoming(const char *uri, bool has_channels, + MigrationChannelList *channels, Error **errp) { Error *local_err = NULL; static bool once = true; @@ -1515,7 +1528,7 @@ void qmp_migrate_incoming(const char *uri, Error **errp) return; } - qemu_start_incoming_migration(uri, &local_err); + qemu_start_incoming_migration(uri, has_channels, channels, &local_err); if (local_err) { yank_unregister_instance(MIGRATION_YANK_INSTANCE); @@ -1551,7 +1564,7 @@ void qmp_migrate_recover(const char *uri, Error **errp) * only re-setup the migration stream and poke existing migration * to continue using that newly established channel. */ - qemu_start_incoming_migration(uri, errp); + qemu_start_incoming_migration(uri, false, NULL, errp); } void qmp_migrate_pause(Error **errp) @@ -1685,7 +1698,8 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc, return true; } -void qmp_migrate(const char *uri, bool has_blk, bool blk, +void qmp_migrate(const char *uri, bool has_channels, + MigrationChannelList *channels, bool has_blk, bool blk, bool has_inc, bool inc, bool has_detach, bool detach, bool has_resume, bool resume, Error **errp) { @@ -1694,6 +1708,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, MigrationState *s = migrate_get_current(); g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); + /* + * Having preliminary checks for uri and channel + */ + if (uri && has_channels) { + error_setg(errp, "'uri' and 'channels' arguments are mutually " + "exclusive; exactly one of the two should be present in " + "'migrate' qmp command "); + return; + } + /* URI is not suitable for migration? */ if (!migration_channels_and_uri_compatible(uri, errp)) { return; diff --git a/qapi/migration.json b/qapi/migration.json index 0f4a54a7ed..9a365ac774 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1458,6 +1458,34 @@ 'exec': 'MigrationExecCommand', 'rdma': 'InetSocketAddress' } } +## +# @MigrationChannelType: +# +# The migration channel-type request options. +# +# @main: Main outbound migration channel. +# +# Since 8.1 +## +{ 'enum': 'MigrationChannelType', + 'data': [ 'main' ] } + +## +# @MigrationChannel: +# +# Migration stream channel parameters. +# +# @channel-type: Channel type for transfering packet information. +# +# @addr: Migration endpoint configuration on destination interface. +# +# Since 8.1 +## +{ 'struct': 'MigrationChannel', + 'data': { + 'channel-type': 'MigrationChannelType', + 'addr': 'MigrationAddress' } } + ## # @migrate: # @@ -1465,6 +1493,9 @@ # # @uri: the Uniform Resource Identifier of the destination VM # +# @channels: list of migration stream channels with each stream in the +# list connected to a destination interface endpoint. +# # @blk: do block migration (full disk copy) # # @inc: incremental disk copy migration @@ -1489,14 +1520,50 @@ # 3. The user Monitor's "detach" argument is invalid in QMP and should # not be used # +# 4. The uri argument should have the Uniform Resource Identifier of +# default destination VM. This connection will be bound to default +# network. +# +# 5. For now, number of migration streams is restricted to one, i.e +# number of items in 'channels' list is just 1. +# +# 6. The 'uri' and 'channels' arguments are mutually exclusive; +# exactly one of the two should be present. +# # Example: # # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } # <- { "return": {} } +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channel-type": "main", +# "addr": { "transport": "socket", +# "type": "inet", +# "host": "10.12.34.9", +# "port": "1050" } } ] } } +# <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channel-type": "main", +# "addr": { "transport": "exec", +# "args": [ "/bin/nc", "-p", "6000", +# "/some/sock" ] } } ] } } +# <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channel-type": "main", +# "addr": { "transport": "rdma", +# "host": "10.12.34.9", +# "port": "1050" } } ] } } +# <- { "return": {} } +# ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', - '*detach': 'bool', '*resume': 'bool' } } + 'data': {'*uri': 'str', '*channels': [ 'MigrationChannel' ], + '*blk': 'bool', '*inc': 'bool', '*detach': 'bool', + '*resume': 'bool' } } ## # @migrate-incoming: @@ -1507,6 +1574,9 @@ # @uri: The Uniform Resource Identifier identifying the source or # address to listen on # +# @channels: list of migration stream channels with each stream in the +# list connected to a destination interface endpoint. +# # Returns: nothing on success # # Since: 2.3 @@ -1522,13 +1592,46 @@ # # 3. The uri format is the same as for -incoming # +# 5. For now, number of migration streams is restricted to one, i.e +# number of items in 'channels' list is just 1. +# +# 4. The 'uri' and 'channels' arguments are mutually exclusive; +# exactly one of the two should be present. +# # Example: # # -> { "execute": "migrate-incoming", # "arguments": { "uri": "tcp::4446" } } # <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channel-type": "main", +# "addr": { "transport": "socket", +# "type": "inet", +# "host": "10.12.34.9", +# "port": "1050" } } ] } } +# <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channel-type": "main", +# "addr": { "transport": "exec", +# "args": [ "/bin/nc", "-p", "6000", +# "/some/sock" ] } } ] } } +# <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channel-type": "main", +# "addr": { "transport": "rdma", +# "host": "10.12.34.9", +# "port": "1050" } } ] } } +# <- { "return": {} } ## -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } } +{ 'command': 'migrate-incoming', + 'data': {'*uri': 'str', + '*channels': [ 'MigrationChannel' ] } } ## # @xen-save-devices-state: diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..d811f3f878 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,7 +2651,7 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { - qmp_migrate_incoming(incoming, &local_err); + qmp_migrate_incoming(incoming, false, NULL, &local_err); if (local_err) { error_reportf_err(local_err, "-incoming %s: ", incoming); exit(1);