Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move reboot handling to client #2845

Closed
wants to merge 1 commit into from
Closed

Move reboot handling to client #2845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/app/rpmostree-builtin-deploy.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ rpmostree_builtin_deploy (int argc,

GVariantDict dict;
g_variant_dict_init (&dict, NULL);
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);
g_variant_dict_insert (&dict, "allow-downgrade", "b", !opt_disallow_downgrade);
/* If we're not specifying a revision, then don't touch the network */
if (revision == NULL)
Expand Down Expand Up @@ -225,8 +224,12 @@ rpmostree_builtin_deploy (int argc,
}

rpmostree_print_package_diffs (result);
return TRUE;
}
else if (!opt_reboot)

if (opt_reboot)
return rpmostree_client_reboot (error);
else
{
if (!rpmostree_has_new_default_deployment (os_proxy, previous_deployment))
{
Expand Down
3 changes: 2 additions & 1 deletion src/app/rpmostree-builtin-finalize-deployment.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ rpmostree_builtin_finalize_deployment (int argc,
g_variant_dict_insert (&dict, "allow-missing-checksum", "b", opt_allow_missing);
g_variant_dict_insert (&dict, "allow-unlocked", "b", opt_allow_unlocked);
g_variant_dict_insert (&dict, "initiating-command-line", "s", invocation->command_line);
g_variant_dict_insert (&dict, "reboot", "b", FALSE);
g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_dict_end (&dict));

g_autofree char *transaction_address = NULL;
Expand All @@ -98,5 +99,5 @@ rpmostree_builtin_finalize_deployment (int argc,
error))
return FALSE;

return TRUE;
return rpmostree_client_reboot (error);
}
5 changes: 3 additions & 2 deletions src/app/rpmostree-builtin-initramfs-etc.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ rpmostree_ex_builtin_initramfs_etc (int argc,

GVariantDict dict;
g_variant_dict_init (&dict, NULL);
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);
g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_dict_end (&dict));
Expand All @@ -138,7 +137,9 @@ rpmostree_ex_builtin_initramfs_etc (int argc,
error))
return FALSE;

if (!opt_reboot)
if (opt_reboot)
return rpmostree_client_reboot (error);
else
{
if (!rpmostree_has_new_default_deployment (os_proxy, previous_deployment))
{
Expand Down
4 changes: 3 additions & 1 deletion src/app/rpmostree-builtin-initramfs.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ rpmostree_builtin_initramfs (int argc,

GVariantDict dict;
g_variant_dict_init (&dict, NULL);
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);
g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_dict_end (&dict));
Expand All @@ -156,6 +155,9 @@ rpmostree_builtin_initramfs (int argc,
return FALSE;

g_print ("Initramfs regeneration is now: %s\n", opt_enable ? "enabled" : "disabled");

if (opt_reboot)
return rpmostree_client_reboot (error);
}

return TRUE;
Expand Down
4 changes: 3 additions & 1 deletion src/app/rpmostree-builtin-kargs.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,6 @@ rpmostree_builtin_kargs (int argc,

GVariantDict dict;
g_variant_dict_init (&dict, NULL);
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)
Expand Down Expand Up @@ -363,5 +362,8 @@ rpmostree_builtin_kargs (int argc,
else if (opt_unchanged_exit_77)
invocation->exit_code = RPM_OSTREE_EXIT_UNCHANGED;

if (opt_reboot)
return rpmostree_client_reboot (error);

return TRUE;
}
3 changes: 1 addition & 2 deletions src/app/rpmostree-builtin-rebase.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ rpmostree_builtin_rebase (int argc,

GVariantDict dict;
g_variant_dict_init (&dict, NULL);
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);
g_variant_dict_insert (&dict, "allow-downgrade", "b", !opt_disallow_downgrade);
g_variant_dict_insert (&dict, "cache-only", "b", opt_cache_only);
g_variant_dict_insert (&dict, "download-only", "b", opt_download_only);
Expand Down Expand Up @@ -231,7 +230,7 @@ rpmostree_builtin_rebase (int argc,
}

return rpmostree_transaction_client_run (invocation, sysroot_proxy, os_proxy,
options, FALSE,
options, FALSE, opt_reboot,
transaction_address,
previous_deployment,
cancellable, error);
Expand Down
3 changes: 1 addition & 2 deletions src/app/rpmostree-builtin-reset.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ rpmostree_builtin_reset (int argc,

GVariantDict dict;
g_variant_dict_init (&dict, NULL);
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);
g_variant_dict_insert (&dict, "no-pull-base", "b", TRUE);
g_variant_dict_insert (&dict, "no-layering", "b", opt_overlays);
g_variant_dict_insert (&dict, "no-overrides", "b", opt_overrides);
Expand All @@ -110,7 +109,7 @@ rpmostree_builtin_reset (int argc,
return FALSE;

return rpmostree_transaction_client_run (invocation, sysroot_proxy, os_proxy,
options, FALSE,
options, FALSE, opt_reboot,
transaction_address,
previous_deployment,
cancellable, error);
Expand Down
4 changes: 1 addition & 3 deletions src/app/rpmostree-builtin-rollback.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ get_args_variant (void)
GVariantDict dict;

g_variant_dict_init (&dict, NULL);
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);

return g_variant_dict_end (&dict);
}
Expand Down Expand Up @@ -78,7 +77,6 @@ rpmostree_builtin_rollback (int argc,
/* really, rollback only supports the "reboot" option; all others are ignored */
GVariantDict dict;
g_variant_dict_init (&dict, NULL);
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);
g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_dict_end (&dict));

