Juan Hernandez has uploaded a new change for review. Change subject: tools: Don't use console for debug log ......................................................................
tools: Don't use console for debug log Currently we use a console appender for the debug log level of the engine-config tool, this means that the debug messages are mixed with the regular output of the tool, and that confuses its users, including engine-setup and engine-manage-domains. This change introduces a new Console class for the tools. This class centralizes all input and output to the console and sends all the text to the log as well. The engine-config tool uses this new class for regular input and output. This change also removes the console appender from the log4j configuration and changes the default log level to DEBUG so that debug messages will be sent to the log as they used to be. Change-Id: I556e20f6333e22faeb7d212767ebefb99edce54e Signed-off-by: Juan Hernandez <[email protected]> --- M backend/manager/tools/src/main/conf/engine-config-log4j.xml M backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfig.java M backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigCLIParser.java M backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigLogic.java M backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/ConfigKey.java M backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/CompositePasswordValueHelper.java M backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/PasswordValueHelper.java A backend/manager/tools/src/main/java/org/ovirt/engine/core/tools/Console.java 8 files changed, 199 insertions(+), 103 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/87/12987/1 diff --git a/backend/manager/tools/src/main/conf/engine-config-log4j.xml b/backend/manager/tools/src/main/conf/engine-config-log4j.xml index dff6837..fbb4eae 100644 --- a/backend/manager/tools/src/main/conf/engine-config-log4j.xml +++ b/backend/manager/tools/src/main/conf/engine-config-log4j.xml @@ -1,45 +1,22 @@ -<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE log4j:configuration SYSTEM "log4j.dtd"> +<?xml version="1.0" encoding="UTF-8"?> -<log4j:configuration xmlns:log4j="http://jakarta.apache.org/log4j/"; debug="false"> +<!DOCTYPE log4j:configuration SYSTEM "log4j.dtd"> - <appender name="FILE" class="org.apache.log4j.RollingFileAppender"> - <param name="File" value="/var/log/ovirt-engine/engine-config.log" /> - <param name="Append" value="true" /> - <param name="MaxFileSize" value="1500KB" /> - <param name="MaxBackupIndex" value="1" /> +<log4j:configuration xmlns:log4j="http://jakarta.apache.org/log4j/";> - <layout class="org.apache.log4j.PatternLayout"> - <param name="ConversionPattern" value="%d %-5p [%c] %m%n" /> - <!-- <param name="ConversionPattern" value="%d{ABSOLUTE} %-5p [%c{1}] %m%n" /> --> + <appender name="FILE" class="org.apache.log4j.RollingFileAppender"> + <param name="File" value="/var/log/ovirt-engine/engine-config.log"/> + <param name="Append" value="true"/> + <param name="MaxFileSize" value="1500KB"/> + <param name="MaxBackupIndex" value="1"/> + <layout class="org.apache.log4j.PatternLayout"> + <param name="ConversionPattern" value="%d %-5p [%c] %m%n"/> + </layout> + </appender> - </layout> - </appender> - - - <appender name="CONSOLE" class="org.apache.log4j.ConsoleAppender"> - <!-- errorHandler class="org.jboss.logging.util.OnlyOnceErrorHandler"/> --> - <param name="Target" value="System.out"/> - <layout class="org.apache.log4j.PatternLayout"> - <!-- The default pattern: Date Priority [Category] Message\n --> - </layout> - </appender> - - <logger name="org.ovirt.engine.core.config"> - <level value="INFO"/> - </logger> - - <logger name="org.apache.commons.configuration.ConfigurationUtils"> - <level value="INFO"/> - </logger> - - <logger name="org.ovirt.engine.core.utils.crypt"> - <level value="FATAL"/> - </logger> - - <root> - <priority value="INFO"/> - <appender-ref ref="FILE"/> - <appender-ref ref="CONSOLE"/> - </root> + <root> + <priority value="DEBUG"/> + <appender-ref ref="FILE"/> + </root> </log4j:configuration> diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfig.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfig.java index 69574be..af04e32 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfig.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfig.java @@ -1,17 +1,21 @@ package org.ovirt.engine.core.config; -import org.apache.log4j.Level; import org.apache.log4j.Logger; import org.ovirt.engine.core.config.validation.ConfigActionType; +import org.ovirt.engine.core.tools.Console; /** * The <code>EngineConfig</code> class represents the main class of the EngineConfig tool. */ public class EngineConfig { + // The log: + private static final Logger log = Logger.getLogger(EngineConfig.class); + + // The console: + private static final Console console = Console.getInstance(); public static final String CONFIG_FILE_PATH_PROPERTY = "engine-config.config.file.path"; public static final String DEFAULT_CONFIG_PATH = "/etc/ovirt-engine/engine-config/"; - private static final Logger log = Logger.getLogger(EngineConfig.class); private EngineConfigCLIParser parser; private EngineConfigLogic engineConfigLogic; private static EngineConfig instance = new EngineConfig(); @@ -45,13 +49,12 @@ */ public static void main(String... args) { try { - Logger.getRootLogger().setLevel(Level.DEBUG); getInstance().setParser(new EngineConfigCLIParser()); getInstance().setUpAndExecute(args); } catch (Throwable t) { log.debug("Exiting with error: ", t); - System.err.println(t.getMessage()); + console.writeLine(t.getMessage()); System.exit(1); } } diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigCLIParser.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigCLIParser.java index 21af05f..976e426 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigCLIParser.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigCLIParser.java @@ -11,8 +11,9 @@ * not as a char that is actually part of a key/value. */ public class EngineConfigCLIParser { - + // The log: private static final Logger log = Logger.getLogger(EngineConfigCLIParser.class); + private HashMap<String, String> argsMap = new HashMap<String, String>(); private EngineConfigMap engineConfigMap = new EngineConfigMap(); diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigLogic.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigLogic.java index 7aba34a..a52a790 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigLogic.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigLogic.java @@ -4,11 +4,10 @@ import java.io.File; import java.io.FileReader; import java.io.IOException; -import java.io.InputStreamReader; import java.net.ConnectException; import java.security.InvalidParameterException; import java.sql.SQLException; -import java.text.MessageFormat; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -25,16 +24,20 @@ import org.ovirt.engine.core.config.entity.ConfigKey; import org.ovirt.engine.core.config.entity.ConfigKeyFactory; import org.ovirt.engine.core.config.validation.ConfigActionType; +import org.ovirt.engine.core.tools.Console; /** * The <code>EngineConfigLogic</code> class is responsible for the logic of the EngineConfig tool. */ public class EngineConfigLogic { + // The log: + private static final Logger log = Logger.getLogger(EngineConfigLogic.class); + + // The console: + private static final Console console = Console.getInstance(); private final static String ALTERNATE_KEY = "alternateKey"; - private final static String DEFAULT_LOG4J_CONF_PATH = "/etc/ovirt-engine/engine-config/log4j.xml"; - private final static Logger log = Logger.getLogger(EngineConfigLogic.class); private Configuration appConfig; private HierarchicalConfiguration keysConfig; @@ -181,8 +184,8 @@ log.debug("starting user dialog."); String user = null; while (StringUtils.isBlank(user)) { - System.out.println("Please enter user:"); - user = System.console().readLine(); + console.writeLine("Please enter user: "); + user = console.readLine(); } return user; } @@ -193,17 +196,17 @@ * @return The user's password */ public static String startPasswordDialog(String user) throws IOException { - return startPasswordDialog(user, "Please enter a password"); + return startPasswordDialog(user, "Please enter a password: "); } public static String startPasswordDialog(String user, String msg) throws IOException { log.debug("starting password dialog."); if (user == null) { - System.out.printf(msg); + console.write(msg); } else { - System.out.printf("%s for %s: ",msg , user); + console.writeLine(msg + " for " + user); } - return new String(System.console().readPassword()); + return new String(console.readPassword()); } /** @@ -214,24 +217,25 @@ private void printAllValuesForKey(String key) throws Exception { List<ConfigKey> keysForName = getConfigDAO().getKeysForName(key); if (keysForName.size() == 0) { - log.debug("printKeyValues: failed to fetch key " + key + " value: no such entry with default version."); + log.debug("Failed to fetch value for key \"" + key + "\", no such entry with default version."); throw new RuntimeException("Error fetching " + key + " value: no such entry with default version."); } - StringBuilder buffer = new StringBuilder(); - boolean isPasswordKey = false; for (ConfigKey configKey : keysForName) { - buffer.append(String.format("%s: %s version: %s\n", - key, - configKey.getDisplayValue(), - configKey.getVersion())); - isPasswordKey = isPasswordKey || configKey.isPasswordKey(); - } - buffer.deleteCharAt(buffer.length() - 1); - if (isPasswordKey) { - System.out.print(buffer); - } else { - log.info(buffer); + console.write(key); + console.write(": "); + if (!configKey.isPasswordKey()) { + console.write(configKey.getDisplayValue()); + } + else { + char[] value = configKey.getDisplayValue().toCharArray(); + console.writePassword(value); + Arrays.fill(value, '\0'); + } + console.write(" "); + console.write("version: "); + console.write(configKey.getVersion()); + console.writeLine(); } } @@ -245,9 +249,9 @@ // TODO - move to one statement for all - time permitting; try { printAllValuesForKey(key.getKey()); - } catch (Exception e) { - log.error("Skipping " + key.getKey() + " due to an error: " + e.getMessage()); - log.debug("details:", e); + } + catch (Exception exception) { + log.error("Error while retriving value for key \"" + key.getKey() + "\".", exception); } } } @@ -294,10 +298,12 @@ } private void printKeyInFormat(ConfigKey key) { - log.info(MessageFormat.format("{0}: {1} (Value Type: {2})", - key.getKey(), - key.getDescription(), - key.getType())); + console.writeFormat( + "%s: %s (Value Type: %s)\n", + key.getKey(), + key.getDescription(), + key.getType() + ); } /** @@ -335,11 +341,7 @@ throw new RuntimeException("Error fetching " + key + " value: no such entry with version '" + version + "'."); } - if (configKey.isPasswordKey()) { - System.out.println(configKey.getDisplayValue()); - } else { - log.info(configKey.getDisplayValue()); - } + console.writeLine(configKey.getDisplayValue()); } /** @@ -369,7 +371,7 @@ @Override public boolean handle(ConfigKey key) { if (key.getKey().equals(keyName)) { - log.info(key.getValueHelper().getHelpNote(key)); + console.writeLine(key.getValueHelper().getHelpNote(key)); return false; } return true; @@ -377,8 +379,10 @@ }); if (!foundKey) { - log.error(String.format("Cannot display help for key %1$s. The key does not exist at the configuration file of engine-config", - keyName)); + console.writeFormat( + "Cannot display help for key %1$s. The key does not " + + "exist at the configuration file of engine-config.", + keyName); } } @@ -399,15 +403,14 @@ if (keys.size() == 1) { version = keys.get(0).getVersion(); } else if (keys.size() > 1) { - BufferedReader br = new BufferedReader(new InputStreamReader(System.in)); while (true) { - System.out.println("Please select a version:"); + console.writeLine("Please select a version:"); for (int i = 0; i < keys.size(); i++) { - System.out.println(i + 1 + ". " + keys.get(i).getVersion()); + console.writeFormat("%d. %s\n", i + 1, keys.get(i).getVersion()); } int index = 0; try { - index = Integer.valueOf(br.readLine()); + index = Integer.valueOf(console.readLine()); } catch (NumberFormatException e) { continue; } diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/ConfigKey.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/ConfigKey.java index 9ccea30..72f990d 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/ConfigKey.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/ConfigKey.java @@ -6,7 +6,6 @@ import java.util.List; import org.apache.commons.lang.StringUtils; -import org.apache.log4j.Logger; import org.ovirt.engine.core.config.EngineConfigCLIParser; import org.ovirt.engine.core.config.entity.helper.CompositePasswordValueHelper; import org.ovirt.engine.core.config.entity.helper.PasswordValueHelper; @@ -22,7 +21,6 @@ private boolean reloadable; private List<String> validValues; private static final ArrayList<String> EMPTY_LIST = new ArrayList<String>(0); - private final static Logger log = Logger.getLogger(ConfigKey.class); private String version; private ValueHelper valueHelper; diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/CompositePasswordValueHelper.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/CompositePasswordValueHelper.java index e42a86f..f03523c 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/CompositePasswordValueHelper.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/CompositePasswordValueHelper.java @@ -14,7 +14,6 @@ public class CompositePasswordValueHelper implements ValueHelper { private final PasswordValueHelper pwdValueHelper = new PasswordValueHelper(); - private EngineConfigCLIParser parser; private static enum ReformatType { ENCRYPT, @@ -78,7 +77,6 @@ @Override public void setParser(EngineConfigCLIParser parser) { - this.parser = parser; this.pwdValueHelper.setParser(parser); } diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/PasswordValueHelper.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/PasswordValueHelper.java index 1c84c1f..cdb965a 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/PasswordValueHelper.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/PasswordValueHelper.java @@ -3,6 +3,7 @@ import java.io.File; import java.io.IOException; import java.security.GeneralSecurityException; +import java.util.Arrays; import org.apache.commons.lang.StringUtils; import org.apache.log4j.Logger; @@ -12,14 +13,20 @@ import org.ovirt.engine.core.config.db.ConfigDAO; import org.ovirt.engine.core.config.entity.ConfigKey; import org.ovirt.engine.core.config.entity.ConfigKeyFactory; +import org.ovirt.engine.core.tools.Console; import org.ovirt.engine.core.utils.crypt.EncryptionUtils; public class PasswordValueHelper implements ValueHelper { + // The log: + private static final Logger log = Logger.getLogger(PasswordValueHelper.class); + + // The console: + private static final Console console = Console.getInstance(); + private static ConfigDAO configDAO; private static String certAlias; private static String keyStoreURL; private static String keyStorePass; - private static final Logger log = Logger.getLogger(PasswordValueHelper.class); public static final String INTERACTIVE_MODE = "Interactive"; private EngineConfigCLIParser parser; @@ -36,8 +43,11 @@ keyStorePass = configDAO.getKey(keyFactory.generateBlankConfigKey("keystorePass", "String")) .getValue(); - } catch (Exception e) { - log.error(e); + } + catch (Exception exception) { + String msg = "Error loading private key."; + console.writeLine(msg); + log.error(exception); } } @@ -64,9 +74,11 @@ try { decrypt(value); returnedValue = "Set"; - } catch (Exception e) { - String msg = "Failed to decrypt the current value"; - log.error(msg, e); + } + catch (Exception exception) { + String msg = "Failed to decrypt the current value."; + console.writeLine(msg); + log.error(exception); throw new GeneralSecurityException(msg); } } @@ -91,9 +103,11 @@ return StringUtils.EMPTY; } returnedValue = encrypt(password); - } catch (Throwable e) { - String msg = "Failed to encrypt the current value"; - log.error(msg, e); + } + catch (Throwable exception) { + String msg = "Failed to encrypt the current value."; + console.writeLine(msg); + log.error(msg, exception); throw new GeneralSecurityException(msg); } @@ -104,9 +118,9 @@ String password = null; if (StringUtils.isNotBlank(value) && value.equalsIgnoreCase(INTERACTIVE_MODE)) { password = EngineConfigLogic.startPasswordDialog(null); - String passwordConfirm = EngineConfigLogic.startPasswordDialog(null, "Please reenter password"); - if (!password.equals(passwordConfirm)) { - log.error("Passwords don't match"); + String passwordConfirm = EngineConfigLogic.startPasswordDialog(null, "Please reenter password: "); + if (!StringUtils.equals(password, passwordConfirm)) { + console.writeLine("Passwords don't match."); return extractPasswordValue(value); } } else { diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/tools/Console.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/tools/Console.java new file mode 100644 index 0000000..6b988f9 --- /dev/null +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/tools/Console.java @@ -0,0 +1,102 @@ +package org.ovirt.engine.core.tools; + +import org.apache.log4j.Logger; + +/** + * This class is intended to handle the regular input and output generated by + * the tools and destined usually to human users. Output will be sent to the + * system console and also to the log, input will be taken from the system + * console and sent to the log as well, so that the log keeps a record of all + * what happened in the console. + */ +public class Console { + // The log: + private static final Logger log = Logger.getLogger(Console.class); + + // This is a singleton and this is the instance: + private static final Console instance = new Console(); + + public static Console getInstance() { + return instance; + } + + // We need + + private Console() { + // Nothing, just prevent creation of new instances. + } + + /** + * Write the result of converting an object to a string to the system + * console. + * + * @param what the object to convert to string and write + */ + public void write(Object what) { + System.out.print(what); + log.info("Written to console \"" + what + "\"."); + System.out.flush(); + } + + /** + * Write and end of line to the system console. + */ + public void writeLine() { + write("\n"); + } + + /** + * Write the result of converting an object to a string to the system + * console followed by an end of line. + * + * @param what the object to convert to string and write + */ + public void writeLine(Object what) { + write(what + "\n"); + } + + /** + * Formats a set of objects with the given format string (the same used + * with <code>System.out.printf()</code> and write the result to the + * system console. + * + * @param format the format specification + * @param args the objects to convert to strings according to the given + * format + */ + public void writeFormat(String format, Object... args) { + String text = String.format(format, args); + write(text); + } + + /** + * Write the password to the system console, but not to the log. Note that + * the parameter is an array of characters in order to make it possible + * to clean it after calling this method. + * + * @param password the characters of the password + */ + public void writePassword(char[] password) { + System.out.print(password); + System.out.flush(); + log.info("Written password to console."); + } + + /** + * Read a line from the system console. + */ + public String readLine() { + String line = System.console().readLine(); + log.info("Read line \"" + line + "\" from console."); + return line; + } + + /** + * Read a password from the system console without echoing it. + */ + public char[] readPassword() { + char[] password = System.console().readPassword(); + log.info("Read password from console."); + return password; + } +} -- To view, visit http://gerrit.ovirt.org/12987 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I556e20f6333e22faeb7d212767ebefb99edce54e Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Juan Hernandez <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
