Re: [libvirt] [PATCH] libxl: advertise support for migration V3
On 08/30/2016 06:21 PM, Jim Fehlig wrote: > On 08/30/2016 08:45 AM, Joao Martins wrote: >> On 08/29/2016 06:20 PM, Jim Fehlig wrote: >>> The libxl driver has long supported migration V3 but has never >>> indicated so in the connectSupportsFeature API. >> Hmm, but IIRC it was only since "recent" commit 8db77b3 right? > > The libxl driver has supported the VIR_DRV_FEATURE_MIGRATION_PARAMS V3 family > of > migration functions since commit 9b8d6e1e in May 2014. Indeed. >> Or rather Nikolay's >> reworking top-level migration code, which effectively would convert to the >> params >> versions accordingly. Before that rework I think one had to implement both >> params and >> non-params variants. Would it be worth adding a comment mainly to help the >> reader? > > I think you are right, but support for V3 is independent of the params vs > non-params variants. virt-manager uses the generic virDomainMigrate() function > in src/libvirt-domain.c, and at least as far back as libvirt 1.2.5 that > function > tests whether both source and destination support > VIR_DRV_FEATURE_MIGRATION_V{1,2,3}. If the driver doesn't advertise support > for > any of the versions, virDomainMigrate() returns VIR_ERR_NO_SUPPORT. Hmm, OK. IIRC the problems I once observed where more around virDomainMigrateToURI - which got addressed in the series containing the commit above. Anyhow sorry for the noise! > >> >> (I vaguely remember this as I dropped my v3 patch as being no longer >> necessary) >> >>> As a result, apps >>> such as virt-manager that use the more generic virDomainMigrate API >>> fail with >>> >>> libvirtError: this function is not supported by the connection driver: >>> virDomainMigrate >>> >>> Add VIR_DRV_FEATURE_MIGRATION_V3 to the list of features marked as >>> supported in the connectSupportsFeature API. >> FWIW and irrespective of the comment above: >> >> Reviewed-by: Joao Martins > > Thanks. Along with Cedric's ACK, and considering it is a trivial bug fix, I > plan > to push this for RC2. Cool. Regards, Joao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: advertise support for migration V3
On 08/30/2016 12:52 AM, Cedric Bosdonnat wrote: > On Mon, 2016-08-29 at 11:20 -0600, Jim Fehlig wrote: >> The libxl driver has long supported migration V3 but has never >> indicated so in the connectSupportsFeature API. As a result, apps >> such as virt-manager that use the more generic virDomainMigrate API >> fail with >> >> libvirtError: this function is not supported by the connection driver: >> virDomainMigrate >> >> Add VIR_DRV_FEATURE_MIGRATION_V3 to the list of features marked as >> supported in the connectSupportsFeature API. >> >> Signed-off-by: Jim Fehlig >> --- >> src/libxl/libxl_driver.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c >> index a573c82..3ffaa74 100644 >> --- a/src/libxl/libxl_driver.c >> +++ b/src/libxl/libxl_driver.c >> @@ -5677,6 +5677,7 @@ libxlConnectSupportsFeature(virConnectPtr conn, int >> feature) >> return -1; >> >> switch (feature) { >> +case VIR_DRV_FEATURE_MIGRATION_V3: >> case VIR_DRV_FEATURE_TYPED_PARAM_STRING: >> case VIR_DRV_FEATURE_MIGRATION_PARAMS: >> case VIR_DRV_FEATURE_MIGRATION_P2P: > ACK Thanks. Since this is a trivial bug fix, and given a second review by Joao, I've pushed this for RC2. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: advertise support for migration V3
On 08/30/2016 08:45 AM, Joao Martins wrote: > On 08/29/2016 06:20 PM, Jim Fehlig wrote: >> The libxl driver has long supported migration V3 but has never >> indicated so in the connectSupportsFeature API. > Hmm, but IIRC it was only since "recent" commit 8db77b3 right? The libxl driver has supported the VIR_DRV_FEATURE_MIGRATION_PARAMS V3 family of migration functions since commit 9b8d6e1e in May 2014. > Or rather Nikolay's > reworking top-level migration code, which effectively would convert to the > params > versions accordingly. Before that rework I think one had to implement both > params and > non-params variants. Would it be worth adding a comment mainly to help the > reader? I think you are right, but support for V3 is independent of the params vs non-params variants. virt-manager uses the generic virDomainMigrate() function in src/libvirt-domain.c, and at least as far back as libvirt 1.2.5 that function tests whether both source and destination support VIR_DRV_FEATURE_MIGRATION_V{1,2,3}. If the driver doesn't advertise support for any of the versions, virDomainMigrate() returns VIR_ERR_NO_SUPPORT. > > (I vaguely remember this as I dropped my v3 patch as being no longer > necessary) > >> As a result, apps >> such as virt-manager that use the more generic virDomainMigrate API >> fail with >> >> libvirtError: this function is not supported by the connection driver: >> virDomainMigrate >> >> Add VIR_DRV_FEATURE_MIGRATION_V3 to the list of features marked as >> supported in the connectSupportsFeature API. > FWIW and irrespective of the comment above: > > Reviewed-by: Joao Martins Thanks. Along with Cedric's ACK, and considering it is a trivial bug fix, I plan to push this for RC2. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: advertise support for migration V3
On 08/29/2016 06:20 PM, Jim Fehlig wrote: > The libxl driver has long supported migration V3 but has never > indicated so in the connectSupportsFeature API. Hmm, but IIRC it was only since "recent" commit 8db77b3 right? Or rather Nikolay's reworking top-level migration code, which effectively would convert to the params versions accordingly. Before that rework I think one had to implement both params and non-params variants. Would it be worth adding a comment mainly to help the reader? (I vaguely remember this as I dropped my v3 patch as being no longer necessary) > As a result, apps > such as virt-manager that use the more generic virDomainMigrate API > fail with > > libvirtError: this function is not supported by the connection driver: > virDomainMigrate > > Add VIR_DRV_FEATURE_MIGRATION_V3 to the list of features marked as > supported in the connectSupportsFeature API. FWIW and irrespective of the comment above: Reviewed-by: Joao Martins -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: advertise support for migration V3
On Mon, 2016-08-29 at 11:20 -0600, Jim Fehlig wrote: > The libxl driver has long supported migration V3 but has never > indicated so in the connectSupportsFeature API. As a result, apps > such as virt-manager that use the more generic virDomainMigrate API > fail with > > libvirtError: this function is not supported by the connection driver: > virDomainMigrate > > Add VIR_DRV_FEATURE_MIGRATION_V3 to the list of features marked as > supported in the connectSupportsFeature API. > > Signed-off-by: Jim Fehlig > --- > src/libxl/libxl_driver.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index a573c82..3ffaa74 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -5677,6 +5677,7 @@ libxlConnectSupportsFeature(virConnectPtr conn, int > feature) > return -1; > > switch (feature) { > +case VIR_DRV_FEATURE_MIGRATION_V3: > case VIR_DRV_FEATURE_TYPED_PARAM_STRING: > case VIR_DRV_FEATURE_MIGRATION_PARAMS: > case VIR_DRV_FEATURE_MIGRATION_P2P: ACK -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list