GutoVeronezi commented on code in PR #7140:
URL: https://github.com/apache/cloudstack/pull/7140#discussion_r1100684804
##########
plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/XenServerConnectionPool.java:
##########
@@ -210,7 +210,7 @@ public Connection connect(String hostUuid, String poolUuid,
String ipAddress,
throw e;
} catch (Exception e) {
if (s_logger.isDebugEnabled()) {
- s_logger.debug("connect through IP(" + mConn.getIp() +
" for pool(" + poolUuid + ") is broken due to " + e.toString());
+ s_logger.debug("connect through IP(" + (mConn != null
? mConn.getIp() : null) + ") for pool(" + poolUuid + ") is broken due to " +
e.toString());
Review Comment:
@JoaoJandre, the same here about extracting to a variable.
##########
server/src/main/java/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java:
##########
@@ -255,7 +255,7 @@ private boolean isOutOfBandManagementEnabledForHost(Long
hostId) {
Host host = hostDao.findById(hostId);
if (host == null || host.getResourceState() == ResourceState.Degraded)
{
- LOG.debug(String.format("Host [id=%s, state=] was removed or
placed in Degraded state by the Admin.", hostId, host.getResourceState()));
+ LOG.debug(String.format("Host [id=%s, state=%s] was removed or
placed in Degraded state by the Admin.", hostId, host != null ?
host.getResourceState() : null));
Review Comment:
@JoaoJandre, the same here about extracting to a variable.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -1427,7 +1427,8 @@ 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: %s", srcFile != null ? srcFile.getFileName() : null,
+ destFile != null ? destFile.getFileName() : null,
e.getMessage()));
Review Comment:
@JoaoJandre, can we extract the ternaries to variables to improve the
reading?
```suggestion
String srcFileName = srcFile != null ?
srcFile.getFileName() : null;
String destFileName = destFile != null ?
destFile.getFileName() : null;
s_logger.error(String.format("Failed to convert from %s to
%s the error was: %s", srcFileName, destFileName, e.getMessage()));
```
##########
server/src/main/java/com/cloud/servlet/ConsoleProxyServlet.java:
##########
@@ -154,15 +154,15 @@ protected void doGet(HttpServletRequest req,
HttpServletResponse resp) {
String cmd = req.getParameter("cmd");
if (cmd == null || !isValidCmd(cmd)) {
- s_logger.debug("invalid console servlet command: " + cmd);
+ s_logger.debug("invalid console servlet command.");
Review Comment:
Is this right to just remove the variable from the log instead of handling
it?
##########
server/src/main/java/com/cloud/servlet/ConsoleProxyServlet.java:
##########
@@ -154,15 +154,15 @@ protected void doGet(HttpServletRequest req,
HttpServletResponse resp) {
String cmd = req.getParameter("cmd");
if (cmd == null || !isValidCmd(cmd)) {
- s_logger.debug("invalid console servlet command: " + cmd);
+ s_logger.debug("invalid console servlet command.");
sendResponse(resp, "");
return;
}
String vmIdString = req.getParameter("vm");
VirtualMachine vm = _entityMgr.findByUuid(VirtualMachine.class,
vmIdString);
if (vm == null) {
- s_logger.info("invalid console servlet command parameter: " +
vmIdString);
+ s_logger.info("invalid console servlet command vm parameter.");
Review Comment:
Is this right to just remove the variable from the log instead of handling
it?
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -7298,7 +7298,7 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
_securityGroupMgr.addInstanceToGroups(vm.getId(),
securityGroupIdList);
- s_logger.debug("AssignVM: Basic zone, adding security groups no "
+ securityGroupIdList.size() + " to " + vm.getInstanceName());
+ s_logger.debug("AssignVM: Basic zone, adding security groups no "
+ (securityGroupIdList != null ? securityGroupIdList.size() : 0) + " to " +
vm.getInstanceName());
Review Comment:
@JoaoJandre, the same here about extracting to a variable.
##########
server/src/main/java/com/cloud/servlet/ConsoleProxyServlet.java:
##########
@@ -262,7 +262,7 @@ private void handleAuthRequest(HttpServletRequest req,
HttpServletResponse resp,
String sid = req.getParameter("sid");
if (sid == null || !sid.equals(vm.getVncPassword())) {
- s_logger.warn("sid " + sid + " in url does not match stored sid.");
+ s_logger.warn("sid in url does not match stored sid.");
Review Comment:
Is this right to just remove the variable from the log instead of handling
it?
--
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]
