Merge pull request #92 from joseivanlopez/fix_rpcbind_activation

Fix rpcbind activation

package/yast2-nfs-client.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Thu Feb  6 15:29:51 UTC 2020 - José Iván López González <jlopez@suse.com>
+
+- Kill rpcbind process if it was directly executed without using
+  systemd (bsc#1161687).
+- 4.1.8
+
 -------------------------------------------------------------------
 Fri Jan 17 12:49:20 UTC 2020 - José Iván López González <jlopez@suse.com>
 

package/yast2-nfs-client.spec

@@ -17,7 +17,7 @@
 
 
 Name:           yast2-nfs-client
-Version:        4.1.7
+Version:        4.1.8
 Release:        0
 Url:            https://github.com/yast/yast-nfs-client
 
@@ -28,15 +28,15 @@ BuildRequires:  perl-XML-Writer
 BuildRequires:  update-desktop-files
 BuildRequires:  yast2-devtools >= 3.1.27
 BuildRequires:  yast2-testsuite
-# SuSEFirewall2 replaced by firewalld (fate#323460)
-BuildRequires:  yast2 >= 4.0.39
+# Yast::Execute.locally
+BuildRequires:  yast2 >= 4.1.42
 BuildRequires:  rubygem(rspec)
 #for install task
 BuildRequires:  rubygem(%rb_default_ruby_abi:yast-rake)
 # path_matching (RSpec argument matcher)
 BuildRequires:  yast2-ruby-bindings >= 3.1.31
-# SuSEFirewall2 replaced by firewalld (fate#323460)
-Requires:       yast2 >= 4.0.39
+# Yast::Execute.locally
+BuildRequires:  yast2 >= 4.1.42
 #idmapd_conf agent
 Requires:       yast2-nfs-common >= 2.24.0
 # showmount, #150382, #286300

src/modules/Nfs.rb

@@ -19,6 +19,7 @@
 
 require "yast"
 require "y2firewall/firewalld"
+require "yast2/execute"
 
 require "shellwords"
 
@@ -384,15 +385,10 @@ def Write
       # help text
       Wizard.RestoreHelp(_("Writing NFS client settings. Please wait..."))
 
-      if Ops.greater_than(Builtins.size(@nfs_entries), 0)
+      if @nfs_entries.any?
         Progress.NextStage
-        # portmap must not be started if it is running already (see bug # 9999)
-        Service.Start(@portmapper) unless Service.active?(@portmapper)
 
-        unless Service.active?(@portmapper)
-          Report.Error(Message.CannotStartService(@portmapper))
-          return false
-        end
+        return false unless start_portmapper
       end
 
       firewalld.reload
@@ -700,6 +696,28 @@ def check_and_install_required_packages
 
       true
     end
+
+    # Starts portmapper service
+    #
+    # @return [Boolean] true if the service was activated; false otherwise.
+    def start_portmapper
+      if !Service.active?(@portmapper)
+        # Before mounting NFS shares, libstorage-ng could execute rpcbind binary directly without using
+        # systemd. If so, the systemd service would look like stopped and systemd would fail when trying
+        # to start it. The following line ensures that the process is not running before systemd tries to
+        # starts the service (bsc#1161687).
+        Yast::Execute.locally.stdout("killall", @portmapper)
+
+        # portmap must not be started if it is already running (bsc#9999)
+        Service.Start(@portmapper)
+      end
+
+      return true if Service.active?(@portmapper)
+
+      Report.Error(Message.CannotStartService(@portmapper))
+
+      false
+    end
   end
 
   Nfs = NfsClass.new

test/nfs_test.rb

@@ -462,11 +462,14 @@ def find_mp(path)
       allow(Yast::Service).to receive(:Start)
       allow(Yast::Service).to receive(:Stop)
       allow(Yast::Service).to receive(:active?)
+      allow(Yast::Execute).to receive(:locally).and_return(execute_object)
 
       allow_read_side_effects
       mock_entries
     end
 
+    let(:execute_object) { instance_double(Yast::Execute, stdout: "") }
+
     let(:written) { false }
 
     it "writes the nfs configurations" do
@@ -477,26 +480,66 @@ def find_mp(path)
     context "when the configuration is written correctly" do
       let(:written) { true }
 
-      it "tries to start the portmapper service if it is not running" do
-        expect(Yast::Service).to receive(:active?).with("rpcbind").and_return(false)
-        expect(Yast::Service).to receive(:Start).with("rpcbind")
-        subject.Write()
+      before do
+        allow(Yast::Service).to receive(:active?).with("rpcbind")
+          .and_return(service_status1, service_status2)
       end
 
-      context "and the portmapper service was not activated" do
-        before do
-          allow(Yast::Service).to receive(:active?).with("rpcbind").twice.and_return(false)
-          allow(Yast::Message).to receive(:CannotStartService).and_return("cannot_start")
+      let(:service_status1) { nil }
+
+      let(:service_status2) { nil }
+
+      context "and the portmapper service is not active" do
+        let(:service_status1) { false }
+
+        it "tries to kill the portmapper process" do
+          expect(execute_object).to receive(:stdout).with("killall", "rpcbind")
+
+          subject.Write
+        end
+
+        it "tries to activate the portmapper service" do
+          expect(Yast::Service).to receive(:Start).with("rpcbind")
+
+          subject.Write
         end
 
-        it "reports an error" do
-          expect(Yast::Report).to receive(:Error).with("cannot_start")
+        context "and the portmapper service was activated" do
+          let(:service_status2) { true }
+
+          it "returns true" do
+            expect(subject.Write).to eql(true)
+          end
+        end
+
+        context "and the portmapper service was not activated" do
+          let(:service_status2) { false }
+
+          it "reports an error" do
+            expect(Yast::Report).to receive(:Error).with(/Cannot start/)
+
+            subject.Write
+          end
+
+          it "returns false" do
+            expect(subject.Write).to eql(false)
+          end
+        end
+      end
+
+      context "and the portmapper service is already active" do
+        let(:service_status1) { true }
+
+        let(:service_status2) { true }
+
+        it "does not try to activate the portmapper service again" do
+          expect(Yast::Service).to_not receive(:Start).with("rpcbind")
 
           subject.Write
         end
 
-        it "returns false" do
-          expect(subject.Write).to eql(false)
+        it "returns true" do
+          expect(subject.Write).to eql(true)
         end
       end
     end