Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
82 changes: 82 additions & 0 deletions .gitconfig/hooks/pre-push
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#!/bin/bash
# Pre-push hook: validate remote, branch name, and commit before pushing
# Prevents common mistakes:
# - Pushing to fork instead of upstream (or vice versa)
# - Branch name not matching ZSTAC-XXXXX pattern for fix branches
# - Pushing wrong commits

RED='\033[0;31m'
YELLOW='\033[1;33m'
GREEN='\033[0;32m'
NC='\033[0m'

REMOTE="$1"
URL="$2"

BRANCH=$(git branch --show-current)

# Skip validation for non-fix branches (master, release branches, etc.)
if [[ ! "$BRANCH" =~ ^fix/ ]] && [[ ! "$BRANCH" =~ ^feature/ ]]; then
exit 0
fi

ERRORS=0

# --- Check 1: Branch name format ---
if [[ "$BRANCH" =~ ^fix/ ]]; then
if [[ ! "$BRANCH" =~ ^fix/ZSTAC-[0-9]+ ]]; then
echo -e "${RED}[pre-push] Branch name '$BRANCH' does not match fix/ZSTAC-XXXXX pattern${NC}"
echo -e "${YELLOW} Hint: Use 'ZSTAC' not 'ZSTACK'. Example: fix/ZSTAC-82099${NC}"
ERRORS=$((ERRORS + 1))
fi
fi

# --- Check 2: Remote URL sanity ---
REMOTE_URL=$(git remote get-url "$REMOTE" 2>/dev/null)
if [ -z "$REMOTE_URL" ]; then
echo -e "${RED}[pre-push] Cannot resolve remote '$REMOTE'${NC}"
ERRORS=$((ERRORS + 1))
fi

# Warn if pushing to upstream (zstackio) directly — usually want to push to fork (origin)
if echo "$REMOTE_URL" | grep -q "zstackio/" && [ "$REMOTE" = "upstream" ]; then
echo -e "${YELLOW}[pre-push] WARNING: Pushing directly to upstream (zstackio). Are you sure?${NC}"
echo -e "${YELLOW} Remote URL: $REMOTE_URL${NC}"
echo -e "${YELLOW} Usually you want to push to 'origin' (your fork) instead.${NC}"
# Warning only, don't block
fi

# --- Check 3: Verify latest commit has a Change-Id ---
LAST_COMMIT_MSG=$(git log -1 --format=%B)
if ! echo "$LAST_COMMIT_MSG" | grep -q "^Change-Id: I[0-9a-f]\{40\}"; then
echo -e "${RED}[pre-push] Latest commit is missing Change-Id${NC}"
echo -e "${YELLOW} Use 'source scripts/zcommit.sh && zcommit ...' to create commits with Change-Id${NC}"
ERRORS=$((ERRORS + 1))
fi

# --- Check 4: Verify latest commit references a Jira ticket (for fix branches) ---
if [[ "$BRANCH" =~ ^fix/ ]]; then
if ! echo "$LAST_COMMIT_MSG" | grep -qi "ZSTAC-[0-9]\+"; then
echo -e "${RED}[pre-push] Latest commit does not reference a ZSTAC ticket${NC}"
ERRORS=$((ERRORS + 1))
fi
fi

# --- Check 5: Confirm repo identity (main vs premium) ---
TOPLEVEL=$(git rev-parse --show-toplevel 2>/dev/null)
REPO_NAME=$(basename "$TOPLEVEL")
if [[ "$BRANCH" =~ premium ]] && [[ "$REPO_NAME" != "premium" ]]; then
echo -e "${RED}[pre-push] Branch name suggests premium but you're in repo '$REPO_NAME'${NC}"
ERRORS=$((ERRORS + 1))
fi
Comment on lines +65 to +71
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

premium 匹配规则过于宽松,可能导致误报

当前检查 [[ "$BRANCH" =~ premium ]] 会匹配分支名中任意位置出现的 "premium"。例如,在主仓库中创建的 fix/ZSTAC-12345-premium-bugfix 分支会被误判为 premium 仓库的分支而报错。

建议使用更精确的匹配模式:

🐛 建议的修复方案
 # --- Check 5: Confirm repo identity (main vs premium) ---
 TOPLEVEL=$(git rev-parse --show-toplevel 2>/dev/null)
 REPO_NAME=$(basename "$TOPLEVEL")
