mooli tayer has posted comments on this change.
Change subject: core: Introduce new authentication interfaces
......................................................................
Patch Set 8:
(11 comments)
Learned a lot form this patch. Thanks.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/DirectoryManager.java
Line 17: public static DirectoryManager getInstance() {
Line 18: if (instance == null) {
Line 19: synchronized (DirectoryManager.class) {
Line 20: if (instance == null) {
Line 21: instance = new DirectoryManager();
is there a reason not to do:
private static DirectoryManager instance = new DirectoryManager();
to avoid synch?
Line 22: }
Line 23: }
Line 24: }
Line 25: return instance;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Manager.java
Line 20: import org.slf4j.Logger;
Line 21: import org.slf4j.LoggerFactory;
Line 22:
Line 23: /**
Line 24: * This class provides some methods useful for all the managers that
need use a configuration file.
use of a
Line 25: *
Line 26: * @param <O> the type of the managed object
Line 27: */
Line 28: public abstract class Manager<O extends Managed> {
Line 24: * This class provides some methods useful for all the managers that
need use a configuration file.
Line 25: *
Line 26: * @param <O> the type of the managed object
Line 27: */
Line 28: public abstract class Manager<O extends Managed> {
I understand modules are only loaded once?
If there is a running system and someone replaces a jar it will never be
refreshed?
Might be less suitable for the plugin use case.
Also from a plugin point of view it sits better to
1.) load a plugin at one point (uncluding all factories/service it exports)
2.) at all points in the future use services from it
as appose to createObject(file) - load a module for one class
Line 29: // The log:
Line 30: private static final Logger log =
LoggerFactory.getLogger(Manager.class);
Line 31:
Line 32: // Names of the parameters:
Line 53: this.factoryInterface = factoryInterface;
Line 54:
Line 55: // Create the indexes for factories and objects:
Line 56: factoriesByType = new ConcurrentHashMap<String, Factory<O>>();
Line 57: factoriesByClass = new ConcurrentHashMap<Class<?>,
Factory<O>>();
I expect these maps to have many gets and few put calls. consider replacing
with copy on write
Line 58: objectsByName = new ConcurrentHashMap<String, O>();
Line 59:
Line 60: // Create the set of already loaded modules:
Line 61: loadedModules = new HashSet<String>();
Line 73: }
Line 74:
Line 75: /**
Line 76: * Finds a factory using the given configuration. The factory will
be located using the {@code type}, and
Line 77: * {@code module} properties. If the configuration file contains
contains the {@code module} parameter the manager
contains * 2
Line 78: * will try to load that module. Then the factories including in
the SPI configuration of the module will be loaded
Line 79: * and registered with their types. For example, if the
configuration file contains the following:
Line 80: *
Line 81: * <pre>
Line 89: * @param file the file containing the configuration for the
instance, used only to generate useful log messages
Line 90: * @param config the configuration already loaded from the
properties file
Line 91: * @return a reference to the factory or {@code null} if the
factory can't be found for any reason
Line 92: */
Line 93: protected Factory<O> findFactory(File file, Configuration config) {
only one factory per type?
Line 94: // This will be the result:
Line 95: Factory<O> factory = null;
Line 96:
Line 97: // If a module has been specified then load all the factories
inside:
Line 190: for (File file : files) {
Line 191: if (file.getName().endsWith(".conf")) {
Line 192: O object = createObject(file);
Line 193: if (object == null) {
Line 194: log.info(
if object is null we will get both an info from here and a warn from
createObject that looks kind of simillar
Line 195: "The object configured in file \"{}\" hasn't
been created.",
Line 196: file.getAbsolutePath()
Line 197: );
Line 198: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/PasswordAuthenticator.java
Line 1: package org.ovirt.engine.core.authentication;
Line 2:
Line 3: /**
Line 4: * A password authenticator checks an user name and a password and
returns {@code true} if the password is correct.
checks an user => checks a user.
returns {@code true} if => returns {@code true} iff
(short for if and only if)?
Line 5: */
Line 6: public interface PasswordAuthenticator extends Authenticator {
Line 7: /**
Line 8: * Check the given name and password and return {@code true} if
they are correct.
Line 7: /**
Line 8: * Check the given name and password and return {@code true} if
they are correct.
Line 9: *
Line 10: * @param name the name of user being authenticated
Line 11: * @param password the password of the user as a array of
characters so that it can be cleaned once the method
as an array
Line 12: * returns
Line 13: * @return {@code true} if the password is correct or {@code
false} if it isn't correct or if anything fails while
Line 14: * checking
Line 15: */
....................................................
File
frontend/webadmin/modules/gwt-extension/src/main/java/org/ovirt/engine/core/authentication/DirectoryUser_CustomFieldSerializer.java
Line 39: user.setLastName(reader.readString());
Line 40:
user.setStatus(DirectoryEntryStatus.forValue(reader.readInt()));
Line 41: }
Line 42:
Line 43: }
die gwt die!
....................................................
File
frontend/webadmin/modules/gwt-extension/src/main/java/org/ovirt/engine/ui/uioverrides/org/ovirt/engine/core/authentication/DirectoryManager.java
Line 16: /**
Line 17: * Get an instance of the directory manager.
Line 18: */
Line 19: public static DirectoryManager getInstance() {
Line 20: if (instance == null) {
No need for synchronization? can this be moved to top, i.e
private static DirectoryManager instance = new DirectoryManager();
to avoid need for synch?
Line 21: instance = new DirectoryManager();
Line 22: }
Line 23: return instance;
Line 24: }
--
To view, visit http://gerrit.ovirt.org/15596
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If84a0c9d6553d81cdbbe224972696f169cca90d4
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Martin Peřina <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: mooli tayer <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
