[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7140: Fix PR 7131 bugs and vulnerabilities

["DaanHoogland (via GitHub)" <gi...@apache.org>]

DaanHoogland commented on code in PR #7140:
URL: https://github.com/apache/cloudstack/pull/7140#discussion_r1089689949


##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -1427,7 +1427,7 @@ public KVMPhysicalDisk copyPhysicalDisk(KVMPhysicalDisk disk, String name, KVMSt
 
                 r.ioCtxDestroy(io);
             } catch (QemuImgException | LibvirtException e) {
-                s_logger.error("Failed to convert from " + srcFile.getFileName() + " to " + destFile.getFileName() + " the error was: " + e.getMessage());
+                s_logger.error(String.format("Failed to convert from %s to %s the error was: " + e.getMessage(), srcFile != null ? srcFile.getFileName() : null, destFile != null ? destFile.getFileName() : null));

Review Comment:
   ```suggestion
                   s_logger.error(String.format("Failed to convert from %s to %s the error was: %s",
                       srcFile != null ? srcFile.getFileName() : null,
                       destFile != null ? destFile.getFileName() : null,
                       e.getMessage()));
   ```



##########
server/src/main/java/com/cloud/alert/SecondaryStorageVmAlertAdapter.java:
##########
@@ -59,92 +59,100 @@ public void onSSVMAlert(Object sender, SecStorageVmAlertEventArgs args) {
             throw new CloudRuntimeException("Invalid alert arguments, secStorageVm must be set");
         }
 
+        String secStorageVmHostName = "";
+        String secStorageVmPublicIpAddress = "";
+        String secStorageVmPrivateIpAddress = "N/A";
+        Long secStorageVmPodIdToDeployIn = null;
+
+        if (secStorageVm != null) {
+            secStorageVmHostName = secStorageVm.getHostName();
+            secStorageVmPublicIpAddress = secStorageVm.getPublicIpAddress();
+            secStorageVmPrivateIpAddress = secStorageVm.getPrivateIpAddress() == null ? "N/A" : secStorageVm.getPrivateIpAddress();
+            secStorageVmPodIdToDeployIn = secStorageVm.getPodIdToDeployIn();
+        }

Review Comment:
   this is the exact same functionality as https://github.com/apache/cloudstack/pull/7140/files#diff-836e2e853d64b7a2f86c1add5aa76deea112788fe69c19a3c5eb2618d753ff2c lines 63-73. Can we make it a utiity method?
   
   i.e. can we use `VMInstanceVO` instead of `ConsoleProxyVO` and `SecondaryStorageVmVO` and make it a member method of that class.



-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org