-if [[ "$BRANCH" =~ premium ]] && [[ "$REPO_NAME" != "premium" ]]; then
+# Only check branches explicitly targeting premium (e.g., premium/*, */premium/*)
+if [[ "$BRANCH" =~ ^premium/ || "$BRANCH" =~ /premium/ ]] && [[ "$REPO_NAME" != "premium" ]]; then
     echo -e "${RED}[pre-push] Branch name suggests premium but you're in repo '$REPO_NAME'${NC}"
     ERRORS=$((ERRORS + 1))
 fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# --- Check 5: Confirm repo identity (main vs premium) ---
TOPLEVEL=$(git rev-parse --show-toplevel 2>/dev/null)
REPO_NAME=$(basename "$TOPLEVEL")
if [[ "$BRANCH" =~ premium ]] && [[ "$REPO_NAME" != "premium" ]]; then
echo -e "${RED}[pre-push] Branch name suggests premium but you're in repo '$REPO_NAME'${NC}"
ERRORS=$((ERRORS + 1))
fi
# --- Check 5: Confirm repo identity (main vs premium) ---
TOPLEVEL=$(git rev-parse --show-toplevel 2>/dev/null)
REPO_NAME=$(basename "$TOPLEVEL")
# Only check branches explicitly targeting premium (e.g., premium/*, */premium/*)
if [[ "$BRANCH" =~ ^premium/ || "$BRANCH" =~ /premium/ ]] && [[ "$REPO_NAME" != "premium" ]]; then
echo -e "${RED}[pre-push] Branch name suggests premium but you're in repo '$REPO_NAME'${NC}"
ERRORS=$((ERRORS + 1))
fi
🤖 Prompt for AI Agents
In @.gitconfig/hooks/pre-push around lines 65 - 71, The branch-name check using
[[ "$BRANCH" =~ premium ]] is too loose and will match "premium" anywhere;
update the conditional that references the BRANCH variable to use a stricter
pattern (e.g., a regex that matches whole segment names like
(^|/)(premium)(/|$|-) or similar) so only branches whose final/segment name is
exactly "premium" (or clearly intended premium branches) trigger the error;
modify the if that uses BRANCH and REPO_NAME to apply this tighter match logic.


# --- Summary ---
if [ $ERRORS -gt 0 ]; then
echo ""
echo -e "${RED}[pre-push] $ERRORS error(s) found. Push blocked.${NC}"
echo -e "${YELLOW} To bypass: git push --no-verify${NC}"
exit 1
fi

