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.

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

daemon: Respect systemd inhibitor locks #2862

Merged
merged 1 commit into from
Jun 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions src/daemon/rpmostreed-daemon.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,12 @@ rpmostreed_daemon_is_rebooting (RpmostreedDaemon *self)
return self->rebooting;
}

GDBusConnection *
rpmostreed_daemon_connection(void)
{
g_assert (_daemon_instance->connection);
return _daemon_instance->connection;
kelvinfan001 marked this conversation as resolved.
Show resolved Hide resolved
}

void
rpmostreed_daemon_run_until_idle_exit (RpmostreedDaemon *self)
Expand Down
1 change: 1 addition & 0 deletions src/daemon/rpmostreed-daemon.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ G_BEGIN_DECLS

GType rpmostreed_daemon_get_type (void) G_GNUC_CONST;
RpmostreedDaemon * rpmostreed_daemon_get (void);
GDBusConnection * rpmostreed_daemon_connection (void);
gboolean rpmostreed_get_client_uid (RpmostreedDaemon *self,
const char *client,
uid_t *out_uid);
Expand Down
36 changes: 30 additions & 6 deletions src/daemon/rpmostreed-transaction-types.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,11 @@ rollback_transaction_execute (RpmostreedTransaction *transaction,
}

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

return TRUE;
}
Expand Down Expand Up @@ -1526,7 +1530,11 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
}

