DaanHoogland commented on code in PR #6508:
URL: https://github.com/apache/cloudstack/pull/6508#discussion_r908159243
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1169,26 +1177,37 @@ private String getDataStoreUrlFromStore(DataStoreTO
store) {
return store.getUrl();
}
- protected synchronized String attachOrDetachDevice(final Connect conn,
final boolean attach, final String vmName, final String xml) throws
LibvirtException, InternalErrorException {
+ protected synchronized String attachOrDetachDevice(final Connect conn,
final boolean attach, final String vmName, final String xml)
+ throws LibvirtException, InternalErrorException {
Domain dm = null;
try {
dm = conn.domainLookupByName(vmName);
if (attach) {
s_logger.debug("Attaching device: " + xml);
dm.attachDevice(xml);
- } else {
- s_logger.debug("Detaching device: " + xml);
- dm.detachDevice(xml);
- LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
- parser.parseDomainXML(dm.getXMLDesc(0));
- List<DiskDef> disks = parser.getDisks();
- for (DiskDef diskDef : disks) {
- if (StringUtils.contains(xml, diskDef.getDiskPath())) {
- throw new InternalErrorException("Could not detach
volume. Probably the VM is in boot state at the moment");
- }
+ return null;
Review Comment:
@RodrigoDLopez returning null is a bad practice. it can lead to NPEs in
unexpected places. Better to throw a specific exception and hendle it where
appropriate.
You are right that this pattern has been implemented a lot of times but we
shouldn´t add to this.
--
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]
