From bd1c438a1f61aa27a6a95d7f5953ee8e3daa9450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 24 Aug 2018 12:50:21 +0100 Subject: [PATCH 1/5] Adapt dialog to use new ServiceWidget --- src/include/tftp-server/dialogs.rb | 333 ++++++++++++++++------------- src/modules/TftpServer.rb | 52 +++-- 2 files changed, 215 insertions(+), 170 deletions(-) diff --git a/src/include/tftp-server/dialogs.rb b/src/include/tftp-server/dialogs.rb index f79a101..68d2f19 100644 --- a/src/include/tftp-server/dialogs.rb +++ b/src/include/tftp-server/dialogs.rb @@ -8,6 +8,7 @@ # $Id$ require "y2journal" +require "yast2/service_widget" module Yast module TftpServerDialogsInclude @@ -27,8 +28,8 @@ def initialize_tftp_server_dialogs(include_target) end # Check for required packaged to be installed - # @return `abort if aborted and `next otherwise - + # + # @return [Symbol] :abort if aborted and :next otherwise def Packages return :abort if !Package.InstallAll(TftpServer.required_packages) @@ -36,7 +37,8 @@ def Packages end # Read settings dialog - # @return `abort if aborted and `next otherwise + # + # @return [Symbol] :abort if aborted and :next otherwise def ReadDialog ret = true @@ -50,193 +52,222 @@ def ReadDialog end # Write settings dialog - # @return `abort if aborted and `next otherwise + # + # @return [Symbol] :abort if aborted and :next otherwise def WriteDialog ret = TftpServer.Write ret ? :next : :abort end # Main dialog - # @return dialog result + # + # @return [Symbol] dialog result (:next, :cancel, :abort) def MainDialog Wizard.SetScreenShotName("tftp-server-1-main") - start = TftpServer.start + # start = TftpServer.start directory = TftpServer.directory changed = false # Tftp-server dialog caption caption = _("TFTP Server Configuration") - # firewall widget using CWM - fw_settings = { - "services" => ["tftp"], - "display_details" => true - } - fw_cwm_widget = CWMFirewallInterfaces.CreateOpenFirewallWidget( - fw_settings + Wizard.SetContentsButtons( + caption, + contents, + help, + Label.BackButton, + Label.OKButton ) + Wizard.HideBackButton + Wizard.SetAbortButton(:abort, Label.CancelButton) - # dialog help text - help_text = _("

Configuring a TFTP Server

") - # dialog help text - help_text = Ops.add( - help_text, - _( - "

Use this to enable a server for TFTP (trivial file transfer protocol). The server will be started using xinetd.

" - ) - ) - # enlighten newbies, #102946 - # dialog help text - help_text = Ops.add( - help_text, - _("

Note that TFTP and FTP are not the same.

") - ) - # dialog help text - help_text = Ops.add( - help_text, - _( - "

Boot Image Directory:\n" + - "Specify the directory where served files are located. The usual value is\n" + - "/tftpboot. The directory will be created if it does not exist. \n" + - "The server uses this as its root directory (using the -s option).

