Suggested fixes

CHANGELOG.md

@@ -19,3 +19,25 @@
 
 # 0.4.4
   - InfluxDB support added
+
+# TBD
+  - CODE QUALITY: Major RuboCop fixes across the codebase
+    - Added frozen string literal comments
+    - Fixed indentation and spacing issues
+    - Standardized string literals (single quotes)
+    - Improved method parameter formatting
+    - Fixed hash syntax and alignment
+    - Enhanced error handling and rescue clauses
+    - Added proper documentation comments
+    - Fixed code style and layout issues
+    - Improved code readability and maintainability
+  - FIX: Improved thread safety and management in core system
+  - FIX: Added proper database transaction handling
+  - FIX: Fixed SQL injection vulnerability in up_since? method
+  - FIX: Added comprehensive error handling and logging
+  - FIX: Fixed database connection pool configuration
+  - FEATURE: Added configurable database reset option
+  - FEATURE: Added automatic database vacuum after purge
+  - DOCS: Added comprehensive code documentation
+  - DOCS: Added method parameter and return type documentation
+

README.md

@@ -16,3 +16,28 @@
 
 ## Todo
   - Get rid of Sequel+SQLite share Hash or Array instead?
+
+### Code Quality Improvements (RuboCop Findings)
+
+#### Critical Security Issues
+  - Replace Kernel#open with safer alternatives
+  - Remove or secure any eval usage
+
+#### High Priority Code Quality
+  - Refactor long methods (21 instances)
+  - Reduce method complexity (13 instances)
+  - Fix complex control flow (2 instances)
+  - Replace Exception rescue with StandardError
+
+#### Style and Layout
+  - Add missing class documentation (24 instances)
+  - Fix indentation and spacing issues
+  - Standardize method parameter formatting
+  - Improve hash alignment and syntax
+  - Add frozen string literal comments
+
+#### Code Smells
+  - Remove useless assignments
+  - Fix unused block arguments
+  - Remove redundant begin blocks
+  - Add proper guard clauses

lib/ring/sqa/cfg.rb

@@ -1,17 +1,27 @@
+# frozen_string_literal: true
+
+# Configuration management for Ring SQA system
+# Handles loading and management of system configuration
+
 require 'asetus'
 
 module Ring
   class SQA
+    # Default configuration directory
     Directory = '/etc/ring-sqa'
+
+    # Custom error classes for configuration issues
     class InvalidConfig < StandardError; end
     class NoConfig < StandardError; end
 
+    # Initialize configuration objects
     Config = Asetus.new name: 'sqa', load: false, usrdir: Directory, cfgfile: 'main.conf'
     hosts  = Asetus.new name: 'sqa', load: false, usrdir: Directory, cfgfile: 'hosts.conf'
 
+    # Set default configuration values
     Config.default.directory          = Directory
     Config.default.debug              = false
-    Config.default.port               = 'ring'.to_i(36)/100
+    Config.default.port               = 'ring'.to_i(36) / 100
     Config.default.analyzer.tolerance.relative = 1.2
     Config.default.analyzer.tolerance.absolute = 10
     Config.default.analyzer.size      = 30
@@ -21,24 +31,31 @@ class NoConfig < StandardError; end
     Config.default.mtr.timeout        = 15
     Config.default.ram_database       = false
     Config.default.paste.url          = 'https://ring.nlnog.net/paste/'
+    Config.default.reset_database     = false
+    Config.default.vacuum_on_purge    = true
 
-    hosts.default.load                = %w( ring.nlnog.net )
-    hosts.default.ignore              = %w( infra.ring.nlnog.net )
+    # Set default host configuration
+    hosts.default.load                = %w[ring.nlnog.net]
+    hosts.default.ignore              = %w[infra.ring.nlnog.net]
 
+    # Load configuration files
     begin
       Config.load
       hosts.load
-    rescue => error
-      raise InvalidConfig, "Error loading configuration: #{error.message}"
+    rescue StandardError => e
+      raise InvalidConfig, "Error loading configuration: #{e.message}"
     end
 
+    # Make configuration available globally
     CFG = Config.cfg
     CFG.hosts = hosts.cfg
 
+    # Set host information
     CFG.host.name = Socket.gethostname
-    CFG.host.ipv4 = Socket::getaddrinfo(CFG.host.name,"echo",Socket::AF_INET)[0][3]
-    CFG.host.ipv6 = Socket::getaddrinfo(CFG.host.name,"echo",Socket::AF_INET6)[0][3]
+    CFG.host.ipv4 = Socket.getaddrinfo(CFG.host.name, 'echo', Socket::AF_INET)[0][3]
+    CFG.host.ipv6 = Socket.getaddrinfo(CFG.host.name, 'echo', Socket::AF_INET6)[0][3]
 
+    # Create configuration files if they don't exist
     hosts.create
     raise NoConfig, 'edit /etc/ring-sqa/main.conf' if Config.create
   end

lib/ring/sqa/cli.rb