echo -e "${GREEN}[pre-push] All checks passed: branch=$BRANCH remote=$REMOTE${NC}"
exit 0
28 changes: 28 additions & 0 deletions compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java
Original file line number Diff line number Diff line change
Expand Up @@ -7226,6 +7226,7 @@ protected void scripts() {
self.setZoneUuid(spec.getDestHost().getZoneUuid());
}
}.execute());
syncVmDevicesAddressInfo(self.getUuid());
logger.debug(String.format("vm[uuid:%s] is running ..", self.getUuid()));
VmInstanceInventory inv = VmInstanceInventory.valueOf(self);
extEmitter.afterStartVm(inv);
Expand Down Expand Up @@ -7495,6 +7496,9 @@ public void run(FlowTrigger trigger, Map data) {
self.setHypervisorType(spec.getDestHost().getHypervisorType());
self.setRootVolumeUuid(spec.getDestRootVolume().getUuid());
});
if (struct.getStrategy() == VmCreationStrategy.InstantStart) {
syncVmDevicesAddressInfo(self.getUuid());
}
logger.debug(String.format("vm[uuid:%s] is started ..", self.getUuid()));
VmInstanceInventory inv = VmInstanceInventory.valueOf(self);
extEmitter.afterStartNewCreatedVm(inv);
Expand Down Expand Up @@ -7931,6 +7935,7 @@ public void handle(Map data) {
public void done() {
self = changeVmStateInDb(VmInstanceStateEvent.running,
() -> self.setHostUuid(originalCopy.getHostUuid()));
syncVmDevicesAddressInfo(self.getUuid());
VmInstanceInventory inv = VmInstanceInventory.valueOf(self);
extEmitter.afterRebootVm(inv);
new StaticIpOperator().deleteIpChange(self.getUuid());
Expand Down Expand Up @@ -8347,6 +8352,7 @@ protected void resumeVm(final Message msg, Completion completion) {
@Override
public void handle(Map Data) {
self = changeVmStateInDb(VmInstanceStateEvent.running);
syncVmDevicesAddressInfo(self.getUuid());
completion.success();
}
}).error(new FlowErrorHandler(completion) {
Expand Down Expand Up @@ -8467,6 +8473,28 @@ public String getName() {
});
}

private void syncVmDevicesAddressInfo(String vmUuid) {
if (self.getHostUuid() == null) {
return;
}
SyncVmDeviceInfoMsg msg = new SyncVmDeviceInfoMsg();
msg.setVmInstanceUuid(vmUuid);
msg.setHostUuid(self.getHostUuid());
bus.makeTargetServiceIdByResourceUuid(msg, HostConstant.SERVICE_ID, msg.getHostUuid());
bus.send(msg, new CloudBusCallBack(msg) {
@Override
public void run(MessageReply reply) {
if (!reply.isSuccess()) {
logger.warn(String.format("Failed to sync vm device info for vm[uuid:%s], %s",
vmUuid, reply.getError()));
} else {
logger.debug(String.format("Sent SyncVmDeviceInfoMsg for vm[uuid:%s] on host[uuid:%s]",
vmUuid, self.getHostUuid()));
}
}
});
}

private void deleteVmCdRom(String cdRomUuid, Completion completion) {
boolean exist = dbf.isExist(cdRomUuid, VmCdRomVO.class);
if (!exist) {
Expand Down
2 changes: 1 addition & 1 deletion conf/i18n/globalErrorCodeMapping/global-error-en_US.json
Original file line number Diff line number Diff line change
Expand Up @@ -3374,7 +3374,7 @@
"ORG_ZSTACK_NETWORK_HUAWEI_IMASTER_10019": "delete token of SDN controller [IP:%s] failed because %s",
"ORG_ZSTACK_STORAGE_PRIMARY_BLOCK_10004": "Cannot execute volume mapping to host flow due to invalid volume ID.%s",
"ORG_ZSTACK_NETWORK_SERVICE_PORTFORWARDING_10007": "port forwarding rule [uuid:%s] has not been attached to any virtual machine network interface, cannot detach",
"ORG_ZSTACK_MEVOCO_10088": "cannot take a snapshot for volumes[%s] when volume[uuid: %s] is not attached",
"ORG_ZSTACK_MEVOCO_10088": "cannot create snapshot for volume[uuid:%s] because it is not attached to any VM instance. Please attach the volume to a VM first. Affected volumes: %s",
"ORG_ZSTACK_STORAGE_PRIMARY_BLOCK_10005": "Cannot execute map LUN to host flow due to invalid LUN type: %s",
"ORG_ZSTACK_NETWORK_SERVICE_PORTFORWARDING_10008": "port forwarding rule [uuid:%s] has been associated with vm nic [uuid:%s], cannot be reassigned again",
"ORG_ZSTACK_MEVOCO_10087": "A Running VM[uuid:%s] has no associated Host UUID.",
Expand Down
2 changes: 1 addition & 1 deletion conf/i18n/globalErrorCodeMapping/global-error-zh_CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -3374,7 +3374,7 @@
"ORG_ZSTACK_NETWORK_HUAWEI_IMASTER_10019": "删除 SDN 控制器 [IP:%s] 的令牌失败,因为 %s",
"ORG_ZSTACK_STORAGE_PRIMARY_BLOCK_10004": "无法执行映射LUN到主机流程,无效的LUN ID",
"ORG_ZSTACK_NETWORK_SERVICE_PORTFORWARDING_10007": "端口转发规则 rule[uuid:%s] 没有绑定到任何 VM 的网卡上,无法解除绑定",
"ORG_ZSTACK_MEVOCO_10088": "无法为挂载状态以外的卷[%s]创建快照",
"ORG_ZSTACK_MEVOCO_10088": "无法为云盘[uuid:%s]创建快照,因为该云盘未挂载到任何云主机。请先将云盘挂载到云主机后再创建快照。相关云盘: %s",
"ORG_ZSTACK_STORAGE_PRIMARY_BLOCK_10005": "无法执行映射LUN到主机流程,无效的LUN类型",
"ORG_ZSTACK_NETWORK_SERVICE_PORTFORWARDING_10008": "端口转发规则[uuid:%s]已绑定到VM网卡[uuid:%s],无法再次绑定",
"ORG_ZSTACK_MEVOCO_10087": "如何一个运行中的VM[uuid:%s]没有宿主机uuid?",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,9 @@ public void setBootMode(String bootMode) {

public long getRootDiskAllocateSize() {
if (rootDiskOffering == null) {
return this.getImageSpec().getInventory().getSize();
long virtualSize = this.getImageSpec().getInventory().getSize();
long actualSize = this.getImageSpec().getInventory().getActualSize();
return Math.max(virtualSize, actualSize);
Comment on lines +850 to +852
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Read the VmInstanceSpec.java file, focusing on the getRootDiskAllocateSize method around line 850
head -860 header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java | tail -100

Repository: MatheMatrix/zstack

Length of output: 3379


🏁 Script executed:

# Find the ImageSpec class definition
fd -n "ImageSpec.java" --type f

Repository: MatheMatrix/zstack

Length of output: 292


🏁 Script executed:

# Search for getInventory() definition and usage
rg -n "getInventory|setInventory" header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java -A 2 -B 2

Repository: MatheMatrix/zstack

Length of output: 1141


🏁 Script executed:

# Check ImageInventory class
fd -n "ImageInventory.java" --type f

Repository: MatheMatrix/zstack

Length of output: 292


🏁 Script executed:

# Find ImageInventory class
rg -l "class ImageInventory" --type java

Repository: MatheMatrix/zstack

Length of output: 179


🏁 Script executed:

# Check the ImageInventory class for getActualSize method
rg -A 5 "getActualSize" --type java | head -50

Repository: MatheMatrix/zstack

Length of output: 5300


🏁 Script executed:

# Look for where getRootDiskAllocateSize is called
rg -B 3 -A 3 "getRootDiskAllocateSize\(\)" --type java | head -80

Repository: MatheMatrix/zstack

Length of output: 4161


🏁 Script executed:

# Check if ImageInventory can be null in ImageSpec
rg -B 5 -A 5 "private ImageInventory inventory" header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java

Repository: MatheMatrix/zstack

Length of output: 457


🏁 Script executed:

# Check the ImageInventory class definition to see getActualSize method
head -150 header/src/main/java/org/zstack/header/image/ImageInventory.java | tail -100

Repository: MatheMatrix/zstack

Length of output: 3377


🏁 Script executed:

# Look for where ImageSpec is instantiated and how inventory is set
rg "new ImageSpec\(\)" --type java -A 10 | head -100

Repository: MatheMatrix/zstack

Length of output: 13363


🏁 Script executed:

# Check if there's any null check pattern used before calling getInventory in the codebase
rg "getInventory\(\)" --type java -B 2 -A 2 | head -150

Repository: MatheMatrix/zstack

Length of output: 12197


🏁 Script executed:

# Look for test cases that use getRootDiskAllocateSize to see expected behavior
rg "getRootDiskAllocateSize" --type java -B 5 -A 5

Repository: MatheMatrix/zstack

Length of output: 6207


🏁 Script executed:

# Check if there are any unit tests for getRootDiskAllocateSize
find . -name "*Test.java" -type f -exec rg -l "getRootDiskAllocateSize" {} \;

Repository: MatheMatrix/zstack

Length of output: 44


🏁 Script executed:

# Look at the full initialization context of VmInstanceSpec.imageSpec
sed -n '320,330p' header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java

Repository: MatheMatrix/zstack

Length of output: 534


🏁 Script executed:

# Check if ImageInventory fields can be null - look at the type of actualSize field
sed -n '1,200p' header/src/main/java/org/zstack/header/image/ImageInventory.java | grep -A 2 "private.*actualSize"

Repository: MatheMatrix/zstack

Length of output: 141


🏁 Script executed:

# Check the return type of getSize() in ImageInventory
rg "public.*getSize\(\)" header/src/main/java/org/zstack/header/image/ImageInventory.java -A 1

Repository: MatheMatrix/zstack

Length of output: 110


🏁 Script executed:

# Check if there are any defensive null checks before calling getActualSize on actualSize fields
rg "actualSize.*null|null.*actualSize" --type java | head -30

Repository: MatheMatrix/zstack

Length of output: 3319


🏁 Script executed:

# Verify Math.max behavior with auto-unboxing of null Long
cat > /tmp/test_autobox.java << 'EOF'
public class Test {
    public static void main(String[] args) {
        Long actualSize = null;
        long virtualSize = 100L;
        // This will throw NPE on auto-unboxing
        long result = Math.max(virtualSize, actualSize);
    }
}
EOF
javac /tmp/test_autobox.java 2>&1 | head -20

Repository: MatheMatrix/zstack

Length of output: 106


存在多处空指针异常风险

第 850-852 行代码存在以下空指针异常隐患:

  1. getInventory() 返回 null 时,调用 getSize()getActualSize() 会抛出 NullPointerException
  2. ImageInventory.getActualSize() 返回 Long 类型(可为 null),在自动拆箱到基本类型 long 时若为 null 同样会触发 NullPointerException

建议添加完整的空值检查,同时考虑 actualSize 为 null 的情况:

🛡️ 建议的修复方案
 public long getRootDiskAllocateSize() {
     if (rootDiskOffering == null) {
+        ImageInventory inventory = this.getImageSpec().getInventory();
+        if (inventory == null) {
+            return 0;
+        }
-        long virtualSize = this.getImageSpec().getInventory().getSize();
-        long actualSize = this.getImageSpec().getInventory().getActualSize();
+        long virtualSize = inventory.getSize();
+        Long actualSizeWrapper = inventory.getActualSize();
+        long actualSize = actualSizeWrapper != null ? actualSizeWrapper : virtualSize;
         return Math.max(virtualSize, actualSize);
     }
     return rootDiskOffering.getDiskSize();
 }
🤖 Prompt for AI Agents
In `@header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java` around lines
850 - 852, The code in VmInstanceSpec that computes virtualSize/actualSize using
getImageSpec().getInventory().getSize() and getActualSize() can NPE when
getInventory() is null or when getActualSize() returns a nullable Long
(auto-unboxing); update the logic in the method (where virtualSize/actualSize
are computed) to null-check getImageSpec() and getInventory() before accessing
size, treat a null inventory as size 0, read actualSize as a Long and fall back
to 0L if it is null (avoid auto-unboxing), then compute Math.max on the safe
long values so no NPE occurs (references: VmInstanceSpec, getImageSpec(),
getInventory(), ImageInventory.getSize(), ImageInventory.getActualSize()).

}
return rootDiskOffering.getDiskSize();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ public enum VmInstanceState {
new Transaction(VmInstanceStateEvent.destroyed, VmInstanceState.Destroyed),
new Transaction(VmInstanceStateEvent.destroying, VmInstanceState.Destroying),
new Transaction(VmInstanceStateEvent.running, VmInstanceState.Running),
new Transaction(VmInstanceStateEvent.stopped, VmInstanceState.Stopped),
new Transaction(VmInstanceStateEvent.expunging, VmInstanceState.Expunging)
);
Destroyed.transactions(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,10 @@ public List<ActiveVolumeClient> getActiveClients(String installPath, String prot
if (VolumeProtocol.CBD.toString().equals(protocol)) {
GetVolumeClientsCmd cmd = new GetVolumeClientsCmd();
cmd.setPath(installPath);
GetVolumeClientsRsp rsp = syncHttpCall(GET_VOLUME_CLIENTS_PATH, cmd, GetVolumeClientsRsp.class);
GetVolumeClientsRsp rsp = new HttpCaller<>(GET_VOLUME_CLIENTS_PATH, cmd, GetVolumeClientsRsp.class,
null, TimeUnit.SECONDS, 30, true)
.setTryNext(true)
.syncCall();
List<ActiveVolumeClient> clients = new ArrayList<>();

if (!rsp.isSuccess()) {
Expand Down Expand Up @@ -1411,6 +1414,11 @@ public class HttpCaller<T extends AgentResponse> {

private boolean tryNext = false;

HttpCaller<T> setTryNext(boolean tryNext) {
this.tryNext = tryNext;
return this;
}

public HttpCaller(String path, AgentCommand cmd, Class<T> retClass, ReturnValueCompletion<T> callback) {
this(path, cmd, retClass, callback, null, 0, false);
}
Expand Down