Vojtech Szocs has posted comments on this change.
Change subject: webadmin: UI Plugins PoC, revision 5
......................................................................
Patch Set 1: (8 inline comments)
Thanks guys, I've replied to all comments, except for "Public/Private" ones.
I'm used to using Java default (package-private) visibility for methods that
should be normally considered as private, since we might have unit tests for
them in future. JSNI (GWT "native") methods are always private though, since
native JavaScript code cannot be tested with vanilla (JRE) unit tests.
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 501: select
fn_db_add_config_value('UseSecureConnectionWithServers','true','general');
Line 502: select fn_db_add_config_value('UseVdsBrokerInProc','true','general');
Line 503: select
fn_db_add_config_value('UtilizationThresholdInPercent','80','general');
Line 504: select
fn_db_add_config_value('UIPluginConfigPath','ui-plugins','general');
Line 505: select
fn_db_add_config_value('UIPluginDataPath','ui-plugins','general');
I agree with your point, this doesn't need to be configurable at all.
Line 506: select fn_db_add_config_value('ValidNumOfMonitors','1,2,4','general');
Line 507: select
fn_db_add_config_value('VcpuConsumptionPercentage','10','general');
Line 508: --Handling Host Installation Bootstrap Script URL
Line 509: select
fn_db_add_config_value('VdcBootStrapUrl','http://example.com/engine/vds_scripts','general');
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/Config.java
Line 101: * DataDir configuration value.
Line 102: *
Line 103: * @return an absolute path for UIPluginDataPath
Line 104: */
Line 105: public static String resolveUIPluginDataPath() {
Yes, this makes sense, in addition to removing UIPluginDataPath from
ConfigValues enum.
Line 106: return ConfigUtil.resolvePath(Config.<String>
GetValue(ConfigValues.DataDir),
Line 107: Config.<String>
GetValue(ConfigValues.UIPluginDataPath));
Line 108: }
Line 109:
Line 112: * ConfigDir configuration value.
Line 113: *
Line 114: * @return an absolute path for UIPluginConfigPath
Line 115: */
Line 116: public static String resolveUIPluginConfigPath() {
Same as before, but with UIPluginConfigPath removed from ConfigValues enum.
Line 117: return ConfigUtil.resolvePath(Config.<String>
GetValue(ConfigValues.ConfigDir),
Line 118: Config.<String>
GetValue(ConfigValues.UIPluginConfigPath));
Line 119: }
Line 120:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1529: UIPluginDataPath(391),
Line 1530:
Line 1531: @TypeConverterAttribute(String.class)
Line 1532: @DefaultValueAttribute("ui-plugins")
Line 1533: UIPluginConfigPath(392),
No :-) we don't need them.
Line 1534:
Line 1535: Invalid(65535);
Line 1536:
Line 1537: private int intValue;
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ServletUtils.java
Line 49: public static boolean isSane(String path) {
Line 50: // Check that the path is not too long:
Line 51: final int lenght = path.length();
Line 52: if (lenght > PATH_MAX) {
Line 53: log.error("The path is " + lenght + " characters long,
which is longer than the maximum allowed " + PATH_MAX + ".");
I'll correct this.
Line 54: return false;
Line 55: }
Line 56:
Line 57: // Check that there aren't potentially dangerous directory
navigation sequences:
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/plugin/PluginResourceServlet.java
Line 20: * descriptor/configuration data as part of its request handling.
Line 21: */
Line 22: public class PluginResourceServlet extends HttpServlet {
Line 23:
Line 24: private static final long serialVersionUID = 1L;
Yes, I'll do that.
Line 25:
Line 26: private static final Logger logger =
Logger.getLogger(PluginResourceServlet.class);
Line 27:
Line 28: private File baseDir;
Line 41: // Ensure non-null request path
Line 42: requestPath = requestPath == null ? slash : requestPath;
Line 43:
Line 44: // Remove leading '/' character
Line 45: requestPath = requestPath.startsWith(slash) ?
requestPath.substring(1) : requestPath;
Good point, though I'm not sure if "request.getPathInfo()" will really return
"///whatever" when user requests "<servlet-root-path>///whatever". I'll fix
this with a regular expression.
Line 46:
Line 47: // Split plugin name from relative file path
Line 48: String[] parts = requestPath.split(slash, 2);
Line 49: if (parts.length == 2 && !parts[0].trim().isEmpty() &&
!parts[1].trim().isEmpty()) {
Line 45: requestPath = requestPath.startsWith(slash) ?
requestPath.substring(1) : requestPath;
Line 46:
Line 47: // Split plugin name from relative file path
Line 48: String[] parts = requestPath.split(slash, 2);
Line 49: if (parts.length == 2 && !parts[0].trim().isEmpty() &&
!parts[1].trim().isEmpty()) {
Good point, will make the code more readable.
Line 50: pluginName = parts[0];
Line 51: requestFilePath = parts[1];
Line 52: } else {
Line 53: logger.error("Missing UI plugin name and/or relative file
path for request [" + request.getRequestURI() + "]"); //$NON-NLS-1$
//$NON-NLS-2$
--
To view, visit http://gerrit.ovirt.org/8120
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibbf7659f023d8929fb0a8303061851bb7ee555bc
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
