Stop ejabberd initialization on invalid/unknown options

Since now, ejabberd doesn't ignore unknown options and doesn't
allow to have options with malformed values. The rationale for
this is to avoid unexpected behaviour during runtime, i.e. to
conform to "fail early" approach. Note that it's safe to reload
a configuration with potentialy invalid and/or unknown options:
this will not halt ejabberd, but will only prevent the configuration
from loading.

***NOTE FOR PACKAGE BUILDERS***
This new behaviour should be documented in the upgrade notes.

3 files changed, 145 insertions, 128 deletions

diff --git a/src/ejabberd_app.erl b/src/ejabberd_app.erl
index f1066815f..c7486a5be 100644
--- a/src/ejabberd_app.erl
+++ b/src/ejabberd_app.erl
@@ -46,22 +46,27 @@ start(normal, _Args) ->
     start_elixir_application(),
     ejabberd:check_app(ejabberd),
     setup_if_elixir_conf_used(),
-    ejabberd_config:start(),
-    ejabberd_mnesia:start(),
-    file_queue_init(),
-    maybe_add_nameservers(),
-    ejabberd_system_monitor:start(),
-    case ejabberd_sup:start_link() of
-	{ok, SupPid} ->
-	    register_elixir_config_hooks(),
-	    ejabberd_cluster:wait_for_sync(infinity),
-	    {T2, _} = statistics(wall_clock),
-	    ?INFO_MSG("ejabberd ~s is started in the node ~p in ~.2fs",
-		      [?VERSION, node(), (T2-T1)/1000]),
-	    lists:foreach(fun erlang:garbage_collect/1, processes()),
-	    {ok, SupPid};
-	Err ->
-	    ?CRITICAL_MSG("Failed to start ejabberd application: ~p", [Err]),
+    case ejabberd_config:start() of
+	ok ->
+	    ejabberd_mnesia:start(),
+	    file_queue_init(),
+	    maybe_add_nameservers(),
+	    ejabberd_system_monitor:start(),
+	    case ejabberd_sup:start_link() of
+		{ok, SupPid} ->
+		    register_elixir_config_hooks(),
+		    ejabberd_cluster:wait_for_sync(infinity),
+		    {T2, _} = statistics(wall_clock),
+		    ?INFO_MSG("ejabberd ~s is started in the node ~p in ~.2fs",
+			      [?VERSION, node(), (T2-T1)/1000]),
+		    lists:foreach(fun erlang:garbage_collect/1, processes()),
+		    {ok, SupPid};
+		Err ->
+		    ?CRITICAL_MSG("Failed to start ejabberd application: ~p", [Err]),
+		    ejabberd:halt()
+	    end;
+	{error, Reason} ->
+	    ?CRITICAL_MSG("Failed to start ejabberd application: ~p", [Reason]),
 	    ejabberd:halt()
     end;
 start(_, _) ->
diff --git a/src/ejabberd_config.erl b/src/ejabberd_config.erl
index 3f88df59a..3fe4d9c71 100644
--- a/src/ejabberd_config.erl
+++ b/src/ejabberd_config.erl
@@ -60,14 +60,9 @@
 -include_lib("stdlib/include/ms_transform.hrl").
 
 -callback opt_type(atom()) -> function() | [atom()].
+-type bad_option() :: invalid_option | unknown_option.
 
-%% @type macro() = {macro_key(), macro_value()}
-
-%% @type macro_key() = atom().
-%% The atom must have all characters in uppercase.
-
-%% @type macro_value() = term().
-
+-spec start() -> ok | {error, bad_option()}.
 start() ->
     ConfigFile = get_ejabberd_config_path(),
     ?INFO_MSG("Loading configuration from ~s", [ConfigFile]),
@@ -75,17 +70,23 @@ start() ->
 		  [named_table, public, {read_concurrency, true}]),
     catch ets:new(ejabberd_db_modules,
 		  [named_table, public, {read_concurrency, true}]),
