Copilot commented on code in PR #13286:
URL: https://github.com/apache/cloudstack/pull/13286#discussion_r3324537850
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStorageVolumeXMLParser.java:
##########
@@ -35,6 +35,26 @@
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();
+ Element backingStore =
(Element)rootElement.getElementsByTagName("backingStore").item(0);
+ if (backingStore != null) {
+ String[] paths = getTagValue("path", backingStore).split("/");
+ return paths[paths.length-1];
+ }
Review Comment:
`getBackingFileNameIfExists` assumes `<backingStore>` always contains a
`<path>` element, but libvirt can emit an empty `<backingStore/>` to indicate
the end of the chain. In that case `getTagValue("path", backingStore)` will
throw a NPE and break volume inspection. Parse the `<path>` element defensively
and return null when it is missing/blank.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -507,6 +507,12 @@ public LibvirtStoragePoolDef getStoragePoolDef(Connect
conn, StoragePool pool) t
return parser.parseStoragePoolXML(poolDefXML);
}
+ public String getBackingFileOfVolumeIfExists(StorageVol vol) throws
LibvirtException {
+ String volDefXML = vol.getXMLDesc(0);
+ LibvirtStorageVolumeXMLParser parser = new
LibvirtStorageVolumeXMLParser();
+ return parser.getBackingFileNameIfExists(volDefXML);
+ }
Review Comment:
This helper appears to be used only within `LibvirtStorageAdaptor` (no other
references found in the codebase). Keeping it `public` expands the class
surface area unnecessarily; it can be `private` to reduce accidental external
coupling.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -665,8 +681,9 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid,
KVMStoragePool pool) {
StorageVol vol = getVolume(libvirtPool.getPool(), volumeUuid);
KVMPhysicalDisk disk;
LibvirtStorageVolumeDef voldef =
getStorageVolumeDef(libvirtPool.getPool().getConnect(), vol);
+ Long allSizes = getBackingFileSizes(libvirtPool.getPool(), vol);
disk = new KVMPhysicalDisk(vol.getPath(), vol.getName(), pool);
- disk.setSize(vol.getInfo().allocation);
+ disk.setSize(allSizes);
Review Comment:
This change alters how `KVMPhysicalDisk.size` is computed (now including
backing files), but there are no unit tests validating the new behavior (e.g.,
a backing chain sums allocations correctly and missing/empty backingStore is
handled). Given there are existing unit tests for `LibvirtStorageAdaptor`,
please add coverage for the new sizing logic to prevent regressions.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -657,6 +663,16 @@ public KVMStoragePool getStoragePool(String uuid, boolean
refreshInfo) {
}
}
+ public 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` calls `getVolume(...)` for the backing file; if the
backing file is not managed as a libvirt volume in this pool (or cannot be
found), `getVolume` throws `CloudRuntimeException`, which will currently abort
`getPhysicalDisk` entirely. This makes volume inspection brittle; it should
ignore missing/unresolvable backing files (or at least stop recursion) instead
of failing the whole operation.
--
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]