if (!rpmostree_os_call_rollback_sync (os_proxy,
Expand All @@ -89,7 +87,7 @@ rpmostree_builtin_rollback (int argc,
return FALSE;

return rpmostree_transaction_client_run (invocation, sysroot_proxy, os_proxy,
options, FALSE,
options, FALSE, opt_reboot,
transaction_address,
previous_deployment,
cancellable, error);
Expand Down
8 changes: 6 additions & 2 deletions src/app/rpmostree-builtin-upgrade.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ rpmostree_builtin_upgrade (int argc,
{
GVariantDict dict;
g_variant_dict_init (&dict, NULL);
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);
g_variant_dict_insert (&dict, "allow-downgrade", "b", opt_allow_downgrade);
g_variant_dict_insert (&dict, "cache-only", "b", opt_cache_only);
g_variant_dict_insert (&dict, "download-only", "b", opt_download_only);
Expand Down Expand Up @@ -233,8 +232,13 @@ rpmostree_builtin_upgrade (int argc,
cancellable, error))
return FALSE;
}

return TRUE;
}
else if (!opt_reboot)

if (opt_reboot)
return rpmostree_client_reboot (error);
else
{
if (!rpmostree_has_new_default_deployment (os_proxy, previous_deployment))
{
Expand Down
18 changes: 15 additions & 3 deletions src/app/rpmostree-clientlib.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,6 +904,7 @@ rpmostree_transaction_client_run (RpmOstreeCommandInvocation *invocation,
RPMOSTreeOS *os_proxy,
GVariant *options,
gboolean exit_unchanged_77,
gboolean opt_reboot,
const char *transaction_address,
GVariant *previous_deployment,
GCancellable *cancellable,
Expand All @@ -918,16 +919,20 @@ rpmostree_transaction_client_run (RpmOstreeCommandInvocation *invocation,

g_auto(GVariantDict) optdict = G_VARIANT_DICT_INIT (options);
/* Parse back the options variant */
gboolean opt_reboot = FALSE;
g_variant_dict_lookup (&optdict, "reboot", "b", &opt_reboot);
gboolean opt_dry_run = FALSE;
g_variant_dict_lookup (&optdict, "dry-run", "b", &opt_dry_run);

if (opt_dry_run)
{
g_print ("Exiting because of '--dry-run' option\n");
return TRUE;
}
else if (!opt_reboot)

if (opt_reboot)
{
return rpmostree_client_reboot (error);
}
else
{
if (!rpmostree_has_new_default_deployment (os_proxy, previous_deployment))
{
Expand Down Expand Up @@ -1782,3 +1787,10 @@ error_if_driver_registered (GBusType bus_type,

return TRUE;
}

gboolean
rpmostree_client_reboot (GError ** error)
{
execlp ("reboot", "reboot", NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this exec really intended here? I was somehow expecting some kind of fork+exec spawning (like in the original rpmostreed_reboot). This way instead it looks a bit like a dangerous landmine, because in the non-error case any other code following this function call won't really be reached.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a non-error case, the process will get killed by SIGTERM at some point soon later, so any code after would be inherently racy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-note, I had a personal doubt related to blocking-behavior of this action, and I double-checked the systemd manual:

This command is asynchronous; it will return after the reboot operation is enqueued, without waiting for it to complete.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare https://doc.rust-lang.org/std/os/unix/process/trait.CommandExt.html#tymethod.exec

Copy link
Contributor

@lucab lucab May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brutal but fair point. Although Rust type-system represents such kind of never-returning functions, while here it's one more thing to keep in mind. At least a comment would help to highlight the intentional blackhole.

Anyway I agree this way it makes potential races obvious upfront, and we don't seem to currently have any critical logic running afterwards, so fine for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it can return, just on error. This is where the Rust type system shows its strength because unlike almost every other function it doesn't return Result<()> it returns Error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you're completely right that the command itself is async...which is not usually what one wants. But that's also not a new issue here. The bug we're fixing is that the client can sometimes spuriously exit with an error because the daemon gets killed before it can send the success reply.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"never" as in the underlying diverging a case which can't be represented here. Sorry English language is confusing :)

Anyway I think we are in agreement. Mind adding a docstring highlighting that the TRUE case does not really return from this function blackhole and the whole execution flow diverges instead, and then we land it?

return glnx_null_throw_errno_prefix (error, "execlp(reboot)");
}
4 changes: 4 additions & 0 deletions src/app/rpmostree-clientlib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ rpmostree_transaction_client_run (RpmOstreeCommandInvocation *invoca
RPMOSTreeOS *os_proxy,
GVariant *options,
gboolean exit_unchanged_77,
gboolean reboot,
const char *transaction_address,
GVariant *previous_deployment,
GCancellable *cancellable,
Expand Down Expand Up @@ -176,4 +177,7 @@ error_if_driver_registered (GBusType bus_type,
GCancellable *cancellable,
GError **error);

gboolean
rpmostree_client_reboot (GError ** error);

G_END_DECLS
7 changes: 5 additions & 2 deletions src/app/rpmostree-override-builtins.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ handle_override (RPMOSTreeSysroot *sysroot_proxy,

GVariantDict dict;
g_variant_dict_init (&dict, NULL);
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);
g_variant_dict_insert (&dict, "cache-only", "b", cache_only);
g_variant_dict_insert (&dict, "no-pull-base", "b", TRUE);
g_variant_dict_insert (&dict, "dry-run", "b", opt_dry_run);
Expand Down Expand Up @@ -116,7 +115,11 @@ handle_override (RPMOSTreeSysroot *sysroot_proxy,
{
g_print ("Exiting because of '--dry-run' option\n");
}
else if (!opt_reboot)
else if (opt_reboot)
{
return rpmostree_client_reboot (error);
}
else
{
/* only print diff if a new deployment was laid down (e.g. reset --all may not) */
if (!rpmostree_has_new_default_deployment (os_proxy, previous_deployment))
Expand Down
3 changes: 1 addition & 2 deletions src/app/rpmostree-pkg-builtins.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ pkg_change (RpmOstreeCommandInvocation *invocation,

GVariantDict dict;
g_variant_dict_init (&dict, NULL);
g_variant_dict_insert (&dict, "reboot", "b", opt_reboot);
g_variant_dict_insert (&dict, "cache-only", "b", opt_cache_only);
g_variant_dict_insert (&dict, "download-only", "b", opt_download_only);
g_variant_dict_insert (&dict, "no-pull-base", "b", TRUE);
Expand Down Expand Up @@ -142,7 +141,7 @@ pkg_change (RpmOstreeCommandInvocation *invocation,
}

return rpmostree_transaction_client_run (invocation, sysroot_proxy, os_proxy,
options, opt_unchanged_exit_77,
options, opt_unchanged_exit_77, opt_reboot,
transaction_address,
previous_deployment,
cancellable, error);
Expand Down
4 changes: 4 additions & 0 deletions src/daemon/org.projectatomic.rpmostree1.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,10 @@
providing a checksum should be a conscious decision.
"allow-unlocked" (type 'b')
Don't error out if the staged deployment wasn't locked.
"reboot" (type 'b')
Have the daemon initiate a reboot. Defaults to `TRUE`.
This option exists for compatibility reasons; it's recommended
to set it to `FALSE` and start the reboot in the calling process.
-->
<method name="FinalizeDeployment">
<arg type="a{sv}" name="options" direction="in"/>
Expand Down
18 changes: 15 additions & 3 deletions src/daemon/rpmostreed-transaction-types.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1526,7 +1526,9 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
}

if (deploy_has_bool_option (self, "reboot"))
rpmostreed_reboot (cancellable, error);
{
rpmostreed_reboot (cancellable, error);
}
}
else
{
Expand Down Expand Up @@ -2604,6 +2606,9 @@ finalize_deployment_transaction_execute (RpmostreedTransaction *transaction,
(char*)vardict_lookup_ptr (self->options, "checksum", "&s");
const gboolean allow_missing_checksum =
vardict_lookup_bool (self->options, "allow-missing-checksum", FALSE);
// Note this defaults to TRUE
const gboolean reboot =
vardict_lookup_bool (self->options, "reboot", TRUE);
if (!expected_checksum && !allow_missing_checksum)
return glnx_throw (error, "Missing expected checksum");
if (expected_checksum && !g_str_equal (checksum, expected_checksum))
Expand All @@ -2624,8 +2629,15 @@ finalize_deployment_transaction_execute (RpmostreedTransaction *transaction,
* version of `rpm-ostree finalize-deployment`). */
(void) rpmostree_syscore_bump_mtime (sysroot, NULL);

sd_journal_print (LOG_INFO, "Finalized deployment; rebooting into %s", checksum);
rpmostreed_reboot (cancellable, error);
if (reboot)
{
sd_journal_print (LOG_INFO, "Finalized deployment; rebooting into %s", checksum);
rpmostreed_reboot (cancellable, error);
}
else
{
sd_journal_print (LOG_INFO, "Finalized deployment for %s", checksum);
}
return TRUE;
}

Expand Down
1 change: 1 addition & 0 deletions src/daemon/rpmostreed-utils.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ rpmostreed_refspec_parse_partial (const gchar *new_provided_refspec,
void
rpmostreed_reboot (GCancellable *cancellable, GError **error)
{
sd_journal_print (LOG_INFO, "daemon-initiated reboot is deprecated");
const char *child_argv[] = { "systemctl", "reboot", NULL };
(void) g_spawn_sync (NULL, (char**)child_argv, NULL, (GSpawnFlags)(G_SPAWN_CHILD_INHERITS_STDIN | G_SPAWN_SEARCH_PATH),
NULL, NULL, NULL, NULL, NULL, NULL);
Expand Down