removing client-socket feature

Author: RishabhSaini

Cargo.lock

@@ -94,7 +94,6 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] }
 tokio = { version = "1.28.2", features = ["time", "process", "rt", "net"] }
 xmlrpc = "0.15.1"
 termcolor = "1.1.3"
-tokio-unix-ipc = { version = "0.2.0", features = ["serde"] }
 
 [build-dependencies]
 anyhow = "1.0"
@@ -126,8 +125,6 @@ lto = "thin"
 # Note: If you add a feature here, you also probably want to update utils.rs:get_features()
 fedora-integration = []
 rhsm = ["libdnf-sys/rhsm"]
-# Enable hard requirement on `rpm-ostreed.socket`; requires https://bugzilla.redhat.com/show_bug.cgi?id=2110012
-client-socket = []
 bin-unit-tests = []
 # ASAN+UBSAN
 sanitizers = []

Cargo.toml

@@ -66,12 +66,9 @@ systemdunit_service_files = $(addprefix $(srcdir)/src/daemon/,$(systemdunit_serv
 systemdunit_other_files = \
 	$(srcdir)/src/daemon/rpm-ostreed-automatic.timer \
 	$(srcdir)/src/daemon/rpm-ostree-countme.timer \
+       $(srcdir)/src/daemon/rpm-ostreed.socket \
 	$(NULL)
 
-if CLIENT_SOCKET
-systemdunit_other_files += $(srcdir)/src/daemon/rpm-ostreed.socket
-endif
-
 systemdunit_DATA = \
 	$(systemdunit_service_files) \
 	$(systemdunit_other_files) \

Makefile-daemon.am

@@ -70,13 +70,6 @@ AC_ARG_ENABLE(featuresrs,
                              [Rust features, see Cargo.toml for more information]),,
               [enable_featuresrs=])
 
-AC_ARG_ENABLE(client-socket,
-              AS_HELP_STRING([--enable-client-socket],
-                             [(default: no)]),,
-              [enable_client_socket=no])
-AS_IF([test x$enable_client_socket = xyes], [enable_featuresrs="$enable_featuresrs client-socket"])
-AM_CONDITIONAL(CLIENT_SOCKET, [echo $enable_featuresrs | grep -q 'client-socket'])
-
 AC_SUBST([RUST_FEATURES], $enable_featuresrs)
 
 # Initialize libtool

configure.ac

@@ -34,13 +34,6 @@ BuildRequires: rust
 %bcond_with rhsm
 %endif
 
-# https://bugzilla.redhat.com/show_bug.cgi?id=2110012
-%if 0%{?fedora} >= 37
-%bcond_without client_socket
-%else
-%bcond_with client_socket
-%endif
-
 # RHEL (8,9) doesn't ship zchunk today.  Keep this in sync
 # with libdnf: https://gitlab.com/redhat/centos-stream/rpms/libdnf/-/blob/762f631e36d1e42c63a794882269d26c156b68c1/libdnf.spec#L45
 %if 0%{?rhel}
@@ -182,7 +175,7 @@ env NOCONFIGURE=1 ./autogen.sh
 export RUSTFLAGS="%{build_rustflags}"
 %endif
 %configure --disable-silent-rules --enable-gtk-doc %{?rpmdb_default} %{?with_sanitizers:--enable-sanitizers}  %{?with_bin_unit_tests:--enable-bin-unit-tests} \
-  %{?with_rhsm:--enable-featuresrs=rhsm} %{?with_client_socket:--enable-client-socket} 
+  %{?with_rhsm:--enable-featuresrs=rhsm} 
 
 %make_build
 

packaging/rpm-ostree.spec.in

@@ -159,16 +159,20 @@ pub(crate) fn client_handle_fd_argument(
 /// Connect to the client socket and ensure the daemon is initialized;
 /// this avoids DBus and ensures that we get any early startup errors
 /// returned cleanly.
-#[cfg(feature = "client-socket")]
+#[context("Starting daemon via socket")]
 fn start_daemon_via_socket() -> Result<()> {
     use cap_std::io_lifetimes::IntoSocketlike;
 
-    let conn = tokio::net::UnixStream::connect("/run/rpm-ostree/client.sock")?;
+    futures::executor::block_on(async {
+        let c = tokio::net::UnixStream::connect("/run/rpm-ostree/client.sock").await?;
+        Ok::<_, anyhow::Error>(c)
+    })
+    .context("Connecting")?;
 
     let address = sockaddr()?;
     let socket = rustix::net::socket(
         rustix::net::AddressFamily::UNIX,
-        rustix::net::SocketType::SEQPACKET,
+        rustix::net::SocketType::STREAM,
         rustix::net::Protocol::from_raw(0),
     )?;
     let addr = crate::client::sockaddr()?;
@@ -181,7 +185,8 @@ fn start_daemon_via_socket() -> Result<()> {
         crate::daemon::SocketMessage::ClientHello {
             selfid: crate::core::self_id()?,
         },
-    )?;
+    )
+    .context("Writing ClientHello")?;
     let resp = crate::daemon::recv_message(&socket)?;
     match resp {
         crate::daemon::SocketMessage::ServerOk => Ok(()),
@@ -197,62 +202,13 @@ pub(crate) fn sockaddr() -> Result<rustix::net::SocketAddrUnix> {
     rustix::net::SocketAddrUnix::new("/run/rpm-ostree/client.sock").map_err(anyhow::Error::msg)
 }
 
-/// Explicitly ensure the daemon is started via systemd, if possible.
-///
-/// This works around bugs from DBus activation, see
-/// https://github.com/coreos/rpm-ostree/pull/2932
-///
-/// Basically we load too much data before claiming the bus name,
-/// and dbus doesn't give us a useful error.  Instead, let's talk
-/// to systemd directly and use its client tools to scrape errors.
-///
-/// What we really should do probably is use native socket activation.
-#[cfg(not(feature = "client-socket"))]
-fn start_daemon_via_systemctl() -> Result<()> {
-    use std::process::Command;
-
-    let service = "rpm-ostreed.service";
-    // Assume non-root can't use systemd right now.
-    if rustix::process::getuid().as_raw() != 0 {
-        return Ok(());
-    }
-
-    // Unfortunately, RHEL8 systemd will count "systemctl start"
-    // invocations against the restart limit, so query the status
-    // first.
-    let activeres = Command::new("systemctl")
-        .args(&["is-active", "rpm-ostreed"])
-        .output()?;
-    // Explicitly don't check the error return value, we don't want to
-    // hard fail on it.
-    if String::from_utf8_lossy(&activeres.stdout).starts_with("active") {
-        // It's active, we're done.  Note that while this is a race
-        // condition, that's fine because it will be handled by DBus
-        // activation.
-        return Ok(());
-    }
-    let res = Command::new("systemctl")
-        .args(&["--no-ask-password", "start", service])
-        .status()?;
-    if !res.success() {
-        let _ = Command::new("systemctl")
-            .args(&["--no-pager", "status", service])
-            .status();
-        return Err(anyhow!("{}", res).into());
-    }
-    Ok(())
-}
-
 pub(crate) fn client_start_daemon() -> CxxResult<()> {
     // systemctl and socket paths only work for root right now; in the future
     // the socket may be opened up.
     if rustix::process::getuid().as_raw() != 0 {
         return Ok(());
     }
-    #[cfg(feature = "client-socket")]
     return start_daemon_via_socket().map_err(Into::into);
-    #[cfg(not(feature = "client-socket"))]
-    return start_daemon_via_systemctl().map_err(Into::into);
 }
 
 /// Convert the GVariant parameters from the DownloadProgress DBus API to a human-readable English string.

rust/src/client.rs

@@ -239,72 +239,44 @@ pub(crate) fn daemon_main(debug: bool) -> Result<()> {
     if !systemd::daemon::booted()? {
         return Err(anyhow!("not running as a systemd service"));
     }
-
     let init_res: Result<()> = crate::ffi::daemon_init_inner(debug).map_err(|e| e.into());
     tracing::debug!("Initialization result: {init_res:?}");
 
     let mut fds = systemd::daemon::listen_fds(false)?.iter();
-    let (listener, init_res) = match fds.next() {
-        None => {
-            // If started directly via `systemctl start` or DBus activation, we
-            // directly propagate the error back to our exit code without even bothering
-            // with a socket.
-            init_res.context("Initializing via dbus")?;
-            tracing::debug!("Initializing directly (not socket activated)");
-            let listener = cfg!(feature = "client-socket")
-                .then(|| {
-                    let socket = rustix::net::socket(
-                        rustix::net::AddressFamily::UNIX,
-                        rustix::net::SocketType::SEQPACKET,
-                        rustix::net::Protocol::from_raw(0),
-                    )?;
-                    let addr = crate::client::sockaddr()?;
-                    rustix::net::bind_unix(&socket, &addr)?;
-                    Ok::<_, anyhow::Error>(socket.into_socketlike())
-                })
-                .transpose()
-                .context("Binding to socket")?;
-            (listener, Ok(()))
+    let fd = fds.next().ok_or_else(|| anyhow!("Missing socket"))?;
+    if fds.next().is_some() {
+        return Err(anyhow!("Expected exactly 1 fd from systemd activation"));
+    }
+    tracing::debug!("Initializing from socket activation; fd={fd}");
+    let listener = unsafe { OwnedFd::from_raw_fd(fd) }.into_socketlike();
+    // In the socket case, we will process the initialization error later.
+
+    // On success, we spawn a helper task that just responds with
+    // sucess to clients that connect via the socket.  In the future,
+    // perhaps we'll expose an API here.
+    tracing::debug!("Spawning acknowledgement task");
+    match init_res {
+        Ok(()) => {
+            dbg!(&listener);
+            std::thread::spawn(move || run_acknowledgement_worker(listener));
         }
-        Some(fd) => {
-            if fds.next().is_some() {
-                return Err(anyhow!("Expected exactly 1 fd from systemd activation"));
+        Err(e) => {
+            tracing::debug!("Sending error to client: {}", e);
+            let err_copy = anyhow::format_err!("{e}");
+            let r = std::thread::spawn(move || {
+                if let Err(suberr) = process_one_client(listener, err_copy) {
+                    tracing::warn!("Failed to respond to client: {suberr}")
+                }
+            });
+            // Block until we've written the reply to the client;
+            if let Err(e) = r.join() {
+                tracing::warn!("Failed to join response thread: {e:?}");
             }
-            tracing::debug!("Initializing from socket activation; fd={fd}");
-            let listener = unsafe { OwnedFd::from_raw_fd(fd) }.into_socketlike();
-            // In the socket case, we will process the initialization error later.
-            (Some(listener), init_res)
+            // And finally propagate out the error
+            return Err(e);
         }
     };
 
-    if let Some(listener) = listener {
-        // On success, we spawn a helper task that just responds with
-        // sucess to clients that connect via the socket.  In the future,
-        // perhaps we'll expose an API here.
-        tracing::debug!("Spawning acknowledgement task");
-        match init_res {
-            Ok(()) => {
-                std::thread::spawn(move || run_acknowledgement_worker(listener));
-            }
-            Err(e) => {
-                let err_copy = anyhow::format_err!("{e}");
-                let r = std::thread::spawn(move || {
-                    if let Err(suberr) = process_one_client(listener, err_copy) {
-                        tracing::warn!("Failed to respond to client: {suberr}")
-                    }
-                });
-                // Block until we've written the reply to the client;
-                if let Err(e) = r.join() {
-                    tracing::warn!("Failed to join response thread: {e:?}");
-                }
-                // And finally propagate out the error
-                return Err(e);
-            }
-        };
-    } else {
-        init_res?;
-    }
-
     tracing::debug!("Entering daemon mainloop");
     // And now, enter the main loop.
     Ok(crate::ffi::daemon_main_inner()?)

rust/src/daemon.rs

@@ -21,6 +21,7 @@ MountFlags=slave
 # but as a subprocess.
 ProtectHome=true
 NotifyAccess=main
+Sockets=rpm-ostreed.socket
 # Significantly bump this timeout from the default because
 # we do a lot of stuff on daemon startup.
 TimeoutStartSec=5m

src/daemon/rpm-ostreed.service

@@ -109,6 +109,10 @@ rpmostree_busctl_call_os Search as 1 should-not-exist-p-equals-np > out.txt
 assert_file_has_content_literal out.txt 'aa{sv} 0'
 echo "ok dbus Search"
 
+systemctl stop rpm-ostreed
+rpmostree_busctl_call_os ListRepos
+echo "restarting the daemon works"
+
 rpm-ostree search testdaemon > out.txt
 assert_file_has_content_literal out.txt '===== Name Matched ====='
 assert_file_has_content_literal out.txt 'testdaemon : awesome-daemon-for-testing'