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

daemon: Remove support for session bus #2854

Merged
merged 2 commits into from
May 31, 2021
Merged

daemon: Remove support for session bus #2854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d57532
daemon: Remove support for session bus
cgwalters May 23, 2021
4eb55ea
daemon: Remove even more session bus code
cgwalters May 25, 2021
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
1 change: 0 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
/config.status.lineno
/configure
/configure.lineno
/dbus-run-session
/gtk-doc.make
/insttree/
/libglnx.la
Expand Down
13 changes: 0 additions & 13 deletions Makefile-tests.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,8 @@ uninstalled_test_scripts = \
$(NULL)

uninstalled_test_extra_programs = \
dbus-run-session \
$(NULL)

dbus_run_session_SOURCES = tests/utils/dbus-run-session.c

check-local:
@echo " *** NOTE ***"
@echo " \"make check\" only runs unit tests, which have limited coverage currently."
Expand All @@ -52,13 +49,3 @@ vmoverlay:
# development so that e.g. we automatically overlay.
vmcheck: vmoverlay
@tests/vmcheck.sh

testenv:
@echo "===== ENTERING TESTENV ====="
test_tmpdir=$$(mktemp -d test.XXXXXX) && \
cd $$test_tmpdir && \
env $(BASE_TESTS_ENVIRONMENT) PATH=$(abs_builddir):$$PATH TESTENV=1 \
sh ../tests/utils/setup-session.sh bash && \
cd .. && \
rm -rf $$test_tmpdir
@echo "===== LEAVING TESTENV ====="
2 changes: 1 addition & 1 deletion buildutil/glib-tap.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ installed_test_meta_DATA = $(installed_testcases:=.test)
%.test: %$(EXEEXT) Makefile
$(AM_V_GEN) (echo '[Test]' > $@.tmp; \
echo 'Type=session' >> $@.tmp; \
echo 'Exec=$(installed_testdir)/setup-session.sh $(installed_testdir)/$(notdir $<)' >> $@.tmp; \
echo 'Exec=$(installed_testdir)/$(notdir $<)' >> $@.tmp; \
mv $@.tmp $@)