if (deploy_has_bool_option (self, "reboot"))
rpmostreed_daemon_reboot (rpmostreed_daemon_get ());
{
if (!check_sd_inhibitor_locks (cancellable, error))
return FALSE;
rpmostreed_daemon_reboot (rpmostreed_daemon_get ());
}
}
else
{
Expand Down Expand Up @@ -1928,7 +1936,11 @@ initramfs_etc_transaction_execute (RpmostreedTransaction *transaction,
return FALSE;

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

return TRUE;
}
Expand Down Expand Up @@ -2066,7 +2078,11 @@ initramfs_state_transaction_execute (RpmostreedTransaction *transaction,
return FALSE;

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

return TRUE;
}
Expand Down Expand Up @@ -2581,7 +2597,7 @@ finalize_deployment_transaction_execute (RpmostreedTransaction *transaction,
FinalizeDeploymentTransaction *self = (FinalizeDeploymentTransaction *) transaction;
OstreeSysroot *sysroot = rpmostreed_transaction_get_sysroot (transaction);
OstreeRepo *repo = ostree_sysroot_repo (sysroot);

auto command_line = (const char*)
vardict_lookup_ptr (self->options, "initiating-command-line", "&s");

Expand Down Expand Up @@ -2610,6 +2626,10 @@ finalize_deployment_transaction_execute (RpmostreedTransaction *transaction,
return glnx_throw (error, "Expected staged base checksum %s, but found %s",
expected_checksum, checksum);

// Check for inhibitor locks before unlocking staged deployment.
if (!check_sd_inhibitor_locks (cancellable, error))
return FALSE;

if (unlink (_OSTREE_SYSROOT_RUNSTATE_STAGED_LOCKED) < 0)
{
if (errno != ENOENT)
Expand Down Expand Up @@ -2802,7 +2822,11 @@ kernel_arg_transaction_execute (RpmostreedTransaction *transaction,
return FALSE;

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

return TRUE;
}
Expand Down
58 changes: 58 additions & 0 deletions src/daemon/rpmostreed-utils.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "rpmostree-util.h"
#include "rpmostreed-utils.h"
#include "rpmostreed-errors.h"
#include "rpmostreed-daemon.h"
#include "libglnx.h"
#include <systemd/sd-journal.h>
#include <stdint.h>
Expand Down Expand Up @@ -609,3 +610,60 @@ rpmostreed_parse_revision (const char *revision,
out:
return ret;
}

/* Throw an error if there exists systemd inhibitor locks in `block` mode only.
* Note: systemd 248 provides a `--check-inhibitors` option, but it also checks
* for inhibitors in `delay` mode, which isn't what we want. */
gboolean
check_sd_inhibitor_locks (GCancellable *cancellable,
GError **error)
{
GDBusConnection *connection = rpmostreed_daemon_connection ();
// https://www.freedesktop.org/software/systemd/man/org.freedesktop.login1.html
g_autoptr(GVariant) inhibitors_array_tuple =
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a link around here to the systemd D-Bus API docs for this?

g_dbus_connection_call_sync (connection, "org.freedesktop.login1", "/org/freedesktop/login1",
kelvinfan001 marked this conversation as resolved.
Show resolved Hide resolved
"org.freedesktop.login1.Manager", "ListInhibitors",
NULL, (const GVariantType*)"(a(ssssuu))",
G_DBUS_CALL_FLAGS_NONE, -1, cancellable, error);
if (!inhibitors_array_tuple)
return glnx_prefix_error (error, "Checking systemd inhibitor locks");
else if (g_variant_n_children (inhibitors_array_tuple) < 1)
return glnx_throw (error, "ListInhibitors returned empty tuple");
g_autoptr(GVariant) inhibitors_array =
g_variant_get_child_value (inhibitors_array_tuple, 0);

char *what = NULL;
char *who = NULL;
char *why = NULL;
char *mode = NULL;
int num_block_inhibitors = 0;
g_autoptr(GString) error_msg = g_string_new(NULL);
GVariantIter viter;
g_variant_iter_init (&viter, inhibitors_array);
while (g_variant_iter_loop (&viter, "(ssssuu)", &what, &who, &why, &mode, NULL, NULL))
{
// Only consider shutdown inhibitors in `block` mode.
if (strstr (what, "shutdown") && g_str_equal (mode, "block"))
{
num_block_inhibitors++;
if (num_block_inhibitors == 1)
Copy link
Member

Choose a reason for hiding this comment

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

This is fine as is, though I think we could simplify the logic by doing const guint num_block_inhibitors = g_variant_n_children (inhibitors_array); and dispatch on that. But fine as is!

{
g_string_append_printf (error_msg,
"Reboot blocked by a systemd inhibitor lock in `block` mode\n"
"Held by: %s\nReason: %s",
who, why);
}
}
}

if (num_block_inhibitors == 0)
return TRUE;
else if (num_block_inhibitors > 1)
{
g_string_append_printf (error_msg,
"\nand %d other blocking inhibitor lock(s)\n"
"Use `systemd-inhibit --list` to see details",
num_block_inhibitors - 1);
}
return glnx_throw (error, "%s", error_msg->str);
}
3 changes: 3 additions & 0 deletions src/daemon/rpmostreed-utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,7 @@ gboolean rpmostreed_parse_revision (const char *revision,
char **out_version,
GError **error);

gboolean check_sd_inhibitor_locks (GCancellable *cancellable,
GError **error);

G_END_DECLS
3 changes: 0 additions & 3 deletions tests/common/libvm.sh
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,6 @@ EOF

vm_setup

# XXX: hack around https://github.com/systemd/systemd/issues/14328
vm_cmd systemctl mask --now systemd-logind

# Some tests expect the ref to be on `vmcheck`. We should drop that
# requirement, but for now let's just mangle the origin
local deployment_root
Expand Down
14 changes: 11 additions & 3 deletions tests/vmcheck/test-misc-2.sh
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ assert_streq "$(vm_get_booted_csum)" "${booted_csum}"
vm_assert_journal_has_content $cursor 'Not finalizing; found /run/ostree/staged-deployment-locked'
echo "ok locked rebase staging"

# This also now tests custom client IDs in the journal.
# This also tests custom client IDs in the journal and interaction with systemd inhibitor locks.
cursor=$(vm_get_journal_cursor)
vm_cmd env RPMOSTREE_CLIENT_ID=testing-agent-id \
rpm-ostree deploy revision="${commit}" \
Expand All @@ -58,7 +58,15 @@ fi
vm_cmd journalctl --after-cursor "'$cursor'" -u rpm-ostreed -o json | jq -r '.AGENT//""' > agent.txt
assert_file_has_content agent.txt testing-agent-id
vm_cmd journalctl --after-cursor "'$cursor'" -u rpm-ostreed -o json | jq -r '.AGENT_SD_UNIT//""' > agent_sd_unit.txt
assert_file_has_content agent_sd_unit.txt sshd.service
assert_file_has_content agent_sd_unit.txt session-1.scope
vm_cmd "systemd-inhibit --what=shutdown --mode=block sh -c 'while ! test -f /run/wakeup; do sleep 0.1; done'" &
if vm_rpmostree finalize-deployment "${commit}" 2>err.txt; then
assert_not_reached "finalized with inhibitor lock in block mode present"
fi
assert_file_has_content err.txt 'Reboot blocked'
vm_cmd test -f /run/ostree/staged-deployment-locked # Make sure that staged deployment is still locked.
vm_cmd touch /run/wakeup
sleep 1 # Wait one second for the process holding the lock to exit.
vm_reboot_cmd rpm-ostree finalize-deployment "${commit}"
assert_streq "$(vm_get_booted_csum)" "${commit}"
vm_assert_journal_has_content $cursor "Finalized deployment; rebooting into ${commit}"
Expand Down Expand Up @@ -364,7 +372,7 @@ vm_cmd rpm-ostree deploy \'\' \
--register-driver=OtherTestDriver --bypass-driver
# Make sure OtherTestDriver's systemd unit will be the same as the commandline's
vm_cmd rpm-ostree status -v > verbose_status.txt
assert_file_has_content verbose_status.txt 'AutomaticUpdatesDriver: OtherTestDriver (sshd.service)'
assert_file_has_content verbose_status.txt 'AutomaticUpdatesDriver: OtherTestDriver (session-1.scope)'
vm_rpmostree upgrade 2>err.txt
assert_not_file_has_content err.txt 'Updates and deployments are driven by OtherTestDriver'
vm_rpmostree cleanup -p
Expand Down