[dheeraj12347]

Description

This PR fixes a storage pool reference-count leak in createStoragePoolLocked().

When the reference count is incremented for a secondary storage pool, subsequent failures in checkNetfsStoragePoolMounted() or getStoragePool() can leave the reference count incremented because the existing exception handling only catches LibvirtException.

Solution

• Track whether the storage pool reference count was successfully incremented.
• Track whether the operation completed successfully.
• Use a finally block to decrement the reference count if the operation fails after the increment.
• Preserve existing LibvirtException handling.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Bug Severity

• Minor

How Has This Been Tested?

Added testCreateStoragePoolDecrementsRefCountWhenGetStoragePoolFails() to verify that the reference count is decremented when getStoragePool() throws a CloudRuntimeException.

• git diff --check: Passed.
• Maven tests: Not rerun after the final changes.

Related: #14189

[bhouse-nexthop]

Thanks for picking this up. The fix itself looks right to me: decrementing in a finally covers the CloudRuntimeException from getStoragePool(), and gating it on refCountIncremented also stops the old catch from decrementing a primary pool that was never counted.

One problem: this depends on #14189 but targets 4.22 without it. createStoragePoolLocked only exists in #14189, and incStoragePoolRefCount/decStoragePoolRefCount are private on 4.22 (#14189 makes them protected). Applied to 4.22 alone, LibvirtStorageAdaptorTest doesn't compile:

LibvirtStorageAdaptorTest.java:[197,19] error: cannot find symbol
  symbol:   method createStoragePoolLocked(String,String,int,String,<null>,StoragePoolType,<null>,boolean)
LibvirtStorageAdaptorTest.java:[210,35] error: incStoragePoolRefCount(String) has private access in LibvirtStorageAdaptor
LibvirtStorageAdaptorTest.java:[211,35] error: decStoragePoolRefCount(String) has private access in LibvirtStorageAdaptor

On top of #14189, the main change applies cleanly but the test file conflicts with the tests #14189 adds there.

Two ways to go: rebase this onto 4.22 once #14189 merges, which is the simplest; or make it standalone now by testing through the public createStoragePool(...) and checking the ref count through storagePoolRefCounts rather than spying on the private methods. Either works for me.

[dheeraj12347]

Thanks for picking this up. The fix itself looks right to me: decrementing in a finally covers the CloudRuntimeException from getStoragePool(), and gating it on refCountIncremented also stops the old catch from decrementing a primary pool that was never counted.

One problem: this depends on #14189 but targets 4.22 without it. createStoragePoolLocked only exists in #14189, and incStoragePoolRefCount/decStoragePoolRefCount are private on 4.22 (#14189 makes them protected). Applied to 4.22 alone, LibvirtStorageAdaptorTest doesn't compile:

LibvirtStorageAdaptorTest.java:[197,19] error: cannot find symbol
  symbol:   method createStoragePoolLocked(String,String,int,String,<null>,StoragePoolType,<null>,boolean)
LibvirtStorageAdaptorTest.java:[210,35] error: incStoragePoolRefCount(String) has private access in LibvirtStorageAdaptor
LibvirtStorageAdaptorTest.java:[211,35] error: decStoragePoolRefCount(String) has private access in LibvirtStorageAdaptor

On top of #14189, the main change applies cleanly but the test file conflicts with the tests #14189 adds there.

Two ways to go: rebase this onto 4.22 once #14189 merges, which is the simplest; or make it standalone now by testing through the public createStoragePool(...) and checking the ref count through storagePoolRefCounts rather than spying on the private methods. Either works for me.

Thanks for pointing that out, @bhouse-nexthop! I’ll wait for #14189 to get merged and then update my PR on top of it. Appreciate you taking a look!