Re: [Xen-devel] [libvirt] [PATCH 1/3] libxl: Move job acquisition in libxlDomainStart to callers

[Jim Fehlig <jfehlig@xxxxxxxx>]

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@xxxxxxxx>
>> ---
>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel