-
Notifications
You must be signed in to change notification settings - Fork 0
Extract mumble_protocol library and refactor server architecture #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| type = "rebar3" | ||
|
|
||
| [otp] | ||
| exclude_apps = ["megaco"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| # AGENTS.md - erlmur Development Guide | ||
|
|
||
| ## Build Commands | ||
|
|
||
| ```bash | ||
| just build # Format, compile, and build release | ||
| just dev # Start development shell | ||
| just test # Run all tests (xref, dialyzer, proper, eunit, ct) | ||
| just eunit # Run eunit tests | ||
| just ct # Run common test suite | ||
| just proper # Run property-based tests (1000 iterations) | ||
| just dialyzer # Static analysis | ||
| just xref # Cross-reference analysis | ||
| just format # Format code with erlfmt | ||
| just clean # Clean build artifacts | ||
| ``` | ||
|
|
||
| ## Code Style Guidelines | ||
|
|
||
| ### Imports and Includes | ||
| - Use `-include("module.hrl")` for local records | ||
| - Use `-include_lib("app/include/module.hrl")` for other app headers | ||
| - Include gpb headers last: `-include("Mumble_gpb.hrl")` | ||
|
|
||
| ### Module Documentation | ||
| - All modules must have `-moduledoc` with description | ||
| - Export functions must have `-doc` with input/output specs | ||
| - Use `-spec` for all exported functions with proper types | ||
| - Callback functions need `-doc` in behaviour definitions | ||
|
|
||
| ### Records and State | ||
| - Use records for process state (`-record(state, {...})`) | ||
| - Use maps for protocol messages | ||
| - Access record fields via `#state.field` or `#state{field = Value}` | ||
| - Define opaque types with `-opaque` and `-export_type` | ||
|
|
||
| ### Naming Conventions | ||
| - Modules: `snake_case.erl` | ||
| - Functions: `snake_case` | ||
| - Records: `snake_case` | ||
| - Constants: `UPPER_SNAKE` | ||
| - Variables: `UpperCamelCase` | ||
| - Message types: `'CamelCase'` | ||
|
|
||
| ### Function Structure | ||
| ```erlang | ||
| -spec function_name(arg1(), arg2()) -> return_type(). | ||
| function_name(Arg1, Arg2) -> | ||
| case do_something(Arg1) of | ||
| {ok, Result} -> | ||
| handle_result(Result, Arg2); | ||
| {error, Reason} -> | ||
| handle_error(Reason) | ||
| end. | ||
| ``` | ||
|
|
||
| ### Guards and Pattern Matching | ||
| - Use guards for input validation | ||
| - Pattern match in function heads when possible | ||
| - Use `when` guards for type/size checks | ||
| - Prefer specific patterns over catch-all clauses | ||
|
|
||
| ### Error Handling | ||
| - Return `{ok, Value}` or `{error, Reason}` for recoverable errors | ||
| - Use `try...of...else` for exceptions that need cleanup | ||
| - Log errors with appropriate level (`logger:error`, `logger:warning`) | ||
| - Match specific errors before general ones | ||
|
|
||
| ### gen_statem Conventions | ||
| - Use named state functions (`state_name/3`) | ||
| - Export callback mode as list: `callback_mode() -> [state_functions, state_enter]` | ||
| - Handle `enter` events for state transitions | ||
| - Return `{keep_state, Data}` or `{next_state, Name, Data}` | ||
| - Use `?TIMEOUT` constant for timeouts | ||
|
|
||
| ### Logging | ||
| - Use `logger:info/notice/debug` for normal events | ||
| - Use `logger:warning` for unexpected but handled situations | ||
| - Use `logger:error` for failures requiring attention | ||
| - Include context in log messages: `"Failed to ~p: ~p", [Action, Reason]` | ||
|
|
||
| ### Testing | ||
| - EUnit: Use `?_test(Fun)` and `?_assert*` macros | ||
| - Common Test: Define `-suite()` and `-export([suite/0, ...])` | ||
| - Proper: Define properties with `prop_*` and `?FORALL` | ||
| - Mocks: Use Meck for OTP module mocking | ||
|
|
||
| ### Protocol Messages | ||
| - Messages are maps with `message_type => 'MessageName'` | ||
| - Use atoms for message types (matching GPB definitions) | ||
| - Validate required fields before processing | ||
| - Include version information in handshake | ||
|
|
||
| ### Log Level Configuration | ||
|
|
||
| The log level can be configured via: | ||
|
|
||
| 1. **Environment variable** (highest priority): | ||
| ```bash | ||
| ERLMUR_LOG_LEVEL=info rebar3 shell | ||
| ERLMUR_LOG_LEVEL=notice rebar3 shell | ||
| ERLMUR_LOG_LEVEL=debug rebar3 shell | ||
| ``` | ||
|
|
||
| 2. **Application environment** (in sys.config): | ||
| ```erlang | ||
| [{erlmur, [{log_level, info}]}]. | ||
| ``` | ||
|
|
||
| 3. **Default**: `info` (shows all `logger:info` messages) | ||
|
|
||
| Available levels: `debug`, `info`, `notice`, `warning`, `error`, `critical`, `alert`, `emergency` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| -module(erlmur_app). | ||
|
|
||
| -moduledoc "Main application entry point for the erlmur server.\n\nThis " | ||
| "module defines the application behavior and is responsible " | ||
| "for starting\nand stopping the main supervisor tree.". | ||
|
|
||
| -behaviour(application). | ||
|
|
||
| %% Application callbacks | ||
| -export([start/2, stop/1]). | ||
|
|
||
| %% =================================================================== | ||
| %% Application callbacks | ||
| %% =================================================================== | ||
|
|
||
| start(_StartType, _StartArgs) -> | ||
| %% Configure log level: ERLMUR_LOG_LEVEL env var > application env > default (info) | ||
| LogLevel = case os:getenv("ERLMUR_LOG_LEVEL") of | ||
| false -> application:get_env(erlmur, log_level, info); | ||
| LevelStr -> list_to_atom(LevelStr) | ||
| end, | ||
| logger:set_primary_config(level, LogLevel), | ||
| logger:notice("[erlmur] Log level set to: ~p", [LogLevel]), | ||
|
|
||
| logger:info("[erlmur] Starting erlmur application..."), | ||
| PrivDir = code:priv_dir(erlmur), | ||
| Port = application:get_env(erlmur, listen_port, 64738), | ||
| CertFile = application:get_env(erlmur, cert_pem, filename:join(PrivDir, "server.pem")), | ||
| KeyFile = application:get_env(erlmur, key_pem, filename:join(PrivDir, "server.key")), | ||
| logger:info("PrivDir Port CertFile KeyFile",[PrivDir, Port,CertFile,KeyFile]), | ||
| logger:info("[erlmur] Starting Mumble server with SSL cert: ~s", [CertFile]), | ||
| %% Use erlmur_server_handler for production, not mock_mumble_handler | ||
| case mumble:start_server(CertFile, KeyFile, Port, erlmur_server_handler) of | ||
| {ok, ServerRef} -> | ||
| logger:info("[erlmur] Mumble server started: ~p", [ServerRef]), | ||
| erlmur_sup:start_link(), | ||
| logger:info("[erlmur] erlmur application started successfully"), | ||
| {ok, self()}; | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| {error, Reason} -> | ||
| logger:error("[erlmur] Failed to start Mumble server: ~p", [Reason]), | ||
| {error, Reason} | ||
| end. | ||
|
|
||
| stop(_State) -> | ||
| ok. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| -module(erlmur_server_handler). | ||
| -moduledoc """ | ||
| Implements the mumble_server_behaviour for the Erlmur server. | ||
|
|
||
| This module bridges the mumble_protocol connection handling with the | ||
| Erlmur application logic, handling authentication and message routing. | ||
| """. | ||
|
|
||
| -behaviour(mumble_server_behaviour). | ||
|
|
||
| %% mumble_server_behaviour callbacks | ||
| -export([init/1, authenticate/2, handle_msg/2, get_caps/1]). | ||
|
|
||
| -record(state, { | ||
| session_id :: pos_integer() | undefined, | ||
| username :: binary() | undefined | ||
| }). | ||
|
|
||
| %%%=================================================================== | ||
| %%% mumble_server_behaviour callbacks | ||
| %%%=================================================================== | ||
|
|
||
| init(_Opts) -> | ||
| {ok, #state{}}. | ||
|
|
||
| authenticate(#{message_type := 'Authenticate', username := Username}, State) -> | ||
| %% MVP: Accept all users unconditionally | ||
| {ok, SessionId} = erlmur_user_manager:register_user(self(), Username), | ||
| UserInfo = #{session_id => SessionId, username => Username}, | ||
| logger:info("User ~s authenticated with session ~p", [Username, SessionId]), | ||
| {ok, UserInfo, State#state{session_id = SessionId, username = Username}}; | ||
| authenticate(Msg, State) -> | ||
| logger:warning("Invalid authenticate message: ~p", [Msg]), | ||
| {error, invalid_auth, State}. | ||
|
Comment on lines
+26
to
+34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unhandled error from Line 28 uses ♻️ Proposed fix authenticate(#{message_type := 'Authenticate', username := Username}, State) ->
- %% MVP: Accept all users unconditionally
- {ok, SessionId} = erlmur_user_manager:register_user(self(), Username),
- UserInfo = #{session_id => SessionId, username => Username},
- logger:info("User ~s authenticated with session ~p", [Username, SessionId]),
- {ok, UserInfo, State#state{session_id = SessionId, username = Username}};
+ %% MVP: Accept all users unconditionally
+ case erlmur_user_manager:register_user(self(), Username) of
+ {ok, SessionId} ->
+ UserInfo = #{session_id => SessionId, username => Username},
+ logger:info("User ~s authenticated with session ~p", [Username, SessionId]),
+ {ok, UserInfo, State#state{session_id = SessionId, username = Username}};
+ {error, Reason} ->
+ logger:error("Failed to register user ~s: ~p", [Username, Reason]),
+ {error, registration_failed, State}
+ end;🤖 Prompt for AI Agents |
||
|
|
||
| handle_msg(#{message_type := connection_status, status := established, session_id := SessionId}, State) -> | ||
| logger:info("Session ~p established", [SessionId]), | ||
| {ok, State}; | ||
|
|
||
| handle_msg(#{message_type := connection_status, status := udp_verified, session_id := SessionId}, State) -> | ||
| logger:info("UDP verified for session ~p", [SessionId]), | ||
| {ok, State}; | ||
|
|
||
| handle_msg(#{message_type := connection_status, status := udp_lost, session_id := SessionId}, State) -> | ||
| logger:info("UDP lost for session ~p, falling back to TCP", [SessionId]), | ||
| {ok, State}; | ||
|
|
||
| handle_msg(#{message_type := 'TextMessage', message := Message}, State = #state{session_id = SessionId}) -> | ||
| logger:info("Text message from session ~p: ~s", [SessionId, Message]), | ||
| erlmur_user_manager:broadcast_text(SessionId, Message), | ||
| {ok, State}; | ||
|
|
||
| handle_msg(#{message_type := 'UserState'}, State) -> | ||
| %% Ignore user state updates for MVP | ||
| {ok, State}; | ||
|
|
||
| handle_msg(#{message_type := 'ChannelState'}, State) -> | ||
| %% Ignore channel state updates for MVP | ||
| {ok, State}; | ||
|
|
||
| handle_msg(#{message_type := 'PermissionQuery'}, State) -> | ||
| %% Respond with default permissions for MVP | ||
| mumble_server_conn:send(self(), #{ | ||
| message_type => 'PermissionQuery', | ||
| channel_id => 0, | ||
| permissions => 16#FFFFFFFF %% All permissions | ||
| }), | ||
| {ok, State}; | ||
|
Comment on lines
+61
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: granting all permissions unconditionally.
🤖 Prompt for AI Agents |
||
|
|
||
| handle_msg(#{message_type := 'VoiceTarget'}, State) -> | ||
| %% Ignore voice target setup for MVP | ||
| {ok, State}; | ||
|
|
||
| handle_msg(Msg, State) -> | ||
| logger:debug("Unhandled message: ~p", [Msg]), | ||
| {ok, State}. | ||
|
|
||
| get_caps(_State) -> | ||
| #{ | ||
| major => 1, | ||
| minor => 2, | ||
| patch => 4, | ||
| release => <<"Erlmur MVP">>, | ||
| os => <<"Erlang/OTP">>, | ||
| os_version => erlang:system_info(otp_release) | ||
| }. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| -module(erlmur_sup). | ||
|
|
||
| -moduledoc("The main supervisor for the erlmur application."). | ||
|
|
||
| -behaviour(supervisor). | ||
|
|
||
| %% API | ||
| -export([start_link/0]). | ||
| %% Supervisor callbacks | ||
| -export([init/1]). | ||
|
|
||
| %% =================================================================== | ||
| %% API functions | ||
| %% =================================================================== | ||
|
|
||
| start_link() -> | ||
| supervisor:start_link({local, ?MODULE}, ?MODULE, []). | ||
|
|
||
| %% =================================================================== | ||
| %% Supervisor callbacks | ||
| %% =================================================================== | ||
|
|
||
| init([]) -> | ||
| logger:info("[erlmur_sup] Starting user manager..."), | ||
| UserManager = #{ | ||
| id => erlmur_user_manager, | ||
| start => {erlmur_user_manager, start_link, []}, | ||
| restart => permanent, | ||
| type => worker | ||
| }, | ||
| logger:info("[erlmur_sup] User manager started"), | ||
| {ok, {{one_for_one, 5, 10}, [UserManager]}}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logger format string has no placeholders — will crash or produce a logger error at runtime.
The format string
"PrivDir Port CertFile KeyFile"contains no~p/~sspecifiers, but a 4-element list is passed as the second argument. This will cause aloggerformatting error.Proposed fix
🤖 Prompt for AI Agents