Changes based on CR.

library/network/src/lib/y2firewall/firewalld.rb

@@ -51,20 +51,19 @@ class Firewalld
     include Yast::Logger
     extend Forwardable
 
-    # @param Y2Firewall::Firewalld::Api instance
     attr_writer :api
-    # @return [Array <Y2Firewall::Firewalld::Zone>] firewalld zones
+    # @return [Array<Y2Firewall::Firewalld::Zone>] firewalld zones
     attr_accessor :zones
-    # @return [Array <String>] current zone names.
+    # @return [Array<String>] current zone names.
     attr_accessor :current_zone_names
-    # @return [Array <String>] current service names.
+    # @return [Array<String>] current service names.
     attr_accessor :current_service_names
-    # @return [Array <Y2Firewall::Firewalld::Service>] firewalld services. To
-    # avoid performance problems it is empty by default and the services are
-    # added when needed by the find_service method.
+    # @return [Array<Y2Firewall::Firewalld::Service>] firewalld services. To
+    #   avoid performance problems it is empty by default and the services are
+    #   added when needed by the find_service method.
     attr_accessor :services
     # @return [String] Type of log denied packets (reject & drop rules).
-    # Possible values are: all, unicast, broadcast, multicast and off
+    #   Possible values are: all, unicast, broadcast, multicast and off
     attr_accessor :log_denied_packets
     # @return [String] firewalld default zone name
     attr_accessor :default_zone
@@ -99,9 +98,12 @@ def read
       @read = true
     end
 
-    # Add a not existent zones to the list of zones
+    # Given a zone name it will add a new Zone to the current list of defined
+    # ones just in case it does not exist yet.
     #
-    # @return [Boolean] true if a new one is added; false otherwise
+    # @param name [String] zone name
+    # @return [Boolean] true if the new zone was added; false in case the zone
+    #   was alredy defined
     def add_zone(name)
       return false if find_zone(name)
       zones << Y2Firewall::Firewalld::Zone.new(name: name)
@@ -110,6 +112,7 @@ def add_zone(name)
 
     # Remove the given zone from the list of zones
     #
+    # @param name [String] zone name
     # @return [Boolean] true if it was removed; false otherwise
     def remove_zone(name)
       removed = zones.reject! { |z| z.name == name }
@@ -120,7 +123,7 @@ def remove_zone(name)
     #
     # @param name [String] the zone name
     # @return [Y2Firewall::Firewalld::Zone, nil] the firewalld zone with the
-    # given name
+    #   given name
     def find_zone(name)
       zones.find { |z| z.name == name }
     end
@@ -129,7 +132,7 @@ def find_zone(name)
     #
     # @param name [String] the service name
     # @return [Y2Firewall::Firewalld::Service] the firewalld service with
-    # the given name
+    #   the given name
     def find_service(name)
       services.find { |s| s.name == name } || read_service(name)
     end
@@ -179,9 +182,8 @@ def apply_zones_changes!
         zone.apply_changes! if zone.modified?
       end
       current_zone_names.each do |name|
-        api.delete_zone(name) unless zones.any? { |zone| zone.name == name }
+        api.delete_zone(name) if !zones.any? { |z| z.name == name }
       end
-      true
     end
 
     # Return whether the current zones have been modified or not
@@ -253,26 +255,26 @@ def start
     # Return whether the configuration has been read
     #
     # @return [Boolean] true if the configuration has been read; false
-    # otherwise
+    #   otherwise
     def read?
       @read
     end
 
-    # Convenience method for initializing and retrieving an API instance
+    # Convenience method to instantiate the firewalld API
     def api
       @api ||= Api.new
     end
 
   private
 
-    # Convenience method for instantiate a new zone parser
+    # Convenience method to isntantiate a new zone parser
     #
     # @return [ZoneParser]
     def zone_parser
       ZoneParser.new(api.zones, api.list_all_zones(verbose: true))
     end
 
-    # Convenience method for instantiate a services parser
+    # Convenience method tosisntantiate a services parser
     #
     # @return [ServiceParser]
     def service_parser

library/network/src/lib/y2firewall/firewalld/api.rb

@@ -59,12 +59,7 @@ class Api
 
       # Constructor
       def initialize(mode: nil, permanent: true)
