Eric Blake wrote:
> 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@xxxxxxxx>
>>>
>
>
>>> +
>>> + 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.
Oh, right. Thanks for catching that.
> 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.
>
Agreed. Bamvor, ignore my comments about changing this pattern
throughout the patch.
Regards,
Jim
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list