diff mbox

[v3,2/3] libxl: move begin phase job handling

Message ID 1453316408-30373-3-git-send-email-joao.m.martins@oracle.com
State New, archived
Headers show

Commit Message

Joao Martins Jan. 20, 2016, 7 p.m. UTC
. From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
This is a preparatory patch to be able to begin a job in the
perform phase.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
 src/libxl/libxl_migration.c | 16 +++-------------
 2 files changed, 20 insertions(+), 14 deletions(-)

Comments

Jim Fehlig Feb. 2, 2016, 1:23 a.m. UTC | #1
On 01/20/2016 12:00 PM, Joao Martins wrote:
> . From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
> This is a preparatory patch to be able to begin a job in the
> perform phase.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
>  src/libxl/libxl_migration.c | 16 +++-------------
>  2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d34f843..28220b2 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>  {
>      const char *xmlin = NULL;
>      virDomainObjPtr vm = NULL;
> +    libxlDriverPrivatePtr driver;
> +    char *ret;
>  
>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>      virReportUnsupportedError();
> @@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>          return NULL;
>      }
>  
> -    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
> +    driver = domain->conn->privateData;
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
> +        virObjectUnlock(vm);
> +        return NULL;
> +    }
> +
> +    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
> +
> +    if (!libxlDomainObjEndJob(driver, vm))
> +        vm = NULL;

IIRC the qemu driver handles this a bit differently, and probably more
correctly. On the sender, a job is started during the begin phase and ended in
the confirm phase. On the receiver, a job is started in the prepare phase and
ended in the finish phase. This prevents modifying the virDomainObj between
phases, e.g. the begin and perform phases on the sender. Is it possible to
change the job handling similarly in the libxl migration phases?

Regards,
Jim
Joao Martins Feb. 2, 2016, 12:05 p.m. UTC | #2
On 02/02/2016 01:23 AM, Jim Fehlig wrote:
> On 01/20/2016 12:00 PM, Joao Martins wrote:
>> . From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
>> This is a preparatory patch to be able to begin a job in the
>> perform phase.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
>>  src/libxl/libxl_migration.c | 16 +++-------------
>>  2 files changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index d34f843..28220b2 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>  {
>>      const char *xmlin = NULL;
>>      virDomainObjPtr vm = NULL;
>> +    libxlDriverPrivatePtr driver;
>> +    char *ret;
>>  
>>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>>      virReportUnsupportedError();
>> @@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>          return NULL;
>>      }
>>  
>> -    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>> +    driver = domain->conn->privateData;
>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
>> +        virObjectUnlock(vm);
>> +        return NULL;
>> +    }
>> +
>> +    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>> +
>> +    if (!libxlDomainObjEndJob(driver, vm))
>> +        vm = NULL;
> 
> IIRC the qemu driver handles this a bit differently, and probably more
> correctly. On the sender, a job is started during the begin phase and ended in
> the confirm phase. On the receiver, a job is started in the prepare phase and
> ended in the finish phase. This prevents modifying the virDomainObj between
> phases, e.g. the begin and perform phases on the sender. Is it possible to
> change the job handling similarly in the libxl migration phases?
> 
Yeah, IIUC I believe this is the behavior depicted by
VIR_MIGRATE_CHANGE_PROTECTION flag in which is set by default if the driver
support it. So since we're going this way, we should adversite it too in
libxlConnectSupportsFeature() ?

Btw, the P2P patch keeps the original behavior wrt to jobs, and these two
patches meant solely for improving job handling in migration in general. What do
you think in making it a separate patch from this series? (with the changes
suggested above)

Joao

> Regards,
> Jim
>
Jim Fehlig Feb. 2, 2016, 3:39 p.m. UTC | #3
Joao Martins wrote:
> 
> On 02/02/2016 01:23 AM, Jim Fehlig wrote:
>> On 01/20/2016 12:00 PM, Joao Martins wrote:
>>> . From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
>>> This is a preparatory patch to be able to begin a job in the
>>> perform phase.
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>>  src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
>>>  src/libxl/libxl_migration.c | 16 +++-------------
>>>  2 files changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index d34f843..28220b2 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>>  {
>>>      const char *xmlin = NULL;
>>>      virDomainObjPtr vm = NULL;
>>> +    libxlDriverPrivatePtr driver;
>>> +    char *ret;
>>>  
>>>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>>>      virReportUnsupportedError();
>>> @@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>>          return NULL;
>>>      }
>>>  
>>> -    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>>> +    driver = domain->conn->privateData;
>>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
>>> +        virObjectUnlock(vm);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>>> +
>>> +    if (!libxlDomainObjEndJob(driver, vm))
>>> +        vm = NULL;
>> IIRC the qemu driver handles this a bit differently, and probably more
>> correctly. On the sender, a job is started during the begin phase and ended in
>> the confirm phase. On the receiver, a job is started in the prepare phase and
>> ended in the finish phase. This prevents modifying the virDomainObj between
>> phases, e.g. the begin and perform phases on the sender. Is it possible to
>> change the job handling similarly in the libxl migration phases?
>>
> Yeah, IIUC I believe this is the behavior depicted by
> VIR_MIGRATE_CHANGE_PROTECTION flag in which is set by default if the driver
> support it. So since we're going this way, we should adversite it too in
> libxlConnectSupportsFeature() ?