-    State1 = load_file(ConfigFile),
-    UnixTime = p1_time_compat:system_time(seconds),
-    SharedKey = case erlang:get_cookie() of
-                    nocookie ->
-                        str:sha(randoms:get_string());
-                    Cookie ->
-                        str:sha(misc:atom_to_binary(Cookie))
-                end,
-    State2 = set_option({node_start, global}, UnixTime, State1),
-    State3 = set_option({shared_key, global}, SharedKey, State2),
-    set_opts(State3).
+    case load_file(ConfigFile) of
+	{ok, State1} ->
+	    UnixTime = p1_time_compat:system_time(seconds),
+	    SharedKey = case erlang:get_cookie() of
+			    nocookie ->
+				str:sha(randoms:get_string());
+			    Cookie ->
+				str:sha(misc:atom_to_binary(Cookie))
+			end,
+	    State2 = set_option({node_start, global}, UnixTime, State1),
+	    State3 = set_option({shared_key, global}, SharedKey, State2),
+	    set_opts(State3),
+	    ok;
+	{error, _} = Err ->
+	    ?ERROR_MSG("Failed to load configuration file ~s", [ConfigFile]),
+	    Err
+    end.
 
 %% When starting ejabberd for testing, we sometimes want to start a
 %% subset of hosts from the one define in the config file.
@@ -189,7 +190,7 @@ read_file(File, Opts) ->
     State1 = lists:foldl(fun process_term/2, State, Head ++ Tail),
     State1#state{opts = compact(State1#state.opts)}.
 
--spec load_file(string()) -> #state{}.
+-spec load_file(string()) -> {ok, #state{}} | {error, bad_option()}.
 
 load_file(File) ->
     State0 = read_file(File),
@@ -199,23 +200,27 @@ load_file(File) ->
     ModOpts = get_modules_with_options(AllMods),
     validate_opts(State1, ModOpts).
 
--spec reload_file() -> ok.
+-spec reload_file() -> ok | {error, bad_option()}.
 
 reload_file() ->
     Config = get_ejabberd_config_path(),
     OldHosts = get_myhosts(),
-    State = load_file(Config),
-    set_opts(State),
-    NewHosts = get_myhosts(),
-    lists:foreach(
-      fun(Host) ->
-	      ejabberd_hooks:run(host_up, [Host])
-      end, NewHosts -- OldHosts),
-    lists:foreach(
-      fun(Host) ->
-	      ejabberd_hooks:run(host_down, [Host])
-      end, OldHosts -- NewHosts),
-    ejabberd_hooks:run(config_reloaded, []).
+    case load_file(Config) of
+	{error, _} = Err ->
+	    Err;
+	{ok, State} ->
+	    set_opts(State),
+	    NewHosts = get_myhosts(),
+	    lists:foreach(
+	      fun(Host) ->
+		      ejabberd_hooks:run(host_up, [Host])
+	      end, NewHosts -- OldHosts),
+	    lists:foreach(
+	      fun(Host) ->
+		      ejabberd_hooks:run(host_down, [Host])
+	      end, OldHosts -- NewHosts),
+	    ejabberd_hooks:run(config_reloaded, [])
+    end.
 
 -spec convert_to_yaml(file:filename()) -> ok | {error, any()}.
 
@@ -1017,33 +1022,39 @@ get_modules_with_options(Modules) ->
 	      end
       end, dict:new(), Modules).
 
