Re: [libvirt] [PATCH 2/2] Add lock in libxl api

On 10/22/2012 05:10 PM, Jim Fehlig wrote: > Bamvor Jian Zhang wrote: > >> Add long-running jobs for save, dump. Add normal job for the >> api maybe modify the domain. >> >> Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com&gt; >> >> + >> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) >> + goto cleanup; >> + >> if (!virDomainObjIsActive(vm)) { >> virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running")); >> - goto cleanup; >> + goto endjob; >> } >> >> > IMO, we should check if the domain is active before calling > libxlDomainObjBeginJob(). > That's a possible optimization to avoid contending for the lock, but the point remains that libxlDomainObjBeginJob() temporarily drops locks, and while locks are down, the domain can stop.

Therefore, you must ALWAYS check if the domain is active after obtaining the job, even if you already checked prior to requesting the job. This particular section of code looks correct to me as-is.