Yep, sounds good.

> Btw, the P2P patch keeps the original behavior wrt to jobs, and these two
> patches meant solely for improving job handling in migration in general. What do
> you think in making it a separate patch from this series? (with the changes
> suggested above)

Agreed. I'll take another look at the P2P patch and push it (after fixing the
nits) if there are no further comments. Implementing the CHANGE_PROTECTION
behavior and improving job handling should be done separately.

Regards,
Jim
Joao Martins Feb. 2, 2016, 4:55 p.m. UTC | #4
On 02/02/2016 03:39 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>>
>> On 02/02/2016 01:23 AM, Jim Fehlig wrote:
>>> On 01/20/2016 12:00 PM, Joao Martins wrote:
>>>> . From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
>>>> This is a preparatory patch to be able to begin a job in the
>>>> perform phase.
>>>>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> ---
>>>>  src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
>>>>  src/libxl/libxl_migration.c | 16 +++-------------
>>>>  2 files changed, 20 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>>> index d34f843..28220b2 100644
>>>> --- a/src/libxl/libxl_driver.c
>>>> +++ b/src/libxl/libxl_driver.c
>>>> @@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>>>  {
>>>>      const char *xmlin = NULL;
>>>>      virDomainObjPtr vm = NULL;
>>>> +    libxlDriverPrivatePtr driver;
>>>> +    char *ret;
>>>>  
>>>>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>>>>      virReportUnsupportedError();
>>>> @@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>>>          return NULL;
>>>>      }
>>>>  
>>>> -    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>>>> +    driver = domain->conn->privateData;
>>>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
>>>> +        virObjectUnlock(vm);
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>>>> +
>>>> +    if (!libxlDomainObjEndJob(driver, vm))
>>>> +        vm = NULL;
>>> IIRC the qemu driver handles this a bit differently, and probably more
>>> correctly. On the sender, a job is started during the begin phase and ended in
>>> the confirm phase. On the receiver, a job is started in the prepare phase and
>>> ended in the finish phase. This prevents modifying the virDomainObj between
>>> phases, e.g. the begin and perform phases on the sender. Is it possible to
>>> change the job handling similarly in the libxl migration phases?
>>>
>> Yeah, IIUC I believe this is the behavior depicted by
>> VIR_MIGRATE_CHANGE_PROTECTION flag in which is set by default if the driver
>> support it. So since we're going this way, we should adversite it too in
>> libxlConnectSupportsFeature() ?
> 
> Yep, sounds good.
> 
>> Btw, the P2P patch keeps the original behavior wrt to jobs, and these two
>> patches meant solely for improving job handling in migration in general. What do
>> you think in making it a separate patch from this series? (with the changes
>> suggested above)
> 
> Agreed. I'll take another look at the P2P patch and push it (after fixing the
> nits) if there are no further comments. Implementing the CHANGE_PROTECTION
> behavior and improving job handling should be done separately.
> 
Cool, Thanks!

> Regards,
> Jim
>
diff mbox

Patch

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d34f843..28220b2 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5187,6 +5187,8 @@  libxlDomainMigrateBegin3Params(virDomainPtr domain,
 {
     const char *xmlin = NULL;
     virDomainObjPtr vm = NULL;
+    libxlDriverPrivatePtr driver;
+    char *ret;
 
 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
     virReportUnsupportedError();
@@ -5223,7 +5225,21 @@  libxlDomainMigrateBegin3Params(virDomainPtr domain,
         return NULL;
     }
 
-    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
+    driver = domain->conn->privateData;
+    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+        virObjectUnlock(vm);
+        return NULL;
+    }
+
+    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
+
+    if (!libxlDomainObjEndJob(driver, vm))
+        vm = NULL;
+
+    if (vm)
+        virObjectUnlock(vm);
+
+    return ret;
 }
 
 static int
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 28445fc..e3c7914 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -235,17 +235,14 @@  libxlDomainMigrationBegin(virConnectPtr conn,
     virDomainDefPtr def;
     char *xml = NULL;
 
-    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
-        goto cleanup;
-
     if (xmlin) {
         if (!(tmpdef = virDomainDefParseString(xmlin, cfg->caps,
                                                driver->xmlopt,
                                                VIR_DOMAIN_DEF_PARSE_INACTIVE)))
-            goto endjob;
+            goto cleanup;
 
         if (!libxlDomainDefCheckABIStability(driver, vm->def, tmpdef))
-            goto endjob;
+            goto cleanup;
 
         def = tmpdef;
     } else {
@@ -253,18 +250,11 @@  libxlDomainMigrationBegin(virConnectPtr conn,
     }
 
     if (!libxlDomainMigrationIsAllowed(def))
-        goto endjob;
+        goto cleanup;
 
     xml = virDomainDefFormat(def, VIR_DOMAIN_DEF_FORMAT_SECURE);
 
- endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
-
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
-
     virDomainDefFree(tmpdef);
     virObjectUnref(cfg);
     return xml;