Skip to content

Commit

Permalink
builtin/kargs: fix editor flow
Browse files Browse the repository at this point in the history 
This reworks the `kargs --editor`, fixing an existing regression and
adding support for `--unchanged-exit-77` too (for the sake of easier
testing).
  • Loading branch information
@lucab @cgwalters
lucab authored and cgwalters committed Nov 12, 2021
1 parent ac22a18 commit 551436a
Show file tree
Hide file tree
Showing 5 changed files with 177 additions and 73 deletions.
75 changes: 47 additions & 28 deletions src/app/rpmostree-builtin-kargs.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,15 @@ static GOptionEntry option_entries[] = {
static gboolean
kernel_arg_handle_editor (const char *input_kernel_arg,
const char **out_kernel_arg,
gboolean *out_kargs_changed,
GCancellable *cancellable,
GError **error)
{
g_autofree char *chomped_input = g_strchomp (g_strdup (input_kernel_arg));

g_autoptr(OstreeKernelArgs) temp_kargs = ostree_kernel_args_from_string (chomped_input);

*out_kargs_changed = FALSE;

/* We check existance of ostree argument, if it
* exists, we directly remove it. Also Note, since the current kernel
* arguments are directly collected from the boot config, we expect that
Expand Down Expand Up @@ -133,9 +135,11 @@ kernel_arg_handle_editor (const char *input_kernel_arg,
g_autofree char *kernel_args_str = g_string_free (util::move_nullify (kernel_arg_buf), FALSE);
g_strchomp (kernel_args_str);

g_autoptr(OstreeKernelArgs) input_kargs = ostree_kernel_args_from_string (kernel_args_str);
/* Compare input and user content, in order to signal whether there was any real change */
*out_kargs_changed = !g_str_equal (filtered_input, kernel_args_str);

/* We check again to see if the ostree argument got added by the user */
g_autoptr(OstreeKernelArgs) input_kargs = ostree_kernel_args_from_string (kernel_args_str);
if (ostree_kernel_args_get_last_value (input_kargs, "ostree") != NULL)
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
Expand All @@ -151,14 +155,6 @@ kernel_arg_handle_editor (const char *input_kernel_arg,
return FALSE;
}

/* Notify the user that nothing has been changed */
if (g_str_equal (filtered_input, kernel_args_str))
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
"The kernel arguments remained the same");
return FALSE;
}

*out_kernel_arg = util::move_nullify (kernel_args_str);

return TRUE;
Expand Down Expand Up @@ -266,6 +262,8 @@ rpmostree_builtin_kargs (int argc,
return TRUE;
}

g_autoptr(GVariant) previous_deployment = rpmostree_os_dup_default_deployment (os_proxy);

g_autofree char *transaction_address = NULL;
char *empty_strv[] = {NULL};

Expand All @@ -274,12 +272,6 @@ rpmostree_builtin_kargs (int argc,
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);
g_variant_dict_insert (&dict, "initiating-command-line", "s", invocation->command_line);
g_variant_dict_insert (&dict, "lock-finalization", "b", opt_lock_finalization);
if (opt_kernel_append_if_missing_strings && *opt_kernel_append_if_missing_strings)
g_variant_dict_insert (&dict, "append-if-missing", "^as", opt_kernel_append_if_missing_strings);
if (opt_kernel_delete_if_present_strings && *opt_kernel_delete_if_present_strings)
g_variant_dict_insert (&dict, "delete-if-present", "^as", opt_kernel_delete_if_present_strings);
g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_dict_end (&dict));
g_autoptr(GVariant) previous_deployment = rpmostree_os_dup_default_deployment (os_proxy);

if (opt_editor)
{
Expand All @@ -291,33 +283,55 @@ rpmostree_builtin_kargs (int argc,
return FALSE;

const char* current_kernel_arg_string = NULL;
gboolean kargs_changed = FALSE;
if (!kernel_arg_handle_editor (old_kernel_arg_string, &current_kernel_arg_string,
cancellable, error))
&kargs_changed, cancellable, error))
return FALSE;
gboolean out_changed = FALSE;
if (!kargs_changed)
{
if (opt_unchanged_exit_77)
{
g_print("No changes.\n");
invocation->exit_code = RPM_OSTREE_EXIT_UNCHANGED;
return TRUE;
}
else
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
"The kernel arguments remained the same");
return FALSE;
}
}

/* Here we load the sysroot again, if the sysroot
* has been changed since then, we directly error
* out.
*/
gboolean sysroot_changed = FALSE;
if (!ostree_sysroot_load_if_changed (before_sysroot,
&out_changed,
&sysroot_changed,
cancellable,
error))
return FALSE;
if (out_changed)
if (sysroot_changed)
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
"Conflict: bootloader configuration changed. Saved kernel arguments: \n%s", current_kernel_arg_string);

