From 9482ecfe5ad355a405d17272564c8b15c9f1d84b Mon Sep 17 00:00:00 2001 From: William Manley Date: Mon, 6 Jul 2020 15:50:28 +0100 Subject: [PATCH] Refactor: sysroot.bootloader: Store enum value rather than string It's easier to extend and it centralises the config parsing. In other places we will no longer need to use `g_str_equal` to match these values, a `switch` statement will be sufficient. --- src/libostree/ostree-repo-private.h | 18 +++++++- src/libostree/ostree-repo.c | 41 +++++++++-------- src/libostree/ostree-sysroot.c | 69 +++++++++++++++-------------- 3 files changed, 73 insertions(+), 55 deletions(-) diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index cbbe6971..14e2f753 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -110,6 +110,22 @@ typedef enum { _OSTREE_FEATURE_YES, } _OstreeFeatureSupport; +/* Possible values for the sysroot.bootloader configuration variable */ +typedef enum { + CFG_SYSROOT_BOOTLOADER_OPT_AUTO = 0, + CFG_SYSROOT_BOOTLOADER_OPT_NONE, + CFG_SYSROOT_BOOTLOADER_OPT_ZIPL, + /* Non-exhaustive */ +} OstreeCfgSysrootBootloaderOpt; + +static const char* const CFG_SYSROOT_BOOTLOADER_OPTS_STR[] = { + /* This must be kept in the same order as the enum */ + "auto", + "none", + "zipl", + NULL, +}; + /** * OstreeRepo: * @@ -193,7 +209,7 @@ struct OstreeRepo { guint64 payload_link_threshold; gint fs_support_reflink; /* The underlying filesystem has support for ioctl (FICLONE..) */ gchar **repo_finders; - gchar *bootloader; /* Configure which bootloader to use. */ + OstreeCfgSysrootBootloaderOpt bootloader; /* Configure which bootloader to use. */ OstreeRepo *parent_repo; }; diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index d3066e6a..11a209a4 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -1048,7 +1048,6 @@ ostree_repo_finalize (GObject *object) g_mutex_clear (&self->txn_lock); g_free (self->collection_id); g_strfreev (self->repo_finders); - g_free (self->bootloader); g_clear_pointer (&self->remotes, g_hash_table_destroy); g_mutex_clear (&self->remotes_lock); @@ -3186,28 +3185,28 @@ reload_sysroot_config (OstreeRepo *self, GCancellable *cancellable, GError **error) { - { g_autofree char *bootloader = NULL; + g_autofree char *bootloader = NULL; - if (!ot_keyfile_get_value_with_default_group_optional (self->config, "sysroot", - "bootloader", "auto", - &bootloader, error)) - return FALSE; + if (!ot_keyfile_get_value_with_default_group_optional (self->config, "sysroot", + "bootloader", "auto", + &bootloader, error)) + return FALSE; - /* TODO: possibly later add support for specifying a generic bootloader - * binary "x" in /usr/lib/ostree/bootloaders/x). See: - * https://github.com/ostreedev/ostree/issues/1719 - * https://github.com/ostreedev/ostree/issues/1801 - * Also, dedup these strings with the bootloader implementations - */ - if (!(g_str_equal (bootloader, "auto") || g_str_equal (bootloader, "none") - || g_str_equal (bootloader, "zipl"))) - return glnx_throw (error, "Invalid bootloader configuration: '%s'", bootloader); + /* TODO: possibly later add support for specifying a generic bootloader + * binary "x" in /usr/lib/ostree/bootloaders/x). See: + * https://github.com/ostreedev/ostree/issues/1719 + * https://github.com/ostreedev/ostree/issues/1801 + */ + for (int i = 0; CFG_SYSROOT_BOOTLOADER_OPTS_STR[i]; i++) + { + if (g_str_equal (bootloader, CFG_SYSROOT_BOOTLOADER_OPTS_STR[i])) + { + self->bootloader = (OstreeCfgSysrootBootloaderOpt) i; + return TRUE; + } + } - g_free (self->bootloader); - self->bootloader = g_steal_pointer (&bootloader); - } - - return TRUE; + return glnx_throw (error, "Invalid bootloader configuration: '%s'", bootloader); } /** @@ -6331,7 +6330,7 @@ ostree_repo_get_bootloader (OstreeRepo *self) { g_return_val_if_fail (OSTREE_IS_REPO (self), NULL); - return self->bootloader; + return CFG_SYSROOT_BOOTLOADER_OPTS_STR[self->bootloader]; } diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c index f183edd1..63065fb9 100644 --- a/src/libostree/ostree-sysroot.c +++ b/src/libostree/ostree-sysroot.c @@ -1340,50 +1340,53 @@ _ostree_sysroot_query_bootloader (OstreeSysroot *sysroot, GError **error) { OstreeRepo *repo = ostree_sysroot_repo (sysroot); - g_autofree gchar *bootloader_config = ostree_repo_get_bootloader (repo); + OstreeCfgSysrootBootloaderOpt bootloader_config = repo->bootloader; - g_debug ("Using bootloader configuration: %s", bootloader_config); + g_debug ("Using bootloader configuration: %s", + CFG_SYSROOT_BOOTLOADER_OPTS_STR[bootloader_config]); g_autoptr(OstreeBootloader) ret_loader = NULL; - if (g_str_equal (bootloader_config, "none")) + switch (repo->bootloader) { + case CFG_SYSROOT_BOOTLOADER_OPT_NONE: /* No bootloader specified; do not query bootloaders to run. */ ret_loader = NULL; - } - else if (g_str_equal (bootloader_config, "zipl")) - { + break; + case CFG_SYSROOT_BOOTLOADER_OPT_ZIPL: /* We never consider zipl as active by default, so it can only be created * if it's explicitly requested in the config */ ret_loader = (OstreeBootloader*) _ostree_bootloader_zipl_new (sysroot); - } - else if (g_str_equal (bootloader_config, "auto")) - { - gboolean is_active; - ret_loader = (OstreeBootloader*)_ostree_bootloader_syslinux_new (sysroot); - if (!_ostree_bootloader_query (ret_loader, &is_active, - cancellable, error)) - return FALSE; + break; + case CFG_SYSROOT_BOOTLOADER_OPT_AUTO: + { + gboolean is_active; + ret_loader = (OstreeBootloader*)_ostree_bootloader_syslinux_new (sysroot); + if (!_ostree_bootloader_query (ret_loader, &is_active, + cancellable, error)) + return FALSE; - if (!is_active) - { - g_object_unref (ret_loader); - ret_loader = (OstreeBootloader*)_ostree_bootloader_grub2_new (sysroot); - if (!_ostree_bootloader_query (ret_loader, &is_active, - cancellable, error)) - return FALSE; - } - if (!is_active) - { - g_object_unref (ret_loader); - ret_loader = (OstreeBootloader*)_ostree_bootloader_uboot_new (sysroot); - if (!_ostree_bootloader_query (ret_loader, &is_active, cancellable, error)) - return FALSE; - } - if (!is_active) - g_clear_object (&ret_loader); + if (!is_active) + { + g_object_unref (ret_loader); + ret_loader = (OstreeBootloader*)_ostree_bootloader_grub2_new (sysroot); + if (!_ostree_bootloader_query (ret_loader, &is_active, + cancellable, error)) + return FALSE; + } + if (!is_active) + { + g_object_unref (ret_loader); + ret_loader = (OstreeBootloader*)_ostree_bootloader_uboot_new (sysroot); + if (!_ostree_bootloader_query (ret_loader, &is_active, cancellable, error)) + return FALSE; + } + if (!is_active) + g_clear_object (&ret_loader); + break; + } + default: + g_assert_not_reached (); } - else - g_assert_not_reached (); ot_transfer_out_value(out_bootloader, &ret_loader); return TRUE;