[libvirt] [PATCH] migration: remove direct migration dependency on version1 of driver

[Nikolay Shirokovskiy]

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 at 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 at 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
>