From dd3256cdc7048de9e2f32569b875cd104e03a11a Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Fri, 31 Oct 2014 13:08:16 +0000 Subject: Inline parse_field_value/1 and thus prevent sub-binary construction in accordance with the principles documented at http://www.erlang.org/doc/efficiency_guide/binaryhandling.html --- Makefile | 2 +- src/rabbit_binary_parser.erl | 103 ++++++++++++++++++++++++++++++------------- 2 files changed, 74 insertions(+), 31 deletions(-) diff --git a/Makefile b/Makefile index c955a8fc..1b66a306 100644 --- a/Makefile +++ b/Makefile @@ -57,7 +57,7 @@ USE_PROPER_QC:=$(shell erl -noshell -eval 'io:format({module, proper} =:= code:e endif #other args: +native +"{hipe,[o3,verbose]}" -Ddebug=true +debug_info +no_strict_record_tests -ERLC_OPTS=-I $(INCLUDE_DIR) -Wall -v +debug_info $(call boolean_macro,$(USE_SPECS),use_specs) $(call boolean_macro,$(USE_PROPER_QC),use_proper_qc) +ERLC_OPTS=-I $(INCLUDE_DIR) -Wall -v +debug_info +bin_opt_info $(call boolean_macro,$(USE_SPECS),use_specs) $(call boolean_macro,$(USE_PROPER_QC),use_proper_qc) ifdef INSTRUMENT_FOR_QC ERLC_OPTS += -DINSTR_MOD=gm_qc diff --git a/src/rabbit_binary_parser.erl b/src/rabbit_binary_parser.erl index 3ab82cad..8850d69e 100644 --- a/src/rabbit_binary_parser.erl +++ b/src/rabbit_binary_parser.erl @@ -41,48 +41,91 @@ %% parse_table supports the AMQP 0-8/0-9 standard types, S, I, D, T %% and F, as well as the QPid extensions b, d, f, l, s, t, x, and V. +-define(SIMPLE_PARSE_TABLE(BType, Pattern, RType), + parse_table(<>) -> + [{NameString, RType, Value} | parse_table(Rest)]). + +%% Note that we try to put these in approximately the order we expect +%% to hit them, that's why the empty binary is half way through. + +parse_table(<>) -> + [{NameString, longstr, Value} | parse_table(Rest)]; + +?SIMPLE_PARSE_TABLE($I, Value:32/signed, signedint); +?SIMPLE_PARSE_TABLE($T, Value:64/unsigned, timestamp); + parse_table(<<>>) -> []; -parse_table(<>) -> - {Type, Value, Rest} = parse_field_value(ValueAndRest), - [{NameString, Type, Value} | parse_table(Rest)]. -parse_array(<<>>) -> - []; -parse_array(<>) -> - {Type, Value, Rest} = parse_field_value(ValueAndRest), - [{Type, Value} | parse_array(Rest)]. +?SIMPLE_PARSE_TABLE($b, Value:8/signed, byte); +?SIMPLE_PARSE_TABLE($d, Value:64/float, double); +?SIMPLE_PARSE_TABLE($f, Value:32/float, float); +?SIMPLE_PARSE_TABLE($l, Value:64/signed, long); +?SIMPLE_PARSE_TABLE($s, Value:16/signed, short); + +parse_table(<>) -> + [{NameString, bool, (Value /= 0)} | parse_table(Rest)]; + +parse_table(<>) -> + [{NameString, decimal, {Before, After}} | parse_table(Rest)]; -parse_field_value(<<$S, VLen:32/unsigned, V:VLen/binary, R/binary>>) -> - {longstr, V, R}; +parse_table(<>) -> + [{NameString, table, parse_table(Value)} | parse_table(Rest)]; -parse_field_value(<<$I, V:32/signed, R/binary>>) -> - {signedint, V, R}; +parse_table(<>) -> + [{NameString, array, parse_array(Value)} | parse_table(Rest)]; + +parse_table(<>) -> + [{NameString, binary, Value} | parse_table(Rest)]; + +parse_table(<>) -> + [{NameString, void, undefined} | parse_table(Rest)]. + +-define(SIMPLE_PARSE_ARRAY(BType, Pattern, RType), + parse_array(<>) -> + [{RType, Value} | parse_table(Rest)]). + +parse_array(<>) -> + [{NameString, longstr, Value} | parse_array(Rest)]; + +?SIMPLE_PARSE_ARRAY($I, Value:32/signed, signedint); +?SIMPLE_PARSE_ARRAY($T, Value:64/unsigned, timestamp); + +parse_array(<<>>) -> + []; -parse_field_value(<<$D, Before:8/unsigned, After:32/unsigned, R/binary>>) -> - {decimal, {Before, After}, R}; +?SIMPLE_PARSE_ARRAY($b, Value:8/signed, byte); +?SIMPLE_PARSE_ARRAY($d, Value:64/float, double); +?SIMPLE_PARSE_ARRAY($f, Value:32/float, float); +?SIMPLE_PARSE_ARRAY($l, Value:64/signed, long); +?SIMPLE_PARSE_ARRAY($s, Value:16/signed, short); -parse_field_value(<<$T, V:64/unsigned, R/binary>>) -> - {timestamp, V, R}; +parse_array(<<$t, Value:8/unsigned, Rest/binary>>) -> + [{bool, (Value /= 0)} | parse_array(Rest)]; -parse_field_value(<<$F, VLen:32/unsigned, Table:VLen/binary, R/binary>>) -> - {table, parse_table(Table), R}; +parse_array(<<$D, Before:8/unsigned, After:32/unsigned, Rest/binary>>) -> + [{decimal, {Before, After}} | parse_array(Rest)]; -parse_field_value(<<$A, VLen:32/unsigned, Array:VLen/binary, R/binary>>) -> - {array, parse_array(Array), R}; +parse_array(<<$F, VLen:32/unsigned, Value:VLen/binary, Rest/binary>>) -> + [{table, parse_table(Value)} | parse_array(Rest)]; -parse_field_value(<<$b, V:8/signed, R/binary>>) -> {byte, V, R}; -parse_field_value(<<$d, V:64/float, R/binary>>) -> {double, V, R}; -parse_field_value(<<$f, V:32/float, R/binary>>) -> {float, V, R}; -parse_field_value(<<$l, V:64/signed, R/binary>>) -> {long, V, R}; -parse_field_value(<<$s, V:16/signed, R/binary>>) -> {short, V, R}; -parse_field_value(<<$t, V:8/unsigned, R/binary>>) -> {bool, (V /= 0), R}; +parse_array(<<$A, VLen:32/unsigned, Value:VLen/binary, Rest/binary>>) -> + [{array, parse_array(Value)} | parse_array(Rest)]; -parse_field_value(<<$x, VLen:32/unsigned, V:VLen/binary, R/binary>>) -> - {binary, V, R}; +parse_array(<<$x, VLen:32/unsigned, Value:VLen/binary, Rest/binary>>) -> + [{binary, Value} | parse_array(Rest)]; -parse_field_value(<<$V, R/binary>>) -> - {void, undefined, R}. +parse_array(<<$V, Rest/binary>>) -> + [{void, undefined} | parse_array(Rest)]. ensure_content_decoded(Content = #content{properties = Props}) when Props =/= none -> -- cgit v1.2.1 From e947a3923bfe20d99d43c9341370b137a33607b7 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Fri, 31 Oct 2014 13:09:18 +0000 Subject: Oops --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 1b66a306..c955a8fc 100644 --- a/Makefile +++ b/Makefile @@ -57,7 +57,7 @@ USE_PROPER_QC:=$(shell erl -noshell -eval 'io:format({module, proper} =:= code:e endif #other args: +native +"{hipe,[o3,verbose]}" -Ddebug=true +debug_info +no_strict_record_tests -ERLC_OPTS=-I $(INCLUDE_DIR) -Wall -v +debug_info +bin_opt_info $(call boolean_macro,$(USE_SPECS),use_specs) $(call boolean_macro,$(USE_PROPER_QC),use_proper_qc) +ERLC_OPTS=-I $(INCLUDE_DIR) -Wall -v +debug_info $(call boolean_macro,$(USE_SPECS),use_specs) $(call boolean_macro,$(USE_PROPER_QC),use_proper_qc) ifdef INSTRUMENT_FOR_QC ERLC_OPTS += -DINSTR_MOD=gm_qc -- cgit v1.2.1 From 4d7c548ddb5e8109fe18984e4dadb4f0cbb86078 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Mon, 3 Nov 2014 11:28:31 +0000 Subject: That shouldn't be a complete copy-paste of the parse_table/1 version. --- src/rabbit_binary_parser.erl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/rabbit_binary_parser.erl b/src/rabbit_binary_parser.erl index 8850d69e..6e277d35 100644 --- a/src/rabbit_binary_parser.erl +++ b/src/rabbit_binary_parser.erl @@ -93,9 +93,8 @@ parse_table(<>) -> [{RType, Value} | parse_table(Rest)]). -parse_array(<>) -> - [{NameString, longstr, Value} | parse_array(Rest)]; +parse_array(<<$S, VLen:32/unsigned, Value:VLen/binary, Rest/binary>>) -> + [{longstr, Value} | parse_array(Rest)]; ?SIMPLE_PARSE_ARRAY($I, Value:32/signed, signedint); ?SIMPLE_PARSE_ARRAY($T, Value:64/unsigned, timestamp); -- cgit v1.2.1 From 694943dc70c2466ee8d3cdcaf190081645a3b160 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Mon, 3 Nov 2014 13:19:43 +0000 Subject: ...and another stupid bug --- src/rabbit_binary_parser.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rabbit_binary_parser.erl b/src/rabbit_binary_parser.erl index 6e277d35..ee8147f4 100644 --- a/src/rabbit_binary_parser.erl +++ b/src/rabbit_binary_parser.erl @@ -91,7 +91,7 @@ parse_table(<>) -> - [{RType, Value} | parse_table(Rest)]). + [{RType, Value} | parse_array(Rest)]). parse_array(<<$S, VLen:32/unsigned, Value:VLen/binary, Rest/binary>>) -> [{longstr, Value} | parse_array(Rest)]; -- cgit v1.2.1 From c8c5c1504d8b39ceb956adf07f1cbba1147e6990 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 6 Nov 2014 11:39:25 +0000 Subject: Simple statistics about reads, writes and syncs through the FHC. --- src/file_handle_cache.erl | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/file_handle_cache.erl b/src/file_handle_cache.erl index 3a7a692c..cec4bccc 100644 --- a/src/file_handle_cache.erl +++ b/src/file_handle_cache.erl @@ -335,7 +335,7 @@ read(Ref, Count) -> fun ([#handle { is_read = false }]) -> {error, not_open_for_reading}; ([Handle = #handle { hdl = Hdl, offset = Offset }]) -> - case prim_file:read(Hdl, Count) of + case prim_file_read(Hdl, Count) of {ok, Data} = Obj -> Offset1 = Offset + iolist_size(Data), {Obj, [Handle #handle { offset = Offset1 }]}; @@ -355,7 +355,7 @@ append(Ref, Data) -> write_buffer_size_limit = 0, at_eof = true } = Handle1} -> Offset1 = Offset + iolist_size(Data), - {prim_file:write(Hdl, Data), + {prim_file_write(Hdl, Data), [Handle1 #handle { is_dirty = true, offset = Offset1 }]}; {{ok, _Offset}, #handle { write_buffer = WriteBuffer, write_buffer_size = Size, @@ -382,7 +382,7 @@ sync(Ref) -> ok; ([Handle = #handle { hdl = Hdl, is_dirty = true, write_buffer = [] }]) -> - case prim_file:sync(Hdl) of + case prim_file_sync(Hdl) of ok -> {ok, [Handle #handle { is_dirty = false }]}; Error -> {Error, [Handle]} end @@ -539,6 +539,17 @@ info(Items) -> gen_server2:call(?SERVER, {info, Items}, infinity). %% Internal functions %%---------------------------------------------------------------------------- +prim_file_read(Hdl, Size) -> + file_handle_cache_stats:update( + read, Size, fun() -> prim_file:read(Hdl, Size) end). + +prim_file_write(Hdl, Bytes) -> + file_handle_cache_stats:update( + write, iolist_size(Bytes), fun() -> prim_file:write(Hdl, Bytes) end). + +prim_file_sync(Hdl) -> + file_handle_cache_stats:update(sync, fun() -> prim_file:sync(Hdl) end). + is_reader(Mode) -> lists:member(read, Mode). is_writer(Mode) -> lists:member(write, Mode). @@ -742,7 +753,7 @@ soft_close(Handle) -> is_dirty = IsDirty, last_used_at = Then } = Handle1 } -> ok = case IsDirty of - true -> prim_file:sync(Hdl); + true -> prim_file_sync(Hdl); false -> ok end, ok = prim_file:close(Hdl), @@ -817,7 +828,7 @@ write_buffer(Handle = #handle { hdl = Hdl, offset = Offset, write_buffer = WriteBuffer, write_buffer_size = DataSize, at_eof = true }) -> - case prim_file:write(Hdl, lists:reverse(WriteBuffer)) of + case prim_file_write(Hdl, lists:reverse(WriteBuffer)) of ok -> Offset1 = Offset + DataSize, {ok, Handle #handle { offset = Offset1, is_dirty = true, @@ -843,6 +854,7 @@ used(#fhc_state{open_count = C1, %%---------------------------------------------------------------------------- init([AlarmSet, AlarmClear]) -> + file_handle_cache_stats:init(), Limit = case application:get_env(file_handles_high_watermark) of {ok, Watermark} when (is_integer(Watermark) andalso Watermark > 0) -> -- cgit v1.2.1 From 28274a6f2ed0a6b0a26e9ea0a80ef3a16e98274b Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 6 Nov 2014 12:49:44 +0000 Subject: Forgot to add that... --- src/file_handle_cache_stats.erl | 53 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 src/file_handle_cache_stats.erl diff --git a/src/file_handle_cache_stats.erl b/src/file_handle_cache_stats.erl new file mode 100644 index 00000000..c8af20a2 --- /dev/null +++ b/src/file_handle_cache_stats.erl @@ -0,0 +1,53 @@ +%% The contents of this file are subject to the Mozilla Public License +%% Version 1.1 (the "License"); you may not use this file except in +%% compliance with the License. You may obtain a copy of the License +%% at http://www.mozilla.org/MPL/ +%% +%% Software distributed under the License is distributed on an "AS IS" +%% basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See +%% the License for the specific language governing rights and +%% limitations under the License. +%% +%% The Original Code is RabbitMQ. +%% +%% The Initial Developer of the Original Code is GoPivotal, Inc. +%% Copyright (c) 2007-2014 GoPivotal, Inc. All rights reserved. +%% + +-module(file_handle_cache_stats). + +%% stats about read / write operations that go through the fhc. + +-export([init/0, update/3, update/2, get/0]). + +-define(TABLE, ?MODULE). +-define(MICRO_TO_MILLI, 1000). + +init() -> + ets:new(?TABLE, [public, named_table]), + [ets:insert(?TABLE, {{Op, Counter}, 0}) || Op <- [read, write], + Counter <- [count, bytes, time]], + [ets:insert(?TABLE, {{Op, Counter}, 0}) || Op <- [sync], + Counter <- [count, time]]. + +update(Op, Bytes, Thunk) -> + {Time, Res} = timer:tc(Thunk), + ets:update_counter(?TABLE, {Op, count}, 1), + ets:update_counter(?TABLE, {Op, bytes}, Bytes), + ets:update_counter(?TABLE, {Op, time}, Time), + Res. + +update(Op, Thunk) -> + {Time, Res} = timer:tc(Thunk), + ets:update_counter(?TABLE, {Op, count}, 1), + ets:update_counter(?TABLE, {Op, time}, Time), + Res. + +get() -> + lists:sort([output(K, V) || {K, V} <- ets:tab2list(?TABLE)]). + +output({Op, time}, Val) -> {flatten_key(Op, time), Val / ?MICRO_TO_MILLI}; +output({Op, Ctr}, Val) -> {flatten_key(Op, Ctr), Val}. + +flatten_key(A, B) -> + list_to_atom("fhc_" ++ atom_to_list(A) ++ "_" ++ atom_to_list(B)). -- cgit v1.2.1 From 5957db9e3d058335f70d0c80acf8e28f14176bfc Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 6 Nov 2014 13:51:48 +0000 Subject: Move this formatting up to the agent. --- src/file_handle_cache_stats.erl | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/file_handle_cache_stats.erl b/src/file_handle_cache_stats.erl index c8af20a2..d055d84a 100644 --- a/src/file_handle_cache_stats.erl +++ b/src/file_handle_cache_stats.erl @@ -21,7 +21,6 @@ -export([init/0, update/3, update/2, get/0]). -define(TABLE, ?MODULE). --define(MICRO_TO_MILLI, 1000). init() -> ets:new(?TABLE, [public, named_table]), @@ -44,10 +43,4 @@ update(Op, Thunk) -> Res. get() -> - lists:sort([output(K, V) || {K, V} <- ets:tab2list(?TABLE)]). - -output({Op, time}, Val) -> {flatten_key(Op, time), Val / ?MICRO_TO_MILLI}; -output({Op, Ctr}, Val) -> {flatten_key(Op, Ctr), Val}. - -flatten_key(A, B) -> - list_to_atom("fhc_" ++ atom_to_list(A) ++ "_" ++ atom_to_list(B)). + lists:sort(ets:tab2list(?TABLE)). -- cgit v1.2.1 From c2088028ff1cbf4a3d4d680f87054e9b5aecb345 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Mon, 10 Nov 2014 13:59:05 +0000 Subject: First pass at adding a read buffer to file_handle_cache. --- src/file_handle_cache.erl | 104 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 82 insertions(+), 22 deletions(-) diff --git a/src/file_handle_cache.erl b/src/file_handle_cache.erl index cec4bccc..126b3f81 100644 --- a/src/file_handle_cache.erl +++ b/src/file_handle_cache.erl @@ -178,6 +178,9 @@ write_buffer_size, write_buffer_size_limit, write_buffer, + read_buffer, + read_buffer_size, + read_buffer_size_limit, at_eof, path, mode, @@ -334,13 +337,42 @@ read(Ref, Count) -> [Ref], fun ([#handle { is_read = false }]) -> {error, not_open_for_reading}; - ([Handle = #handle { hdl = Hdl, offset = Offset }]) -> - case prim_file_read(Hdl, Count) of - {ok, Data} = Obj -> Offset1 = Offset + iolist_size(Data), - {Obj, - [Handle #handle { offset = Offset1 }]}; - eof -> {eof, [Handle #handle { at_eof = true }]}; - Error -> {Error, [Handle]} + ([Handle = #handle{read_buffer = Buf, + read_buffer_size = BufSz, + offset = Offset}]) when BufSz >= Count -> + <> = Buf, + {{ok, Hd}, [Handle#handle{offset = Offset + Count, + read_buffer = Tl, + read_buffer_size = BufSz - Count}]}; + ([Handle = #handle{read_buffer = Buf, + read_buffer_size = BufSz, + read_buffer_size_limit = Limit, + hdl = Hdl, + offset = Offset}]) -> + WantedCount = Count - BufSz, + case prim_file_read(Hdl, Limit) of + {ok, Data} -> + ReadCount = size(Data), + case ReadCount < WantedCount of + true -> + OffSet1 = Offset + BufSz + ReadCount, + {{ok, <>}, + [Handle#handle{offset = OffSet1, + read_buffer = <<>>, + read_buffer_size = 0}]}; + false -> + <> = Data, + OffSet1 = Offset + BufSz + WantedCount, + BufSz1 = ReadCount - WantedCount, + {{ok, <>}, + [Handle#handle{offset = OffSet1, + read_buffer = Tl, + read_buffer_size = BufSz1}]} + end; + eof -> + {eof, [Handle #handle { at_eof = true }]}; + Error -> %% TODO correct or change handle? + {Error, [Handle]} end end). @@ -465,8 +497,10 @@ clear(Ref) -> fun ([#handle { at_eof = true, write_buffer_size = 0, offset = 0 }]) -> ok; ([Handle]) -> - case maybe_seek(bof, Handle #handle { write_buffer = [], - write_buffer_size = 0 }) of + case maybe_seek(bof, Handle #handle { write_buffer = [], + write_buffer_size = 0, + read_buffer = <<>>, + read_buffer_size = 0}) of {{ok, 0}, Handle1 = #handle { hdl = Hdl }} -> case prim_file:truncate(Hdl) of ok -> {ok, [Handle1 #handle { at_eof = true }]}; @@ -633,9 +667,11 @@ reopen([{Ref, NewOrReopen, Handle = #handle { hdl = closed, {ok, Hdl} -> Now = now(), {{ok, _Offset}, Handle1} = - maybe_seek(Offset, Handle #handle { hdl = Hdl, - offset = 0, - last_used_at = Now }), + maybe_seek(Offset, Handle #handle { hdl = Hdl, + offset = 0, + read_buffer = <<>>, + read_buffer_size = 0, + last_used_at = Now }), put({Ref, fhc_handle}, Handle1), reopen(RefNewOrReopenHdls, gb_trees:insert(Now, Ref, Tree), [{Ref, Handle1} | RefHdls]); @@ -727,6 +763,9 @@ new_closed_handle(Path, Mode, Options) -> write_buffer_size = 0, write_buffer_size_limit = WriteBufferSize, write_buffer = [], + read_buffer_size = 0, + read_buffer_size_limit = 1000000, %% TODO + read_buffer = <<>>, at_eof = false, path = Path, mode = Mode, @@ -787,17 +826,38 @@ hard_close(Handle) -> Result end. -maybe_seek(NewOffset, Handle = #handle { hdl = Hdl, offset = Offset, - at_eof = AtEoF }) -> +maybe_seek(NewOffset, Handle = #handle{hdl = Hdl, + offset = Offset, + read_buffer = Buf, + read_buffer_size = BufSz, + at_eof = AtEoF}) -> {AtEoF1, NeedsSeek} = needs_seek(AtEoF, Offset, NewOffset), - case (case NeedsSeek of - true -> prim_file:position(Hdl, NewOffset); - false -> {ok, Offset} - end) of - {ok, Offset1} = Result -> - {Result, Handle #handle { offset = Offset1, at_eof = AtEoF1 }}; - {error, _} = Error -> - {Error, Handle} + case NeedsSeek of + true -> + case not is_number(NewOffset) orelse + NewOffset < Offset orelse + NewOffset > BufSz + Offset of + true -> + case prim_file:position(Hdl, NewOffset) of + {ok, Offset1} = Result -> + {Result, Handle#handle{offset = Offset1, + at_eof = AtEoF1, + read_buffer = <<>>, + read_buffer_size = 0}}; + {error, _} = Error -> + {Error, Handle} + end; + false -> + Diff = NewOffset - Offset, + <<_:Diff/binary, Rest/binary>> = Buf, + {{ok, NewOffset}, + Handle#handle{offset = NewOffset, + at_eof = AtEoF1, + read_buffer = Rest, + read_buffer_size = BufSz - Diff}} + end; + false -> + {{ok, Offset}, Handle} end. needs_seek( AtEoF, _CurOffset, cur ) -> {AtEoF, false}; -- cgit v1.2.1 From 0e338d6fc8638f8eadd594ec72c1e766f3350958 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Tue, 11 Nov 2014 16:46:20 +0000 Subject: Also generate stats on seek. --- src/file_handle_cache.erl | 6 +++++- src/file_handle_cache_stats.erl | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/file_handle_cache.erl b/src/file_handle_cache.erl index cec4bccc..3364c3ae 100644 --- a/src/file_handle_cache.erl +++ b/src/file_handle_cache.erl @@ -550,6 +550,10 @@ prim_file_write(Hdl, Bytes) -> prim_file_sync(Hdl) -> file_handle_cache_stats:update(sync, fun() -> prim_file:sync(Hdl) end). +prim_file_position(Hdl, NewOffset) -> + file_handle_cache_stats:update( + seek, fun() -> prim_file:position(Hdl, NewOffset) end). + is_reader(Mode) -> lists:member(read, Mode). is_writer(Mode) -> lists:member(write, Mode). @@ -791,7 +795,7 @@ maybe_seek(NewOffset, Handle = #handle { hdl = Hdl, offset = Offset, at_eof = AtEoF }) -> {AtEoF1, NeedsSeek} = needs_seek(AtEoF, Offset, NewOffset), case (case NeedsSeek of - true -> prim_file:position(Hdl, NewOffset); + true -> prim_file_position(Hdl, NewOffset); false -> {ok, Offset} end) of {ok, Offset1} = Result -> diff --git a/src/file_handle_cache_stats.erl b/src/file_handle_cache_stats.erl index d055d84a..5a8d7b29 100644 --- a/src/file_handle_cache_stats.erl +++ b/src/file_handle_cache_stats.erl @@ -26,7 +26,7 @@ init() -> ets:new(?TABLE, [public, named_table]), [ets:insert(?TABLE, {{Op, Counter}, 0}) || Op <- [read, write], Counter <- [count, bytes, time]], - [ets:insert(?TABLE, {{Op, Counter}, 0}) || Op <- [sync], + [ets:insert(?TABLE, {{Op, Counter}, 0}) || Op <- [sync, seek], Counter <- [count, time]]. update(Op, Bytes, Thunk) -> -- cgit v1.2.1 From 54483ee88d6516caa56ced399e5a8cea7a72a1e3 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Tue, 11 Nov 2014 17:06:42 +0000 Subject: Emit stats on FHC recycling. --- src/file_handle_cache.erl | 12 +++++++----- src/file_handle_cache_stats.erl | 10 ++++++++-- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/file_handle_cache.erl b/src/file_handle_cache.erl index 3364c3ae..65f3f45b 100644 --- a/src/file_handle_cache.erl +++ b/src/file_handle_cache.erl @@ -626,14 +626,16 @@ reopen([], Tree, RefHdls) -> {ok, lists:reverse(RefHdls)}; reopen([{Ref, NewOrReopen, Handle = #handle { hdl = closed, path = Path, - mode = Mode, + mode = Mode0, offset = Offset, last_used_at = undefined }} | RefNewOrReopenHdls] = ToOpen, Tree, RefHdls) -> - case prim_file:open(Path, case NewOrReopen of - new -> Mode; - reopen -> [read | Mode] - end) of + Mode = case NewOrReopen of + new -> Mode0; + reopen -> file_handle_cache_stats:update(reopen), + [read | Mode0] + end, + case prim_file:open(Path, Mode) of {ok, Hdl} -> Now = now(), {{ok, _Offset}, Handle1} = diff --git a/src/file_handle_cache_stats.erl b/src/file_handle_cache_stats.erl index 5a8d7b29..b1fbb3f4 100644 --- a/src/file_handle_cache_stats.erl +++ b/src/file_handle_cache_stats.erl @@ -18,7 +18,7 @@ %% stats about read / write operations that go through the fhc. --export([init/0, update/3, update/2, get/0]). +-export([init/0, update/3, update/2, update/1, get/0]). -define(TABLE, ?MODULE). @@ -27,7 +27,9 @@ init() -> [ets:insert(?TABLE, {{Op, Counter}, 0}) || Op <- [read, write], Counter <- [count, bytes, time]], [ets:insert(?TABLE, {{Op, Counter}, 0}) || Op <- [sync, seek], - Counter <- [count, time]]. + Counter <- [count, time]], + [ets:insert(?TABLE, {{Op, Counter}, 0}) || Op <- [reopen], + Counter <- [count]]. update(Op, Bytes, Thunk) -> {Time, Res} = timer:tc(Thunk), @@ -42,5 +44,9 @@ update(Op, Thunk) -> ets:update_counter(?TABLE, {Op, time}, Time), Res. +update(Op) -> + ets:update_counter(?TABLE, {Op, count}, 1), + ok. + get() -> lists:sort(ets:tab2list(?TABLE)). -- cgit v1.2.1 From cab3f90898beac09f77d75219800f84b52f16c05 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 13 Nov 2014 09:22:02 +0000 Subject: R13B03 compatibility. --- src/file_handle_cache_stats.erl | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/file_handle_cache_stats.erl b/src/file_handle_cache_stats.erl index b1fbb3f4..832f0b3d 100644 --- a/src/file_handle_cache_stats.erl +++ b/src/file_handle_cache_stats.erl @@ -32,14 +32,14 @@ init() -> Counter <- [count]]. update(Op, Bytes, Thunk) -> - {Time, Res} = timer:tc(Thunk), + {Time, Res} = timer_tc(Thunk), ets:update_counter(?TABLE, {Op, count}, 1), ets:update_counter(?TABLE, {Op, bytes}, Bytes), ets:update_counter(?TABLE, {Op, time}, Time), Res. update(Op, Thunk) -> - {Time, Res} = timer:tc(Thunk), + {Time, Res} = timer_tc(Thunk), ets:update_counter(?TABLE, {Op, count}, 1), ets:update_counter(?TABLE, {Op, time}, Time), Res. @@ -50,3 +50,11 @@ update(Op) -> get() -> lists:sort(ets:tab2list(?TABLE)). + +%% TODO timer:tc/1 was introduced in R14B03; use that function once we +%% require that version. +timer_tc(Thunk) -> + T1 = os:timestamp(), + Res = Thunk(), + T2 = os:timestamp(), + {timer:now_diff(T2, T1), Res}. -- cgit v1.2.1 From 16b013458097e1a9a7d20f0138c24f705a8251f3 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 13 Nov 2014 10:20:27 +0000 Subject: If they ask to read more than the buffer size, do so. --- src/file_handle_cache.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/file_handle_cache.erl b/src/file_handle_cache.erl index 519c596d..a7d5ce15 100644 --- a/src/file_handle_cache.erl +++ b/src/file_handle_cache.erl @@ -346,11 +346,11 @@ read(Ref, Count) -> read_buffer_size = BufSz - Count}]}; ([Handle = #handle{read_buffer = Buf, read_buffer_size = BufSz, - read_buffer_size_limit = Limit, + read_buffer_size_limit = BufSzLimit, hdl = Hdl, offset = Offset}]) -> WantedCount = Count - BufSz, - case prim_file_read(Hdl, Limit) of + case prim_file_read(Hdl, lists:max([BufSzLimit, WantedCount])) of {ok, Data} -> ReadCount = size(Data), case ReadCount < WantedCount of -- cgit v1.2.1 From b573d47f639fdc8f8b5ff869252543b00b1a8fda Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 13 Nov 2014 10:56:33 +0000 Subject: Be a bit more systematic about reseting the read buffer. --- src/file_handle_cache.erl | 51 ++++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/src/file_handle_cache.erl b/src/file_handle_cache.erl index a7d5ce15..fa896c67 100644 --- a/src/file_handle_cache.erl +++ b/src/file_handle_cache.erl @@ -334,7 +334,7 @@ close(Ref) -> read(Ref, Count) -> with_flushed_handles( - [Ref], + [Ref], keep, fun ([#handle { is_read = false }]) -> {error, not_open_for_reading}; ([Handle = #handle{read_buffer = Buf, @@ -357,9 +357,8 @@ read(Ref, Count) -> true -> OffSet1 = Offset + BufSz + ReadCount, {{ok, <>}, - [Handle#handle{offset = OffSet1, - read_buffer = <<>>, - read_buffer_size = 0}]}; + [reset_read_buffer( + Handle#handle{offset = OffSet1})]}; false -> <> = Data, OffSet1 = Offset + BufSz + WantedCount, @@ -409,7 +408,7 @@ append(Ref, Data) -> sync(Ref) -> with_flushed_handles( - [Ref], + [Ref], keep, fun ([#handle { is_dirty = false, write_buffer = [] }]) -> ok; ([Handle = #handle { hdl = Hdl, @@ -429,7 +428,7 @@ needs_sync(Ref) -> position(Ref, NewOffset) -> with_flushed_handles( - [Ref], + [Ref], keep, fun ([Handle]) -> {Result, Handle1} = maybe_seek(NewOffset, Handle), {Result, [Handle1]} end). @@ -497,10 +496,8 @@ clear(Ref) -> fun ([#handle { at_eof = true, write_buffer_size = 0, offset = 0 }]) -> ok; ([Handle]) -> - case maybe_seek(bof, Handle #handle { write_buffer = [], - write_buffer_size = 0, - read_buffer = <<>>, - read_buffer_size = 0}) of + case maybe_seek(bof, Handle#handle{write_buffer = [], + write_buffer_size = 0}) of {{ok, 0}, Handle1 = #handle { hdl = Hdl }} -> case prim_file:truncate(Hdl) of ok -> {ok, [Handle1 #handle { at_eof = true }]}; @@ -599,8 +596,15 @@ append_to_write(Mode) -> end. with_handles(Refs, Fun) -> + with_handles(Refs, reset, Fun). + +with_handles(Refs, ReadBuffer, Fun) -> case get_or_reopen([{Ref, reopen} || Ref <- Refs]) of - {ok, Handles} -> + {ok, Handles0} -> + Handles = case ReadBuffer of + reset -> [reset_read_buffer(H) || H <- Handles0]; + keep -> Handles0 + end, case Fun(Handles) of {Result, Handles1} when is_list(Handles1) -> lists:zipwith(fun put_handle/2, Refs, Handles1), @@ -613,8 +617,11 @@ with_handles(Refs, Fun) -> end. with_flushed_handles(Refs, Fun) -> + with_flushed_handles(Refs, reset, Fun). + +with_flushed_handles(Refs, ReadBuffer, Fun) -> with_handles( - Refs, + Refs, ReadBuffer, fun (Handles) -> case lists:foldl( fun (Handle, {ok, HandlesAcc}) -> @@ -673,11 +680,10 @@ reopen([{Ref, NewOrReopen, Handle = #handle { hdl = closed, {ok, Hdl} -> Now = now(), {{ok, _Offset}, Handle1} = - maybe_seek(Offset, Handle #handle { hdl = Hdl, - offset = 0, - read_buffer = <<>>, - read_buffer_size = 0, - last_used_at = Now }), + maybe_seek(Offset, reset_read_buffer( + Handle#handle{hdl = Hdl, + offset = 0, + last_used_at = Now})), put({Ref, fhc_handle}, Handle1), reopen(RefNewOrReopenHdls, gb_trees:insert(Now, Ref, Tree), [{Ref, Handle1} | RefHdls]); @@ -846,10 +852,9 @@ maybe_seek(NewOffset, Handle = #handle{hdl = Hdl, true -> case prim_file_position(Hdl, NewOffset) of {ok, Offset1} = Result -> - {Result, Handle#handle{offset = Offset1, - at_eof = AtEoF1, - read_buffer = <<>>, - read_buffer_size = 0}}; + {Result, reset_read_buffer( + Handle#handle{offset = Offset1, + at_eof = AtEoF1})}; {error, _} = Error -> {Error, Handle} end; @@ -903,6 +908,10 @@ write_buffer(Handle = #handle { hdl = Hdl, offset = Offset, {Error, Handle} end. +reset_read_buffer(Handle) -> + Handle#handle{read_buffer = <<>>, + read_buffer_size = 0}. + infos(Items, State) -> [{Item, i(Item, State)} || Item <- Items]. i(total_limit, #fhc_state{limit = Limit}) -> Limit; -- cgit v1.2.1 From 246e224e0fd4d49de6cccb9f76f365a729da0c61 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 13 Nov 2014 11:10:14 +0000 Subject: Small refactor suggested by Matthias. --- src/file_handle_cache.erl | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/file_handle_cache.erl b/src/file_handle_cache.erl index fa896c67..06a72aa4 100644 --- a/src/file_handle_cache.erl +++ b/src/file_handle_cache.erl @@ -845,27 +845,21 @@ maybe_seek(NewOffset, Handle = #handle{hdl = Hdl, at_eof = AtEoF}) -> {AtEoF1, NeedsSeek} = needs_seek(AtEoF, Offset, NewOffset), case NeedsSeek of + true when is_number(NewOffset) andalso + NewOffset >= Offset andalso NewOffset =< BufSz + Offset -> + Diff = NewOffset - Offset, + <<_:Diff/binary, Rest/binary>> = Buf, + {{ok, NewOffset}, Handle#handle{offset = NewOffset, + at_eof = AtEoF1, + read_buffer = Rest, + read_buffer_size = BufSz - Diff}}; true -> - case not is_number(NewOffset) orelse - NewOffset < Offset orelse - NewOffset > BufSz + Offset of - true -> - case prim_file_position(Hdl, NewOffset) of - {ok, Offset1} = Result -> - {Result, reset_read_buffer( - Handle#handle{offset = Offset1, - at_eof = AtEoF1})}; - {error, _} = Error -> - {Error, Handle} - end; - false -> - Diff = NewOffset - Offset, - <<_:Diff/binary, Rest/binary>> = Buf, - {{ok, NewOffset}, - Handle#handle{offset = NewOffset, - at_eof = AtEoF1, - read_buffer = Rest, - read_buffer_size = BufSz - Diff}} + case prim_file_position(Hdl, NewOffset) of + {ok, Offset1} = Result -> + {Result, reset_read_buffer(Handle#handle{offset = Offset1, + at_eof = AtEoF1})}; + {error, _} = Error -> + {Error, Handle} end; false -> {{ok, Offset}, Handle} -- cgit v1.2.1 From ae2b78b00a2c6844dfe0e0d68136ba104de359b2 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 13 Nov 2014 11:57:32 +0000 Subject: Remove a couple of TODOs, make the read buffer size configurable, and don't use the read buffer for the QI or msg store transform since they already read in decent sized chunks. --- src/file_handle_cache.erl | 14 ++++++++++---- src/rabbit_msg_store.erl | 3 ++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/file_handle_cache.erl b/src/file_handle_cache.erl index 06a72aa4..2922e146 100644 --- a/src/file_handle_cache.erl +++ b/src/file_handle_cache.erl @@ -240,7 +240,8 @@ -spec(register_callback/3 :: (atom(), atom(), [any()]) -> 'ok'). -spec(open/3 :: (file:filename(), [any()], - [{'write_buffer', (non_neg_integer() | 'infinity' | 'unbuffered')}]) + [{'write_buffer', (non_neg_integer() | 'infinity' | 'unbuffered')} | + {'read_buffer', (non_neg_integer() | 'unbuffered')}]) -> val_or_error(ref())). -spec(close/1 :: (ref()) -> ok_or_error()). -spec(read/2 :: (ref(), non_neg_integer()) -> @@ -370,8 +371,8 @@ read(Ref, Count) -> end; eof -> {eof, [Handle #handle { at_eof = true }]}; - Error -> %% TODO correct or change handle? - {Error, [Handle]} + Error -> + {Error, [reset_read_buffer(Handle)]} end end). @@ -768,6 +769,11 @@ new_closed_handle(Path, Mode, Options) -> infinity -> infinity; N when is_integer(N) -> N end, + ReadBufferSize = + case proplists:get_value(read_buffer, Options, unbuffered) of + unbuffered -> 0; + N2 when is_integer(N2) -> N2 + end, Ref = make_ref(), put({Ref, fhc_handle}, #handle { hdl = closed, offset = 0, @@ -776,7 +782,7 @@ new_closed_handle(Path, Mode, Options) -> write_buffer_size_limit = WriteBufferSize, write_buffer = [], read_buffer_size = 0, - read_buffer_size_limit = 1000000, %% TODO + read_buffer_size_limit = ReadBufferSize, read_buffer = <<>>, at_eof = false, path = Path, diff --git a/src/rabbit_msg_store.erl b/src/rabbit_msg_store.erl index b829ae94..6c80ddcd 100644 --- a/src/rabbit_msg_store.erl +++ b/src/rabbit_msg_store.erl @@ -1299,7 +1299,8 @@ should_mask_action(CRef, MsgId, open_file(Dir, FileName, Mode) -> file_handle_cache:open(form_filename(Dir, FileName), ?BINARY_MODE ++ Mode, - [{write_buffer, ?HANDLE_CACHE_BUFFER_SIZE}]). + [{write_buffer, ?HANDLE_CACHE_BUFFER_SIZE}, + {read_buffer, ?HANDLE_CACHE_BUFFER_SIZE}]). close_handle(Key, CState = #client_msstate { file_handle_cache = FHC }) -> CState #client_msstate { file_handle_cache = close_handle(Key, FHC) }; -- cgit v1.2.1 From 5c02c826bf447c07cd6ecb7bb1070b9c5ea59b7f Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 13 Nov 2014 13:01:35 +0000 Subject: Import changes from https://github.com/gotthardp/rabbitmq-server/tree/multi_authorization --- include/rabbit.hrl | 4 +- src/rabbit_access_control.erl | 95 +++++++++++++++++++++++------------- src/rabbit_auth_backend.erl | 11 +++-- src/rabbit_auth_backend_dummy.erl | 14 +++--- src/rabbit_auth_backend_internal.erl | 13 +++-- src/rabbit_channel.erl | 2 +- src/rabbit_direct.erl | 2 +- src/rabbit_reader.erl | 2 +- src/rabbit_types.erl | 8 +-- test/src/rabbit_tests.erl | 11 +++-- 10 files changed, 96 insertions(+), 66 deletions(-) diff --git a/include/rabbit.hrl b/include/rabbit.hrl index 74e165cd..627d0479 100644 --- a/include/rabbit.hrl +++ b/include/rabbit.hrl @@ -16,8 +16,8 @@ -record(user, {username, tags, - auth_backend, %% Module this user came from - impl %% Scratch space for that module + authN_backend, %% Authentication module this user came from + authZ_backends %% List of authorization modules }). -record(internal_user, {username, password_hash, tags}). diff --git a/src/rabbit_access_control.erl b/src/rabbit_access_control.erl index b0a9c0d8..dcec0ff5 100644 --- a/src/rabbit_access_control.erl +++ b/src/rabbit_access_control.erl @@ -19,7 +19,7 @@ -include("rabbit.hrl"). -export([check_user_pass_login/2, check_user_login/2, check_user_loopback/2, - check_vhost_access/2, check_resource_access/3]). + check_vhost_access/3, check_resource_access/3]). %%---------------------------------------------------------------------------- @@ -38,8 +38,8 @@ -spec(check_user_loopback/2 :: (rabbit_types:username(), rabbit_net:socket() | inet:ip_address()) -> 'ok' | 'not_allowed'). --spec(check_vhost_access/2 :: - (rabbit_types:user(), rabbit_types:vhost()) +-spec(check_vhost_access/3 :: + (rabbit_types:user(), rabbit_types:vhost(), rabbit_net:socket()) -> 'ok' | rabbit_types:channel_exit()). -spec(check_resource_access/3 :: (rabbit_types:user(), rabbit_types:r(atom()), permission_atom()) @@ -58,33 +58,52 @@ check_user_login(Username, AuthProps) -> fun ({ModN, ModZ}, {refused, _, _}) -> %% Different modules for authN vs authZ. So authenticate %% with authN module, then if that succeeds do - %% passwordless (i.e pre-authenticated) login with authZ - %% module, and use the #user{} the latter gives us. - case try_login(ModN, Username, AuthProps) of - {ok, _} -> try_login(ModZ, Username, []); + %% passwordless (i.e pre-authenticated) login with authZ. + case try_authenticate(ModN, Username, AuthProps) of + {ok, User, _AuthZ} -> try_authorize(ModZ, User, []); Else -> Else end; (Mod, {refused, _, _}) -> %% Same module for authN and authZ. Just take the result %% it gives us - try_login(Mod, Username, AuthProps); - (_, {ok, User}) -> + try_authenticate(Mod, Username, AuthProps); + (_, {ok, User, AuthZ}) -> %% We've successfully authenticated. Skip to the end... - {ok, User} + {ok, User, AuthZ} end, {refused, "No modules checked '~s'", [Username]}, Modules), - rabbit_event:notify(case R of - {ok, _User} -> user_authentication_success; - _ -> user_authentication_failure - end, [{name, Username}]), - R. -try_login(Module, Username, AuthProps) -> + case R of + {ok, RUser, RAuthZ} -> + rabbit_event:notify(user_authentication_success, [{name, Username}]), + %% Store the list of authorization backends + {ok, RUser#user{authZ_backends=RAuthZ}}; + _ -> + rabbit_event:notify(user_authentication_failure, [{name, Username}]), + R + end. + +try_authenticate(Module, Username, AuthProps) -> case Module:check_user_login(Username, AuthProps) of + {ok, User, AuthZ} -> {ok, User, [{Module, AuthZ}]}; {error, E} -> {refused, "~s failed authenticating ~s: ~p~n", [Module, Username, E]}; Else -> Else end. +try_authorize(Modules, User, AuthZList) when is_list(Modules) -> + lists:foldr( + fun (Module, {ok, _User, AuthZList2}) -> try_authorize(Module, User, AuthZList2); + (_, {refused, _, _} = Error) -> Error + end, {ok, User, AuthZList}, Modules); + +try_authorize(Module, User = #user{username = Username}, AuthZList) -> + case Module:check_user_login(Username, []) of + {ok, _User, AuthZ} -> {ok, User, [{Module, AuthZ}|AuthZList]}; + {error, E} -> {refused, "~s failed authorizing ~s: ~p~n", + [Module, Username, E]}; + Else -> Else + end. + check_user_loopback(Username, SockOrAddr) -> {ok, Users} = application:get_env(rabbit, loopback_users), case rabbit_net:is_loopback(SockOrAddr) @@ -93,29 +112,39 @@ check_user_loopback(Username, SockOrAddr) -> false -> not_allowed end. -check_vhost_access(User = #user{ username = Username, - auth_backend = Module }, VHostPath) -> - check_access( - fun() -> - %% TODO this could be an andalso shortcut under >R13A - case rabbit_vhost:exists(VHostPath) of - false -> false; - true -> Module:check_vhost_access(User, VHostPath) - end - end, - Module, "access to vhost '~s' refused for user '~s'", - [VHostPath, Username]). +check_vhost_access(User = #user{ username = Username, + authZ_backends = Modules }, VHostPath, Sock) -> + lists:foldl( + fun({Module, Impl}, ok) -> + check_access( + fun() -> + %% TODO this could be an andalso shortcut under >R13A + case rabbit_vhost:exists(VHostPath) of + false -> false; + true -> Module:check_vhost_access(User, Impl, VHostPath, Sock) + end + end, + Module, "access to vhost '~s' refused for user '~s'", + [VHostPath, Username]); + + (_, Else) -> Else + end, ok, Modules). check_resource_access(User, R = #resource{kind = exchange, name = <<"">>}, Permission) -> check_resource_access(User, R#resource{name = <<"amq.default">>}, Permission); -check_resource_access(User = #user{username = Username, auth_backend = Module}, +check_resource_access(User = #user{username = Username, authZ_backends = Modules}, Resource, Permission) -> - check_access( - fun() -> Module:check_resource_access(User, Resource, Permission) end, - Module, "access to ~s refused for user '~s'", - [rabbit_misc:rs(Resource), Username]). + lists:foldl( + fun({Module, Impl}, ok) -> + check_access( + fun() -> Module:check_resource_access(User, Impl, Resource, Permission) end, + Module, "access to ~s refused for user '~s'", + [rabbit_misc:rs(Resource), Username]); + + (_, Else) -> Else + end, ok, Modules). check_access(Fun, Module, ErrStr, ErrArgs) -> Allow = case Fun() of diff --git a/src/rabbit_auth_backend.erl b/src/rabbit_auth_backend.erl index a7dd6494..99f291f1 100644 --- a/src/rabbit_auth_backend.erl +++ b/src/rabbit_auth_backend.erl @@ -33,7 +33,7 @@ %% {refused, Msg, Args} %% Client failed authentication. Log and die. -callback check_user_login(rabbit_types:username(), [term()]) -> - {'ok', rabbit_types:user()} | + {'ok', rabbit_types:user(), any()} | {'refused', string(), [any()]} | {'error', any()}. @@ -43,7 +43,8 @@ %% false %% {error, Error} %% Something went wrong. Log and die. --callback check_vhost_access(rabbit_types:user(), rabbit_types:vhost()) -> +-callback check_vhost_access(rabbit_types:user(), any(), + rabbit_types:vhost(), rabbit_net:socket()) -> boolean() | {'error', any()}. @@ -54,7 +55,7 @@ %% false %% {error, Error} %% Something went wrong. Log and die. --callback check_resource_access(rabbit_types:user(), +-callback check_resource_access(rabbit_types:user(), any(), rabbit_types:r(atom()), rabbit_access_control:permission_atom()) -> boolean() | {'error', any()}. @@ -64,8 +65,8 @@ -export([behaviour_info/1]). behaviour_info(callbacks) -> - [{description, 0}, {check_user_login, 2}, {check_vhost_access, 2}, - {check_resource_access, 3}]; + [{description, 0}, {check_user_login, 2}, {check_vhost_access, 4}, + {check_resource_access, 4}]; behaviour_info(_Other) -> undefined. diff --git a/src/rabbit_auth_backend_dummy.erl b/src/rabbit_auth_backend_dummy.erl index 5daca368..8e2d8269 100644 --- a/src/rabbit_auth_backend_dummy.erl +++ b/src/rabbit_auth_backend_dummy.erl @@ -21,7 +21,7 @@ -export([description/0]). -export([user/0]). --export([check_user_login/2, check_vhost_access/2, check_resource_access/3]). +-export([check_user_login/2, check_vhost_access/4, check_resource_access/4]). -ifdef(use_specs). @@ -31,10 +31,10 @@ %% A user to be used by the direct client when permission checks are %% not needed. This user can do anything AMQPish. -user() -> #user{username = <<"none">>, - tags = [], - auth_backend = ?MODULE, - impl = none}. +user() -> #user{username = <<"none">>, + tags = [], + authN_backend = ?MODULE, + authZ_backends = []}. %% Implementation of rabbit_auth_backend @@ -45,5 +45,5 @@ description() -> check_user_login(_, _) -> {refused, "cannot log in conventionally as dummy user", []}. -check_vhost_access(#user{}, _VHostPath) -> true. -check_resource_access(#user{}, #resource{}, _Permission) -> true. +check_vhost_access(#user{}, _Impl, _VHostPath, _Sock) -> true. +check_resource_access(#user{}, _Impl, #resource{}, _Permission) -> true. diff --git a/src/rabbit_auth_backend_internal.erl b/src/rabbit_auth_backend_internal.erl index fd1c4e8e..5cdff985 100644 --- a/src/rabbit_auth_backend_internal.erl +++ b/src/rabbit_auth_backend_internal.erl @@ -20,7 +20,7 @@ -behaviour(rabbit_auth_backend). -export([description/0]). --export([check_user_login/2, check_vhost_access/2, check_resource_access/3]). +-export([check_user_login/2, check_vhost_access/4, check_resource_access/4]). -export([add_user/2, delete_user/1, lookup_user/1, change_password/2, clear_password/1, @@ -98,17 +98,16 @@ internal_check_user_login(Username, Fun) -> case lookup_user(Username) of {ok, User = #internal_user{tags = Tags}} -> case Fun(User) of - true -> {ok, #user{username = Username, - tags = Tags, - auth_backend = ?MODULE, - impl = User}}; + true -> {ok, #user{username = Username, + tags = Tags, + authN_backend = ?MODULE}, User}; _ -> Refused end; {error, not_found} -> Refused end. -check_vhost_access(#user{username = Username}, VHostPath) -> +check_vhost_access(#user{username = Username}, _Impl, VHostPath, _Sock) -> case mnesia:dirty_read({rabbit_user_permission, #user_vhost{username = Username, virtual_host = VHostPath}}) of @@ -116,7 +115,7 @@ check_vhost_access(#user{username = Username}, VHostPath) -> [_R] -> true end. -check_resource_access(#user{username = Username}, +check_resource_access(#user{username = Username}, _Impl, #resource{virtual_host = VHostPath, name = Name}, Permission) -> case mnesia:dirty_read({rabbit_user_permission, diff --git a/src/rabbit_channel.erl b/src/rabbit_channel.erl index 8632e1b3..2f7e234d 100644 --- a/src/rabbit_channel.erl +++ b/src/rabbit_channel.erl @@ -581,7 +581,7 @@ check_user_id_header(#'P_basic'{user_id = Username}, #ch{user = #user{username = Username}}) -> ok; check_user_id_header( - #'P_basic'{}, #ch{user = #user{auth_backend = rabbit_auth_backend_dummy}}) -> + #'P_basic'{}, #ch{user = #user{authN_backend = rabbit_auth_backend_dummy}}) -> ok; check_user_id_header(#'P_basic'{user_id = Claimed}, #ch{user = #user{username = Actual, diff --git a/src/rabbit_direct.erl b/src/rabbit_direct.erl index 749a67b1..f6140f09 100644 --- a/src/rabbit_direct.erl +++ b/src/rabbit_direct.erl @@ -92,7 +92,7 @@ connect0(AuthFun, VHost, Protocol, Pid, Infos) -> end. connect1(User, VHost, Protocol, Pid, Infos) -> - try rabbit_access_control:check_vhost_access(User, VHost) of + try rabbit_access_control:check_vhost_access(User, VHost, undefined) of ok -> ok = pg_local:join(rabbit_direct, Pid), rabbit_event:notify(connection_created, Infos), {ok, {User, rabbit_reader:server_properties(Protocol)}} diff --git a/src/rabbit_reader.erl b/src/rabbit_reader.erl index ca73006a..2033dd14 100644 --- a/src/rabbit_reader.erl +++ b/src/rabbit_reader.erl @@ -944,7 +944,7 @@ handle_method0(#'connection.open'{virtual_host = VHostPath}, helper_sup = SupPid, sock = Sock, throttle = Throttle}) -> - ok = rabbit_access_control:check_vhost_access(User, VHostPath), + ok = rabbit_access_control:check_vhost_access(User, VHostPath, Sock), NewConnection = Connection#connection{vhost = VHostPath}, ok = send_on_channel0(Sock, #'connection.open_ok'{}, Protocol), Conserve = rabbit_alarm:register(self(), {?MODULE, conserve_resources, []}), diff --git a/src/rabbit_types.erl b/src/rabbit_types.erl index ba48867a..b3158cc8 100644 --- a/src/rabbit_types.erl +++ b/src/rabbit_types.erl @@ -132,10 +132,10 @@ -type(protocol() :: rabbit_framing:protocol()). -type(user() :: - #user{username :: username(), - tags :: [atom()], - auth_backend :: atom(), - impl :: any()}). + #user{username :: username(), + tags :: [atom()], + authN_backend :: atom(), + authZ_backends :: [{atom(), any()}]}). -type(internal_user() :: #internal_user{username :: username(), diff --git a/test/src/rabbit_tests.erl b/test/src/rabbit_tests.erl index ef6b756b..a227f3d2 100644 --- a/test/src/rabbit_tests.erl +++ b/test/src/rabbit_tests.erl @@ -1292,11 +1292,12 @@ test_spawn_remote() -> end. user(Username) -> - #user{username = Username, - tags = [administrator], - auth_backend = rabbit_auth_backend_internal, - impl = #internal_user{username = Username, - tags = [administrator]}}. + #user{username = Username, + tags = [administrator], + authN_backend = rabbit_auth_backend_internal, + authZ_backends = [{rabbit_auth_backend_internal, + #internal_user{username = Username, + tags = [administrator]}}]}. test_confirms() -> {_Writer, Ch} = test_spawn(), -- cgit v1.2.1 From 592c9b0b2dd726a8dc7879f969793eef430eaf5b Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 13 Nov 2014 14:36:48 +0000 Subject: Cosmetic. --- src/rabbit_access_control.erl | 61 +++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/src/rabbit_access_control.erl b/src/rabbit_access_control.erl index dcec0ff5..a6d3cd6c 100644 --- a/src/rabbit_access_control.erl +++ b/src/rabbit_access_control.erl @@ -61,7 +61,7 @@ check_user_login(Username, AuthProps) -> %% passwordless (i.e pre-authenticated) login with authZ. case try_authenticate(ModN, Username, AuthProps) of {ok, User, _AuthZ} -> try_authorize(ModZ, User, []); - Else -> Else + Else -> Else end; (Mod, {refused, _, _}) -> %% Same module for authN and authZ. Just take the result @@ -71,30 +71,31 @@ check_user_login(Username, AuthProps) -> %% We've successfully authenticated. Skip to the end... {ok, User, AuthZ} end, {refused, "No modules checked '~s'", [Username]}, Modules), - case R of {ok, RUser, RAuthZ} -> - rabbit_event:notify(user_authentication_success, [{name, Username}]), + rabbit_event:notify(user_authentication_success, [{name,Username}]), %% Store the list of authorization backends {ok, RUser#user{authZ_backends=RAuthZ}}; _ -> - rabbit_event:notify(user_authentication_failure, [{name, Username}]), + rabbit_event:notify(user_authentication_failure, [{name,Username}]), R end. try_authenticate(Module, Username, AuthProps) -> case Module:check_user_login(Username, AuthProps) of {ok, User, AuthZ} -> {ok, User, [{Module, AuthZ}]}; - {error, E} -> {refused, "~s failed authenticating ~s: ~p~n", - [Module, Username, E]}; - Else -> Else + {error, E} -> {refused, "~s failed authenticating ~s: ~p~n", + [Module, Username, E]}; + {refused, F, A} -> {refused, F, A} end. try_authorize(Modules, User, AuthZList) when is_list(Modules) -> lists:foldr( - fun (Module, {ok, _User, AuthZList2}) -> try_authorize(Module, User, AuthZList2); - (_, {refused, _, _} = Error) -> Error - end, {ok, User, AuthZList}, Modules); + fun (Module, {ok, _User, AuthZList2}) -> + try_authorize(Module, User, AuthZList2); + (_, {refused, _, _} = Error) -> + Error + end, {ok, User, AuthZList}, Modules); try_authorize(Module, User = #user{username = Username}, AuthZList) -> case Module:check_user_login(Username, []) of @@ -112,37 +113,35 @@ check_user_loopback(Username, SockOrAddr) -> false -> not_allowed end. -check_vhost_access(User = #user{ username = Username, - authZ_backends = Modules }, VHostPath, Sock) -> +check_vhost_access(User = #user{username = Username, + authZ_backends = Modules}, VHostPath, Sock) -> lists:foldl( - fun({Module, Impl}, ok) -> - check_access( - fun() -> - %% TODO this could be an andalso shortcut under >R13A - case rabbit_vhost:exists(VHostPath) of - false -> false; - true -> Module:check_vhost_access(User, Impl, VHostPath, Sock) - end - end, - Module, "access to vhost '~s' refused for user '~s'", - [VHostPath, Username]); - - (_, Else) -> Else + fun({Mod, Impl}, ok) -> + check_access( + fun() -> + rabbit_vhost:exists(VHostPath) andalso + Mod:check_vhost_access(User, Impl, VHostPath, Sock) + end, + Mod, "access to vhost '~s' refused for user '~s'", + [VHostPath, Username]); + (_, Else) -> + Else end, ok, Modules). check_resource_access(User, R = #resource{kind = exchange, name = <<"">>}, Permission) -> check_resource_access(User, R#resource{name = <<"amq.default">>}, Permission); -check_resource_access(User = #user{username = Username, authZ_backends = Modules}, +check_resource_access(User = #user{username = Username, + authZ_backends = Modules}, Resource, Permission) -> lists:foldl( fun({Module, Impl}, ok) -> - check_access( - fun() -> Module:check_resource_access(User, Impl, Resource, Permission) end, - Module, "access to ~s refused for user '~s'", - [rabbit_misc:rs(Resource), Username]); - + check_access( + fun() -> Module:check_resource_access( + User, Impl, Resource, Permission) end, + Module, "access to ~s refused for user '~s'", + [rabbit_misc:rs(Resource), Username]); (_, Else) -> Else end, ok, Modules). -- cgit v1.2.1 From 606e3656008e1ca677e0cde9c3514e24f9ab1468 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Thu, 13 Nov 2014 16:18:20 +0000 Subject: Never pass the #user{} record into auth backends, they should only see their own stuff. Get rid of authN_backend, it has little reason for existing. Flatten case of authZ_backend. --- include/rabbit.hrl | 11 +++-- src/rabbit_access_control.erl | 87 ++++++++++++++++++++---------------- src/rabbit_auth_backend.erl | 20 ++++++--- src/rabbit_auth_backend_dummy.erl | 13 +++--- src/rabbit_auth_backend_internal.erl | 12 ++--- src/rabbit_channel.erl | 3 +- src/rabbit_types.erl | 3 +- 7 files changed, 87 insertions(+), 62 deletions(-) diff --git a/include/rabbit.hrl b/include/rabbit.hrl index 627d0479..86c30fc5 100644 --- a/include/rabbit.hrl +++ b/include/rabbit.hrl @@ -14,12 +14,17 @@ %% Copyright (c) 2007-2014 GoPivotal, Inc. All rights reserved. %% +%% Passed around most places -record(user, {username, tags, - authN_backend, %% Authentication module this user came from - authZ_backends %% List of authorization modules - }). + authz_backends}). %% List of {Module, AuthUser} pairs +%% Passed to auth backends +-record(auth_user, {username, + tags, + impl}). + +%% Implementation for the internal auth backend -record(internal_user, {username, password_hash, tags}). -record(permission, {configure, write, read}). -record(user_vhost, {username, virtual_host}). diff --git a/src/rabbit_access_control.erl b/src/rabbit_access_control.erl index a6d3cd6c..0ebd2fcf 100644 --- a/src/rabbit_access_control.erl +++ b/src/rabbit_access_control.erl @@ -55,55 +55,64 @@ check_user_pass_login(Username, Password) -> check_user_login(Username, AuthProps) -> {ok, Modules} = application:get_env(rabbit, auth_backends), R = lists:foldl( - fun ({ModN, ModZ}, {refused, _, _}) -> + fun ({ModN, ModZs0}, {refused, _, _}) -> + ModZs = case ModZs0 of + A when is_atom(A) -> [A]; + L when is_list(L) -> L + end, %% Different modules for authN vs authZ. So authenticate %% with authN module, then if that succeeds do %% passwordless (i.e pre-authenticated) login with authZ. case try_authenticate(ModN, Username, AuthProps) of - {ok, User, _AuthZ} -> try_authorize(ModZ, User, []); - Else -> Else + {ok, ModNUser = #auth_user{username = Username2}} -> + user(ModNUser, try_authorize(ModZs, Username2)); + Else -> + Else end; (Mod, {refused, _, _}) -> %% Same module for authN and authZ. Just take the result %% it gives us - try_authenticate(Mod, Username, AuthProps); - (_, {ok, User, AuthZ}) -> + case try_authenticate(Mod, Username, AuthProps) of + {ok, ModNUser} -> user(ModNUser, {ok, [{Mod, ModNUser}]}); + Else -> Else + end; + (_, {ok, User}) -> %% We've successfully authenticated. Skip to the end... - {ok, User, AuthZ} + {ok, User} end, {refused, "No modules checked '~s'", [Username]}, Modules), - case R of - {ok, RUser, RAuthZ} -> - rabbit_event:notify(user_authentication_success, [{name,Username}]), - %% Store the list of authorization backends - {ok, RUser#user{authZ_backends=RAuthZ}}; - _ -> - rabbit_event:notify(user_authentication_failure, [{name,Username}]), - R - end. + rabbit_event:notify(case R of + {ok, _User} -> user_authentication_success; + _ -> user_authentication_failure + end, [{name, Username}]), + R. try_authenticate(Module, Username, AuthProps) -> case Module:check_user_login(Username, AuthProps) of - {ok, User, AuthZ} -> {ok, User, [{Module, AuthZ}]}; - {error, E} -> {refused, "~s failed authenticating ~s: ~p~n", - [Module, Username, E]}; - {refused, F, A} -> {refused, F, A} + {ok, AuthUser} -> {ok, AuthUser}; + {error, E} -> {refused, "~s failed authenticating ~s: ~p~n", + [Module, Username, E]}; + {refused, F, A} -> {refused, F, A} end. -try_authorize(Modules, User, AuthZList) when is_list(Modules) -> +try_authorize(Modules, Username) -> lists:foldr( - fun (Module, {ok, _User, AuthZList2}) -> - try_authorize(Module, User, AuthZList2); + fun (Module, {ok, AUsers}) -> + case Module:check_user_login(Username, []) of + {ok, AUser} -> {ok, [{Module, AUser} | AUsers]}; + {error, E} -> {refused, "~s failed authorizing ~s: ~p~n", + [Module, Username, E]}; + {refused, F, A} -> {refused, F, A} + end; (_, {refused, _, _} = Error) -> Error - end, {ok, User, AuthZList}, Modules); - -try_authorize(Module, User = #user{username = Username}, AuthZList) -> - case Module:check_user_login(Username, []) of - {ok, _User, AuthZ} -> {ok, User, [{Module, AuthZ}|AuthZList]}; - {error, E} -> {refused, "~s failed authorizing ~s: ~p~n", - [Module, Username, E]}; - Else -> Else - end. + end, {ok, []}, Modules). + +user(#auth_user{username = Username, tags = Tags}, {ok, ModZUsers}) -> + {ok, #user{username = Username, + tags = Tags, + authz_backends = ModZUsers}}; +user(_AuthUser, Error) -> + Error. check_user_loopback(Username, SockOrAddr) -> {ok, Users} = application:get_env(rabbit, loopback_users), @@ -113,14 +122,14 @@ check_user_loopback(Username, SockOrAddr) -> false -> not_allowed end. -check_vhost_access(User = #user{username = Username, - authZ_backends = Modules}, VHostPath, Sock) -> +check_vhost_access(#user{username = Username, + authz_backends = Modules}, VHostPath, Sock) -> lists:foldl( - fun({Mod, Impl}, ok) -> + fun({Mod, AUser}, ok) -> check_access( fun() -> rabbit_vhost:exists(VHostPath) andalso - Mod:check_vhost_access(User, Impl, VHostPath, Sock) + Mod:check_vhost_access(AUser, VHostPath, Sock) end, Mod, "access to vhost '~s' refused for user '~s'", [VHostPath, Username]); @@ -132,14 +141,14 @@ check_resource_access(User, R = #resource{kind = exchange, name = <<"">>}, Permission) -> check_resource_access(User, R#resource{name = <<"amq.default">>}, Permission); -check_resource_access(User = #user{username = Username, - authZ_backends = Modules}, +check_resource_access(#user{username = Username, + authz_backends = Modules}, Resource, Permission) -> lists:foldl( - fun({Module, Impl}, ok) -> + fun({Module, AUser}, ok) -> check_access( fun() -> Module:check_resource_access( - User, Impl, Resource, Permission) end, + AUser, Resource, Permission) end, Module, "access to ~s refused for user '~s'", [rabbit_misc:rs(Resource), Username]); (_, Else) -> Else diff --git a/src/rabbit_auth_backend.erl b/src/rabbit_auth_backend.erl index 99f291f1..315d5719 100644 --- a/src/rabbit_auth_backend.erl +++ b/src/rabbit_auth_backend.erl @@ -16,8 +16,17 @@ -module(rabbit_auth_backend). +-include("rabbit.hrl"). + -ifdef(use_specs). +-export_type([auth_user/0]). + +-type(auth_user() :: + #auth_user{username :: rabbit_types:username(), + tags :: [atom()], + impl :: any()}). + %% A description proplist as with auth mechanisms, %% exchanges. Currently unused. -callback description() -> [proplists:property()]. @@ -33,7 +42,7 @@ %% {refused, Msg, Args} %% Client failed authentication. Log and die. -callback check_user_login(rabbit_types:username(), [term()]) -> - {'ok', rabbit_types:user(), any()} | + {'ok', auth_user()} | {'refused', string(), [any()]} | {'error', any()}. @@ -43,11 +52,10 @@ %% false %% {error, Error} %% Something went wrong. Log and die. --callback check_vhost_access(rabbit_types:user(), any(), +-callback check_vhost_access(auth_user(), rabbit_types:vhost(), rabbit_net:socket()) -> boolean() | {'error', any()}. - %% Given #user, resource and permission, can a user access a resource? %% %% Possible responses: @@ -55,7 +63,7 @@ %% false %% {error, Error} %% Something went wrong. Log and die. --callback check_resource_access(rabbit_types:user(), any(), +-callback check_resource_access(auth_user(), rabbit_types:r(atom()), rabbit_access_control:permission_atom()) -> boolean() | {'error', any()}. @@ -65,8 +73,8 @@ -export([behaviour_info/1]). behaviour_info(callbacks) -> - [{description, 0}, {check_user_login, 2}, {check_vhost_access, 4}, - {check_resource_access, 4}]; + [{description, 0}, {check_user_login, 2}, {check_vhost_access, 3}, + {check_resource_access, 3}]; behaviour_info(_Other) -> undefined. diff --git a/src/rabbit_auth_backend_dummy.erl b/src/rabbit_auth_backend_dummy.erl index 8e2d8269..e279e4bf 100644 --- a/src/rabbit_auth_backend_dummy.erl +++ b/src/rabbit_auth_backend_dummy.erl @@ -21,7 +21,7 @@ -export([description/0]). -export([user/0]). --export([check_user_login/2, check_vhost_access/4, check_resource_access/4]). +-export([check_user_login/2, check_vhost_access/3, check_resource_access/3]). -ifdef(use_specs). @@ -33,8 +33,11 @@ %% not needed. This user can do anything AMQPish. user() -> #user{username = <<"none">>, tags = [], - authN_backend = ?MODULE, - authZ_backends = []}. + authz_backends = [{?MODULE, buser()}]}. + +buser() -> #auth_user{username = <<"none">>, + tags = [], + impl = none}. %% Implementation of rabbit_auth_backend @@ -45,5 +48,5 @@ description() -> check_user_login(_, _) -> {refused, "cannot log in conventionally as dummy user", []}. -check_vhost_access(#user{}, _Impl, _VHostPath, _Sock) -> true. -check_resource_access(#user{}, _Impl, #resource{}, _Permission) -> true. +check_vhost_access(#auth_user{}, _VHostPath, _Sock) -> true. +check_resource_access(#auth_user{}, #resource{}, _Permission) -> true. diff --git a/src/rabbit_auth_backend_internal.erl b/src/rabbit_auth_backend_internal.erl index 5cdff985..c8f09be9 100644 --- a/src/rabbit_auth_backend_internal.erl +++ b/src/rabbit_auth_backend_internal.erl @@ -20,7 +20,7 @@ -behaviour(rabbit_auth_backend). -export([description/0]). --export([check_user_login/2, check_vhost_access/4, check_resource_access/4]). +-export([check_user_login/2, check_vhost_access/3, check_resource_access/3]). -export([add_user/2, delete_user/1, lookup_user/1, change_password/2, clear_password/1, @@ -98,16 +98,16 @@ internal_check_user_login(Username, Fun) -> case lookup_user(Username) of {ok, User = #internal_user{tags = Tags}} -> case Fun(User) of - true -> {ok, #user{username = Username, - tags = Tags, - authN_backend = ?MODULE}, User}; + true -> {ok, #auth_user{username = Username, + tags = Tags, + impl = none}}; _ -> Refused end; {error, not_found} -> Refused end. -check_vhost_access(#user{username = Username}, _Impl, VHostPath, _Sock) -> +check_vhost_access(#auth_user{username = Username}, VHostPath, _Sock) -> case mnesia:dirty_read({rabbit_user_permission, #user_vhost{username = Username, virtual_host = VHostPath}}) of @@ -115,7 +115,7 @@ check_vhost_access(#user{username = Username}, _Impl, VHostPath, _Sock) -> [_R] -> true end. -check_resource_access(#user{username = Username}, _Impl, +check_resource_access(#auth_user{username = Username}, #resource{virtual_host = VHostPath, name = Name}, Permission) -> case mnesia:dirty_read({rabbit_user_permission, diff --git a/src/rabbit_channel.erl b/src/rabbit_channel.erl index 2f7e234d..13cc925c 100644 --- a/src/rabbit_channel.erl +++ b/src/rabbit_channel.erl @@ -581,7 +581,8 @@ check_user_id_header(#'P_basic'{user_id = Username}, #ch{user = #user{username = Username}}) -> ok; check_user_id_header( - #'P_basic'{}, #ch{user = #user{authN_backend = rabbit_auth_backend_dummy}}) -> + #'P_basic'{}, #ch{user = #user{authz_backends = + [{rabbit_auth_backend_dummy, _}]}}) -> ok; check_user_id_header(#'P_basic'{user_id = Claimed}, #ch{user = #user{username = Actual, diff --git a/src/rabbit_types.erl b/src/rabbit_types.erl index b3158cc8..27fbae88 100644 --- a/src/rabbit_types.erl +++ b/src/rabbit_types.erl @@ -134,8 +134,7 @@ -type(user() :: #user{username :: username(), tags :: [atom()], - authN_backend :: atom(), - authZ_backends :: [{atom(), any()}]}). + authz_backends :: [{atom(), any()}]}). -type(internal_user() :: #internal_user{username :: username(), -- cgit v1.2.1 From aa1b10af8fcf7b95596f8db530ed3cd103afbe0a Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Fri, 14 Nov 2014 11:16:17 +0000 Subject: Fix tests. --- test/src/rabbit_tests.erl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/src/rabbit_tests.erl b/test/src/rabbit_tests.erl index a227f3d2..e614bfd7 100644 --- a/test/src/rabbit_tests.erl +++ b/test/src/rabbit_tests.erl @@ -1294,10 +1294,12 @@ test_spawn_remote() -> user(Username) -> #user{username = Username, tags = [administrator], - authN_backend = rabbit_auth_backend_internal, - authZ_backends = [{rabbit_auth_backend_internal, - #internal_user{username = Username, - tags = [administrator]}}]}. + authz_backends = [{rabbit_auth_backend_internal, auser(Username)}]}. + +auser(Username) -> + #auth_user{username = Username, + tags = [administrator], + impl = none}. test_confirms() -> {_Writer, Ch} = test_spawn(), -- cgit v1.2.1 From 627f4c0e15db86bdff439ccc01bcc81bb087a79f Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Fri, 14 Nov 2014 11:17:19 +0000 Subject: Rename. --- src/rabbit_auth_backend_dummy.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rabbit_auth_backend_dummy.erl b/src/rabbit_auth_backend_dummy.erl index e279e4bf..d2905334 100644 --- a/src/rabbit_auth_backend_dummy.erl +++ b/src/rabbit_auth_backend_dummy.erl @@ -33,9 +33,9 @@ %% not needed. This user can do anything AMQPish. user() -> #user{username = <<"none">>, tags = [], - authz_backends = [{?MODULE, buser()}]}. + authz_backends = [{?MODULE, auser()}]}. -buser() -> #auth_user{username = <<"none">>, +auser() -> #auth_user{username = <<"none">>, tags = [], impl = none}. -- cgit v1.2.1 From 8a6ad3517e031b8b7b85c63ab24d062d1d647b5b Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Fri, 14 Nov 2014 14:16:39 +0000 Subject: Tweak the APIs again, so that authz plugins aren't expected to create a #auth_user record in the first place, just whatever impl they want. Which necessitates seperate login functions for authz and authn, and if we're going to do that we might as well split the behaviours so that we have the possibility of making an authz-only plugin. --- include/rabbit.hrl | 2 +- src/rabbit_access_control.erl | 40 +++++++++++------- src/rabbit_auth_backend.erl | 81 ------------------------------------ src/rabbit_auth_backend_dummy.erl | 20 ++++----- src/rabbit_auth_backend_internal.erl | 23 +++++----- src/rabbit_authn_backend.erl | 49 ++++++++++++++++++++++ src/rabbit_authz_backend.erl | 74 ++++++++++++++++++++++++++++++++ src/rabbit_types.erl | 7 +++- test/src/rabbit_tests.erl | 7 +--- 9 files changed, 176 insertions(+), 127 deletions(-) delete mode 100644 src/rabbit_auth_backend.erl create mode 100644 src/rabbit_authn_backend.erl create mode 100644 src/rabbit_authz_backend.erl diff --git a/include/rabbit.hrl b/include/rabbit.hrl index 86c30fc5..9cbd978e 100644 --- a/include/rabbit.hrl +++ b/include/rabbit.hrl @@ -17,7 +17,7 @@ %% Passed around most places -record(user, {username, tags, - authz_backends}). %% List of {Module, AuthUser} pairs + authz_backends}). %% List of {Module, AuthUserImpl} pairs %% Passed to auth backends -record(auth_user, {username, diff --git a/src/rabbit_access_control.erl b/src/rabbit_access_control.erl index 0ebd2fcf..d1577432 100644 --- a/src/rabbit_access_control.erl +++ b/src/rabbit_access_control.erl @@ -73,8 +73,10 @@ check_user_login(Username, AuthProps) -> %% Same module for authN and authZ. Just take the result %% it gives us case try_authenticate(Mod, Username, AuthProps) of - {ok, ModNUser} -> user(ModNUser, {ok, [{Mod, ModNUser}]}); - Else -> Else + {ok, ModNUser = #auth_user{impl = Impl}} -> + user(ModNUser, {ok, [{Mod, Impl}]}); + Else -> + Else end; (_, {ok, User}) -> %% We've successfully authenticated. Skip to the end... @@ -87,7 +89,7 @@ check_user_login(Username, AuthProps) -> R. try_authenticate(Module, Username, AuthProps) -> - case Module:check_user_login(Username, AuthProps) of + case Module:user_login_authentication(Username, AuthProps) of {ok, AuthUser} -> {ok, AuthUser}; {error, E} -> {refused, "~s failed authenticating ~s: ~p~n", [Module, Username, E]}; @@ -96,9 +98,9 @@ try_authenticate(Module, Username, AuthProps) -> try_authorize(Modules, Username) -> lists:foldr( - fun (Module, {ok, AUsers}) -> - case Module:check_user_login(Username, []) of - {ok, AUser} -> {ok, [{Module, AUser} | AUsers]}; + fun (Module, {ok, ModsImpls}) -> + case Module:user_login_authorization(Username) of + {ok, Impl} -> {ok, [{Module, Impl} | ModsImpls]}; {error, E} -> {refused, "~s failed authorizing ~s: ~p~n", [Module, Username, E]}; {refused, F, A} -> {refused, F, A} @@ -107,13 +109,18 @@ try_authorize(Modules, Username) -> Error end, {ok, []}, Modules). -user(#auth_user{username = Username, tags = Tags}, {ok, ModZUsers}) -> +user(#auth_user{username = Username, tags = Tags}, {ok, ModZImpls}) -> {ok, #user{username = Username, tags = Tags, - authz_backends = ModZUsers}}; + authz_backends = ModZImpls}}; user(_AuthUser, Error) -> Error. +auth_user(#user{username = Username, tags = Tags}, Impl) -> + #auth_user{username = Username, + tags = Tags, + impl = Impl}. + check_user_loopback(Username, SockOrAddr) -> {ok, Users} = application:get_env(rabbit, loopback_users), case rabbit_net:is_loopback(SockOrAddr) @@ -122,14 +129,15 @@ check_user_loopback(Username, SockOrAddr) -> false -> not_allowed end. -check_vhost_access(#user{username = Username, - authz_backends = Modules}, VHostPath, Sock) -> +check_vhost_access(User = #user{username = Username, + authz_backends = Modules}, VHostPath, Sock) -> lists:foldl( - fun({Mod, AUser}, ok) -> + fun({Mod, Impl}, ok) -> check_access( fun() -> rabbit_vhost:exists(VHostPath) andalso - Mod:check_vhost_access(AUser, VHostPath, Sock) + Mod:check_vhost_access( + auth_user(User, Impl), VHostPath, Sock) end, Mod, "access to vhost '~s' refused for user '~s'", [VHostPath, Username]); @@ -141,14 +149,14 @@ check_resource_access(User, R = #resource{kind = exchange, name = <<"">>}, Permission) -> check_resource_access(User, R#resource{name = <<"amq.default">>}, Permission); -check_resource_access(#user{username = Username, - authz_backends = Modules}, +check_resource_access(User = #user{username = Username, + authz_backends = Modules}, Resource, Permission) -> lists:foldl( - fun({Module, AUser}, ok) -> + fun({Module, Impl}, ok) -> check_access( fun() -> Module:check_resource_access( - AUser, Resource, Permission) end, + auth_user(User, Impl), Resource, Permission) end, Module, "access to ~s refused for user '~s'", [rabbit_misc:rs(Resource), Username]); (_, Else) -> Else diff --git a/src/rabbit_auth_backend.erl b/src/rabbit_auth_backend.erl deleted file mode 100644 index 315d5719..00000000 --- a/src/rabbit_auth_backend.erl +++ /dev/null @@ -1,81 +0,0 @@ -%% The contents of this file are subject to the Mozilla Public License -%% Version 1.1 (the "License"); you may not use this file except in -%% compliance with the License. You may obtain a copy of the License -%% at http://www.mozilla.org/MPL/ -%% -%% Software distributed under the License is distributed on an "AS IS" -%% basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See -%% the License for the specific language governing rights and -%% limitations under the License. -%% -%% The Original Code is RabbitMQ. -%% -%% The Initial Developer of the Original Code is GoPivotal, Inc. -%% Copyright (c) 2007-2014 GoPivotal, Inc. All rights reserved. -%% - --module(rabbit_auth_backend). - --include("rabbit.hrl"). - --ifdef(use_specs). - --export_type([auth_user/0]). - --type(auth_user() :: - #auth_user{username :: rabbit_types:username(), - tags :: [atom()], - impl :: any()}). - -%% A description proplist as with auth mechanisms, -%% exchanges. Currently unused. --callback description() -> [proplists:property()]. - -%% Check a user can log in, given a username and a proplist of -%% authentication information (e.g. [{password, Password}]). -%% -%% Possible responses: -%% {ok, User} -%% Authentication succeeded, and here's the user record. -%% {error, Error} -%% Something went wrong. Log and die. -%% {refused, Msg, Args} -%% Client failed authentication. Log and die. --callback check_user_login(rabbit_types:username(), [term()]) -> - {'ok', auth_user()} | - {'refused', string(), [any()]} | - {'error', any()}. - -%% Given #user and vhost, can a user log in to a vhost? -%% Possible responses: -%% true -%% false -%% {error, Error} -%% Something went wrong. Log and die. --callback check_vhost_access(auth_user(), - rabbit_types:vhost(), rabbit_net:socket()) -> - boolean() | {'error', any()}. - -%% Given #user, resource and permission, can a user access a resource? -%% -%% Possible responses: -%% true -%% false -%% {error, Error} -%% Something went wrong. Log and die. --callback check_resource_access(auth_user(), - rabbit_types:r(atom()), - rabbit_access_control:permission_atom()) -> - boolean() | {'error', any()}. - --else. - --export([behaviour_info/1]). - -behaviour_info(callbacks) -> - [{description, 0}, {check_user_login, 2}, {check_vhost_access, 3}, - {check_resource_access, 3}]; -behaviour_info(_Other) -> - undefined. - --endif. diff --git a/src/rabbit_auth_backend_dummy.erl b/src/rabbit_auth_backend_dummy.erl index d2905334..d2f07c1d 100644 --- a/src/rabbit_auth_backend_dummy.erl +++ b/src/rabbit_auth_backend_dummy.erl @@ -17,11 +17,12 @@ -module(rabbit_auth_backend_dummy). -include("rabbit.hrl"). --behaviour(rabbit_auth_backend). +-behaviour(rabbit_authn_backend). +-behaviour(rabbit_authz_backend). --export([description/0]). -export([user/0]). --export([check_user_login/2, check_vhost_access/3, check_resource_access/3]). +-export([user_login_authentication/2, user_login_authorization/1, + check_vhost_access/3, check_resource_access/3]). -ifdef(use_specs). @@ -33,19 +34,14 @@ %% not needed. This user can do anything AMQPish. user() -> #user{username = <<"none">>, tags = [], - authz_backends = [{?MODULE, auser()}]}. - -auser() -> #auth_user{username = <<"none">>, - tags = [], - impl = none}. + authz_backends = [{?MODULE, none}]}. %% Implementation of rabbit_auth_backend -description() -> - [{name, <<"Dummy">>}, - {description, <<"Database for the dummy user">>}]. +user_login_authentication(_, _) -> + {refused, "cannot log in conventionally as dummy user", []}. -check_user_login(_, _) -> +user_login_authorization(_) -> {refused, "cannot log in conventionally as dummy user", []}. check_vhost_access(#auth_user{}, _VHostPath, _Sock) -> true. diff --git a/src/rabbit_auth_backend_internal.erl b/src/rabbit_auth_backend_internal.erl index c8f09be9..20a5766d 100644 --- a/src/rabbit_auth_backend_internal.erl +++ b/src/rabbit_auth_backend_internal.erl @@ -17,10 +17,11 @@ -module(rabbit_auth_backend_internal). -include("rabbit.hrl"). --behaviour(rabbit_auth_backend). +-behaviour(rabbit_authn_backend). +-behaviour(rabbit_authz_backend). --export([description/0]). --export([check_user_login/2, check_vhost_access/3, check_resource_access/3]). +-export([user_login_authentication/2, user_login_authorization/1, + check_vhost_access/3, check_resource_access/3]). -export([add_user/2, delete_user/1, lookup_user/1, change_password/2, clear_password/1, @@ -76,13 +77,9 @@ %%---------------------------------------------------------------------------- %% Implementation of rabbit_auth_backend -description() -> - [{name, <<"Internal">>}, - {description, <<"Internal user / password database">>}]. - -check_user_login(Username, []) -> +user_login_authentication(Username, []) -> internal_check_user_login(Username, fun(_) -> true end); -check_user_login(Username, [{password, Cleartext}]) -> +user_login_authentication(Username, [{password, Cleartext}]) -> internal_check_user_login( Username, fun (#internal_user{password_hash = <>}) -> @@ -90,9 +87,15 @@ check_user_login(Username, [{password, Cleartext}]) -> (#internal_user{}) -> false end); -check_user_login(Username, AuthProps) -> +user_login_authentication(Username, AuthProps) -> exit({unknown_auth_props, Username, AuthProps}). +user_login_authorization(Username) -> + case user_login_authentication(Username, []) of + {ok, #auth_user{impl = Impl}} -> {ok, Impl}; + Else -> Else + end. + internal_check_user_login(Username, Fun) -> Refused = {refused, "user '~s' - invalid credentials", [Username]}, case lookup_user(Username) of diff --git a/src/rabbit_authn_backend.erl b/src/rabbit_authn_backend.erl new file mode 100644 index 00000000..cfc3f5db --- /dev/null +++ b/src/rabbit_authn_backend.erl @@ -0,0 +1,49 @@ +%% The contents of this file are subject to the Mozilla Public License +%% Version 1.1 (the "License"); you may not use this file except in +%% compliance with the License. You may obtain a copy of the License +%% at http://www.mozilla.org/MPL/ +%% +%% Software distributed under the License is distributed on an "AS IS" +%% basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See +%% the License for the specific language governing rights and +%% limitations under the License. +%% +%% The Original Code is RabbitMQ. +%% +%% The Initial Developer of the Original Code is GoPivotal, Inc. +%% Copyright (c) 2007-2014 GoPivotal, Inc. All rights reserved. +%% + +-module(rabbit_authn_backend). + +-include("rabbit.hrl"). + +-ifdef(use_specs). + +%% Check a user can log in, given a username and a proplist of +%% authentication information (e.g. [{password, Password}]). If your +%% backend is not to be used for authentication, this should always +%% refuse access. +%% +%% Possible responses: +%% {ok, User} +%% Authentication succeeded, and here's the user record. +%% {error, Error} +%% Something went wrong. Log and die. +%% {refused, Msg, Args} +%% Client failed authentication. Log and die. +-callback user_login_authentication(rabbit_types:username(), [term()]) -> + {'ok', rabbit_types:auth_user()} | + {'refused', string(), [any()]} | + {'error', any()}. + +-else. + +-export([behaviour_info/1]). + +behaviour_info(callbacks) -> + [{user_login_authentication, 2}]; +behaviour_info(_Other) -> + undefined. + +-endif. diff --git a/src/rabbit_authz_backend.erl b/src/rabbit_authz_backend.erl new file mode 100644 index 00000000..ff5f014e --- /dev/null +++ b/src/rabbit_authz_backend.erl @@ -0,0 +1,74 @@ +%% The contents of this file are subject to the Mozilla Public License +%% Version 1.1 (the "License"); you may not use this file except in +%% compliance with the License. You may obtain a copy of the License +%% at http://www.mozilla.org/MPL/ +%% +%% Software distributed under the License is distributed on an "AS IS" +%% basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See +%% the License for the specific language governing rights and +%% limitations under the License. +%% +%% The Original Code is RabbitMQ. +%% +%% The Initial Developer of the Original Code is GoPivotal, Inc. +%% Copyright (c) 2007-2014 GoPivotal, Inc. All rights reserved. +%% + +-module(rabbit_authz_backend). + +-include("rabbit.hrl"). + +-ifdef(use_specs). + +%% Check a user can log in, when this backend is being used for +%% authorisation only. Authentication has already taken place +%% successfully, but we need to check that the user exists in this +%% backend, and initialise any impl field we will want to have passed +%% back in future calls to check_vhost_access/3 and +%% check_resource_access/3. +%% +%% Possible responses: +%% {ok, Impl} +%% User authorisation succeeded, and here's the impl field. +%% {error, Error} +%% Something went wrong. Log and die. +%% {refused, Msg, Args} +%% User authorisation failed. Log and die. +-callback user_login_authorization(rabbit_types:username()) -> + {'ok', any()} | + {'refused', string(), [any()]} | + {'error', any()}. + +%% Given #auth_user and vhost, can a user log in to a vhost? +%% Possible responses: +%% true +%% false +%% {error, Error} +%% Something went wrong. Log and die. +-callback check_vhost_access(rabbit_types:auth_user(), + rabbit_types:vhost(), rabbit_net:socket()) -> + boolean() | {'error', any()}. + +%% Given #auth_user, resource and permission, can a user access a resource? +%% +%% Possible responses: +%% true +%% false +%% {error, Error} +%% Something went wrong. Log and die. +-callback check_resource_access(rabbit_types:auth_user(), + rabbit_types:r(atom()), + rabbit_access_control:permission_atom()) -> + boolean() | {'error', any()}. + +-else. + +-export([behaviour_info/1]). + +behaviour_info(callbacks) -> + [{user_login_authorization, 1}, + {check_vhost_access, 3}, {check_resource_access, 3}]; +behaviour_info(_Other) -> + undefined. + +-endif. diff --git a/src/rabbit_types.erl b/src/rabbit_types.erl index 27fbae88..039568df 100644 --- a/src/rabbit_types.erl +++ b/src/rabbit_types.erl @@ -27,7 +27,7 @@ vhost/0, ctag/0, amqp_error/0, r/1, r2/2, r3/3, listener/0, binding/0, binding_source/0, binding_destination/0, amqqueue/0, exchange/0, - connection/0, protocol/0, user/0, internal_user/0, + connection/0, protocol/0, auth_user/0, user/0, internal_user/0, username/0, password/0, password_hash/0, ok/1, error/1, ok_or_error/1, ok_or_error2/2, ok_pid_or_error/0, channel_exit/0, connection_exit/0, mfargs/0, proc_name/0, @@ -131,6 +131,11 @@ -type(protocol() :: rabbit_framing:protocol()). +-type(auth_user() :: + #auth_user{username :: username(), + tags :: [atom()], + impl :: any()}). + -type(user() :: #user{username :: username(), tags :: [atom()], diff --git a/test/src/rabbit_tests.erl b/test/src/rabbit_tests.erl index e614bfd7..dcbec8f6 100644 --- a/test/src/rabbit_tests.erl +++ b/test/src/rabbit_tests.erl @@ -1294,12 +1294,7 @@ test_spawn_remote() -> user(Username) -> #user{username = Username, tags = [administrator], - authz_backends = [{rabbit_auth_backend_internal, auser(Username)}]}. - -auser(Username) -> - #auth_user{username = Username, - tags = [administrator], - impl = none}. + authz_backends = [{rabbit_auth_backend_internal, none}]}. test_confirms() -> {_Writer, Ch} = test_spawn(), -- cgit v1.2.1 From a416ba84b97459952c2467cf0cdea0b76b6bf241 Mon Sep 17 00:00:00 2001 From: Jean-S?bastien P?dron Date: Thu, 20 Nov 2014 09:45:37 +0100 Subject: Remove support for the legacy 'cluster_nodes' values Before this change, a list of nodes without the node type was accepted. In this case, the node type was guessed and a warning suggesting how to update the configuration was logged. Now, the node type is mandatory and the RabbitMQ server refuses to start if the node type is unspecified. --- src/rabbit_mnesia.erl | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/rabbit_mnesia.erl b/src/rabbit_mnesia.erl index fa51dd70..d3cacb17 100644 --- a/src/rabbit_mnesia.erl +++ b/src/rabbit_mnesia.erl @@ -116,18 +116,9 @@ init_from_config() -> {TryNodes, NodeType} = case application:get_env(rabbit, cluster_nodes) of {ok, Nodes} when is_list(Nodes) -> - Config = {Nodes -- [node()], case lists:member(node(), Nodes) of - true -> disc; - false -> ram - end}, - rabbit_log:warning( - "Converting legacy 'cluster_nodes' configuration~n ~w~n" - "to~n ~w.~n~n" - "Please update the configuration to the new format " - "{Nodes, NodeType}, where Nodes contains the nodes that the " - "node will try to cluster with, and NodeType is either " - "'disc' or 'ram'~n", [Nodes, Config]), - Config; + %% The legacy syntax (a nodes list without the node + %% type) is unsupported. + e(cluster_node_type_mandatory); {ok, Config} -> Config end, @@ -865,4 +856,7 @@ error_description(removing_node_from_offline_node) -> "To remove a node remotely from an offline node, the node you are removing " "from must be a disc node and all the other nodes must be offline."; error_description(no_running_cluster_nodes) -> - "You cannot leave a cluster if no online nodes are present.". + "You cannot leave a cluster if no online nodes are present."; +error_description(cluster_node_type_mandatory) -> + "The 'cluster_nodes' configuration key must indicate the node type: " + "either {[...], disc} or {[...], ram}". -- cgit v1.2.1 From 2b28700c0ac996ac37e106c5c1b7e58ce968f391 Mon Sep 17 00:00:00 2001 From: Jean-Sebastien Pedron Date: Fri, 21 Nov 2014 13:06:12 +0100 Subject: Detect other invalid 'cluster_nodes' values In all cases, abort startup with an explanatory message. --- src/rabbit_mnesia.erl | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-) diff --git a/src/rabbit_mnesia.erl b/src/rabbit_mnesia.erl index d3cacb17..8fbacdae 100644 --- a/src/rabbit_mnesia.erl +++ b/src/rabbit_mnesia.erl @@ -113,14 +113,29 @@ init() -> ok. init_from_config() -> + FindBadNodeNames = fun + (Name, BadNames) when is_atom(Name) -> BadNames; + (Name, BadNames) -> [Name | BadNames] + end, {TryNodes, NodeType} = case application:get_env(rabbit, cluster_nodes) of + {ok, {Nodes, Type} = Config} + when is_list(Nodes) andalso (Type == disc orelse Type == ram) -> + case lists:foldl(FindBadNodeNames, [], Nodes) of + [] -> Config; + BadNames -> e({invalid_cluster_node_names, BadNames}) + end; + {ok, {_, BadType}} when BadType /= disc andalso BadType /= ram -> + e({invalid_cluster_node_type, BadType}); {ok, Nodes} when is_list(Nodes) -> %% The legacy syntax (a nodes list without the node %% type) is unsupported. - e(cluster_node_type_mandatory); - {ok, Config} -> - Config + case lists:foldl(FindBadNodeNames, [], Nodes) of + [] -> e(cluster_node_type_mandatory); + BadNames -> e(invalid_cluster_nodes_conf) + end; + {ok, _} -> + e(invalid_cluster_nodes_conf) end, case TryNodes of [] -> init_db_and_upgrade([node()], disc, false); @@ -829,6 +844,20 @@ nodes_excl_me(Nodes) -> Nodes -- [node()]. e(Tag) -> throw({error, {Tag, error_description(Tag)}}). +error_description({invalid_cluster_node_names, BadNames}) -> + "In the 'cluster_nodes' configuration key, the following node names " + "are invalid: " ++ lists:flatten(io_lib:format("~p", [BadNames])); +error_description({invalid_cluster_node_type, BadType}) -> + "In the 'cluster_nodes' configuration key, the node type is invalid " + "(expected 'disc' or 'ram'): " ++ + lists:flatten(io_lib:format("~p", [BadType])); +error_description(cluster_node_type_mandatory) -> + "The 'cluster_nodes' configuration key must indicate the node type: " + "either {[...], disc} or {[...], ram}"; +error_description(invalid_cluster_nodes_conf) -> + "The 'cluster_nodes' configuration key is invalid, it must be of the " + "form {[Nodes], Type}, where Nodes is a list of node names and " + "Type is either 'disc' or 'ram'"; error_description(clustering_only_disc_node) -> "You cannot cluster a node if it is the only disc node in its existing " " cluster. If new nodes joined while this node was offline, use " @@ -856,7 +885,4 @@ error_description(removing_node_from_offline_node) -> "To remove a node remotely from an offline node, the node you are removing " "from must be a disc node and all the other nodes must be offline."; error_description(no_running_cluster_nodes) -> - "You cannot leave a cluster if no online nodes are present."; -error_description(cluster_node_type_mandatory) -> - "The 'cluster_nodes' configuration key must indicate the node type: " - "either {[...], disc} or {[...], ram}". + "You cannot leave a cluster if no online nodes are present.". -- cgit v1.2.1 From dbc16de3e4b67c2e90ccb8b92b396a4c2862a738 Mon Sep 17 00:00:00 2001 From: Jean-Sebastien Pedron Date: Mon, 24 Nov 2014 17:40:23 +0100 Subject: When reporting invalid cluster node names, keep configuration order While here, fix an unused variable warning. --- src/rabbit_mnesia.erl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rabbit_mnesia.erl b/src/rabbit_mnesia.erl index 8fbacdae..91a8b140 100644 --- a/src/rabbit_mnesia.erl +++ b/src/rabbit_mnesia.erl @@ -121,7 +121,7 @@ init_from_config() -> case application:get_env(rabbit, cluster_nodes) of {ok, {Nodes, Type} = Config} when is_list(Nodes) andalso (Type == disc orelse Type == ram) -> - case lists:foldl(FindBadNodeNames, [], Nodes) of + case lists:foldr(FindBadNodeNames, [], Nodes) of [] -> Config; BadNames -> e({invalid_cluster_node_names, BadNames}) end; @@ -130,9 +130,9 @@ init_from_config() -> {ok, Nodes} when is_list(Nodes) -> %% The legacy syntax (a nodes list without the node %% type) is unsupported. - case lists:foldl(FindBadNodeNames, [], Nodes) of - [] -> e(cluster_node_type_mandatory); - BadNames -> e(invalid_cluster_nodes_conf) + case lists:foldr(FindBadNodeNames, [], Nodes) of + [] -> e(cluster_node_type_mandatory); + _ -> e(invalid_cluster_nodes_conf) end; {ok, _} -> e(invalid_cluster_nodes_conf) -- cgit v1.2.1 From 5122b4cc7a74bea9fe0a00bf4ac3c818b1f4259c Mon Sep 17 00:00:00 2001 From: Jean-Sebastien Pedron Date: Wed, 26 Nov 2014 12:35:49 +0100 Subject: During startup, log statistics after modules were hipe-compiled A similar message was already displayed on stdout. A warning was also logged when HiPE was desired but unavailable. To be consistent with the "HiPE enabled" case, the same warning is now displayed on stdout too. --- src/rabbit.erl | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/rabbit.erl b/src/rabbit.erl index 664da206..ca1d5ba8 100644 --- a/src/rabbit.erl +++ b/src/rabbit.erl @@ -243,15 +243,19 @@ maybe_hipe_compile() -> {ok, Want} = application:get_env(rabbit, hipe_compile), Can = code:which(hipe) =/= non_existing, case {Want, Can} of - {true, true} -> hipe_compile(), - true; + {true, true} -> hipe_compile(); {true, false} -> false; - {false, _} -> true + {false, _} -> {ok, disabled} end. -warn_if_hipe_compilation_failed(true) -> +warn_if_hipe_compilation_failed({ok, disabled}) -> ok; +warn_if_hipe_compilation_failed({ok, Count, Duration}) -> + rabbit_log:info( + "HiPE in use: compiled ~B modules in ~Bs.~n", [Count, Duration]); warn_if_hipe_compilation_failed(false) -> + io:format( + "~nNot HiPE compiling: HiPE not found in this Erlang installation.~n"), rabbit_log:warning( "Not HiPE compiling: HiPE not found in this Erlang installation.~n"). @@ -276,8 +280,9 @@ hipe_compile() -> {'DOWN', MRef, process, _, Reason} -> exit(Reason) end || {_Pid, MRef} <- PidMRefs], T2 = erlang:now(), - io:format("|~n~nCompiled ~B modules in ~Bs~n", - [Count, timer:now_diff(T2, T1) div 1000000]). + Duration = timer:now_diff(T2, T1) div 1000000, + io:format("|~n~nCompiled ~B modules in ~Bs~n", [Count, Duration]), + {ok, Count, Duration}. split(L, N) -> split0(L, [[] || _ <- lists:seq(1, N)]). -- cgit v1.2.1 -- cgit v1.2.1 -- cgit v1.2.1 From 943bff538149eb83c96ec44566958d82ee394356 Mon Sep 17 00:00:00 2001 From: Simon MacMullen Date: Wed, 26 Nov 2014 12:20:43 +0000 Subject: Rename this since it's not quite just success / failure any more. --- src/rabbit.erl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rabbit.erl b/src/rabbit.erl index ca1d5ba8..40f24efc 100644 --- a/src/rabbit.erl +++ b/src/rabbit.erl @@ -248,12 +248,12 @@ maybe_hipe_compile() -> {false, _} -> {ok, disabled} end. -warn_if_hipe_compilation_failed({ok, disabled}) -> +log_hipe_result({ok, disabled}) -> ok; -warn_if_hipe_compilation_failed({ok, Count, Duration}) -> +log_hipe_result({ok, Count, Duration}) -> rabbit_log:info( "HiPE in use: compiled ~B modules in ~Bs.~n", [Count, Duration]); -warn_if_hipe_compilation_failed(false) -> +log_hipe_result(false) -> io:format( "~nNot HiPE compiling: HiPE not found in this Erlang installation.~n"), rabbit_log:warning( @@ -312,9 +312,9 @@ start() -> boot() -> start_it(fun() -> ok = ensure_application_loaded(), - Success = maybe_hipe_compile(), + HipeResult = maybe_hipe_compile(), ok = ensure_working_log_handlers(), - warn_if_hipe_compilation_failed(Success), + log_hipe_result(HipeResult), rabbit_node_monitor:prepare_cluster_status_files(), ok = rabbit_upgrade:maybe_upgrade_mnesia(), %% It's important that the consistency check happens after -- cgit v1.2.1