[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4565: Fix RBD primary storage host port null error

[GitBox <gi...@apache.org>]

GabrielBrascher commented on a change in pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#discussion_r552692404



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
##########
@@ -84,7 +84,11 @@ public boolean parseDomainXML(String domXML) {
                     String authUserName = getAttrValue("auth", "username", disk);
                     String poolUuid = getAttrValue("secret", "uuid", disk);
                     String host = getAttrValue("host", "name", disk);
-                    int port = Integer.parseInt(getAttrValue("host", "port", disk));
+                    int port = 0;
+                    String _xmlPort = getAttrValue("host", "port", disk);
+                    if ( ! _xmlPort.isEmpty()) {

Review comment:
       1. @div8cn can you please change the variable name from `_xmlPort` to `xmlPort`?
   
   According to [Java conventions](https://docs.oracle.com/javase/tutorial/java/nutsandbolts/variables.html), 
   
   > while it's technically legal to begin your variable's name with "_", this practice is discouraged
   
   2. Regarding the String validation. Is it possible to have a `tagNode` of length 0?
   
   `getAttrValue()` has this piece of code:
   ```
           NodeList tagNode = eElement.getElementsByTagName(tag);
           if (tagNode.getLength() == 0) {
               return null;
           }
   ```
   
   I would recommend, just for safety, to use something as `org.apache.commons.lang.StringUtils`; it provides String validations such as `StringUtils.isNotBlank(Str)`.
   
   `isNotBlank` returns true only if not null, not empty, and not blank.
   
   ```
   String xmlPort = getAttrValue("host", "port", disk);
   if (StringUtils.isNotBlank(xmlPort)) {
        ....
   }
   ```
   
   




----------------------------------------------------------------
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.

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