-        @mode =
-          if mode == :running || running?
-            :running
-          else
-            :offline
-          end
+        @mode = (mode || running? ? :running : :offline)
         @permanent = permanent
       end
 
@@ -109,7 +104,7 @@ def disable!
       # @return [String] firewalld service state
       # @see http://www.firewalld.org/documentation/man-pages/firewall-cmd.html
       def state
-        case Yast::Execute.on_target!("firewall-cmd", "--state", allowed_exitstatus: [0, 252])
+        case Yast::Execute.on_target("firewall-cmd", "--state", allowed_exitstatus: [0, 252])
         when 0
           "running"
         when 252
@@ -155,16 +150,16 @@ def complete_reload
       ### Logging ###
 
       # @param kind [String] Denied packets to log. Possible values are:
-      # all, unicast, broadcast, multicast and off
+      #   all, unicast, broadcast, multicast and off
       # @return [Boolean] True if desired packet type is being logged when denied
       def log_denied_packets?(kind)
-        string_command("--get-log-denied").strip == kind ? true : false
+        string_command("--get-log-denied").strip == kind
       end
 
       # @param kind [String] Denied packets to log. Possible values are:
-      # all, unicast, broadcast, multicast and off
+      #   all, unicast, broadcast, multicast and off
       # @return [Boolean] True if desired packet type was set to being logged
-      # when denied
+      #   when denied
       def log_denied_packets=(kind)
         run_command("--set-log-denied=#{kind}")
       end
@@ -188,16 +183,15 @@ def command
       # @see Yast::Execute
       # @param args [Array<String>] list of command optional arguments
       # @param permanent [Boolean] if true it adds the --permanent option the
-      # command to be executed
+      #   command to be executed
       # @param allowed_exitstatus [Fixnum, .include?, nil] allowed exit codes
-      # which do not cause an exception.
-      # command to be executed
+      #   which do not cause an exception.
       def run_command(*args, permanent: false, allowed_exitstatus: nil)
         arguments = permanent ? ["--permanent"] : []
         arguments.concat(args)
         log.info("Executing #{command} with #{arguments.inspect}")
 
