Alon Bar-Lev has posted comments on this change.

Change subject: aaa: Engine should start even if exception occured in load of 
legacy provider
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.ovirt.org/#/c/29879/5/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/utils/dns/DnsSRVLocator.java
File 
backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/utils/dns/DnsSRVLocator.java:

Line 399:         }
Line 400:         return results;
Line 401:     }
Line 402: 
Line 403:     public URI constructURI(String protocol, String address, String 
defaultLdapSeverPort) throws URISyntaxException {
> sounds good to me.
if you move anything to uutils you need to properly/generic write it :)

searching for :// is not something that is valid without parsing the entire url.

from a source I wrote:

 public class XURLParts implements Cloneable {

        private static final Pattern URL_PATTERN = 
Pattern.compile("(?<protocol>[^:]+)://(?<host>[^:/]+)?(:(?<port>\\d+))?(?<path>.*)");

        private String protocol;
        private String host;
        private int port;
        private String path;

        public static XURLParts parseURL(String url) {
                XURLParts ret = null;
                Matcher m = URL_PATTERN.matcher(url);
                if (!m.matches()) {
                        throw new 
IllegalArgumentException(String.format("Failed to parse URL '%s'", url));
                }

                ret = new XURLParts();
                ret.protocol = m.group("protocol");
                ret.host = m.group("host");
                ret.port = m.group("port") == null ? 0 : 
Integer.parseInt(m.group("port"));
                ret.path = m.group("path");
                return ret;
        }

        @Override
        public String toString() {
                return String.format(
                        "%s://%s%s%s",
                        protocol,
                        host == null ? "" : host,
                        port == 0 ? "" : String.format(":%s", port),
                        path
                );
        }

 <snip>

I suggest to keep this here for now.
Line 404:         address = address.indexOf("://") != -1 ? 
address.substring(address.indexOf("://") + 3) : address;
Line 405:         String[] parts = address.split("\\:");
Line 406:         String hostname = address;
Line 407:         String port = defaultLdapSeverPort;


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ba595253c2569cf581e690b68348e6ae1c804b1
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
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