Martin Peřina has posted comments on this change.

Change subject: backend: getLdapServers() use getCanonicalHostName
......................................................................


Patch Set 3:

(3 comments)

Maybe we should also test validness of ldap servers returned from DNS and not 
only those entered manually by -ldapServers.
What do you think?

....................................................
File 
backend/manager/tools/src/main/java/org/ovirt/engine/core/domains/ManageDomains.java
Line 483:             }
Line 484:             return result;
Line 485:         }
Line 486: 
Line 487:         for (String host: argValue.split(DOMAIN_SEPERATOR)) {
Please, don't execute split method twice, convert entered LDAP server to 
List<String> before for cycle and use it in for cycle.
Line 488:             try {
Line 489:                 InetAddress.getByName(host).getCanonicalHostName();
Line 490:             } catch (UnknownHostException e) {
Line 491:                 String warnMessage = "Warning: Cannot resolve FQDN: " 
+ e.getMessage();


Line 485:         }
Line 486: 
Line 487:         for (String host: argValue.split(DOMAIN_SEPERATOR)) {
Line 488:             try {
Line 489:                 InetAddress.getByName(host).getCanonicalHostName();
Maybe I'm paranoid, but if ldap servers are entered as FQDNs, multiple IP 
addresses could be returned, so getAllByName would be better to test if all IP 
address are valid
Line 490:             } catch (UnknownHostException e) {
Line 491:                 String warnMessage = "Warning: Cannot resolve FQDN: " 
+ e.getMessage();
Line 492:                 log.warn(warnMessage);
Line 493:                 System.out.println(warnMessage);


Line 492:                 log.warn(warnMessage);
Line 493:                 System.out.println(warnMessage);
Line 494:             }
Line 495:         }
Line 496:         return new 
ArrayList<String>(Arrays.asList(argValue.split(DOMAIN_SEPERATOR)));
Please don't create new list instance twice: Arrays.asList creates new List 
instance, there's no need to pass it to another ArrayList constructor.
Line 497:     }
Line 498: 
Line 499:     public void addDomain(CLIParser parser) throws 
ManageDomainsResult {
Line 500:         String authMode = LdapAuthModeEnum.GSSAPI.name();


-- 
To view, visit http://gerrit.ovirt.org/19547
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibae81ccd6b2a402c856934839f1dbdad316bbc3d
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsl...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com>
Gerrit-Reviewer: Eli Mesika <elimes...@gmail.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Martin Peřina <mper...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to