@@ -1,57 +1,56 @@
+# frozen_string_literal: true
+
+require 'English'
 require 'slop'
 require 'ring/sqa'
 
 module Ring
-class SQA
-
-  class CLI
-    attr_reader :opts
-
-    def run
-      pid = $$
-      puts "Running as pid: #{pid}"
-      Process.daemon if @opts.daemonize?
-      SQA.new
-    rescue Exception => error
-      crash error
-      raise
-    end
+  class SQA
+    class CLI
+      attr_reader :opts
+
+      def run
+        pid = $PROCESS_ID
+        puts "Running as pid: #{pid}"
+        Process.daemon if @opts.daemonize?
+        SQA.new
+      rescue Exception => e
+        crash e
+        raise
+      end
 
-    private
+      private
 
-    def initialize
-      _args, @opts = opts_parse
-      CFG.debug = @opts.debug?
-      CFG.afi = @opts.ipv6? ? "ipv6" : "ipv4"
-      CFG.fake  = @opts.fake?
-      require_relative 'log'
-      Log.level = Logger::DEBUG if @opts.debug?
-      run
-    end
+      def initialize
+        _args, @opts = opts_parse
+        CFG.debug = @opts.debug?
+        CFG.afi = @opts.ipv6? ? 'ipv6' : 'ipv4'
+        CFG.fake = @opts.fake?
+        require_relative 'log'
+        Log.level = Logger::DEBUG if @opts.debug?
+        run
+      end
 
-    def opts_parse
-      slop = Slop.new(:help=>true) do
-        banner 'Usage: ring-sqad [options]'
-        on 'd', '--debug', 'turn on debugging'
-        on '6', '--ipv6',  'use ipv6 instead of ipv4'
-        on '--fake',       'initialize analyzebuffer with 0 nodes'
-        on '--daemonize',  'run in background'
+      def opts_parse
+        slop = Slop.new(help: true) do
+          banner 'Usage: ring-sqad [options]'
+          on 'd', '--debug', 'turn on debugging'
+          on '6', '--ipv6',  'use ipv6 instead of ipv4'
+          on '--fake',       'initialize analyzebuffer with 0 nodes'
+          on '--daemonize',  'run in background'
+        end
+        [slop.parse!, slop]
       end
-      [slop.parse!, slop]
-    end
 
-    def crash error
-      file = File.join '/tmp', "ring-sqa-crash.txt.#{$$}"
-      open file, 'w' do |file|
-        file.puts error.class.to_s + ' => ' + error.message
-        file.puts '-' * 70
-        file.puts error.backtrace
-        file.puts '-' * 70
+      def crash(error)
+        file = File.join '/tmp', "ring-sqa-crash.txt.#{$PROCESS_ID}"
+        open file, 'w' do |file|
+          file.puts "#{error.class} => #{error.message}"
+          file.puts '-' * 70
+          file.puts error.backtrace
+          file.puts '-' * 70
+        end
       end
     end
-
   end
-
-end
 end
-

lib/ring/sqa/core.rb

@@ -1,4 +1,8 @@
-require 'pp'
+# frozen_string_literal: true
+
+# Core functionality for the Ring SQA (Service Quality Assurance) system
+# This module handles the main application logic and thread management
+
 require 'socket'
 require_relative 'cfg'
 require_relative 'database'
@@ -8,24 +12,47 @@
 
 module Ring
   class SQA
+    # Main execution method that manages all system components
+    # Creates and manages threads for different system components:
+    # - Responder: Handles incoming requests
+    # - Sender: Manages outgoing communications
+    # - Receiver: Processes incoming data
+    # - Analyzer: Analyzes system metrics
     def run
+      # Ensure thread exceptions are not silently ignored
       Thread.abort_on_exception = true
-      Thread.new { Responder.new }
-      Thread.new { Sender.new @database, @nodes }
-      Thread.new { Receiver.new @database }
-      Analyzer.new(@database, @nodes).run
+      @threads = []
+
+      begin
+        # Create and store references to all component threads
+        @threads << Thread.new { Responder.new }
+        @threads << Thread.new { Sender.new @database, @nodes }
+        @threads << Thread.new { Receiver.new @database }
+        @threads << Thread.new { Analyzer.new(@database, @nodes).run }
+
+        # Wait for all threads to complete (they typically run indefinitely)
+        @threads.each(&:join)
+      rescue StandardError => e
+        # Log any errors and ensure all threads are properly terminated
+        Log.error "Error in SQA run: #{e.message}"
+        @threads.each { |t| t.kill if t.alive? }
+        raise
+      end
     end
 
     private
 
+    # Initialize the SQA system
+    # Sets up logging, database, and node management
     def initialize
       require_relative 'log'
-      @database  = Database.new
-      # make sure Ping is created
-      raise "Table 'pings' does not exist" unless @database.table_exists?
-      @nodes     = Nodes.new
+      @database = Database.new
+
+      # Verify database is properly initialized
+      raise StandardError, "Database initialization failed: Table 'pings' does not exist" unless @database.table_exists?
+
+      @nodes = Nodes.new
       run
     end
-
   end
 end