From f59cdbbd7ac5d962f7f7661b275ac2cbe26bfe90 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Fri, 23 Jan 2026 14:01:52 -0600 Subject: [PATCH] credo fixes --- lib/snmp_mgr.ex | 29 ++++--- lib/snmpkit/snmp_lib/manager.ex | 51 ++++++------ lib/snmpkit/snmp_lib/mib/compiler.ex | 9 ++- lib/snmpkit/snmp_lib/mib/parser.ex | 95 ++++++++++------------- lib/snmpkit/snmp_lib/mib/utilities.ex | 2 +- lib/snmpkit/snmp_lib/pdu/builder.ex | 3 +- lib/snmpkit/snmp_lib/security/usm.ex | 8 +- lib/snmpkit/snmp_lib/walker.ex | 3 +- lib/snmpkit/snmp_mgr/core.ex | 3 +- lib/snmpkit/snmp_mgr/format.ex | 9 ++- lib/snmpkit/snmp_mgr/mib.ex | 66 +++++++--------- lib/snmpkit/snmp_mgr/types.ex | 4 +- lib/snmpkit/snmp_mgr/walk.ex | 3 +- test/snmpkit/snmp_lib/v3_encoder_test.exs | 5 +- 14 files changed, 142 insertions(+), 148 deletions(-) diff --git a/lib/snmp_mgr.ex b/lib/snmp_mgr.ex index eb346013..e0a73ebc 100644 --- a/lib/snmp_mgr.ex +++ b/lib/snmp_mgr.ex @@ -14,6 +14,7 @@ defmodule SnmpKit.SnmpMgr do - `set/4` - Set OID value """ + alias SnmpKit.SnmpLib.OID alias SnmpKit.SnmpMgr.Bulk alias SnmpKit.SnmpMgr.Config alias SnmpKit.SnmpMgr.Core @@ -200,22 +201,20 @@ defmodule SnmpKit.SnmpMgr do - `opts` - Options including :community, :timeout """ def get_column(target, table_oid, column, opts \\ []) do - case resolve_oid_if_needed(table_oid) do - {:ok, resolved_table_oid} -> - column_oid = - if is_integer(column) do - resolved_table_oid ++ [1, column] - else - case MIB.resolve(column) do - {:ok, oid} -> oid - error -> error - end - end + with {:ok, resolved_table_oid} <- resolve_oid_if_needed(table_oid), + {:ok, column_oid} <- build_column_oid(resolved_table_oid, column) do + walk(target, column_oid, opts) + end + end - walk(target, column_oid, opts) + defp build_column_oid(table_oid, column) when is_integer(column) do + {:ok, table_oid ++ [1, column]} + end - error -> - error + defp build_column_oid(_table_oid, column) do + case MIB.resolve(column) do + {:ok, oid} -> {:ok, oid} + error -> error end end @@ -310,7 +309,7 @@ defmodule SnmpKit.SnmpMgr do # Private helper function defp resolve_oid_if_needed(oid) when is_binary(oid) do - case SnmpKit.SnmpLib.OID.string_to_list(oid) do + case OID.string_to_list(oid) do {:ok, oid_list} -> {:ok, oid_list} diff --git a/lib/snmpkit/snmp_lib/manager.ex b/lib/snmpkit/snmp_lib/manager.ex index 57ddc1b8..3415d1c1 100644 --- a/lib/snmpkit/snmp_lib/manager.ex +++ b/lib/snmpkit/snmp_lib/manager.ex @@ -45,6 +45,7 @@ defmodule SnmpKit.SnmpLib.Manager do alias SnmpKit.SnmpLib.OID alias SnmpKit.SnmpLib.PDU alias SnmpKit.SnmpLib.Transport + alias SnmpKit.SnmpLib.Utils require Logger @@ -476,7 +477,7 @@ defmodule SnmpKit.SnmpLib.Manager do # Parse target to handle both host:port strings and :port option {parsed_host, parsed_port} = - case SnmpKit.SnmpLib.Utils.parse_target(host) do + case Utils.parse_target(host) do {:ok, %{host: h, port: p}} -> # Check if host contained a port specification if host_contains_port?(host) do @@ -897,17 +898,7 @@ defmodule SnmpKit.SnmpLib.Manager do cond do # RFC 3986 bracket notation: [IPv6]:port String.starts_with?(host, "[") and String.contains?(host, "]:") -> - # Check if it's valid [addr]:port format - case String.split(host, "]:", parts: 2) do - [_ipv6_part, port_part] -> - case Integer.parse(port_part) do - {port, ""} when port > 0 and port <= 65_535 -> true - _ -> false - end - - _ -> - false - end + check_bracketed_ipv6_port(host) # Plain IPv6 addresses (contain :: or multiple colons) - no port embedded String.contains?(host, "::") -> @@ -918,17 +909,7 @@ defmodule SnmpKit.SnmpLib.Manager do # IPv4 or simple hostname with port String.contains?(host, ":") -> - # Single colon - check if part after colon looks like a port number - case String.split(host, ":", parts: 2) do - [_host_part, port_part] -> - case Integer.parse(port_part) do - {port, ""} when port > 0 and port <= 65_535 -> true - _ -> false - end - - _ -> - false - end + check_simple_host_port(host) # No colon at all true -> @@ -938,6 +919,30 @@ defmodule SnmpKit.SnmpLib.Manager do defp host_contains_port?(_), do: false + # Check if bracketed IPv6 address has valid port + defp check_bracketed_ipv6_port(host) do + case String.split(host, "]:", parts: 2) do + [_ipv6_part, port_part] -> valid_port?(port_part) + _ -> false + end + end + + # Check if simple host:port format has valid port + defp check_simple_host_port(host) do + case String.split(host, ":", parts: 2) do + [_host_part, port_part] -> valid_port?(port_part) + _ -> false + end + end + + # Validate port number + defp valid_port?(port_string) do + case Integer.parse(port_string) do + {port, ""} when port > 0 and port <= 65_535 -> true + _ -> false + end + end + # Check if all results failed with the same network-related error defp check_for_global_failure(results) do errors = diff --git a/lib/snmpkit/snmp_lib/mib/compiler.ex b/lib/snmpkit/snmp_lib/mib/compiler.ex index 5fc92cde..75b34cc4 100644 --- a/lib/snmpkit/snmp_lib/mib/compiler.ex +++ b/lib/snmpkit/snmp_lib/mib/compiler.ex @@ -35,6 +35,7 @@ defmodule SnmpKit.SnmpLib.MIB.Compiler do alias SnmpKit.SnmpLib.MIB.Error alias SnmpKit.SnmpLib.MIB.Logger + alias SnmpKit.SnmpLib.MIB.Parser @type compile_opts :: [ output_dir: Path.t(), @@ -116,7 +117,7 @@ defmodule SnmpKit.SnmpLib.MIB.Compiler do opts = Keyword.merge(@default_opts, opts) # The Parser module implements the full compilation pipeline - case SnmpKit.SnmpLib.MIB.Parser.parse(mib_content) do + case Parser.parse(mib_content) do {:ok, mib} -> # Convert parser output to compiled_mib format compiled = %{ @@ -182,7 +183,7 @@ defmodule SnmpKit.SnmpLib.MIB.Compiler do end defp validate_compiled_mib(compiled, _opts) do - # TODO: Add semantic validation + # Note: Semantic validation not yet implemented # For now, just return the compiled MIB {:ok, compiled} end @@ -270,8 +271,8 @@ defmodule SnmpKit.SnmpLib.MIB.Compiler do # Private helper functions - # TODO: The following functions are for future MIB compilation features - # They are commented out to avoid Dialyzer warnings until MIB compilation is fully implemented + # Note: The following functions are placeholders for future MIB compilation features. + # They are commented out to avoid Dialyzer warnings until MIB compilation is fully implemented. # defp post_process_mib(mib, opts) do # with {:ok, validated_mib} <- validate_mib(mib, opts), diff --git a/lib/snmpkit/snmp_lib/mib/parser.ex b/lib/snmpkit/snmp_lib/mib/parser.ex index 60568ae7..c7b7be36 100644 --- a/lib/snmpkit/snmp_lib/mib/parser.ex +++ b/lib/snmpkit/snmp_lib/mib/parser.ex @@ -122,53 +122,43 @@ defmodule SnmpKit.SnmpLib.MIB.Parser do This is the production MIB parser with 100% native compatibility. """ def parse(mib_content) when is_binary(mib_content) do - # First ensure parser is compiled - case init_parser() do - {:ok, parser_module} -> - # No preprocessing needed - we now have 100% native parsing success - processed_content = mib_content + with {:ok, parser_module} <- init_parser(), + {:ok, tokens} <- tokenize(mib_content), + {:ok, parse_tree} <- parse_with_module(parser_module, tokens) do + {:ok, convert_to_elixir_format(parse_tree)} + else + {:error, {:tokenize, reason}} -> + Logger.debug("Tokenize failed: #{inspect(reason)}") + {:error, reason} - # Tokenize the input - case tokenize(processed_content) do - {:ok, tokens} -> - # Parse using the generated parser - case apply(parser_module, :parse, [tokens]) do - {:ok, parse_tree} -> - {:ok, convert_to_elixir_format(parse_tree)} - - {:error, reason} -> - Logger.debug("Parse failed: #{inspect(reason)}") - # Direct error return - 100% native parsing - {:error, convert_error_to_string(reason)} - end - - {:error, reason} -> - Logger.debug("Tokenize failed: #{inspect(reason)}") - {:error, reason} - end + {:error, {:parse, reason}} -> + Logger.debug("Parse failed: #{inspect(reason)}") + {:error, convert_error_to_string(reason)} {:error, reason} -> {:error, reason} end end + defp parse_with_module(parser_module, tokens) do + case apply(parser_module, :parse, [tokens]) do + {:ok, parse_tree} -> {:ok, parse_tree} + {:error, reason} -> {:error, {:parse, reason}} + end + end + @doc """ Parse pre-tokenized MIB tokens using native grammar parsing. This function takes tokens directly without tokenizing. """ def parse_tokens(tokens) when is_list(tokens) do - # First ensure parser is compiled - case init_parser() do - {:ok, parser_module} -> - # Parse using the generated parser - case apply(parser_module, :parse, [tokens]) do - {:ok, parse_tree} -> - {:ok, convert_to_elixir_format(parse_tree)} - - {:error, reason} -> - Logger.debug("Parse failed: #{inspect(reason)}") - {:error, convert_error_to_string(reason)} - end + with {:ok, parser_module} <- init_parser(), + {:ok, parse_tree} <- parse_with_module(parser_module, tokens) do + {:ok, convert_to_elixir_format(parse_tree)} + else + {:error, {:parse, reason}} -> + Logger.debug("Parse failed: #{inspect(reason)}") + {:error, convert_error_to_string(reason)} {:error, reason} -> {:error, reason} @@ -554,24 +544,7 @@ defmodule SnmpKit.SnmpLib.MIB.Parser do Logger.debug("Processing #{Path.basename(directory)}: #{length(mib_files)} files") - results = - Enum.map(mib_files, fn file -> - file_path = Path.join(directory, file) - - case File.read(file_path) do - {:ok, content} -> - case parse(content) do - {:ok, mib_data} -> - {:success, file, mib_data} - - {:error, reason} -> - {:error, file, reason} - end - - {:error, reason} -> - {:error, file, {:file_read_error, reason}} - end - end) + results = Enum.map(mib_files, &process_mib_file(directory, &1)) successes = Enum.filter(results, &(elem(&1, 0) == :success)) failures = Enum.filter(results, &(elem(&1, 0) == :error)) @@ -611,6 +584,22 @@ defmodule SnmpKit.SnmpLib.MIB.Parser do end end + # Process a single MIB file + defp process_mib_file(directory, file) do + file_path = Path.join(directory, file) + + with {:ok, content} <- File.read(file_path), + {:ok, mib_data} <- parse(content) do + {:success, file, mib_data} + else + {:error, {:file_read_error, _}} = error -> + error + + {:error, reason} -> + {:error, file, reason} + end + end + # Helper to identify MIB files defp mib_file?(filename) do String.ends_with?(filename, ".mib") or diff --git a/lib/snmpkit/snmp_lib/mib/utilities.ex b/lib/snmpkit/snmp_lib/mib/utilities.ex index 4512f7fe..ad3820e4 100644 --- a/lib/snmpkit/snmp_lib/mib/utilities.ex +++ b/lib/snmpkit/snmp_lib/mib/utilities.ex @@ -400,7 +400,7 @@ defmodule SnmpKit.SnmpLib.MIB.Utilities do end defp validate_integer_constraints([{:enum, _values} | rest]) do - # TODO: Validate enum values + # Note: Enum value validation not yet implemented validate_integer_constraints(rest) end diff --git a/lib/snmpkit/snmp_lib/pdu/builder.ex b/lib/snmpkit/snmp_lib/pdu/builder.ex index 3f3f9b89..512c32ed 100644 --- a/lib/snmpkit/snmp_lib/pdu/builder.ex +++ b/lib/snmpkit/snmp_lib/pdu/builder.ex @@ -7,6 +7,7 @@ defmodule SnmpKit.SnmpLib.PDU.Builder do """ alias SnmpKit.SnmpLib.PDU.Constants + alias SnmpKit.SnmpLib.PDU.Decoder @type snmp_version :: Constants.snmp_version() @type pdu_type :: Constants.pdu_type() @@ -245,7 +246,7 @@ defmodule SnmpKit.SnmpLib.PDU.Builder do @spec validate_community(binary(), binary()) :: :ok | {:error, atom()} def validate_community(encoded_message, expected_community) when is_binary(encoded_message) and is_binary(expected_community) do - case SnmpKit.SnmpLib.PDU.Decoder.decode_message(encoded_message) do + case Decoder.decode_message(encoded_message) do {:ok, %{community: community}} when community == expected_community -> :ok {:ok, %{community: _other}} -> {:error, :invalid_community} {:error, _reason} -> {:error, :decode_failed} diff --git a/lib/snmpkit/snmp_lib/security/usm.ex b/lib/snmpkit/snmp_lib/security/usm.ex index 5bca2718..cb23c16d 100644 --- a/lib/snmpkit/snmp_lib/security/usm.ex +++ b/lib/snmpkit/snmp_lib/security/usm.ex @@ -520,9 +520,9 @@ defmodule SnmpKit.SnmpLib.Security.USM do ## Private Implementation - # TODO: The following helper functions are for future SNMPv3 support + # Note: The following helper functions are placeholders for future SNMPv3 support. # They are commented out to avoid Dialyzer warnings until a proper - # SNMPv3 encoder is implemented that handles scoped_pdu and security_parameters + # SNMPv3 encoder is implemented that handles scoped_pdu and security_parameters. # defp build_discovery_request do # # SNMPv3 discovery message with empty security parameters @@ -628,7 +628,7 @@ defmodule SnmpKit.SnmpLib.Security.USM do # end # end - # TODO: Additional SNMPv3 helper functions - commented out until proper v3 support is implemented + # Note: Additional SNMPv3 helper functions - commented out until proper v3 support is implemented # defp determine_message_flags(:no_auth_no_priv) do # {:ok, %{auth_flag: false, priv_flag: false, reportable_flag: false}} @@ -679,7 +679,7 @@ defmodule SnmpKit.SnmpLib.Security.USM do # {:ok, params} # end - # TODO: SNMPv3 message building - commented out until proper v3 encoder is implemented + # Note: SNMPv3 message building - commented out until proper v3 encoder is implemented # defp build_secure_message(scoped_pdu, security_params, flags) do # message = %{ # message_id: :rand.uniform(2_147_483_647), diff --git a/lib/snmpkit/snmp_lib/walker.ex b/lib/snmpkit/snmp_lib/walker.ex index f81269a7..180a2281 100644 --- a/lib/snmpkit/snmp_lib/walker.ex +++ b/lib/snmpkit/snmp_lib/walker.ex @@ -44,6 +44,7 @@ defmodule SnmpKit.SnmpLib.Walker do """ alias SnmpKit.SnmpLib.Manager + alias SnmpKit.SnmpLib.OID require Logger @@ -594,7 +595,7 @@ defmodule SnmpKit.SnmpLib.Walker do defp normalize_oid(oid) when is_list(oid), do: oid defp normalize_oid(oid) when is_binary(oid) do - case SnmpKit.SnmpLib.OID.string_to_list(oid) do + case OID.string_to_list(oid) do {:ok, oid_list} -> oid_list {:error, _} -> [1, 3, 6, 1] end diff --git a/lib/snmpkit/snmp_mgr/core.ex b/lib/snmpkit/snmp_mgr/core.ex index 45e52fdf..4a8903cb 100644 --- a/lib/snmpkit/snmp_mgr/core.ex +++ b/lib/snmpkit/snmp_mgr/core.ex @@ -22,6 +22,7 @@ defmodule SnmpKit.SnmpMgr.Core do alias SnmpKit.SnmpLib.Manager alias SnmpKit.SnmpLib.OID + alias SnmpKit.SnmpMgr.MIB alias SnmpKit.SnmpMgr.Target @type snmp_result :: {:ok, term()} | {:error, atom() | tuple()} @@ -519,7 +520,7 @@ defmodule SnmpKit.SnmpMgr.Core do {:error, _} -> # Fall back to MIB GenServer for container OIDs like "system", "interfaces" - case SnmpKit.SnmpMgr.MIB.resolve(trimmed) do + case MIB.resolve(trimmed) do {:ok, oid_list} -> {:ok, oid_list} error -> error end diff --git a/lib/snmpkit/snmp_mgr/format.ex b/lib/snmpkit/snmp_mgr/format.ex index 0a13dca8..5958371b 100644 --- a/lib/snmpkit/snmp_mgr/format.ex +++ b/lib/snmpkit/snmp_mgr/format.ex @@ -29,11 +29,12 @@ defmodule SnmpKit.SnmpMgr.Format do # => {"1.3.6.1.2.1.1.1.0", :octet_string, "Cisco IOS Router"} """ - # Delegate core formatting functions to SnmpKit.SnmpLib.Types - # These have negligible performance overhead (~1-2ns per call) - alias SnmpKit.SnmpLib.OID alias SnmpKit.SnmpLib.Types + alias SnmpKit.SnmpMgr.MIB + + # Delegate core formatting functions to SnmpKit.SnmpLib.Types + # These have negligible performance overhead (~1-2ns per call) @doc """ Formats timeticks (hundredths of seconds) into human-readable uptime. @@ -247,7 +248,7 @@ defmodule SnmpKit.SnmpMgr.Format do name = if include_names do try do - case SnmpKit.SnmpMgr.MIB.reverse_lookup(oid_string) do + case MIB.reverse_lookup(oid_string) do {:ok, mib_name} -> mib_name _ -> nil end diff --git a/lib/snmpkit/snmp_mgr/mib.ex b/lib/snmpkit/snmp_mgr/mib.ex index d0d2ea44..ee524b89 100644 --- a/lib/snmpkit/snmp_mgr/mib.ex +++ b/lib/snmpkit/snmp_mgr/mib.ex @@ -766,51 +766,45 @@ defmodule SnmpKit.SnmpMgr.MIB do # Derive base syntax from parsed syntax term defp syntax_base_from(syntax) do case syntax do - :integer -> - :integer + atom when is_atom(atom) -> + syntax_atom_to_base(atom) - :octet_string -> - :octet_string - - :object_identifier -> - :object_identifier - - :timeticks -> - :timeticks - - :counter32 -> - :counter32 - - :counter64 -> - :counter64 - - :gauge32 -> - :gauge32 - - :ip_address -> - :ip_address - - {:integer, _} -> - :integer - - {:octet_string, _} -> - :octet_string - - {:object_identifier, _} -> - :object_identifier + {type, _} when type in [:integer, :octet_string, :object_identifier] -> + type {:type, t} when is_atom(t) -> - case t do - :"octet string" -> :octet_string - :"object identifier" -> :object_identifier - other -> other - end + syntax_type_to_base(t) _ -> nil end end + # Map simple syntax atoms to base types + defp syntax_atom_to_base(atom) do + base_types = [ + :integer, + :octet_string, + :object_identifier, + :timeticks, + :counter32, + :counter64, + :gauge32, + :ip_address + ] + + if atom in base_types, do: atom + end + + # Handle typed syntax conversions + defp syntax_type_to_base(type) do + case type do + :"octet string" -> :octet_string + :"object identifier" -> :object_identifier + other -> other + end + end + # Best-effort textual convention detection from syntax term defp textual_convention_from(syntax) do case syntax do diff --git a/lib/snmpkit/snmp_mgr/types.ex b/lib/snmpkit/snmp_mgr/types.ex index bf9df0f2..d8f8f744 100644 --- a/lib/snmpkit/snmp_mgr/types.ex +++ b/lib/snmpkit/snmp_mgr/types.ex @@ -6,6 +6,8 @@ defmodule SnmpKit.SnmpMgr.Types do and explicit type specification. """ + alias SnmpKit.SnmpLib.OID + @doc """ Encodes a value for SNMP with optional type specification. @@ -245,7 +247,7 @@ defmodule SnmpKit.SnmpMgr.Types do end defp encode_with_explicit_type(value, :objectIdentifier) when is_binary(value) do - case SnmpKit.SnmpLib.OID.string_to_list(value) do + case OID.string_to_list(value) do {:ok, oid_list} -> {:ok, {:objectIdentifier, oid_list}} {:error, _reason} -> {:error, {:unsupported_type_conversion, value, :objectIdentifier}} end diff --git a/lib/snmpkit/snmp_mgr/walk.ex b/lib/snmpkit/snmp_mgr/walk.ex index 0a09fa94..610e0503 100644 --- a/lib/snmpkit/snmp_mgr/walk.ex +++ b/lib/snmpkit/snmp_mgr/walk.ex @@ -6,6 +6,7 @@ defmodule SnmpKit.SnmpMgr.Walk do using the GETNEXT operation repeatedly until the end of the subtree. """ + alias SnmpKit.SnmpLib.OID alias SnmpKit.SnmpMgr.Bulk alias SnmpKit.SnmpMgr.Core @@ -119,7 +120,7 @@ defmodule SnmpKit.SnmpMgr.Walk do defp walk_from_oid(target, current_oid, root_oid, acc, remaining, opts) when remaining > 0 do case Core.send_get_next_request(target, current_oid, opts) do {:ok, {next_oid_string, type, value}} -> - case SnmpKit.SnmpLib.OID.string_to_list(next_oid_string) do + case OID.string_to_list(next_oid_string) do {:ok, next_oid} -> if still_in_scope?(next_oid, root_oid) do new_acc = [{next_oid_string, type, value} | acc] diff --git a/test/snmpkit/snmp_lib/v3_encoder_test.exs b/test/snmpkit/snmp_lib/v3_encoder_test.exs index 355feed4..7a088d1b 100644 --- a/test/snmpkit/snmp_lib/v3_encoder_test.exs +++ b/test/snmpkit/snmp_lib/v3_encoder_test.exs @@ -344,9 +344,8 @@ defmodule SnmpKit.SnmpLib.PDU.V3EncoderTest do test "handles large string values" do # Note: Known limitation - very large encrypted messages (>500 bytes) may be truncated - # due to encryption/decryption boundary handling. This test verifies that large messages - # can be processed without crashing, which is the primary requirement. - # TODO: Fix encryption/decryption for very large payloads (issue with ASN.1 boundaries) + # due to encryption/decryption boundary handling in ASN.1. This test verifies that large + # messages can be processed without crashing, which is the primary requirement. # Reduced size to avoid encryption limits large_string = String.duplicate("X", 200)