+-spec validate_opts(#state{}, dict:dict()) -> {ok, #state{}} | {error, bad_option()}.
 validate_opts(#state{opts = Opts} = State, ModOpts) ->
-    NewOpts = lists:filtermap(
-		fun(#local_config{key = {Opt, _Host}, value = Val} = In) ->
-			case dict:find(Opt, ModOpts) of
-			    {ok, [Mod|_]} ->
-				VFun = Mod:opt_type(Opt),
-				try VFun(Val) of
-				    NewVal ->
-					{true, In#local_config{value = NewVal}}
-				catch {invalid_syntax, Error} ->
-					?ERROR_MSG("ignoring option '~s' with "
-						   "invalid value: ~p: ~s",
-						   [Opt, Val, Error]),
-					false;
-				      _:_ ->
-					?ERROR_MSG("ignoring option '~s' with "
-						   "invalid value: ~p",
-						   [Opt, Val]),
-					false
-				end;
-			    _ ->
-				?ERROR_MSG("unknown option '~s' will be likely"
-					   " ignored", [Opt]),
-				true
-			end
-		end, Opts),
-    State#state{opts = NewOpts}.
+    try
+	NewOpts = lists:map(
+		    fun(#local_config{key = {Opt, _Host}, value = Val} = In) ->
+			    case dict:find(Opt, ModOpts) of
+				{ok, [Mod|_]} ->
+				    VFun = Mod:opt_type(Opt),
+				    try VFun(Val) of
+					NewVal ->
+					    In#local_config{value = NewVal}
+				    catch {invalid_syntax, Error} ->
+					    ?ERROR_MSG("Invalid value '~p' for "
+						       "option '~s': ~s",
+						       [Val, Opt, Error]),
+					    erlang:error(invalid_option);
+					  _:_ ->
+					    ?ERROR_MSG("Invalid value '~p' for "
+						       "option '~s'",
+						       [Val, Opt]),
+					    erlang:error(invalid_option)
+				    end;
+				_ ->
+				    ?ERROR_MSG("Unknown option '~s'", [Opt]),
+				    erlang:error(unknown_option)
+			    end
+		    end, Opts),
+	{ok, State#state{opts = NewOpts}}
+    catch _:invalid_option ->
+	    {error, invalid_option};
+	  _:unknown_option ->
+	    {error, unknown_option}
+    end.
 
 %% @spec (Path::string()) -> true | false
 is_file_readable(Path) ->
diff --git a/src/gen_mod.erl b/src/gen_mod.erl
index 1e069be73..ed04767ca 100644
--- a/src/gen_mod.erl
+++ b/src/gen_mod.erl
@@ -546,89 +546,94 @@ validate_opts(Host, Module, Opts0) ->
 			    catch _:undef -> []
 			    end
 		    end, [Module|SubMods]),
-    Required = lists:filter(fun is_atom/1, DefaultOpts),
     try
 	Opts = merge_opts(Opts0, DefaultOpts, Module),
 	{ok, case get_validators(Host, {Module, SubMods}) of
 		 undef ->
 		     Opts;
 		 Validators ->
-		     Opts1 = validate_opts(Host, Module, Opts, Required, Validators),
+		     Opts1 = validate_opts(Host, Module, Opts, Validators),
 		     remove_duplicated_opts(Opts1)
 	     end}
     catch _:{missing_required_option, Opt} ->
 	    ErrTxt = io_lib:format("Module '~s' is missing required option '~s'",
 				   [Module, Opt]),
-	    ?ERROR_MSG(ErrTxt, []),
-	    {error, ErrTxt}
+	    module_error(ErrTxt);
+	  _:{invalid_option, Opt, Val} ->
+	    ErrTxt = io_lib:format("Invalid value '~p' for option '~s' of "
+				   "module '~s'", [Val, Opt, Module]),
+	    module_error(ErrTxt);
+	  _:{invalid_option, Opt, Val, Reason} ->
+	    ErrTxt = io_lib:format("Invalid value '~p' for option '~s' of "
+				   "module '~s': ~s", [Val, Opt, Module, Reason]),
+	    module_error(ErrTxt);
+	  _:{unknown_option, Opt, []} ->
+	    ErrTxt = io_lib:format("Unknown option '~s' of module '~s': "
+				   "the module doesn't have any options",
+				   [Opt, Module]),
+	    module_error(ErrTxt);
+	  _:{unknown_option, Opt, KnownOpts} ->
+	    ErrTxt = io_lib:format("Unknown option '~s' of module '~s',"
+				   " available options are: ~s",
+				   [Opt, Module,
+				    misc:join_atoms(KnownOpts, <<", ">>)]),
+	    module_error(ErrTxt)
     end.
 
-validate_opts(Host, Module, Opts, Required, Validators) when is_list(Opts) ->
+-spec module_error(iolist()) -> {error, iolist()}.
+module_error(ErrTxt) ->
+    ?ERROR_MSG(ErrTxt, []),
+    {error, ErrTxt}.
+
+-spec err_invalid_option(atom(), any()) -> no_return().
+err_invalid_option(Opt, Val) ->
+    erlang:error({invalid_option, Opt, Val}).
+
+-spec err_invalid_option(atom(), any(), iolist()) -> no_return().
+err_invalid_option(Opt, Val, Reason) ->
+    erlang:error({invalid_option, Opt, Val, Reason}).
+
+-spec err_unknown_option(atom(), [atom()]) -> no_return().
+err_unknown_option(Opt, KnownOpts) ->
+    erlang:error({unknown_option, Opt, KnownOpts}).
+
+-spec err_missing_required_option(atom()) -> no_return().
+err_missing_required_option(Opt) ->
+    erlang:error({missing_required_option, Opt}).
+
+validate_opts(Host, Module, Opts, Validators) when is_list(Opts) ->
     lists:flatmap(
       fun({Opt, Val}) when is_atom(Opt) ->
 	      case lists:keyfind(Opt, 1, Validators) of
 		  {_, L} ->
 		      case lists:partition(fun is_function/1, L) of
 			  {[VFun|_], []} ->
-			      validate_opt(Module, Opt, Val, Required, VFun);
+			      validate_opt(Opt, Val, VFun);
 			  {[VFun|_], SubValidators} ->
-			      try validate_opts(Host, Module, Val, Required, SubValidators) of
+			      try validate_opts(Host, Module, Val, SubValidators) of
 				  SubOpts ->
-				      validate_opt(Module, Opt, SubOpts, Required, VFun)
+				      validate_opt(Opt, SubOpts, VFun)
 			      catch _:bad_option ->
-				      ?ERROR_MSG("Ignoring invalid value '~p' for "
-						 "option '~s' of module '~s'",
-						 [Val, Opt, Module]),
-				      fail_if_option_is_required(Opt, Required),
-				      []
+				      err_invalid_option(Opt, Val)
 			      end
 		      end;
 		  false ->
-		      case Validators of
-			  [] ->
-			      ?ERROR_MSG("Ignoring unknown option '~s' of '~s':"
-					 " the module doesn't have any options",
-					 [Opt, Module]);
-			  _ ->
-			      ?ERROR_MSG("Ignoring unknown option '~s' of '~s',"
-					 " available options are: ~s",
-					 [Opt, Module,
-					  misc:join_atoms(
-					    [K || {K, _} <- Validators],
-					    <<", ">>)])
-		      end,
-		      []
+		      err_unknown_option(Opt, [K || {K, _} <- Validators])
 	      end;
 	 (_) ->
 	      erlang:error(bad_option)
       end, Opts);
-validate_opts(_, _, _, _, _) ->
+validate_opts(_, _, _, _) ->
     erlang:error(bad_option).
 
--spec validate_opt(module(), atom(), any(), [atom()],
-		   [{atom(), check_fun(), any()}]) -> [{atom(), any()}].
-validate_opt(Module, Opt, Val, Required, VFun) ->
+-spec validate_opt(atom(), any(), check_fun()) -> [{atom(), any()}].
+validate_opt(Opt, Val, VFun) ->
     try VFun(Val) of
 	NewVal -> [{Opt, NewVal}]
     catch {invalid_syntax, Error} ->
-	    ?ERROR_MSG("Ignoring invalid value '~p' for "
-		       "option '~s' of module '~s': ~s",
-		       [Val, Opt, Module, Error]),
-	    fail_if_option_is_required(Opt, Required),
-	    [];
+	    err_invalid_option(Opt, Val, Error);
 	  _:_ ->
-	    ?ERROR_MSG("Ignoring invalid value '~p' for "
-		       "option '~s' of module '~s'",
-		       [Val, Opt, Module]),
-	    fail_if_option_is_required(Opt, Required),
-	    []
-    end.
-
--spec fail_if_option_is_required(atom(), [atom()]) -> ok | no_return().
-fail_if_option_is_required(Opt, Required) ->
-    case lists:member(Opt, Required) of
-	true -> erlang:error({missing_required_option, Opt});
-	false -> ok
+	    err_invalid_option(Opt, Val)
     end.
 
 -spec list_known_opts(binary(), module()) -> [atom() | {atom(), atom()}].
@@ -658,11 +663,7 @@ merge_opts(Opts, DefaultOpts, Module) ->
 				      true ->
 					  [{Opt, merge_opts(Val, Default, Module)}|Acc];
 				      false ->
-					  ?ERROR_MSG(
-					     "Ignoring invalid value '~p' for "
-					     "option '~s' of module '~s'",
-					     [Val, Opt, Module]),
-					  [{Opt, Default}|Acc]
+					  err_invalid_option(Opt, Val)
 				  end;
 			      Val ->
 				  [{Opt, Default}|Acc];
@@ -677,7 +678,7 @@ merge_opts(Opts, DefaultOpts, Module) ->
 		      {_, Val} ->
 			  [{Opt, Val}|Acc];
 		      false ->
-			  erlang:error({missing_required_option, Opt})
+			  err_missing_required_option(Opt)
 		  end
 	  end, [], DefaultOpts),
     lists:foldl(