Re: [libvirt] [PATCH 2/8] tests: Add createVHBAByStoragePool-by-parent to fchosttest
On 02/27/2017 11:05 PM, John Ferlan wrote:
[...]
>> Anyway, so as an adjustment at least here... I could move the hunk below
>> the pool->active = 1 and before the event. Then drop the lock entirely
>> before call the testCreateVport. Would also need to add a 'isLocked' so
>> that the unlock isn't called unnecessarily. Of course that's almost as
>> equally as ugly.
>>
>> Unless you had another methodology in mind...
>
> What about these lines (in testNodeDeviceMockCreateVport):
>
> if (!(objcopy = virNodeDeviceFindByName(&driver->devs,
"scsi_host11")) ||
> !(xml = virNodeDeviceDefFormat(objcopy->def)) ||
> !(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
> goto cleanup;
>
> I haven't even compile-tested them, but in general - they call the important
parts of the public APIs without us needing to lock()/unlock() the driver meanwhile. There
might be some additional unlock(objcopy) required, unref() or free(), but I'm willing
to do that if it saves us from weird driver lock()/unlock() calls.
>
> If you have this ^^ patch before this one, you can just drop test driver
lock()/unlock() here.
>
> Michal
>
The above doesn't work as cleanly as one would hope as eventtest hangs,
but after a bit of finagling, the following works:
Ah, now I see why it hangs. The problem lies elsewhere:
testNodeDeviceCreateXML() calls public APIs thus it unlocks the driver
again. I mean, testNodeDeviceMockCreateVport() is not the only function
it calls with unlocked driver. Le sigh.
if (!(objcpy = virNodeDeviceFindByName(&driver->devs,
"scsi_host11")))
goto cleanup;
xml = virNodeDeviceDefFormat(objcpy->def);
virNodeDeviceObjUnlock(objcpy);
if (!xml)
goto cleanup;
if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
goto cleanup;
Yup, I've expected that we will need to unlock the object returned by
virNodeDeviceFindByName(). This looks much nicer IMO. But we still need
to fix testNodeDeviceCreateXML(). Working on the fix now.
Going this route also removes the need for the existing caller to do
unlock/lock game as well.
John
FWIW: The lock gets easier with RFC series and of course that's in the
back of my mind every time I touch this code... All the fun I'll have
merging changes...
Ah, which patches are that? I want to review them.
Michal