return FALSE;
}
/* We use the user defined kernel args as existing arguments here
* and kept other strvs empty, because the existing kernel arguments
* is already enough for the update
*/

/* By default, the daemon performs change detection and can short-circuit
* if nothing has been appended/replace/deleted.
* When going through the editor flow, pre-filtering logic is applied
* client-side and the final command-line is directly passed to the daemon.
* TODO(lucab): switch all flows to perform client-side assembling, like the
* editor currently does: https://github.com/coreos/rpm-ostree/issues/2712 */
g_variant_dict_insert (&dict, "final-kernel-args", "s", current_kernel_arg_string);
g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_dict_end (&dict));

if (!rpmostree_os_call_kernel_args_sync (os_proxy,
current_kernel_arg_string,
old_kernel_arg_string,
(const char* const*) empty_strv,
(const char* const*) empty_strv,
(const char* const*) empty_strv,
Expand All @@ -330,16 +344,21 @@ rpmostree_builtin_kargs (int argc,
else
{
/* dbus does not allow NULL to mean the empty string array,
* assign them to be empty string array here to prevent erroring out
*/
* assign them to be empty string array here to prevent erroring out. */
if (!opt_kernel_replace_strings)
opt_kernel_replace_strings = empty_strv;
if (!opt_kernel_append_strings)
opt_kernel_append_strings = empty_strv;
if (!opt_kernel_delete_strings)
opt_kernel_delete_strings = empty_strv;

/* call the generearted dbus-function */
if (opt_kernel_append_if_missing_strings && *opt_kernel_append_if_missing_strings)
g_variant_dict_insert (&dict, "append-if-missing", "^as", opt_kernel_append_if_missing_strings);
if (opt_kernel_delete_if_present_strings && *opt_kernel_delete_if_present_strings)
g_variant_dict_insert (&dict, "delete-if-present", "^as", opt_kernel_delete_if_present_strings);
g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_dict_end (&dict));

/* call the generated dbus-function */
if (!rpmostree_os_call_kernel_args_sync (os_proxy,
old_kernel_arg_string,
(const char* const*) opt_kernel_append_strings,
Expand Down
5 changes: 5 additions & 0 deletions src/daemon/org.projectatomic.rpmostree1.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,11 @@
</method>

<!-- Available options:
"append-if-missing" (type 'as')
"delete-if-present" (type 'as')
"final-kernel-args" (type 's')
"initiating-command-line" (type 's')
"lock-finalization" (type 'b')
"reboot" (type 'b')
-->
<method name="KernelArgs">
Expand Down
143 changes: 104 additions & 39 deletions src/daemon/rpmostreed-transaction-types.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2781,35 +2781,75 @@ kernel_arg_transaction_finalize (GObject *object)
}

static gboolean
kernel_arg_transaction_execute (RpmostreedTransaction *transaction,
GCancellable *cancellable,
GError **error)
kernel_arg_apply(KernelArgTransaction *self,
RpmOstreeSysrootUpgrader *upgrader,
OstreeKernelArgs *kargs,
gboolean changed,
GCancellable *cancellable,
GError **error)
{
KernelArgTransaction *self = (KernelArgTransaction *) transaction;
OstreeSysroot *sysroot = rpmostreed_transaction_get_sysroot (transaction);
int upgrader_flags = 0;
auto command_line = static_cast<const char *>(vardict_lookup_ptr (self->options, "initiating-command-line", "&s"));
g_autofree char **append_if_missing = static_cast<char **>(vardict_lookup_strv_canonical (self->options, "append-if-missing"));
g_autofree char **delete_if_present = static_cast<char **>(vardict_lookup_strv_canonical (self->options, "delete-if-present"));
gboolean changed = FALSE;
g_return_val_if_fail (self != NULL, FALSE);
g_return_val_if_fail (upgrader != NULL, FALSE);
g_return_val_if_fail (kargs != NULL, FALSE);

/* don't want to pull new content for this */
upgrader_flags |= RPMOSTREE_SYSROOT_UPGRADER_FLAGS_SYNTHETIC_PULL;
upgrader_flags |= RPMOSTREE_SYSROOT_UPGRADER_FLAGS_PKGCACHE_ONLY;
if (vardict_lookup_bool (self->options, "lock-finalization", FALSE))
upgrader_flags |= RPMOSTREE_SYSROOT_UPGRADER_FLAGS_LOCK_FINALIZATION;
if (!changed)
{
rpmostree_output_message ("No changes.");
return TRUE;
}

/* Read in the existing kernel args and convert those to an #OstreeKernelArg instance for API usage */
g_autoptr(OstreeKernelArgs) kargs = ostree_kernel_args_from_string (self->existing_kernel_args);
g_autoptr(RpmOstreeSysrootUpgrader) upgrader =
rpmostree_sysroot_upgrader_new (sysroot, self->osname, static_cast<RpmOstreeSysrootUpgraderFlags>(upgrader_flags),
cancellable, error);
if (upgrader == NULL)
g_auto(GStrv) kargs_strv = ostree_kernel_args_to_strv (kargs);
rpmostree_sysroot_upgrader_set_kargs (upgrader, kargs_strv);

if (!rpmostree_sysroot_upgrader_deploy (upgrader, NULL, cancellable, error))
return FALSE;
rpmostree_sysroot_upgrader_set_caller_info (upgrader, command_line,
rpmostreed_transaction_get_agent_id (RPMOSTREED_TRANSACTION(self)),
rpmostreed_transaction_get_sd_unit (RPMOSTREED_TRANSACTION(self)));

if (vardict_lookup_bool (self->options, "reboot", FALSE))
{
if (!check_sd_inhibitor_locks (cancellable, error))
return FALSE;
rpmostreed_daemon_reboot (rpmostreed_daemon_get ());
}

return TRUE;
}

static gboolean
kernel_arg_apply_final_str(KernelArgTransaction *self,
RpmOstreeSysrootUpgrader *upgrader,
const char *final_kernel_args,
GCancellable *cancellable,
GError **error)
{
g_return_val_if_fail (self != NULL, FALSE);
g_return_val_if_fail (upgrader != NULL, FALSE);
g_return_val_if_fail (final_kernel_args != NULL, FALSE);
g_return_val_if_fail (self->existing_kernel_args != NULL, FALSE);

gboolean changed = (!g_str_equal (self->existing_kernel_args, final_kernel_args));
g_autoptr(OstreeKernelArgs) kargs = ostree_kernel_args_from_string (final_kernel_args);
if (!kernel_arg_apply (self, upgrader, kargs, changed, cancellable, error))
return FALSE;

return TRUE;
}

static gboolean
kernel_arg_apply_patching(KernelArgTransaction *self,
RpmOstreeSysrootUpgrader *upgrader,
OstreeKernelArgs *kargs,
GCancellable *cancellable,
GError **error)
{
g_return_val_if_fail (self != NULL, FALSE);
g_return_val_if_fail (upgrader != NULL, FALSE);
g_return_val_if_fail (kargs != NULL, FALSE);
g_return_val_if_fail (self->existing_kernel_args != NULL, FALSE);

g_autofree char **append_if_missing = static_cast<char **>(vardict_lookup_strv_canonical (self->options, "append-if-missing"));
g_autofree char **delete_if_present = static_cast<char **>(vardict_lookup_strv_canonical (self->options, "delete-if-present"));
g_auto(GStrv) existing_kargs = g_strsplit (self->existing_kernel_args, " ", -1);
gboolean changed = FALSE;

/* Delete all the entries included in the kernel args */
for (char **iter = self->kernel_args_deleted; iter && *iter; iter++)
Expand Down Expand Up @@ -2855,27 +2895,52 @@ kernel_arg_transaction_execute (RpmostreedTransaction *transaction,
}
}

if (!changed)
{
rpmostree_output_message ("No changes.");
return TRUE;
}
if (!kernel_arg_apply(self, upgrader, kargs, changed, cancellable, error))
return FALSE;

/* After all the arguments are processed earlier, we convert it to a string list*/
g_auto(GStrv) kargs_strv = ostree_kernel_args_to_strv (kargs);
rpmostree_sysroot_upgrader_set_kargs (upgrader, kargs_strv);
return TRUE;
}

if (!rpmostree_sysroot_upgrader_deploy (upgrader, NULL, cancellable, error))
static gboolean
kernel_arg_transaction_execute (RpmostreedTransaction *transaction,
GCancellable *cancellable,
GError **error)
{
g_return_val_if_fail (transaction != NULL, FALSE);

KernelArgTransaction *self = (KernelArgTransaction *) transaction;
OstreeSysroot *sysroot = rpmostreed_transaction_get_sysroot (transaction);
auto command_line = static_cast<const char *>(vardict_lookup_ptr (self->options, "initiating-command-line", "&s"));

/* don't want to pull new content for this */
int upgrader_flags = 0;
upgrader_flags |= RPMOSTREE_SYSROOT_UPGRADER_FLAGS_SYNTHETIC_PULL;
upgrader_flags |= RPMOSTREE_SYSROOT_UPGRADER_FLAGS_PKGCACHE_ONLY;
if (vardict_lookup_bool (self->options, "lock-finalization", FALSE))
upgrader_flags |= RPMOSTREE_SYSROOT_UPGRADER_FLAGS_LOCK_FINALIZATION;

/* Read in the existing kernel args and convert those to an #OstreeKernelArg instance for API usage */
g_autoptr(OstreeKernelArgs) kargs = ostree_kernel_args_from_string (self->existing_kernel_args);
g_autoptr(RpmOstreeSysrootUpgrader) upgrader =
rpmostree_sysroot_upgrader_new (sysroot, self->osname, static_cast<RpmOstreeSysrootUpgraderFlags>(upgrader_flags),
cancellable, error);
if (upgrader == NULL)
return FALSE;
rpmostree_sysroot_upgrader_set_caller_info (upgrader, command_line,
rpmostreed_transaction_get_agent_id (RPMOSTREED_TRANSACTION(self)),
rpmostreed_transaction_get_sd_unit (RPMOSTREED_TRANSACTION(self)));

if (vardict_lookup_bool (self->options, "reboot", FALSE))
auto final_kernel_args = static_cast<const char *>(vardict_lookup_ptr (self->options, "final-kernel-args", "&s"));
if (final_kernel_args != NULL)
{
if (!check_sd_inhibitor_locks (cancellable, error))
return FALSE;
rpmostreed_daemon_reboot (rpmostreed_daemon_get ());
/* Pre-assembled kargs string (e.g. coming from --editor) */
return kernel_arg_apply_final_str(self, upgrader, final_kernel_args, cancellable, error);
}
else
{
/* Arguments patching via append/replace/delete */
return kernel_arg_apply_patching(self, upgrader, kargs, cancellable, error);
}

return TRUE;
}

static void
Expand Down
3 changes: 2 additions & 1 deletion tests/common/libvm.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ vm_clean_caches() {
# run rpm-ostree in vm
# - $@ args
vm_rpmostree() {
vm_cmd env ASAN_OPTIONS=detect_leaks=false rpm-ostree "$@"
TESTEDITOR="${EDITOR:-}"
vm_cmd -- env \"EDITOR=${TESTEDITOR}\" ASAN_OPTIONS=detect_leaks=false rpm-ostree "$@"
}

# copy the test repo to the vm
Expand Down
24 changes: 19 additions & 5 deletions tests/vmcheck/test-kernel-args.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,19 +167,33 @@ vm_rpmostree kargs > if_not_present.txt
diff kargs.txt if_not_present.txt
echo "ok kargs deleted with delete-if-present only if present"

#Test for rpm-ostree kargs unchanged-exit-77
# Test for rpm-ostree kargs unchanged-exit-77
vm_rpmostree kargs --append-if-missing=PACKAGE3=TEST3 --unchanged-exit-77
rc=0
vm_rpmostree kargs --append-if-missing=PACKAGE3=TEST3 --unchanged-exit-77 || rc=$?
assert_streq $rc 0
vm_rpmostree kargs --append-if-missing=PACKAGE3=TEST3 --unchanged-exit-77 || rc=$?
assert_streq $rc 77
vm_rpmostree kargs --delete-if-present=PACKAGE3=TEST3 --unchanged-exit-77
rc=0
vm_rpmostree kargs --delete-if-present=PACKAGE3=TEST3 --unchanged-exit-77 || rc=$?
assert_streq $rc 0
vm_rpmostree kargs --delete-if-present=PACKAGE3=TEST3 --unchanged-exit-77 || rc=$?
assert_streq $rc 77
echo "ok exit 77 when unchanged kargs with unchanged-exit-77"

# Test for 'rpm-ostree kargs --editor'.
vm_rpmostree kargs > kargs.txt
assert_not_file_has_content_literal kargs.txt 'nonexisting'
rc=0
EDITOR="sed -i 's/nonexisting//'" vm_rpmostree kargs --editor || rc=$?
assert_streq $rc 1
rc=0
EDITOR="sed -i 's/nonexisting//'" vm_rpmostree kargs --editor --unchanged-exit-77 || rc=$?
assert_streq $rc 77
vm_rpmostree kargs > kargs.txt
assert_not_file_has_content_literal kargs.txt 'nonexisting'
assert_not_file_has_content_literal kargs.txt 'editorkey=editorvalue'
EDITOR="sed -i 's/$/ editorkey=editorvalue/'" vm_rpmostree kargs --editor
vm_rpmostree kargs > kargs.txt
assert_file_has_content_literal kargs.txt 'editorkey=editorvalue'
echo "ok kargs editor"

# XXX: uncomment this when we migrate CI to FCOS
# # And reset this bit
Expand Down

0 comments on commit 551436a

Please sign in to comment.