Copilot commented on code in PR #12465:
URL: https://github.com/apache/cloudstack/pull/12465#discussion_r2704662833
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1873,6 +1882,31 @@ public Answer createSnapshot(final CreateObjectCommand
cmd) {
}
}
+ private long getRdbSnapshotSize(String poolPath, String diskName, String
snapshotName, String rbdMonitor, String authUser, String authSecret) {
+ logger.debug("Get RBD snapshot size for {}/{}@{}", poolPath, diskName,
snapshotName);
+ //cmd: rbd du <pool>/<disk-name>@<snapshot-name> --format json
--mon-host <monitor-host> --id <user> --key <key> 2>/dev/null
+ String snapshotDetailsInJson =
Script.runSimpleBashScript(String.format("rbd du %s/%s@%s --format json
--mon-host %s --id %s --key %s 2>/dev/null", poolPath, diskName, snapshotName,
rbdMonitor, authUser, authSecret));
+ if (StringUtils.isNotBlank(snapshotDetailsInJson)) {
+ ObjectMapper mapper = new ObjectMapper();
+ try {
+ JsonNode root = mapper.readTree(snapshotDetailsInJson);
+ for (JsonNode image : root.path("images")) {
+ if (snapshotName.equals(image.path("snapshot").asText())) {
+ long usedSizeInBytes =
image.path("used_size").asLong();
+ if (usedSizeInBytes > 0) {
+ logger.debug("RBD snapshot {}/{}@{} used size in
bytes: {}", poolPath, diskName, snapshotName, usedSizeInBytes);
+ return usedSizeInBytes;
+ }
+ }
+ }
+ } catch (JsonProcessingException e) {
+ logger.error("Unable to get the RBD snapshot size", e);
+ }
Review Comment:
When the rbd command fails or returns null/empty output, the method silently
returns 0 without logging any details about the command execution failure. This
makes it difficult to distinguish between a genuine 0-size snapshot and a
failed command. Consider adding a debug or warning log when
snapshotDetailsInJson is blank to help troubleshoot issues.
```suggestion
}
} else {
logger.warn("Failed to get RBD snapshot size for {}/{}@{}: 'rbd
du' returned no output or an empty result", poolPath, diskName, snapshotName);
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -43,6 +43,10 @@
import javax.naming.ConfigurationException;
+import com.ceph.rbd.jna.RbdImageInfo;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.cloudstack.agent.directdownload.DirectDownloadAnswer;
import org.apache.cloudstack.agent.directdownload.DirectDownloadCommand;
Review Comment:
The import 'com.ceph.rbd.jna.RbdImageInfo' is added but never used in the
code. Remove this unused import to keep the code clean.
```suggestion
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.cloudstack.agent.directdownload.DirectDownloadAnswer;
import org.apache.cloudstack.agent.directdownload.DirectDownloadCommand;
import org.apache.cloudstack.agent.directdownload.DirectDownloadCommand;
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1873,6 +1882,31 @@ public Answer createSnapshot(final CreateObjectCommand
cmd) {
}
}
+ private long getRdbSnapshotSize(String poolPath, String diskName, String
snapshotName, String rbdMonitor, String authUser, String authSecret) {
Review Comment:
Method name has a typo: 'getRdbSnapshotSize' should be 'getRbdSnapshotSize'
(RBD stands for RADOS Block Device). This inconsistency makes the code harder
to maintain and understand.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1873,6 +1882,31 @@ public Answer createSnapshot(final CreateObjectCommand
cmd) {
}
}
+ private long getRdbSnapshotSize(String poolPath, String diskName, String
snapshotName, String rbdMonitor, String authUser, String authSecret) {
+ logger.debug("Get RBD snapshot size for {}/{}@{}", poolPath, diskName,
snapshotName);
+ //cmd: rbd du <pool>/<disk-name>@<snapshot-name> --format json
--mon-host <monitor-host> --id <user> --key <key> 2>/dev/null
+ String snapshotDetailsInJson =
Script.runSimpleBashScript(String.format("rbd du %s/%s@%s --format json
--mon-host %s --id %s --key %s 2>/dev/null", poolPath, diskName, snapshotName,
rbdMonitor, authUser, authSecret));
Review Comment:
The command construction using String.format with user-controlled parameters
(poolPath, diskName, snapshotName, rbdMonitor, authUser, authSecret) is
vulnerable to command injection. If any of these parameters contain shell
metacharacters like semicolons, pipes, backticks, or dollar signs, they could
be used to execute arbitrary commands. Consider using a more secure approach
such as:
1. Using Script.runSimpleBashScriptWithParameters or similar method that
properly escapes arguments
2. Validating and sanitizing input parameters before using them in shell
commands
3. Using the Java RBD bindings (like the existing Rbd/RbdImage classes)
instead of shell commands
```suggestion
String snapshotDetailsInJson =
Script.runSimpleBashScriptWithParameters(
"rbd du %s/%s@%s --format json --mon-host %s --id %s --key
%s 2>/dev/null",
poolPath, diskName, snapshotName, rbdMonitor, authUser,
authSecret);
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1873,6 +1882,31 @@ public Answer createSnapshot(final CreateObjectCommand
cmd) {
}
}
+ private long getRdbSnapshotSize(String poolPath, String diskName, String
snapshotName, String rbdMonitor, String authUser, String authSecret) {
+ logger.debug("Get RBD snapshot size for {}/{}@{}", poolPath, diskName,
snapshotName);
+ //cmd: rbd du <pool>/<disk-name>@<snapshot-name> --format json
--mon-host <monitor-host> --id <user> --key <key> 2>/dev/null
+ String snapshotDetailsInJson =
Script.runSimpleBashScript(String.format("rbd du %s/%s@%s --format json
--mon-host %s --id %s --key %s 2>/dev/null", poolPath, diskName, snapshotName,
rbdMonitor, authUser, authSecret));
+ if (StringUtils.isNotBlank(snapshotDetailsInJson)) {
+ ObjectMapper mapper = new ObjectMapper();
+ try {
+ JsonNode root = mapper.readTree(snapshotDetailsInJson);
+ for (JsonNode image : root.path("images")) {
+ if (snapshotName.equals(image.path("snapshot").asText())) {
+ long usedSizeInBytes =
image.path("used_size").asLong();
+ if (usedSizeInBytes > 0) {
+ logger.debug("RBD snapshot {}/{}@{} used size in
bytes: {}", poolPath, diskName, snapshotName, usedSizeInBytes);
+ return usedSizeInBytes;
+ }
+ }
+ }
+ } catch (JsonProcessingException e) {
+ logger.error("Unable to get the RBD snapshot size", e);
Review Comment:
The error handling silently logs an error and returns 0 when JSON parsing
fails. This makes debugging difficult and could mask underlying issues.
Consider logging the actual JSON content that failed to parse (taking care not
to log sensitive credentials) to help with troubleshooting.
```suggestion
logger.error("Unable to get the RBD snapshot size. RBD JSON
output: {}", snapshotDetailsInJson, e);
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1873,6 +1882,31 @@ public Answer createSnapshot(final CreateObjectCommand
cmd) {
}
}
+ private long getRdbSnapshotSize(String poolPath, String diskName, String
snapshotName, String rbdMonitor, String authUser, String authSecret) {
+ logger.debug("Get RBD snapshot size for {}/{}@{}", poolPath, diskName,
snapshotName);
+ //cmd: rbd du <pool>/<disk-name>@<snapshot-name> --format json
--mon-host <monitor-host> --id <user> --key <key> 2>/dev/null
+ String snapshotDetailsInJson =
Script.runSimpleBashScript(String.format("rbd du %s/%s@%s --format json
--mon-host %s --id %s --key %s 2>/dev/null", poolPath, diskName, snapshotName,
rbdMonitor, authUser, authSecret));
+ if (StringUtils.isNotBlank(snapshotDetailsInJson)) {
+ ObjectMapper mapper = new ObjectMapper();
+ try {
+ JsonNode root = mapper.readTree(snapshotDetailsInJson);
+ for (JsonNode image : root.path("images")) {
+ if (snapshotName.equals(image.path("snapshot").asText())) {
+ long usedSizeInBytes =
image.path("used_size").asLong();
+ if (usedSizeInBytes > 0) {
+ logger.debug("RBD snapshot {}/{}@{} used size in
bytes: {}", poolPath, diskName, snapshotName, usedSizeInBytes);
+ return usedSizeInBytes;
+ }
+ }
+ }
+ } catch (JsonProcessingException e) {
+ logger.error("Unable to get the RBD snapshot size", e);
+ }
+ }
+
+ return 0;
+ }
Review Comment:
The method uses shell commands to get RBD snapshot size, while the
createSnapshot method already uses Java RBD bindings (Rbd, RbdImage). Consider
using RbdImage.stat() or similar Java API methods to get snapshot size
information instead of relying on shell commands. This would be more consistent
with the existing code, avoid command injection risks, and be more maintainable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
