Copilot commented on code in PR #13286:
URL: https://github.com/apache/cloudstack/pull/13286#discussion_r3526482912
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -657,6 +663,16 @@ public KVMStoragePool getStoragePool(String uuid, boolean
refreshInfo) {
}
}
+ private Long getBackingFileSizes(StoragePool pool, StorageVol vol) throws
LibvirtException {
+ long size = vol.getInfo().allocation;
+ String backingFileOfVolumeIfExists =
getBackingFileOfVolumeIfExists(vol);
+ if (backingFileOfVolumeIfExists != null) {
+ StorageVol backingFile = getVolume(pool,
backingFileOfVolumeIfExists);
+ size += getBackingFileSizes(pool, backingFile);
+ }
+ return size;
+ }
Review Comment:
`getBackingFileSizes` can throw `CloudRuntimeException` if the backing
volume isn't found in the same libvirt storage pool (via `getVolume`). Since
`getPhysicalDisk` only catches `LibvirtException`, this can now fail the whole
disk lookup where it previously succeeded. Also, the unbounded recursion can
cause stack overflows on long backing chains and has no loop detection.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStorageVolumeDef.java:
##########
@@ -83,6 +83,10 @@ public VolumeFormat getFormat() {
return this._volFormat;
}
+ public Long getVolSize() {
+ return _volSize;
+ }
Review Comment:
`getVolSize()` is introduced as a new public API but is not referenced
anywhere in the codebase. If it isn't needed for this change, consider removing
it to avoid expanding the surface area of this class unnecessarily.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStorageVolumeXMLParser.java:
##########
@@ -35,6 +36,35 @@
public class LibvirtStorageVolumeXMLParser {
protected Logger logger = LogManager.getLogger(getClass());
+ public String getBackingFileNameIfExists(String volXML) {
+ try {
+ DocumentBuilder builder =
ParserUtils.getSaferDocumentBuilderFactory().newDocumentBuilder();
+
+ InputSource is = new InputSource();
+ is.setCharacterStream(new StringReader(volXML));
+ Document doc = builder.parse(is);
+
+ Element rootElement = doc.getDocumentElement();
+ NodeList backingStores =
rootElement.getElementsByTagName("backingStore");
+ if (backingStores.getLength() > 0) {
+ Element backingStore = (Element) backingStores.item(0);
+ NodeList pathNodes = backingStore.getElementsByTagName("path");
+ if (pathNodes.getLength() > 0) {
+ String path = pathNodes.item(0).getTextContent();
+ if (StringUtils.isEmpty(path)) {
+ return null;
+ }
+ path = path.trim();
+ int lastSlash = path.lastIndexOf('/');
+ return lastSlash >= 0 ? path.substring(lastSlash + 1) :
path;
Review Comment:
Whitespace-only `<path>` values aren't treated as empty because the
`isEmpty` check happens before `trim()`. That can lead to returning an empty
string and later failing volume lookup with a confusing error. Use `isBlank`
(or re-check after trim) to treat blank/whitespace paths as absent.
--
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]
