Martin Kletzander wrote:
On Wed, Mar 25, 2015 at 02:08:34PM -0600, Jim Fehlig wrote:
Let callers of libxlDomainStart decide when it is appropriate to acquire a job on the associated virDomainObj.
This makes sense, I see many bugs this fixes, but how come libxlDomainShutdownThread(), libxlDomainRestoreFlags() and libxlDoMigrateReceive() don't need to start the job when they all call libxlDomainStart()?
Commit 0521d658 starts a job for libxlDomainShutdownThread. This patch adds starting a job in libxlDomainRestoreFlags. For migration, I'll need to work on a follow-up series that fixes job handling in general.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 24 ++++++++-------------- src/libxl/libxl_driver.c | 53 +++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 52 insertions(+), 25 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 05f6eb1..da4c1c7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -885,17 +893,26 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, goto cleanup; def = NULL;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + goto cleanup; + } +
This should be acquired before virDomainObjListAdd() since you need to check whether it's active after creating the job.
Acquiring the job requires a virDomainObj, which we get from the call to virDomainObjListAdd().
If I'm wrong, then virDomainObjListRemove() should be only called if the vm is not persistent (as CreateXML can be called on persistent ones as well), shouldn't it?
Yes, I think you are right. This was coded up assuming CreateXML only handled transient domains.
[...]
@@ -1695,11 +1712,20 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
def = NULL;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { + if (!vm->persistent) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } + goto cleanup; + } +
Same here, I guess.
Yes :-). A virDomainObj is needed to acquire a job. Thanks for the review. I'll fix calling virDomainObjListRemove() on persistent domains in V2. Regards, Jim