5:23 a.m.
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy(a)virtuozzo.com>
Direct migration should work if *perform3 is present but *perform
is not. This is situation when driver migration is implemented
after new version of driver function is introduced. We should not
be forced to support old version too as its parameter space is
subspace of newer one.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 6ab50ba..c89775b 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain,
NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri),
NULLSTR(miguri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) {
+ if (!domain->conn->driver->domainMigratePerform &&
+ !domain->conn->driver->domainMigratePerform3) {
virReportUnsupportedError();
return -1;
}
--
1.7.1
12:54 a.m.
On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy
nshirokovskiy(a)virtuozzo.com>
Direct migration should work if *perform3 is present but *perform
is not. This is situation when driver migration is implemented
after new version of driver function is introduced. We should not
be forced to support old version too as its parameter space is
subspace of newer one.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/libvirt-domain.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 6ab50ba..c89775b 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain,
NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri),
NULLSTR(miguri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) {
+ if (!domain->conn->driver->domainMigratePerform &&
+ !domain->conn->driver->domainMigratePerform3) {
virReportUnsupportedError();
return -1;
}
Hm.. domainMigratePerform3 will be used iff connection driver has
VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that
regardless. What if we check the presence of implementation with respect
to that?
Moreover, can you please send patches rebased to current HEAD?
Michal
4:29 a.m.
On 28.08.2015 08:54, Michal Privoznik wrote:
> Michal
On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
> From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy(a)virtuozzo.com>
>
> Direct migration should work if *perform3 is present but *perform
> is not. This is situation when driver migration is implemented
> after new version of driver function is introduced. We should not
> be forced to support old version too as its parameter space is
> subspace of newer one.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/libvirt-domain.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 6ab50ba..c89775b 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain,
> NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri),
> NULLSTR(miguri), bandwidth);
>
> - if (!domain->conn->driver->domainMigratePerform) {
> + if (!domain->conn->driver->domainMigratePerform &&
> + !domain->conn->driver->domainMigratePerform3) {
> virReportUnsupportedError();
> return -1;
> }
>
Hm.. domainMigratePerform3 will be used iff connection driver has
VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that
regardless. What if we check the presence of implementation with respect
to that?
I see you mean actual driver could be behind remote one and checking
for perform3 always gives true so we need to check for feature instead?
Moreover, can you please send patches rebased to current HEAD?
Sorry, most time i rebase.
11:04 a.m.
On 28.08.2015 11:29, Nikolay Shirokovskiy wrote:
On 28.08.2015 08:54, Michal Privoznik wrote:
> On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
>> From: Nikolay Shirokovskiy <Nikolay Shirokovskiy
nshirokovskiy(a)virtuozzo.com>
>>
>> Direct migration should work if *perform3 is present but *perform
>> is not. This is situation when driver migration is implemented
>> after new version of driver function is introduced. We should not
>> be forced to support old version too as its parameter space is
>> subspace of newer one.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
>> ---
>> src/libvirt-domain.c | 3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
>> index 6ab50ba..c89775b 100644
>> --- a/src/libvirt-domain.c
>> +++ b/src/libvirt-domain.c
>> @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain,
>> NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri),
>> NULLSTR(miguri), bandwidth);
>>
>> - if (!domain->conn->driver->domainMigratePerform) {
>> + if (!domain->conn->driver->domainMigratePerform &&
>> + !domain->conn->driver->domainMigratePerform3) {
>> virReportUnsupportedError();
>> return -1;
>> }
>>
>
>
> Hm.. domainMigratePerform3 will be used iff connection driver has
> VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that
> regardless. What if we check the presence of implementation with respect
> to that?
I see you mean actual driver could be behind remote one and checking
for perform3 always gives true so we need to check for feature instead?
Sort of. domainMigratePerform3 is used only if the feature is present.
But after your patch, the function implementation would be needed
always, even if the feature is not present.
Michal
3:42 a.m.
On 28.08.2015 19:04, Michal Privoznik wrote:
On 28.08.2015 11:29, Nikolay Shirokovskiy wrote:
>
>
> On 28.08.2015 08:54, Michal Privoznik wrote:
>> On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
>>> From: Nikolay Shirokovskiy <Nikolay Shirokovskiy
nshirokovskiy(a)virtuozzo.com>
>>>
>>> Direct migration should work if *perform3 is present but *perform
>>> is not. This is situation when driver migration is implemented
>>> after new version of driver function is introduced. We should not
>>> be forced to support old version too as its parameter space is
>>> subspace of newer one.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
>>> ---
>>> src/libvirt-domain.c | 3 ++-
>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
>>> index 6ab50ba..c89775b 100644
>>> --- a/src/libvirt-domain.c
>>> +++ b/src/libvirt-domain.c
>>> @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain,
>>> NULLSTR(xmlin), flags, NULLSTR(dname),
NULLSTR(dconnuri),
>>> NULLSTR(miguri), bandwidth);
>>>
>>> - if (!domain->conn->driver->domainMigratePerform) {
>>> + if (!domain->conn->driver->domainMigratePerform &&
>>> + !domain->conn->driver->domainMigratePerform3) {
>>> virReportUnsupportedError();
>>> return -1;
>>> }
>>>
>>
>>
>> Hm.. domainMigratePerform3 will be used iff connection driver has
>> VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that
>> regardless. What if we check the presence of implementation with respect
>> to that?
> I see you mean actual driver could be behind remote one and checking
> for perform3 always gives true so we need to check for feature instead?
Sort of. domainMigratePerform3 is used only if the feature is present.
But after your patch, the function implementation would be needed
always, even if the feature is not present.
Why? You can implement version1 or
version3 or both for this check to pass.
Michal
4:05 a.m.
On 31.08.2015 10:42, Nikolay Shirokovskiy wrote:
On 28.08.2015 19:04, Michal Privoznik wrote:
> On 28.08.2015 11:29, Nikolay Shirokovskiy wrote:
>>
>>
>> On 28.08.2015 08:54, Michal Privoznik wrote:
>>> On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
>>>> From: Nikolay Shirokovskiy <Nikolay Shirokovskiy
nshirokovskiy(a)virtuozzo.com>
>>>>
>>>> Direct migration should work if *perform3 is present but *perform
>>>> is not. This is situation when driver migration is implemented
>>>> after new version of driver function is introduced. We should not
>>>> be forced to support old version too as its parameter space is
>>>> subspace of newer one.
>>>>
>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
>>>> ---
>>>> src/libvirt-domain.c | 3 ++-
>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
>>>> index 6ab50ba..c89775b 100644
>>>> --- a/src/libvirt-domain.c
>>>> +++ b/src/libvirt-domain.c
>>>> @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain,
>>>> NULLSTR(xmlin), flags, NULLSTR(dname),
NULLSTR(dconnuri),
>>>> NULLSTR(miguri), bandwidth);
>>>>
>>>> - if (!domain->conn->driver->domainMigratePerform) {
>>>> + if (!domain->conn->driver->domainMigratePerform &&
>>>> + !domain->conn->driver->domainMigratePerform3) {
>>>> virReportUnsupportedError();
>>>> return -1;
>>>> }
>>>>
>>>
>>>
>>> Hm.. domainMigratePerform3 will be used iff connection driver has
>>> VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that
>>> regardless. What if we check the presence of implementation with respect
>>> to that?
>> I see you mean actual driver could be behind remote one and checking
>> for perform3 always gives true so we need to check for feature instead?
>
> Sort of. domainMigratePerform3 is used only if the feature is present.
> But after your patch, the function implementation would be needed
> always, even if the feature is not present.
Why? You can implement version1 or version3 or both for this check to
pass.
Unfortunately no. I mean, the current code does not have any fallback. Whenever the
feature is detected, domainMigratePerform3 is used and that's it. Therefore I think we
are aiming on something among the following lines:
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cbf08fc..218efe8 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3425,16 +3425,16 @@ virDomainMigrateDirect(virDomainPtr domain,
NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri),
bandwidth);
- if (!domain->conn->driver->domainMigratePerform) {
- virReportUnsupportedError();
- return -1;
- }
-
/* Perform the migration. The driver isn't supposed to return
* until the migration is complete.
*/
if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_V3)) {
+ if (!domain->conn->driver->domainMigratePerform3) {
+ virReportUnsupportedError();
+ return -1;
+ }
+
VIR_DEBUG("Using migration protocol 3");
/* dconn URI not relevant in direct migration, since no
* target libvirtd is involved */
@@ -3450,6 +3450,11 @@ virDomainMigrateDirect(virDomainPtr domain,
dname,
bandwidth);
} else {
+ if (!domain->conn->driver->domainMigratePerform) {
+ virReportUnsupportedError();
+ return -1;
+ }
+
VIR_DEBUG("Using migration protocol 2");
if (xmlin) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
The other approach could be:
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cbf08fc..9d4827a 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3420,12 +3420,18 @@ virDomainMigrateDirect(virDomainPtr domain,
const char *uri,
unsigned long bandwidth)
{
+ bool v3migration;
+
VIR_DOMAIN_DEBUG(domain,
"xmlin=%s, flags=%lx, dname=%s, uri=%s, bandwidth=%lu",
NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri),
bandwidth);
- if (!domain->conn->driver->domainMigratePerform) {
+ v3migration = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_V3);
+
+ if ((!v3migration && !domain->conn->driver->domainMigratePerform)
||
+ (v3migration && !domain->conn->driver->domainMigratePerform3))
{
virReportUnsupportedError();
return -1;
}
@@ -3433,8 +3439,7 @@ virDomainMigrateDirect(virDomainPtr domain,
/* Perform the migration. The driver isn't supposed to return
* until the migration is complete.
*/
- if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_V3)) {
+ if (v3migration) {
VIR_DEBUG("Using migration protocol 3");
/* dconn URI not relevant in direct migration, since no
* target libvirtd is involved */
Michal
5:22 a.m.
On 31.08.2015 12:05, Michal Privoznik wrote:
On 31.08.2015 10:42, Nikolay Shirokovskiy wrote:
>
>
> On 28.08.2015 19:04, Michal Privoznik wrote:
>> On 28.08.2015 11:29, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 28.08.2015 08:54, Michal Privoznik wrote:
>>>> On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
>>>>> From: Nikolay Shirokovskiy <Nikolay Shirokovskiy
nshirokovskiy(a)virtuozzo.com>
>>>>>
>>>>> Direct migration should work if *perform3 is present but *perform
>>>>> is not. This is situation when driver migration is implemented
>>>>> after new version of driver function is introduced. We should not
>>>>> be forced to support old version too as its parameter space is
>>>>> subspace of newer one.
>>>>>
>>>>> Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
>>>>> ---
>>>>> src/libvirt-domain.c | 3 ++-
>>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
>>>>> index 6ab50ba..c89775b 100644
>>>>> --- a/src/libvirt-domain.c
>>>>> +++ b/src/libvirt-domain.c
>>>>> @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain,
>>>>> NULLSTR(xmlin), flags, NULLSTR(dname),
NULLSTR(dconnuri),
>>>>> NULLSTR(miguri), bandwidth);
>>>>>
>>>>> - if (!domain->conn->driver->domainMigratePerform) {
>>>>> + if (!domain->conn->driver->domainMigratePerform
&&
>>>>> + !domain->conn->driver->domainMigratePerform3) {
>>>>> virReportUnsupportedError();
>>>>> return -1;
>>>>> }
>>>>>
>>>>
>>>>
>>>> Hm.. domainMigratePerform3 will be used iff connection driver has
>>>> VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that
>>>> regardless. What if we check the presence of implementation with respect
>>>> to that?
>>> I see you mean actual driver could be behind remote one and checking
>>> for perform3 always gives true so we need to check for feature instead?
>>
>> Sort of. domainMigratePerform3 is used only if the feature is present.
>> But after your patch, the function implementation would be needed
>> always, even if the feature is not present.
> Why? You can implement version1 or version3 or both for this check to pass.
Unfortunately no. I mean, the current code does not have any fallback. Whenever the
feature is detected, domainMigratePerform3 is used and that's it. Therefore I think we
are aiming on something among the following lines:
I see. There are 2 issues in the code.
1. perform must must be implemented even if perform3 will be used.
2. perform3 called without check
I address only 1st case, you addess both.
Among suggested implementations I would prefer first. So I guess I should say ACK.
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cbf08fc..218efe8 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3425,16 +3425,16 @@ virDomainMigrateDirect(virDomainPtr domain,
NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri),
bandwidth);
- if (!domain->conn->driver->domainMigratePerform) {
- virReportUnsupportedError();
- return -1;
- }
-
/* Perform the migration. The driver isn't supposed to return
* until the migration is complete.
*/
if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_V3)) {
+ if (!domain->conn->driver->domainMigratePerform3) {
+ virReportUnsupportedError();
+ return -1;
+ }
+
VIR_DEBUG("Using migration protocol 3");
/* dconn URI not relevant in direct migration, since no
* target libvirtd is involved */
@@ -3450,6 +3450,11 @@ virDomainMigrateDirect(virDomainPtr domain,
dname,
bandwidth);
} else {
+ if (!domain->conn->driver->domainMigratePerform) {
+ virReportUnsupportedError();
+ return -1;
+ }
+
VIR_DEBUG("Using migration protocol 2");
if (xmlin) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
The other approach could be:
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cbf08fc..9d4827a 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3420,12 +3420,18 @@ virDomainMigrateDirect(virDomainPtr domain,
const char *uri,
unsigned long bandwidth)
{
+ bool v3migration;
+
VIR_DOMAIN_DEBUG(domain,
"xmlin=%s, flags=%lx, dname=%s, uri=%s, bandwidth=%lu",
NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri),
bandwidth);
- if (!domain->conn->driver->domainMigratePerform) {
+ v3migration = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_MIGRATION_V3);
+
+ if ((!v3migration && !domain->conn->driver->domainMigratePerform)
||
+ (v3migration && !domain->conn->driver->domainMigratePerform3))
{
virReportUnsupportedError();
return -1;
}
@@ -3433,8 +3439,7 @@ virDomainMigrateDirect(virDomainPtr domain,
/* Perform the migration. The driver isn't supposed to return
* until the migration is complete.
*/
- if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
- VIR_DRV_FEATURE_MIGRATION_V3)) {
+ if (v3migration) {
VIR_DEBUG("Using migration protocol 3");
/* dconn URI not relevant in direct migration, since no
* target libvirtd is involved */
Michal
3373
days inactive
3377
days old
6 comments
3 participants
participants (3)
-
Michal Privoznik -
Nikolay Shirokovskiy -
Nikolay Shirokovskiy