\n" - ) - ) - help_text = Ops.add(help_text, Ops.get_string(fw_cwm_widget, "help", "")) + # Initialize the widget (set the current value) + CWMFirewallInterfaces.OpenFirewallInit(firewall_widget, "") + + UI.ChangeWidget(Id(:viewlog), :Enabled, !Mode.config) + + result = handle_events + + Wizard.RestoreScreenShotName + result + end + + private - contents = HVSquash( + # Dialog contents + # + # @return [Yast::Term] + def contents + HVSquash( VBox( - RadioButtonGroup( - Id(:rbg), - VBox( - Left( - RadioButton( - Id(:tftpno), - Opt(:notify), - # Radio button label, disable TFTP server - _("&Disable"), - !start - ) - ), - Left( - RadioButton( - Id(:tftpyes), - Opt(:notify), - # Radio button label, disable TFTP server - _("&Enable"), - start - ) - ) - ) - ), + service_widget.content, VSpacing(1), - TextAndButton( - # Text entry label + HBox( # Directory where served files (usually boot images) reside - TextEntry(Id(:directory), _("&Boot Image Directory"), directory), - # push button label - # select a directory from the filesystem - PushButton(Id(:browse), _("Bro&wse...")) + Bottom(TextEntry(Id(:directory), _("&Boot Image Directory"), TftpServer.directory)), + HSpacing(0.5), + # Select a directory from the filesystem + Bottom(PushButton(Id(:browse), _("Bro&wse..."))) ), VSpacing(1), - Ops.get_term(fw_cwm_widget, "custom_widget", Empty()), + firewall_widget["custom_widget"] || Empty(), VSpacing(2), - # push button label - # display a log file + # Display a log file PushButton(Id(:viewlog), _("&View Log")) ) ) + end - Wizard.SetContentsButtons( - caption, - contents, - help_text, - Label.BackButton, - Label.OKButton - ) - Wizard.HideBackButton - Wizard.SetAbortButton(:abort, Label.CancelButton) - - # initialize the widget (set the current value) - CWMFirewallInterfaces.OpenFirewallInit(fw_cwm_widget, "") - - UI.ChangeWidget(Id(:viewlog), :Enabled, !Mode.config) - event = nil - ret = nil - begin - UI.ChangeWidget(Id(:directory), :Enabled, start) - UI.ChangeWidget(Id(:browse), :Enabled, start) + # Handles dialog events + # + # @return [Symbol] :next, :cancel, :abort + def handle_events + input = nil + loop do event = UI.WaitForEvent - ret = Ops.get(event, "ID") - ret = :abort if ret == :cancel - - # handle the events, enable/disable the button, show the popup if button clicked - CWMFirewallInterfaces.OpenFirewallHandle(fw_cwm_widget, "", event) - - start = UI.QueryWidget(Id(:rbg), :CurrentButton) == :tftpyes - directory = Convert.to_string(UI.QueryWidget(Id(:directory), :Value)) - - # discard the difference in disabled fields: - # directory is only considered if start is on - changed = CWMFirewallInterfaces.OpenFirewallModified("") || - start != TftpServer.start || # "" because method doesn't use parameter at all, nice :( - start && directory != TftpServer.directory - - if ret == :browse - directory = UI.AskForExistingDirectory( - directory != "" ? directory : "/", - "" - ) - UI.ChangeWidget(Id(:directory), :Value, directory) if directory != nil - elsif ret == :viewlog - # show both service and socket logs for current boot - query = Y2Journal::Query.new(interval: "0", filters: { "unit" => ["tftp.service", "tftp.socket"] }) - Y2Journal::EntriesDialog.new(query: query).run - end + input = event["ID"] - # validity checks - if ret == :next && start - if CheckDirectorySyntax(directory) - #ok, say that it will be created - if !Mode.config && - Ops.less_than(SCR.Read(path(".target.size"), directory), 0) - # the dir does not exist - ret = Popup.YesNo(Message.DirectoryDoesNotExistCreate(directory)) ? ret : nil - end - else - UI.SetFocus(Id(:directory)) - # error popup - Popup.Error( - _( - "The directory must start with a slash (/)\nand must not contain spaces." - ) - ) - ret = nil + # Handle the events, enable/disable the button, show the popup if button clicked + CWMFirewallInterfaces.OpenFirewallHandle(firewall_widget, "", event) + + case input + when :browse + ask_directory + when :viewlog + show_log + when :next + if check_directory + # Grab current settings, store them to SuSEFirewall:: + CWMFirewallInterfaces.OpenFirewallStore(firewall_widget, "", event) + save_service + break end + when :cancel, :abort + break if Popup.ReallyAbort(changes?) end - end until ret == :next || - (ret == :back || ret == :abort) && (!changed || Popup.ReallyAbort(true)) + end + + input + end + + # Help text + # + # @return [String] + def help + _("

Configuring a TFTP Server

") + + _("

Use this to enable a server for TFTP (trivial file transfer protocol). The server will be started using xinetd.

") + + _("

Note that TFTP and FTP are not the same.

") + + _( + "

Boot Image Directory:\n" + + "Specify the directory where served files are located. The usual value is\n" + + "/tftpboot. The directory will be created if it does not exist. \n" + + "The server uses this as its root directory (using the -s option).

