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.

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(),
+    case ejabberd_config:start() of
-    ejabberd_mnesia:start(),
+	ok ->
-    file_queue_init(),
+	    ejabberd_mnesia:start(),
-    maybe_add_nameservers(),
+	    file_queue_init(),
-    ejabberd_system_monitor:start(),
+	    maybe_add_nameservers(),
-    case ejabberd_sup:start_link() of
+	    ejabberd_system_monitor:start(),
-	{ok, SupPid} ->
+	    case ejabberd_sup:start_link() of
-	    register_elixir_config_hooks(),
+		{ok, SupPid} ->
-	    ejabberd_cluster:wait_for_sync(infinity),
+		    register_elixir_config_hooks(),
-	    {T2, _} = statistics(wall_clock),
+		    ejabberd_cluster:wait_for_sync(infinity),
-	    ?INFO_MSG("ejabberd ~s is started in the node ~p in ~.2fs",
+		    {T2, _} = statistics(wall_clock),
-		      [?VERSION, node(), (T2-T1)/1000]),
+		    ?INFO_MSG("ejabberd ~s is started in the node ~p in ~.2fs",
-	    lists:foreach(fun erlang:garbage_collect/1, processes()),
+			      [?VERSION, node(), (T2-T1)/1000]),
-	    {ok, SupPid};
+		    lists:foreach(fun erlang:garbage_collect/1, processes()),
-	Err ->
+		    {ok, SupPid};
-	    ?CRITICAL_MSG("Failed to start ejabberd application: ~p", [Err]),
+		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(_, _) ->

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()}
+-spec start() -> ok | {error, bad_option()}.
-
-%% @type macro_key() = atom().
-%% The atom must have all characters in uppercase.
-
-%% @type macro_value() = term().
-
 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),
+    case load_file(ConfigFile) of
-    UnixTime = p1_time_compat:system_time(seconds),
+	{ok, State1} ->
-    SharedKey = case erlang:get_cookie() of
+	    UnixTime = p1_time_compat:system_time(seconds),
-                    nocookie ->
+	    SharedKey = case erlang:get_cookie() of
-                        str:sha(randoms:get_string());
+			    nocookie ->
-                    Cookie ->
+				str:sha(randoms:get_string());
-                        str:sha(misc:atom_to_binary(Cookie))
+			    Cookie ->
-                end,
+				str:sha(misc:atom_to_binary(Cookie))
-    State2 = set_option({node_start, global}, UnixTime, State1),
+			end,
-    State3 = set_option({shared_key, global}, SharedKey, State2),
+	    State2 = set_option({node_start, global}, UnixTime, State1),
-    set_opts(State3).
+	    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),
+    case load_file(Config) of
-    set_opts(State),
+	{error, _} = Err ->
-    NewHosts = get_myhosts(),
+	    Err;
-    lists:foreach(
+	{ok, State} ->
-      fun(Host) ->
+	    set_opts(State),
-	      ejabberd_hooks:run(host_up, [Host])
+	    NewHosts = get_myhosts(),
-      end, NewHosts -- OldHosts),
+	    lists:foreach(
-    lists:foreach(
+	      fun(Host) ->
-      fun(Host) ->
+		      ejabberd_hooks:run(host_up, [Host])
-	      ejabberd_hooks:run(host_down, [Host])
+	      end, NewHosts -- OldHosts),
-      end, OldHosts -- NewHosts),
+	    lists:foreach(
-    ejabberd_hooks:run(config_reloaded, []).
+	      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(
+    try
-		fun(#local_config{key = {Opt, _Host}, value = Val} = In) ->
+	NewOpts = lists:map(
-			case dict:find(Opt, ModOpts) of
+		    fun(#local_config{key = {Opt, _Host}, value = Val} = In) ->
-			    {ok, [Mod|_]} ->
+			    case dict:find(Opt, ModOpts) of
-				VFun = Mod:opt_type(Opt),
+				{ok, [Mod|_]} ->
-				try VFun(Val) of
+				    VFun = Mod:opt_type(Opt),
-				    NewVal ->
+				    try VFun(Val) of
-					{true, In#local_config{value = NewVal}}
+					NewVal ->
-				catch {invalid_syntax, Error} ->
+					    In#local_config{value = NewVal}
-					?ERROR_MSG("ignoring option '~s' with "
+				    catch {invalid_syntax, Error} ->
-						   "invalid value: ~p: ~s",
+					    ?ERROR_MSG("Invalid value '~p' for "
-						   [Opt, Val, Error]),
+						       "option '~s': ~s",
-					false;
+						       [Val, Opt, Error]),
-				      _:_ ->
+					    erlang:error(invalid_option);
-					?ERROR_MSG("ignoring option '~s' with "
+					  _:_ ->
-						   "invalid value: ~p",
+					    ?ERROR_MSG("Invalid value '~p' for "
-						   [Opt, Val]),
+						       "option '~s'",
-					false
+						       [Val, Opt]),
-				end;
+					    erlang:error(invalid_option)
-			    _ ->
+				    end;
-				?ERROR_MSG("unknown option '~s' will be likely"
+				_ ->
-					   " ignored", [Opt]),
+				    ?ERROR_MSG("Unknown option '~s'", [Opt]),
-				true
+				    erlang:error(unknown_option)
-			end
+			    end
-		end, Opts),
+		    end, Opts),
-    State#state{opts = NewOpts}.
+	{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) ->

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, []),
+	    module_error(ErrTxt);
-	    {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 "
+				      err_invalid_option(Opt, Val)
-						 "option '~s' of module '~s'",
-						 [Val, Opt, Module]),
-				      fail_if_option_is_required(Opt, Required),
-				      []
 			      end
 		      end;
 		  false ->
-		      case Validators of
+		      err_unknown_option(Opt, [K || {K, _} <- Validators])
-			  [] ->
-			      ?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,
-		      []
 	      end;
 	 (_) ->
 	      erlang:error(bad_option)
       end, Opts);
-validate_opts(_, _, _, _, _) ->
+validate_opts(_, _, _, _) ->
     erlang:error(bad_option).
 
--spec validate_opt(module(), atom(), any(), [atom()],
+-spec validate_opt(atom(), any(), check_fun()) -> [{atom(), any()}].
-		   [{atom(), check_fun(), any()}]) -> [{atom(), any()}].
+validate_opt(Opt, Val, VFun) ->
-validate_opt(Module, Opt, Val, Required, VFun) ->
     try VFun(Val) of
 	NewVal -> [{Opt, NewVal}]
     catch {invalid_syntax, Error} ->
-	    ?ERROR_MSG("Ignoring invalid value '~p' for "
+	    err_invalid_option(Opt, Val, Error);
-		       "option '~s' of module '~s': ~s",
-		       [Val, Opt, Module, Error]),
-	    fail_if_option_is_required(Opt, Required),
-	    [];
 	  _:_ ->
-	    ?ERROR_MSG("Ignoring invalid value '~p' for "
+	    err_invalid_option(Opt, Val)
-		       "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
     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(
+					  err_invalid_option(Opt, Val)
-					     "Ignoring invalid value '~p' for "
-					     "option '~s' of module '~s'",
-					     [Val, Opt, Module]),
-					  [{Opt, Default}|Acc]
 				  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(