-        Yast::Execute.on_target!(
+        Yast::Execute.on_target(
           command, *arguments, stdout: :capture, allowed_exitstatus: allowed_exitstatus
         )
       end
@@ -209,7 +203,7 @@ def run_command(*args, permanent: false, allowed_exitstatus: nil)
       # @return [String] the chomped output of the run command
       # @param args [Array<String>] list of command optional arguments
       # @param permanent [Boolean] if true it adds the --permanent option the
-      # command to be executed
+      #   command to be executed
       def string_command(*args, permanent: false)
         run_command(*args, permanent: permanent).to_s.chomp
       end

library/network/src/lib/y2firewall/firewalld/api/services.rb

@@ -28,93 +28,106 @@ class Api
       # This module contains specific api methods for handling services
       # definition and configuration.
       module Services
+        # Creates a new service definition for the given service name
+        #
         # @param service [String] The firewall service
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
-        def new_service(service, permanent: permanent?)
-          query_command("--new-service=#{service}", permanent: permanent)
+        def create_service(service)
+          string_command("--new-service=#{service}", permanent: !offline?) == "success"
         end
 
+        # Creates a new service definition for the given service name
+        #
+        # @param service [String] The firewall service
+        def delete_service(service)
+          string_command("--delete-service=#{service}", permanent: !offline?) == "success"
+        end
+
+        # Return the list of availale firewalld services
+        #
         # @return [Array<String>] List of firewall services
         def services
           string_command("--get-services").split(" ")
         end
 
-        # @param service [String] The firewall service
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
+        # Show all the service declaration (name, description, ports,
+        # protocols and modules)
+        #
+        # @param service [String] The firewall service name
+        # @param permanent [Boolean] if true and firewalld is running it
+        #   reads the permanent configuration
         # @return [Array<String>] list of all information for the given service
-        def info_service(service, permanent: permanent?, verbose: false)
-          if verbose
-            string_command("--info-service=#{service}", "--verbose", permanent: permanent).split("\n")
-          else
-            string_command("--info-service=#{service}", permanent: permanent).split("\n")
-          end
+        def info_service(service, permanent: permanent?)
+          string_command("--info-service=#{service}", "--verbose", permanent: permanent).split("\n")
         end
 
         # @param service [String] The firewall service
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
+        # @param permanent [Boolean] if true and firewalld is running it
+        #   reads the permanent configuration
         # @return [String] Short description for service
         def service_short(service, permanent: permanent?)
           # these may not exist on early firewalld releases
           string_command("--service=#{service}", "--get-short", permanent: permanent)
         end
 
         # @param service [String] the firewall service
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
+        # @param permanent [Boolean] if true and firewalld is running it
+        #   reads the permanent configuration
         # @return [String] Description for service
         def service_description(service, permanent: permanent?)
           string_command("--service=#{service}", "--get-description", permanent: permanent)
         end
 
+        # Returns whether the service definition for the service name given is
+        # present or not.
+        #
         # @param service [String] The firewall service
         # @return [Boolean] True if service definition exists
         def service_supported?(service)
           services.include?(service)
         end
 
+        # Return the list of ports allowed by the given service
+        #
         # @param service [String] The firewall service
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
+        # @param permanent [Boolean] if true and firewalld is running it
+        #   reads the permanent configuration
         # @return [Array<String>] The firewall service ports
         def service_ports(service, permanent: permanent?)
           string_command("--service=#{service}", "--get-ports", permanent: permanent).split(" ")
         end
 
         # @param service [String] The firewall service
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
+        # @param permanent [Boolean] if true and firewalld is running it
+        #   reads the permanent configuration
         # @return [Array<String>] The firewall service protocols
         def service_protocols(service, permanent: permanent?)
           string_command("--service=#{service}", "--get-protocols", permanent: permanent).split(" ")
         end
 
         # @param service [String] The firewall service
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
+        # @param permanent [Boolean] if true and firewalld is running it
+        #   reads the permanent configuration
         # @return [Array<String>] The firewall service modules
         def service_modules(service, permanent: permanent?)
           string_command("--service=#{service}", "--get-modules", permanent: permanent).split(" ")
         end
 
         # @param service [String] The firewall service
         # @param port [String] The firewall port
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
+        # @param permanent [Boolean] if true and firewalld is running it
+        #   modifies the permanent configuration
         # @return [Boolean] True if port was removed from service
         def remove_service_port(service, port, permanent: permanent?)
-          string_command("--service=#{service}", "--remove-port=#{port}", permanent: permanent)
+          string_command("--service=#{service}", "--remove-port=#{port}", permanent: permanent) == "success"
         end
 
         # @param service [String] The firewall firewall
         # @param port [String] The firewall port
-        # @param permanent [Boolean] if true it adds the --permanent option the
-        # command to be executed
+        # @param permanent [Boolean] if true and firewalld is running it
+        #   modifies the permanent configuration
         # @return [Boolean] True if port was removed from service
         def add_service_port(service, port, permanent: permanent?)
-          string_command("--service=#{service}", "--add-port=#{port}", permanent: permanent)
+          string_command("--service=#{service}", "--add-port=#{port}", permanent: permanent) == "success"
         end
       end
     end

library/network/src/lib/y2firewall/firewalld/service_parser.rb

@@ -32,7 +32,7 @@ class ServiceParser
 
       # @return [Array<Y2Firewall::Firewalld::Service>]
       def parse(name)
-        info = Y2Firewall::Firewalld.instance.api.info_service(name, verbose: true)
+        info = Y2Firewall::Firewalld.instance.api.info_service(name)
         raise(Service::NotFound, name) if $CHILD_STATUS.exitstatus == 101
         service = Service.new(name: name)
 

library/network/test/y2firewall/firewalld/service_parser_test.rb

@@ -53,7 +53,7 @@
     context "when the service is not present" do
       let(:service_name) { "not_present" }
       before do
-        allow(api).to receive(:info_service).with(service_name, verbose: true)
+        allow(api).to receive(:info_service).with(service_name)
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(101)
       end
 
@@ -65,7 +65,7 @@
     context "when the service configuration exists" do
       let(:service_name) { "radius" }
       before do
-        allow(api).to receive(:info_service).with(service_name, verbose: true)
+        allow(api).to receive(:info_service).with(service_name)
           .and_return(service_info)
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(1)
       end