From a4487578a72089996237ad41ae51862cee55891c Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 20 Jan 2021 15:22:17 +0000 Subject: [PATCH] Remove some uses of `goto out` All of these cases are actually fine, but in general we can't use `goto out` since we started using C++ exceptions which will skip that control flow. --- src/app/rpmostree-builtin-start-daemon.cxx | 19 +++++++------- src/daemon/rpmostreed-utils.cxx | 18 ++++--------- src/libpriv/rpmostree-bwrap.cxx | 30 ++++++++++++---------- src/libpriv/rpmostree-util.cxx | 10 ++++++++ src/libpriv/rpmostree-util.h | 3 +++ tests/check/postprocess.cxx | 9 ++----- 6 files changed, 45 insertions(+), 44 deletions(-) diff --git a/src/app/rpmostree-builtin-start-daemon.cxx b/src/app/rpmostree-builtin-start-daemon.cxx index 18f16a96..7c1bf2e7 100644 --- a/src/app/rpmostree-builtin-start-daemon.cxx +++ b/src/app/rpmostree-builtin-start-daemon.cxx @@ -100,22 +100,21 @@ on_peer_acquired (GObject *source, GAsyncResult *result, gpointer user_data) { - g_autoptr(GDBusConnection) connection = NULL; g_autoptr(GError) error = NULL; - - connection = g_dbus_connection_new_finish (result, &error); + g_autoptr(GDBusConnection) connection = g_dbus_connection_new_finish (result, &error); if (!connection) - goto out; + { + state_transition_fatal_err (error); + return; + } if (!start_daemon (connection, &error)) - goto out; - - out: - if (error) - state_transition_fatal_err (error); + { + state_transition_fatal_err (error); + return; + } } - static gboolean on_sigint (gpointer user_data) { diff --git a/src/daemon/rpmostreed-utils.cxx b/src/daemon/rpmostreed-utils.cxx index cae3110a..4f51e4c0 100644 --- a/src/daemon/rpmostreed-utils.cxx +++ b/src/daemon/rpmostreed-utils.cxx @@ -521,7 +521,7 @@ rpmostreed_repo_lookup_cached_version (OstreeRepo *repo, g_return_val_if_fail (version != NULL, FALSE); if (!ostree_repo_resolve_rev (repo, refspec, FALSE, &checksum, error)) - goto out; + return FALSE; while (checksum != NULL) { @@ -529,10 +529,10 @@ rpmostreed_repo_lookup_cached_version (OstreeRepo *repo, gboolean stop = FALSE; if (!ostree_repo_load_commit (repo, checksum, &commit, NULL, error)) - goto out; + return FALSE; if (!version_visitor (repo, checksum, commit, &closure, &stop, error)) - goto out; + return FALSE; g_clear_pointer (&checksum, g_free); @@ -541,21 +541,13 @@ rpmostreed_repo_lookup_cached_version (OstreeRepo *repo, } if (closure.checksum == NULL) - { - g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND, - "Version %s not cached in %s", version, refspec); - goto out; - } + return glnx_throw (error, "Version %s not cached in %s", version, refspec); if (out_checksum != NULL) *out_checksum = util::move_nullify (closure.checksum); g_free (closure.checksum); - - ret = TRUE; - -out: - return ret; + return TRUE; } /** diff --git a/src/libpriv/rpmostree-bwrap.cxx b/src/libpriv/rpmostree-bwrap.cxx index be5fcf80..4b29dde6 100644 --- a/src/libpriv/rpmostree-bwrap.cxx +++ b/src/libpriv/rpmostree-bwrap.cxx @@ -199,6 +199,18 @@ setup_rofiles (RpmOstreeBwrap *bwrap, return TRUE; } + +/* We don't want a failure to unmount to be fatal, so all we do here + * is log. Though in practice what we *really* want is for the + * fusermount to be in the bwrap namespace, and hence tied by the + * kernel to the lifecycle of the container. This would require + * special casing for somehow doing FUSE mounts in bwrap. Which + * would be hard because NO_NEW_PRIVS turns off the setuid bits for + * fuse. + * + * Or: just hard switch to overlayfs now that it will soon be + * available even unprivileged https://lwn.net/Articles/803203/ + */ static void teardown_rofiles (GLnxTmpDir *mnt_tmp) { @@ -216,27 +228,17 @@ teardown_rofiles (GLnxTmpDir *mnt_tmp) NULL, NULL, NULL, NULL, &estatus, &tmp_error)) { g_prefix_error (&tmp_error, "Executing fusermount: "); - goto out; + rpmostree_journal_error (tmp_error); + return; } if (!g_spawn_check_exit_status (estatus, &tmp_error)) { g_prefix_error (&tmp_error, "Executing fusermount: "); - goto out; + rpmostree_journal_error (tmp_error); + return; } (void)glnx_tmpdir_delete (mnt_tmp, NULL, NULL); - - out: - /* We don't want a failure to unmount to be fatal, so all we do here - * is log. Though in practice what we *really* want is for the - * fusermount to be in the bwrap namespace, and hence tied by the - * kernel to the lifecycle of the container. This would require - * special casing for somehow doing FUSE mounts in bwrap. Which - * would be hard because NO_NEW_PRIVS turns off the setuid bits for - * fuse. - */ - if (tmp_error) - sd_journal_print (LOG_WARNING, "%s", tmp_error->message); } /* nspawn by default doesn't give us CAP_NET_ADMIN; see diff --git a/src/libpriv/rpmostree-util.cxx b/src/libpriv/rpmostree-util.cxx index 6bdfb4ca..c305fc63 100644 --- a/src/libpriv/rpmostree-util.cxx +++ b/src/libpriv/rpmostree-util.cxx @@ -1190,3 +1190,13 @@ rpmostree_maybe_shell_quote (const char *s) return NULL; return g_shell_quote (s); } + +/* Given an error, log it to the systemd journal; use this + * for code paths where we can't easily propagate it back + * up the stack - particularly "shouldn't happen" errors. + */ +void +rpmostree_journal_error (GError *error) +{ + sd_journal_print (LOG_WARNING, "%s", error->message); +} diff --git a/src/libpriv/rpmostree-util.h b/src/libpriv/rpmostree-util.h index 026b61e5..3ba50feb 100644 --- a/src/libpriv/rpmostree-util.h +++ b/src/libpriv/rpmostree-util.h @@ -287,4 +287,7 @@ rpmostree_relative_path_is_ostree_compliant (const char *path); char* rpmostree_maybe_shell_quote (const char *s); +void +rpmostree_journal_error (GError *error); + G_END_DECLS diff --git a/tests/check/postprocess.cxx b/tests/check/postprocess.cxx index 135be94d..f98a2275 100644 --- a/tests/check/postprocess.cxx +++ b/tests/check/postprocess.cxx @@ -70,15 +70,10 @@ test_postprocess_altfiles (void) { AltfilesTest *test = &altfiles_tests[i]; g_autofree char *newbuf = rpmostree_postprocess_replace_nsswitch (test->input, error); - - if (!newbuf) - goto out; - + g_assert_no_error (error); + g_assert (newbuf); g_assert_cmpstr (newbuf, ==, test->output); } - - out: - g_assert_no_error (local_error); } int