Browse files

Changes suggested by Kostis, Dialyzer -Wunmatched_returns and Tidier

  • Loading branch information...
1 parent b504747 commit efc7b7591b47496d7823e4d3aa90e43e5608d7b3 @Vagabond Vagabond committed Mar 31, 2012
View
2 Makefile
@@ -39,7 +39,7 @@ dialyzer: compile
@echo Use "'make build_plt'" to build PLT prior to using this target.
@echo
@sleep 1
- dialyzer -Wno_return --plt $(COMBO_PLT) ebin | \
+ dialyzer -Wno_return -Wunmatched_returns --plt $(COMBO_PLT) ebin | \
fgrep -v -f ./dialyzer.ignore-warnings
cleanplt:
View
6 src/error_logger_lager_h.erl
@@ -35,14 +35,16 @@
-define(LOG(Level, Pid, Msg),
case ?SHOULD_LOG(Level) of
true ->
- lager:log(Level, Pid, Msg);
+ _ =lager:log(Level, Pid, Msg),
+ ok;
_ -> ok
end).
-define(LOG(Level, Pid, Fmt, Args),
case ?SHOULD_LOG(Level) of
true ->
- lager:log(Level, Pid, Fmt, Args);
+ _ = lager:log(Level, Pid, Fmt, Args),
+ ok;
_ -> ok
end).
View
42 src/lager.erl
@@ -125,22 +125,28 @@ trace_file(File, Filter, Level) ->
{ok, Trace} ->
Handlers = gen_event:which_handlers(lager_event),
%% check if this file backend is already installed
- case lists:member({lager_file_backend, File}, Handlers) of
+ Res = case lists:member({lager_file_backend, File}, Handlers) of
false ->
%% install the handler
supervisor:start_child(lager_handler_watcher_sup,
[lager_event, {lager_file_backend, File}, {File, none}]);
_ ->
ok
end,
- %% install the trace.
- {MinLevel, Traces} = lager_mochiglobal:get(loglevel),
- case lists:member(Trace, Traces) of
- false ->
+ case Res of
+ {ok, _} ->
+ %% install the trace.
+ {MinLevel, Traces} = lager_mochiglobal:get(loglevel),
+ case lists:member(Trace, Traces) of
+ false ->
lager_mochiglobal:put(loglevel, {MinLevel, [Trace|Traces]});
- _ -> ok
- end,
- {ok, Trace};
+ _ ->
+ ok
+ end,
+ {ok, Trace};
+ {error, _} = E ->
+ E
+ end;
Error ->
Error
end.
@@ -184,15 +190,14 @@ stop_trace({_Filter, _Level, Target} = Trace) ->
clear_all_traces() ->
{MinLevel, _Traces} = lager_mochiglobal:get(loglevel),
lager_mochiglobal:put(loglevel, {MinLevel, []}),
- [begin
- case get_loglevel(Handler) of
- none ->
- gen_event:delete_handler(lager_event, Handler, []);
- _ ->
- ok
- end
- end || Handler <- gen_event:which_handlers(lager_event)],
- ok.
+ lists:foreach(fun(Handler) ->
+ case get_loglevel(Handler) of
+ none ->
+ gen_event:delete_handler(lager_event, Handler, []);
+ _ ->
+ ok
+ end
+ end, gen_event:which_handlers(lager_event)).
status() ->
Handlers = gen_event:which_handlers(lager_event),
@@ -283,8 +288,7 @@ safe_format(Fmt, Args, Limit) ->
safe_format(Fmt, Args, Limit, []).
safe_format(Fmt, Args, Limit, Options) ->
- try lager_trunc_io:format(Fmt, Args, Limit, Options) of
- Result -> Result
+ try lager_trunc_io:format(Fmt, Args, Limit, Options)
catch
_:_ -> lager_trunc_io:format("FORMAT ERROR: ~p ~p", [Fmt, Args], Limit)
end.
View
20 src/lager_app.erl
@@ -43,7 +43,8 @@ start(_StartType, _StartArgs) ->
Val
end,
- [supervisor:start_child(lager_handler_watcher_sup, [lager_event, Module, Config]) ||
+ %% handlers failing to start are handled in the handler_watcher
+ _ = [supervisor:start_child(lager_handler_watcher_sup, [lager_event, Module, Config]) ||
{Module, Config} <- expand_handlers(Handlers)],
%% mask the messages we have no use for
@@ -55,18 +56,23 @@ start(_StartType, _StartArgs) ->
{ok, false} ->
[];
_ ->
- supervisor:start_child(lager_handler_watcher_sup, [error_logger, error_logger_lager_h, []]),
- %% Should we allow user to whitelist handlers to not be removed?
- [begin error_logger:delete_report_handler(X), X end ||
- X <- gen_event:which_handlers(error_logger) -- [error_logger_lager_h]]
+ case supervisor:start_child(lager_handler_watcher_sup, [error_logger, error_logger_lager_h, []]) of
+ {ok, _} ->
+ %% Should we allow user to whitelist handlers to not be removed?
+ [begin error_logger:delete_report_handler(X), X end ||
+ X <- gen_event:which_handlers(error_logger) -- [error_logger_lager_h]];
+ {error, _} ->
+ []
+ end
end,
{ok, Pid, SavedHandlers}.
stop(Handlers) ->
- [error_logger:add_report_handler(Handler) || Handler <- Handlers],
- ok.
+ lists:foreach(fun(Handler) ->
+ error_logger:add_report_handler(Handler)
+ end, Handlers).
expand_handlers([]) ->
[];
View
5 src/lager_crash_log.erl
@@ -110,9 +110,10 @@ code_change(_OldVsn, State, _Extra) ->
{ok, State}.
schedule_rotation(undefined) ->
- undefined;
+ ok;
schedule_rotation(Date) ->
- erlang:send_after(lager_util:calculate_next_rotation(Date) * 1000, self(), rotate).
+ erlang:send_after(lager_util:calculate_next_rotation(Date) * 1000, self(), rotate),
+ ok.
%% ===== Begin code lifted from riak_err =====
View
12 src/lager_file_backend.erl
@@ -108,8 +108,8 @@ handle_info(_Info, State) ->
%% @private
terminate(_Reason, #state{fd=FD}) ->
%% flush and close any file handles
- file:datasync(FD),
- file:close(FD),
+ _ = file:datasync(FD),
+ _ = file:close(FD),
ok.
%% @private
@@ -123,7 +123,8 @@ write(#state{name=Name, fd=FD, inode=Inode, flap=Flap, size=RotSize,
lager_util:rotate_logfile(Name, Count),
write(State, Level, Msg);
{ok, {NewFD, NewInode, _}} ->
- file:write(NewFD, Msg),
+ %% delayed_write doesn't report errors
@yfyf
yfyf added a note Jan 23, 2013

Not sure if you want to handle this, but this means that if the file ever gets removed, the FD will be useless, but no errors will be logged (this is somewhat circular, haha). Perhaps this should be handled in some way?

Actually, that is what ensure_logfile does, it checks that the filename still exists and that the inode hasn't changed. Otherwise it'll return the new file handle and the new inode, which is what is happening here.

@yfyf
yfyf added a note Jan 24, 2013

Ah, ok, I missed that part completely. I will investigate this further though, because I observed lager_file_backend not logging anything (probably due to file permission problems), but did not seem to complain about anything.

If you figure out how to make it happen, let me know.

@yfyf
yfyf added a note Jan 25, 2013

Hm, the reason I was confused is because it only logs the error once and ignores the subsequent errors: https://github.com/basho/lager/blob/efc7b7591b47496d7823e4d3aa90e43e5608d7b3/src/lager_file_backend.erl#L148

Perhaps this minimalistic approach makes sense, but if one misses the initial error in the logs, then he/she is under the false impression that lager is functioning successfully, although in my case it was a whole day of missing logs unnoticed.

Anyway, not implying this must be changed, but just giving you some food for thought. Maybe it could at least log an error every 100th attempt or use some other heuristic approach. Or be aggressive about it and complain every time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ _ = file:write(NewFD, Msg),
case Level of
_ when Level =< ?ERROR ->
%% force a sync on any message at error severity or above
@@ -196,9 +197,10 @@ validate_logfile(H) ->
false.
schedule_rotation(_, undefined) ->
- undefined;
+ ok;
schedule_rotation(Name, Date) ->
- erlang:send_after(lager_util:calculate_next_rotation(Date) * 1000, self(), {rotate, Name}).
+ erlang:send_after(lager_util:calculate_next_rotation(Date) * 1000, self(), {rotate, Name}),
+ ok.
-ifdef(TEST).
View
18 src/lager_handler_watcher.erl
@@ -61,9 +61,14 @@ handle_info({gen_event_EXIT, Module, shutdown}, #state{module=Module} = State) -
{stop, normal, State};
handle_info({gen_event_EXIT, Module, Reason}, #state{module=Module,
config=Config, event=Event} = State) ->
- lager:log(error, self(), "Lager event handler ~p exited with reason ~s",
- [Module, error_logger_lager_h:format_reason(Reason)]),
- install_handler(Event, Module, Config),
+ case lager:log(error, self(), "Lager event handler ~p exited with reason ~s",
+ [Module, error_logger_lager_h:format_reason(Reason)]) of
+ ok ->
+ install_handler(Event, Module, Config);
+ {error, _} ->
+ %% lager is not working, so installing a handler won't work
+ ok
+ end,
{noreply, State};
handle_info(reinstall_handler, #state{module=Module, config=Config, event=Event} = State) ->
install_handler(Event, Module, Config),
@@ -82,13 +87,14 @@ code_change(_OldVsn, State, _Extra) ->
install_handler(Event, Module, Config) ->
case gen_event:add_sup_handler(Event, Module, Config) of
ok ->
- lager:log(debug, self(), "Lager installed handler ~p into ~p", [Module, Event]),
+ _ = lager:log(debug, self(), "Lager installed handler ~p into ~p", [Module, Event]),
ok;
Error ->
%% try to reinstall it later
- lager:log(error, self(), "Lager failed to install handler ~p into"
+ _ = lager:log(error, self(), "Lager failed to install handler ~p into"
" ~p, retrying later : ~p", [Module, Event, Error]),
- erlang:send_after(5000, self(), reinstall_handler)
+ erlang:send_after(5000, self(), reinstall_handler),
+ ok
end.
-ifdef(TEST).
View
3 src/lager_trunc_io.erl
@@ -60,8 +60,7 @@ format(Fmt, Args, Max) ->
format(Fmt, Args, Max, []).
format(Fmt, Args, Max, Options) ->
- try lager_format:format(Fmt, Args, Max, Options) of
- Result -> Result
+ try lager_format:format(Fmt, Args, Max, Options)
catch
_:_ ->
erlang:error(badarg, [Fmt, Args])
View
16 src/lager_util.erl
@@ -79,8 +79,8 @@ ensure_logfile(Name, FD, Inode, Buffer) ->
{ok, {FD, Inode, FInfo#file_info.size}};
false ->
%% delayed write can cause file:close not to do a close
- file:close(FD),
- file:close(FD),
+ _ = file:close(FD),
+ _ = file:close(FD),
case open_logfile(Name, Buffer) of
{ok, {FD2, Inode3, Size}} ->
%% inode changed, file was probably moved and
@@ -92,8 +92,8 @@ ensure_logfile(Name, FD, Inode, Buffer) ->
end;
_ ->
%% delayed write can cause file:close not to do a close
- file:close(FD),
- file:close(FD),
+ _ = file:close(FD),
+ _ = file:close(FD),
case open_logfile(Name, Buffer) of
{ok, {FD2, Inode3, Size}} ->
%% file was removed
@@ -117,13 +117,15 @@ maybe_utc({Date, {H, M, S, Ms}}) ->
{Date1, {H1, M1, S1, Ms}}
end.
+%% renames and deletes failing are OK
rotate_logfile(File, 0) ->
- file:delete(File);
+ _ = file:delete(File),
+ ok;
rotate_logfile(File, 1) ->
- file:rename(File, File++".0"),
+ _ = file:rename(File, File++".0"),
rotate_logfile(File, 0);
rotate_logfile(File, Count) ->
- file:rename(File ++ "." ++ integer_to_list(Count - 2), File ++ "." ++
+ _ =file:rename(File ++ "." ++ integer_to_list(Count - 2), File ++ "." ++
integer_to_list(Count - 1)),
rotate_logfile(File, Count - 1).

0 comments on commit efc7b75

Please sign in to comment.