\n" + ) + + firewall_widget["help"] || "" + end + + # Widget to define state and start mode of the service + # + # @return [Yast2::ServiceWidget] + def service_widget + @service_widget ||= Yast2::ServiceWidget.new(TftpServer.service) + end - if ret == :next - # grab current settings, store them to SuSEFirewall:: - CWMFirewallInterfaces.OpenFirewallStore(fw_cwm_widget, "", event) + # Firewall widget using CWM + # + # @return [Hash] see CWMFirewallInterfaces.CreateOpenFirewallWidget + def firewall_widget + @firewall_widget ||= CWMFirewallInterfaces.CreateOpenFirewallWidget( + "services" => ["tftp"], + "display_details" => true + ) + end - TftpServer.start = start - TftpServer.directory = directory if start + # Value of the input field to indicate the Boot Image Directory + # + # @return [String] + def directory + UI.QueryWidget(Id(:directory), :Value) + end + + # Opens a dialog to ask for the directory + # + # @note The input field is updated with the selected directory. + def ask_directory + search_path = directory.empty? ? "/" : directory + + directory = UI.AskForExistingDirectory(search_path, "") + UI.ChangeWidget(Id(:directory), :Value, directory) + end + + # Asks whether to create the directory (usefull when the directory does not exist) + # + # @return [Boolean] + def ask_create_directory + Popup.YesNo(Message.DirectoryDoesNotExistCreate(directory)) + end + + # Checks whether the given path is valid, and if so, it asks for creating the directory + # when it does not exist yet + # + # @return [Boolean] true when the given path is valid and exists (or should be created); + # false otherwise. + def check_directory + if !valid_directory? + show_directory_error + false + elsif !exist_directory? + ask_create_directory + else + true end + end - Wizard.RestoreScreenShotName - Convert.to_symbol(ret) + # Checks whether the given directory path is valid + # + # @return [Boolean] + def valid_directory? + directory.start_with?("/") && !directory.match?(/[ \t]/) end - def TextAndButton(text, button) - text = deep_copy(text) - button = deep_copy(button) - HBox(Bottom(text), HSpacing(0.5), Bottom(button)) + + # Checks whether the given directory path already exists + # + # @return [Boolean] + def exist_directory? + return true if Mode.config + + SCR.Read(path(".target.size"), directory) >= 0 end - def CheckDirectorySyntax(dir) - Builtins.substring(dir, 0, 1) == "/" && - Builtins.filterchars(" \t", dir) == "" + + # Opens a popup to indicate the error when the given directory path is not valid + def show_directory_error + message = _("The directory must start with a slash (/)\nand must not contain spaces.") + + Popup.Error(message) + end + + # Shows both service and socket logs since current boot + def show_log + query = Y2Journal::Query.new(interval: "0", filters: { "unit" => ["tftp.service", "tftp.socket"] }) + Y2Journal::EntriesDialog.new(query: query).run + end + + # Whether something has been edited + # + # @note Changes in the Service Widget are not taken into account. + # + # @return [Boolean] + def changes? + CWMFirewallInterfaces.OpenFirewallModified("") || directory != TftpServer.directory + end + + # Saves the service changes + def save_service + service_widget.store + TftpServer.start = TftpServer.service.active? + TftpServer.directory = directory end end end diff --git a/src/modules/TftpServer.rb b/src/modules/TftpServer.rb index 937f6a6..92068bb 100644 --- a/src/modules/TftpServer.rb +++ b/src/modules/TftpServer.rb @@ -18,15 +18,20 @@ require "yast2/target_file" # allow CFA to work on change scr require "cfa/tftp_sysconfig" require "y2firewall/firewalld" - +require "yast2/system_service" module Yast class TftpServerClass < Module + include Yast::Logger SOCKET_NAME = "tftp" PACKAGE_NAME = "tftp" - include Yast::Logger + # @!method start + # Whether the socket should be enabled and started + # + # @return [Boolean] + attr_reader :start def main textdomain "tftp-server" @@ -35,6 +40,7 @@ def main Yast.import "Progress" Yast.import "Report" Yast.import "Summary" + Yast.import "Mode" # Any settings modified? # As we have only a single dialog which handles it by itself, @@ -61,6 +67,13 @@ def main @foreign_servers = "" end + # Service to configure + # + # @return [Yast2::SystemService] + def service + @service ||= Yast2::SystemService.find(PACKAGE_NAME) + end + # firewall instance def firewall @firewall ||= Y2Firewall::Firewalld.instance @@ -157,13 +170,15 @@ def WriteOnly # and then switch to user which is used for tftp service SCR.Execute(path(".target.bash_output"), "/usr/bin/chown #{@sysconfig.user}: #{Shellwords.escape(@directory)}") - # enable and (re)start systemd socket - if @start - socket.enable - socket.start - else - socket.disable - socket.stop + if Mode.auto || Mode.commandline + # enable and (re)start systemd socket + if start + socket.enable + socket.start + else + socket.disable + socket.stop + end end # TODO only when we have our own Progress @@ -179,9 +194,13 @@ def WriteOnly def Write return false if !WriteOnly() - # in.tftpd will linger around for 15 minutes waiting for a new connection - # so we must kill it otherwise it will be using the old parameters - Yast2::Systemd::Service.find!("tftp").stop + if Mode.auto || Mode.commandline + # in.tftpd will linger around for 15 minutes waiting for a new connection + # so we must kill it otherwise it will be using the old parameters + Yast2::Systemd::Service.find!("tftp").stop + else + service.save + end # TODO only when we have our own Progress #boolean progress_orig = Progress::set (false); @@ -202,7 +221,6 @@ def Set(settings) nil end - # Get all tftp-server settings from the first parameter # (For use by autoinstallation.) # @param [Hash] settings The YCP structure to be imported. @@ -234,7 +252,6 @@ def Export deep_copy(settings) end - # Mergeing config to existing system configuration. It is useful for delayed write. # So if package will be installed later this method re-apply changes on top of newly parsed # file. @@ -261,12 +278,9 @@ def Summary summary end - # Return needed packages and packages to be removed - # during autoinstallation. - # @return [Hash] of lists. - # + # Return needed packages and packages to be removed during autoinstallation # - + # @return [Hash] of lists def AutoPackages install_pkgs = deep_copy(@required_packages) remove_pkgs = [] From f1fea2e88045a1d0b983d0702a24de93b477a554 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 24 Aug 2018 12:50:51 +0100 Subject: [PATCH 2/5] Add unit tests --- test/tftpserver_test.rb | 156 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 156 insertions(+) create mode 100644 test/tftpserver_test.rb diff --git a/test/tftpserver_test.rb b/test/tftpserver_test.rb new file mode 100644 index 0000000..2ae2157 --- /dev/null +++ b/test/tftpserver_test.rb @@ -0,0 +1,156 @@ +#!/usr/bin/env rspec +# encoding: utf-8 + +# Copyright (c) [2018] SUSE LLC +# +# All Rights Reserved. +# +# This program is free software; you can redistribute it and/or modify it +# under the terms of version 2 of the GNU General Public License as published +# by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, contact SUSE LLC. +# +# To contact SUSE LLC about this file by physical or electronic mail, you may +# find current contact information at www.suse.com. + +require_relative "test_helper" + +Yast.import "TftpServer" + +describe "Yast::TftpServer" do + describe "#Write" do + subject(:tftp_server) { Yast::TftpServerClass.new } + + before do + allow(Yast2::SystemService).to receive(:find).with("tftp").and_return(service) + + allow(Yast2::Systemd::Service).to receive(:find!).with("tftp").and_return(systemd_service) + + allow(Yast2::Systemd::Socket).to receive(:find!).with("tftp").and_return(socket) + + allow(Y2Firewall::Firewalld).to receive(:instance).and_return(firewalld) + + allow(CFA::TftpSysconfig).to receive(:new).and_return(sysconfig) + + allow(Yast::SCR).to receive(:Execute) + + allow(Yast::Mode).to receive(:auto) { auto } + allow(Yast::Mode).to receive(:commandline) { commandline } + + tftp_server.main + end + + let(:service) { instance_double(Yast2::SystemService, save: true) } + + let(:systemd_service) { instance_double(Yast2::Systemd::Service, stop: true) } + + let(:socket) { instance_double(Yast2::Systemd::Socket, enable: true, disable: true, start: true, stop: true) } + + let(:sysconfig) do + instance_double( + CFA::TftpSysconfig, + directory: "/path/to/boot_image_directory", + :directory= => nil, + save: true, + user: nil + ) + end + + let(:firewalld) { instance_double(Y2Firewall::Firewalld, write_only: true, reload: true) } + + let(:auto) { false } + let(:commandline) { false } + + shared_examples "old behavior" do + it "does not save the system service" do + expect(service).to_not receive(:save) + + tftp_server.Write + end + + it "stops the systemd service" do + expect(systemd_service).to receive(:stop) + + tftp_server.Write + end + + context "when the socket should not be started" do + before do + allow(tftp_server).to receive(:start).and_return(false) + end + + it "disables the socket" do + expect(socket).to receive(:disable) + + tftp_server.Write + end + + it "stops the socket" do + expect(socket).to receive(:stop) + + tftp_server.Write + end + end + + context "when the socket should be started" do + before do + allow(tftp_server).to receive(:start).and_return(true) + end + + it "enables the socket" do + expect(socket).to receive(:enable) + + tftp_server.Write + end + + it "starts the socket" do + expect(socket).to receive(:start) + + tftp_server.Write + end + end + end + + context "when running in command line" do + let(:commandline) { true } + + include_examples "old behavior" + end + + context "when running in AutoYaST mode" do + let(:auto) { true } + + include_examples "old behavior" + end + + context "when running in normal mode" do + it "does not stop the systemd service directly" do + expect(systemd_service).to_not receive(:stop) + + tftp_server.Write + end + + it "does not modify the systemd socket directly" do + expect(socket).to_not receive(:enable) + expect(socket).to_not receive(:disable) + expect(socket).to_not receive(:start) + expect(socket).to_not receive(:stop) + + tftp_server.Write + end + + it "saves the system service" do + expect(service).to receive(:save) + + tftp_server.Write + end + end + end +end From 1068a6bc55e3a4a7f9776fefd9bf891a205c29f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 24 Aug 2018 12:52:25 +0100 Subject: [PATCH 3/5] Update version and changelog --- package/yast2-tftp-server.changes | 7 +++++++ package/yast2-tftp-server.spec | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/package/yast2-tftp-server.changes b/package/yast2-tftp-server.changes index f40ce1d..9988703 100644 --- a/package/yast2-tftp-server.changes +++ b/package/yast2-tftp-server.changes @@ -1,3 +1,10 @@ +------------------------------------------------------------------- +Fri Aug 24 11:51:13 UTC 2018 - jlopez@suse.com + +- Use new Yast2::ServiceWidget to manage the service status + (part of fate#319428). +- 4.1.4 + ------------------------------------------------------------------- Thu Aug 23 14:41:40 UTC 2018 - dgonzalez@suse.com diff --git a/package/yast2-tftp-server.spec b/package/yast2-tftp-server.spec index 2c2ee55..e6737f8 100644 --- a/package/yast2-tftp-server.spec +++ b/package/yast2-tftp-server.spec @@ -17,7 +17,7 @@ Name: yast2-tftp-server -Version: 4.1.3 +Version: 4.1.4 Release: 0 BuildRoot: %{_tmppath}/%{name}-%{version}-build From 16744895f3f382390a07ee12335df7b2130fe124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 24 Aug 2018 14:19:29 +0100 Subject: [PATCH 4/5] Remove leftover code --- src/include/tftp-server/dialogs.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/include/tftp-server/dialogs.rb b/src/include/tftp-server/dialogs.rb index 68d2f19..87954d0 100644 --- a/src/include/tftp-server/dialogs.rb +++ b/src/include/tftp-server/dialogs.rb @@ -65,10 +65,6 @@ def WriteDialog def MainDialog Wizard.SetScreenShotName("tftp-server-1-main") - # start = TftpServer.start - directory = TftpServer.directory - changed = false - # Tftp-server dialog caption caption = _("TFTP Server Configuration") From 15a8eb6841a37f74defe1ce59cace55cb481346c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 24 Aug 2018 14:24:38 +0100 Subject: [PATCH 5/5] Fix bug with help --- src/include/tftp-server/dialogs.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/include/tftp-server/dialogs.rb b/src/include/tftp-server/dialogs.rb index 87954d0..a51d98b 100644 --- a/src/include/tftp-server/dialogs.rb +++ b/src/include/tftp-server/dialogs.rb @@ -161,7 +161,7 @@ def help "/tftpboot. The directory will be created if it does not exist. \n" + "The server uses this as its root directory (using the -s option).

\n" ) + - firewall_widget["help"] || "" + (firewall_widget["help"] || "") end # Widget to define state and start mode of the service