CLEANFILES += $(installed_test_meta_DATA)
Expand Down
10 changes: 3 additions & 7 deletions src/app/libmain.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,6 @@ rpmostree_option_context_parse (GOptionContext *context,
const char *const* *out_install_pkgs,
const char *const* *out_uninstall_pkgs,
RPMOSTreeSysroot **out_sysroot_proxy,
GBusType *out_bus_type,
GError **error)
{
/* with --version there's no command, don't require a daemon for it */
Expand Down Expand Up @@ -302,11 +301,8 @@ rpmostree_option_context_parse (GOptionContext *context,
/* ignore errors; we print out a warning if we fail to spawn pkttyagent */
(void)rpmostree_polkit_agent_open ();

if (!rpmostree_load_sysroot (opt_sysroot,
opt_force_peer,
cancellable,
if (!rpmostree_load_sysroot (opt_sysroot, cancellable,
out_sysroot_proxy,
out_bus_type,
error))
return FALSE;
}
Expand Down Expand Up @@ -393,7 +389,7 @@ rpmostree_handle_subcommand (int argc, char **argv,
&argc, &argv,
invocation,
cancellable,
NULL, NULL, NULL, NULL, NULL);
NULL, NULL, NULL, NULL);
if (subcommand_name == NULL)
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
Expand Down Expand Up @@ -508,7 +504,7 @@ rpmostree_main_inner (const rust::Slice<const rust::Str> args)
/* This will not return for some options (e.g. --version). */
(void) rpmostree_option_context_parse (context, NULL, &argc, &argv,
NULL, NULL, NULL, NULL, NULL,
NULL, NULL);
NULL);
g_autofree char *help = g_option_context_get_help (context, FALSE, NULL);
g_printerr ("%s", help);
if (command_name == NULL)
Expand Down
1 change: 0 additions & 1 deletion src/app/rpmostree-builtin-cancel.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ rpmostree_builtin_cancel (int argc,
cancellable,
NULL, NULL,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
1 change: 0 additions & 1 deletion src/app/rpmostree-builtin-cleanup.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ rpmostree_builtin_cleanup (int argc,
cancellable,
NULL, NULL,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
2 changes: 1 addition & 1 deletion src/app/rpmostree-builtin-db.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ rpmostree_db_option_context_parse (GOptionContext *context,
argc, argv,
invocation,
cancellable,
NULL, NULL, NULL, NULL,
Copy link
Member Author

Choose a reason for hiding this comment

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

This NULL NULL NULL NULL thing screams bad API design too, so it's good to reduce it!

NULL, NULL, NULL,
error))
return FALSE;

Expand Down
4 changes: 1 addition & 3 deletions src/app/rpmostree-builtin-deploy.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ rpmostree_builtin_deploy (int argc,

context = g_option_context_new ("REVISION");

GBusType bus_type;
if (!rpmostree_option_context_parse (context,
option_entries,
&argc, &argv,
Expand All @@ -87,7 +86,6 @@ rpmostree_builtin_deploy (int argc,
&install_pkgs,
&uninstall_pkgs,
&sysroot_proxy,
&bus_type,
error))
return FALSE;

Expand Down Expand Up @@ -133,7 +131,7 @@ rpmostree_builtin_deploy (int argc,
{
if (!opt_bypass_driver)
{
if (!error_if_driver_registered (bus_type, sysroot_proxy, cancellable, error))
if (!error_if_driver_registered (sysroot_proxy, cancellable, error))
return FALSE;
}

Expand Down
2 changes: 1 addition & 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 @@ -48,7 +48,7 @@ rpmostree_builtin_finalize_deployment (int argc,
glnx_unref_object RPMOSTreeSysroot *sysroot_proxy = NULL;
if (!rpmostree_option_context_parse (context, option_entries, &argc, &argv,
invocation, cancellable, NULL, NULL,
&sysroot_proxy, NULL, error))
&sysroot_proxy, error))
return FALSE;

const char *checksum = NULL;
Expand Down
1 change: 0 additions & 1 deletion src/app/rpmostree-builtin-initramfs-etc.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ rpmostree_ex_builtin_initramfs_etc (int argc,
cancellable,
NULL, NULL,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
1 change: 0 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 @@ -63,7 +63,6 @@ rpmostree_builtin_initramfs (int argc,
cancellable,
NULL, NULL,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
1 change: 0 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 @@ -181,7 +181,6 @@ rpmostree_builtin_kargs (int argc,
cancellable,
NULL, NULL,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
4 changes: 1 addition & 3 deletions src/app/rpmostree-builtin-rebase.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ rpmostree_builtin_rebase (int argc,
const char *const *install_pkgs = NULL;
const char *const *uninstall_pkgs = NULL;

GBusType bus_type;
if (!rpmostree_option_context_parse (context,
option_entries,
&argc, &argv,
Expand All @@ -92,7 +91,6 @@ rpmostree_builtin_rebase (int argc,
&install_pkgs,
&uninstall_pkgs,
&sysroot_proxy,
&bus_type,
error))
return FALSE;

Expand All @@ -103,7 +101,7 @@ rpmostree_builtin_rebase (int argc,
}

if (!opt_bypass_driver)
if (!error_if_driver_registered (bus_type, sysroot_proxy, cancellable, error))
if (!error_if_driver_registered (sysroot_proxy, cancellable, error))
return FALSE;

if (!rpmostree_load_os_proxy (sysroot_proxy, opt_osname,
Expand Down
1 change: 0 additions & 1 deletion src/app/rpmostree-builtin-refresh-md.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ rpmostree_builtin_refresh_md (int argc,
cancellable,
NULL, NULL,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
1 change: 0 additions & 1 deletion src/app/rpmostree-builtin-reload.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ rpmostree_builtin_reload (int argc,
cancellable,
NULL, NULL,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
1 change: 0 additions & 1 deletion src/app/rpmostree-builtin-reset.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ rpmostree_builtin_reset (int argc,
&install_pkgs,
&uninstall_pkgs,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
1 change: 0 additions & 1 deletion src/app/rpmostree-builtin-rollback.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ rpmostree_builtin_rollback (int argc,
cancellable,
NULL, NULL,
&sysroot_proxy,
NULL,
error))
return FALSE;

Expand Down
88 changes: 10 additions & 78 deletions src/app/rpmostree-builtin-start-daemon.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,10 @@ typedef enum {
static AppState appstate = APPSTATE_STARTING;
static gboolean opt_debug = FALSE;
static char *opt_sysroot = NULL;
static gint service_dbus_fd = -1;
static GOptionEntry opt_entries[] =
{
{ "debug", 'd', 0, G_OPTION_ARG_NONE, &opt_debug, "Print debug information on stderr", NULL },
{ "sysroot", 0, 0, G_OPTION_ARG_STRING, &opt_sysroot, "Use system root SYSROOT (default: /)", "SYSROOT" },
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave this one in? It's related to the peer D-Bus stuff, but still mostly independent, right?

There's a bunch of things related to this I'd like to explore. E.g. running rpm-ostree directly after coreos-installer install, running rpm-ostree from the initramfs, etc... I think also Anaconda may need this for the initramfs-etc work.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, this actually works today from a live boot:

[root@cosa-devsh ~]# coreos-installer install /dev/vda
[root@cosa-devsh ~]# mount /dev/disk/by-label/root /mnt/root
[root@cosa-devsh ~]# mount /dev/disk/by-label/boot /mnt/root/boot
[root@cosa-devsh ~]# rpm-ostree start-daemon --sysroot /mnt/root &
[root@cosa-devsh ~]# rpm-ostree status
State: idle
Deployments:
* ostree://fedora:fedora/x86_64/coreos/testing-devel
                   Version: 34.20210520.dev.0 (2021-05-20T21:45:14Z)
                    Commit: a53b7d233ef46c2e1ceb469c0ec8e441a5d67c03c6641630632e89c52f45a516
              GPGSignature: (unsigned)

We'd want to make that easier of course. Maybe just have coreos-installer automatically add a drop-in for rpm-ostreed.service or something. Not sure.

Actually doing a write operation like rpm-ostree ex initramfs-etc on it fails, but at least the base is there.

Copy link
Member Author

Choose a reason for hiding this comment

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

E.g. running rpm-ostree directly after coreos-installer install,

But the more we do that the more it weakens the Ignition story/focus.

running rpm-ostree from the initramfs, etc...

I am very skeptical of this one because any nontrivial use will need to configure rpm-ostree (think e.g. 3rd party repos or local mirroring) and then that gets into configuring the initramfs and...

I think most of the desire for rpm-ostree-in-initramfs has been obviated by the install -A model.

Also we don't have tests for --sysroot now. It might not be really hard with some scaffolding to to basically double our tests and do it though. Or I guess have some basic tests.

Copy link
Member

Choose a reason for hiding this comment

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

E.g. running rpm-ostree directly after coreos-installer install,

But the more we do that the more it weakens the Ignition story/focus.

Oh yes definitely. But I'm hoping to converge towards rpm-ostree ex initramfs-etc for solving bootstrapping problems which hard require pre-configuration. Notably networking and non-default multipath configuration.

running rpm-ostree from the initramfs, etc...

I am very skeptical of this one because any nontrivial use will need to configure rpm-ostree (think e.g. 3rd party repos or local mirroring) and then that gets into configuring the initramfs and...

I think most of the desire for rpm-ostree-in-initramfs has been obviated by the install -A model.

I haven't thought this through yet (and I've actually been meaning to experiment with this during a hack day), but as part of coreos/fedora-coreos-tracker#681, basically I was thinking we would match Ignition semantics and apply layered requests before we switchroot. I think this would be cleaner just doing it in a bwrap container though to avoid actually having to drag rpm-ostree into the initrd. So maybe we don't actually need --sysroot in that case. But yeah, configuration would definitely come from the real root.

The advantage over live-apply in the real root is that it removes any ambiguity wrt ordering and e.g. configuring the layered things via Ignition itself. It also allows us to enforce strict failure semantics like Ignition.

Also we don't have tests for --sysroot now. It might not be really hard with some scaffolding to to basically double our tests and do it though. Or I guess have some basic tests.

Yeah, I think some basic tests at least wouldn't be too hard. I can volunteer for that! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK for now, I kept sysroot since it is orthogonal to removing the session bus bits.

That said, on this topic...my thought here is that we can avoid using DBus for these cases. Basically rpm-ostree ex initramfs-etc --sysroot=/mnt/sysroot ... would directly perform the operation in the client code.

It's much like where systemd ended up where e.g. systemctl can talk to pid1 over dbus, but also happily works offline directly.

Copy link
Member

Choose a reason for hiding this comment

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

That said, on this topic...my thought here is that we can avoid using DBus for these cases. Basically rpm-ostree ex initramfs-etc --sysroot=/mnt/sysroot ... would directly perform the operation in the client code.

I think that makes sense though likely will require a lot of refactoring to pull off. Definitely worth investigating though!

{ "dbus-peer", 0, 0, G_OPTION_ARG_INT, &service_dbus_fd, "Use a peer to peer dbus connection on this fd", "FD" },
{ NULL }
};

Expand All @@ -67,13 +65,6 @@ state_transition (AppState state)
g_main_context_wakeup (NULL);
}

static void
state_transition_fatal_err (GError *error)
{
sd_journal_print (LOG_ERR, "%s", error->message);
state_transition (APPSTATE_FLUSHING);
}

static gboolean
start_daemon (GDBusConnection *connection,
GError **error)
Expand All @@ -84,7 +75,11 @@ start_daemon (GDBusConnection *connection,
"connection", connection,
"sysroot-path", opt_sysroot ?: "/",
NULL);
return rpm_ostree_daemon != NULL;
if (rpm_ostree_daemon == NULL)
return FALSE;
(void) g_bus_own_name_on_connection (connection, DBUS_NAME, G_BUS_NAME_OWNER_FLAGS_NONE,
NULL, NULL, NULL, NULL);
return TRUE;
}

static void
Expand All @@ -95,26 +90,6 @@ on_bus_name_released (GDBusConnection *connection,
state_transition (APPSTATE_EXITING);
}

static void
on_peer_acquired (GObject *source,
GAsyncResult *result,
gpointer user_data)
{
g_autoptr(GError) error = NULL;
g_autoptr(GDBusConnection) connection = g_dbus_connection_new_finish (result, &error);
if (!connection)
{
state_transition_fatal_err (error);
return;
}

if (!start_daemon (connection, &error))
{
state_transition_fatal_err (error);
return;
}
}

static gboolean
on_sigint (gpointer user_data)
{
Expand Down Expand Up @@ -250,29 +225,6 @@ on_log_handler (const gchar *log_domain,
sd_journal_print (priority, "%s", message);
}


static gboolean
connect_to_peer (int fd, GError **error)
{
g_autoptr(GSocketConnection) stream = NULL;
g_autoptr(GSocket) socket = NULL;
g_autofree gchar *guid = NULL;

socket = g_socket_new_from_fd (fd, error);
if (!socket)
return FALSE;

stream = g_socket_connection_factory_create_connection (socket);
g_assert_nonnull (stream);

guid = g_dbus_generate_guid ();
g_dbus_connection_new (G_IO_STREAM (stream), guid,
(GDBusConnectionFlags)(G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_SERVER |
G_DBUS_CONNECTION_FLAGS_DELAY_MESSAGE_PROCESSING),
NULL, NULL, on_peer_acquired, NULL);
return TRUE;
}

gboolean
rpmostree_builtin_start_daemon (int argc,
char **argv,
Expand Down Expand Up @@ -305,31 +257,11 @@ rpmostree_builtin_start_daemon (int argc,
g_unix_signal_add (SIGINT, on_sigint, NULL);
g_unix_signal_add (SIGTERM, on_sigint, NULL);

g_autoptr(GDBusConnection) bus = NULL;
if (service_dbus_fd == -1)
{
GBusType bus_type;

/* To facilitate testing, use whichever message bus activated
* this process. If the process was started directly, assume
* the system bus. */
if (g_getenv ("DBUS_STARTER_BUS_TYPE") != NULL)
bus_type = G_BUS_TYPE_STARTER;
else if (g_getenv ("RPMOSTREE_USE_SESSION_BUS") != NULL)
bus_type = G_BUS_TYPE_SESSION;
else
bus_type = G_BUS_TYPE_SYSTEM;

/* Get an explicit ref to the bus so we can use it later */
bus = g_bus_get_sync (bus_type, NULL, error);
if (!bus)
return FALSE;
if (!start_daemon (bus, error))
return FALSE;
(void) g_bus_own_name_on_connection (bus, DBUS_NAME, G_BUS_NAME_OWNER_FLAGS_NONE,
NULL, NULL, NULL, NULL);
}
else if (!connect_to_peer (service_dbus_fd, error))
/* Get an explicit ref to the bus so we can use it later */
g_autoptr(GDBusConnection) bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, error);
if (!bus)
return FALSE;
if (!start_daemon (bus, error))
return FALSE;

state_transition (APPSTATE_RUNNING);
Expand Down
Loading