Re: [libvirt] [PATCH 1/3] libxl: Move job acquisition in libxlDomainStart to callers
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(a)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