fff-layer3-config: return error values in functions instead of terminating #260

Closed
fbl wants to merge 1 commits from fbl:testmodefix into master
Owner

Many functions of configure-layer3 terminate the program after
successful execution, as they were originally only intended for
execution of configure-layer3 commands.

However, some functions are used both for command exection, but also as
helper functions. For example, revert_changes() is used as a helper
function in test_changes(). Terminating the program at the end of the
function therefore ends the exection of test_changes() prematurely. As a
result, the test mode of configure-layer3 never reloads services after
a successful configuration revert.

Replace exit commands with appropriate function return values, which can
then be evaluated by the caller where appropriate.

While at it, add a missing return to the parameter validation in
execute_subshell().

Fixes: #256

Many functions of configure-layer3 terminate the program after successful execution, as they were originally only intended for execution of configure-layer3 commands. However, some functions are used both for command exection, but also as helper functions. For example, revert_changes() is used as a helper function in test_changes(). Terminating the program at the end of the function therefore ends the exection of test_changes() prematurely. As a result, the test mode of configure-layer3 never reloads services after a successful configuration revert. Replace exit commands with appropriate function return values, which can then be evaluated by the caller where appropriate. While at it, add a missing return to the parameter validation in execute_subshell(). Fixes: #256
fbl added this to the next-bugfix milestone 2022-07-20 14:20:25 +02:00
fbl added the
bug
layer3
labels 2022-07-20 14:20:25 +02:00
fbl added 1 commit 2022-07-20 14:20:25 +02:00
dd4f9cf52b fff-layer3-config: return error values in functions instead of terminating
Many functions of configure-layer3 terminate the program after
successful execution, as they were originally only intended for
execution of configure-layer3 commands.

However, some functions are used both for command exection, but also as
helper functions. For example, revert_changes() is used as a helper
function in test_changes(). Terminating the program at the end of the
function therefore ends the exection of test_changes() prematurely. As a
result, the test mode of configure-layer3 never reloads services after
a successful configuration revert.

Replace exit commands with appropriate function return values, which can
then be evaluated by the caller where appropriate.

While at it, add a missing return to the parameter validation in
execute_subshell().

Fixes: #256

Signed-off-by: Fabian Bläse <fabian@blaese.de>
rohammer approved these changes 2022-07-20 17:52:22 +02:00
rohammer left a comment
Member

Reviewed-by: Robert Langhammer rlanghammer@web.de

Reviewed-by: Robert Langhammer <rlanghammer@web.de>
jkimmel reviewed 2022-07-20 19:09:50 +02:00
@ -40,2 +40,3 @@
execute_subshell configure || return $?
exit 0
return 0
Owner

Ich glaub es reicht einfach execute_subshell configure alleine da stehen zu lassen. Die beiden returns können weg. Die Funktion nimmt den Wert des letzten Befehls an.

#!/bin/sh

success() { true; }
failure() { false; }
success; echo $?
failure; echo $?

Ich glaub es reicht einfach `execute_subshell configure` alleine da stehen zu lassen. Die beiden `return`s können weg. Die Funktion nimmt den Wert des letzten Befehls an. ```sh #!/bin/sh success() { true; } failure() { false; } success; echo $? failure; echo $? ```
Author
Owner

Das ist richtig, ich habe mich aber bewusst für diese etwas explizitere Form entschieden, weil ich das nicht offensichtlich finde. Dann müsste man es kommentieren, und dann kann ichs auch gleich ausschreiben.

Wenn noch mal jemand Code dazwischen fügt, dann bekommt der Code nicht versehentlich return values, die er eigentlich nicht haben soll.

Das ist richtig, ich habe mich aber bewusst für diese etwas explizitere Form entschieden, weil ich das nicht offensichtlich finde. Dann müsste man es kommentieren, und dann kann ichs auch gleich ausschreiben. Wenn noch mal jemand Code dazwischen fügt, dann bekommt der Code nicht versehentlich return values, die er eigentlich nicht haben soll.
@ -45,2 +46,4 @@
execute_subshell reload
reload_config
return 0
Owner

Brauchsts das?

Brauchsts das?
Author
Owner

Ja, sonst gibts als Rückgabewert den Exitstatus von reload_config.

Ja, sonst gibts als Rückgabewert den Exitstatus von reload_config.
@ -51,2 +54,3 @@
reload_services
exit 0
return 0
Owner

Und das?

Und das?
Author
Owner

Da reload_services sowieso immer 0 als return value hat.. Könnte man weglassen (da der Rückgabewert sowieso nirgends geprüft wird sowieso), aber ich finde es aus dem gleichen Grund wie weiter oben schöner, wenn man es explizit stehen lässt.

Da reload_services sowieso immer 0 als return value hat.. Könnte man weglassen (da der Rückgabewert sowieso nirgends geprüft wird sowieso), aber ich finde es aus dem gleichen Grund wie weiter oben schöner, wenn man es explizit stehen lässt.
@ -56,2 +60,3 @@
execute_subshell revert
exit 0
return 0
Owner

Und hier?

Und hier?
fbl modified the milestone from next-bugfix to 20220814 2022-07-21 19:57:45 +02:00
Author
Owner

Applied.

Applied.
fbl closed this pull request 2022-07-22 14:31:51 +02:00
fbl deleted branch testmodefix 2022-07-22 14:32:18 +02:00

Pull request closed

Sign in to join this conversation.
No description provided.