Skip to content

<fix>[storage]: bypass space helper when multi space not support #3206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zstack-robot-1 wants to merge 1 commit into
base: 5.5.6
Choose a base branch
from
Open

<fix>[storage]: bypass space helper when multi space not support #3206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1594,8 +1594,11 @@ protected void handle(DeleteVolumeOnPrimaryStorageMsg msg) {
trashVolume(msg.getVolume().getInstallPath(), msg.getVolume().getProtocol(), force, new Completion(msg) {
@Override
public void success() {
ExternalPrimaryStorageSpaceCapacityHelper helper = new ExternalPrimaryStorageSpaceCapacityHelper(self.getUuid(), controller.getIdentity());
helper.releaseAvailableCapWithRatio(msg.getVolume().getInstallPath(), msg.getVolume().getSize());
if (controller.reportCapabilities().isSupportMultiSpace()) {
ExternalPrimaryStorageSpaceCapacityHelper helper = new ExternalPrimaryStorageSpaceCapacityHelper(self.getUuid(), controller.getIdentity());
helper.releaseAvailableCapWithRatio(msg.getVolume().getInstallPath(), msg.getVolume().getSize());
}

bus.reply(msg, reply);
}

Expand Down Expand Up @@ -1808,8 +1811,11 @@ protected void handle(DeleteSnapshotOnPrimaryStorageMsg msg) {
controller.deleteSnapshot(msg.getSnapshot().getPrimaryStorageInstallPath(), new Completion(msg) {
@Override
public void success() {
ExternalPrimaryStorageSpaceCapacityHelper helper = new ExternalPrimaryStorageSpaceCapacityHelper(self.getUuid(), controller.getIdentity());
helper.releaseAvailableCapacity(msg.getSnapshot().getPrimaryStorageInstallPath(), msg.getSnapshot().getSize());
if (controller.reportCapabilities().isSupportMultiSpace()) {
ExternalPrimaryStorageSpaceCapacityHelper helper = new ExternalPrimaryStorageSpaceCapacityHelper(self.getUuid(), controller.getIdentity());
helper.releaseAvailableCapacity(msg.getSnapshot().getPrimaryStorageInstallPath(), msg.getSnapshot().getSize());
}

bus.reply(msg, reply);
}

Expand Down
10 changes: 6 additions & 4 deletions ...main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,16 @@ public class ExternalPrimaryStorageSpaceCapacityHelper extends ExternalPrimarySt
private String spaceName;
private Map<String, ExternalPrimaryStorageSpaceVO> storageSpacesByUrl;

// TODO: use factory to create helper, diff capabilities helper for diff types
@Deprecated
public ExternalPrimaryStorageSpaceCapacityHelper(ExternalPrimaryStorageVO ps) {
super(ps);
this.primaryStorageUuid = ps.getUuid();
this.spaceName = ps.getIdentity();
}

// TODO: use factory to create helper, diff capabilities helper for diff types
@Deprecated
public ExternalPrimaryStorageSpaceCapacityHelper(String psUuid, String identity) {
super(psUuid, identity);
this.primaryStorageUuid = psUuid;
Expand Down Expand Up @@ -116,10 +120,8 @@ protected void updateStorageSpace(StorageCapacity cap) {
}

private ExternalPrimaryStorageSpaceVO getSpaceFromInstallUrl(String installPath) {
Map<String, ExternalPrimaryStorageSpaceVO> spacesByUrl = getStorageSpacesByUrl();
String spaceUrl = spacesByUrl.keySet().stream().filter(installPath::startsWith)
.findFirst().orElseThrow(() -> new OperationFailureException(operr(ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10002, "cannot find storage space for installPath[%s]", installPath)));
return spacesByUrl.get(spaceUrl);
String spaceUrl = getLocationSpaceUrl(installPath);
return getStorageSpacesByUrl().get(spaceUrl);
}
Comment on lines 122 to 125
Copy link

@coderabbitai coderabbitai bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cd storage && rg -n -B2 -A5 'getSpaceFromInstallUrl' --type java

Repository: MatheMatrix/zstack

Length of output: 8625

🏁 Script executed:

cd storage && rg -n -B2 -A10 'private.*getLocationSpaceUrl' --type java

Repository: MatheMatrix/zstack

Length of output: 44

🏁 Script executed:

cd storage && rg -n -B5 -A10 '(void _release|void _reserve)' --type java

Repository: MatheMatrix/zstack

Length of output: 2461

🏁 Script executed:

cd storage && cat -n src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java | head -130 | tail -40

Repository: MatheMatrix/zstack

Length of output: 2179

🏁 Script executed:

cd storage && rg -n 'getLocationSpaceUrl|getStorageSpacesByUrl' src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java

Repository: MatheMatrix/zstack

Length of output: 371

🏁 Script executed:

cd storage && rg -n -B2 -A8 'protected.*getLocationSpaceUrl|private.*getLocationSpaceUrl|public.*getLocationSpaceUrl' src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java

Repository: MatheMatrix/zstack

Length of output: 44

🏁 Script executed:

cd storage && rg -n -B2 -A8 'protected.*getStorageSpacesByUrl|private.*getStorageSpacesByUrl|public.*getStorageSpacesByUrl' src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java

Repository: MatheMatrix/zstack

Length of output: 44

🏁 Script executed:

cd storage && head -50 src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java

Repository: MatheMatrix/zstack

Length of output: 2256

🏁 Script executed:

cd storage && wc -l src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java && tail -100 src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java

Repository: MatheMatrix/zstack

Length of output: 5379

🏁 Script executed:

cd storage && rg -n 'class ExternalPrimaryStorageSpaceHelper' src/main/java/org/zstack/storage/addon/primary/

Repository: MatheMatrix/zstack

Length of output: 199

🏁 Script executed:

cd storage && fd ExternalPrimaryStorageSpaceHelper.java

Repository: MatheMatrix/zstack

Length of output: 147

🏁 Script executed:

cd storage && cat src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceHelper.java

Repository: MatheMatrix/zstack

Length of output: 2464

🏁 Script executed:

cd storage && rg -n 'storageSpacesByUrl\s*=' src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceHelper.java

Repository: MatheMatrix/zstack

Length of output: 219

🏁 Script executed:

cd storage && rg -n 'storageSpacesByUrl\s*=' src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java

Repository: MatheMatrix/zstack

Length of output: 44

getSpaceFromInstallUrl 返回结果未进行空值检查。

getSpaceFromInstallUrl 通过 getStorageSpacesByUrl().get(spaceUrl) 获取存储空间，当该 URL 不在缓存的 map 中时会返回 null。所有调用方（第 145、171、204、224、241、266 行）直接对返回结果调用方法（如 setAvailableCapacity()、getLocationUrl()）而未进行空值检查，可能导致 NPE。

建议在调用 getSpaceFromInstallUrl 后添加空值验证，或在 getLocationSpaceUrl 中额外校验查找到的 URL 是否存在于 map 中。

🤖 Prompt for AI Agents 
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSpaceCapacityHelper.java`
around lines 122 - 125, The getSpaceFromInstallUrl method can return null
because getStorageSpacesByUrl().get(spaceUrl) may miss the key; update callers
that immediately use its result (the places that call setAvailableCapacity(),
getLocationUrl(), etc.) to null-check the returned ExternalPrimaryStorageSpaceVO
and handle the missing-space case (log a warning and skip processing or throw a
controlled exception), or change getSpaceFromInstallUrl to validate existence
against getStorageSpacesByUrl() and either return a non-null wrapper/Optional or
throw a clear exception; reference getSpaceFromInstallUrl, getLocationSpaceUrl,
and getStorageSpacesByUrl when implementing the check/handling.


@DeadlockAutoRestart
Expand Down