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@xxxxxxxxxxxxx>
>>>>>>
>>>>>> 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@xxxxxxxxxxxxx>
>>>>>> ---
>>>>>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list