apply-live: Avoid clobbering changes in /etc

Gather the current diff of `/etc`, and filter out changes in
the tree which would overwrite it.

There is an OSTree API for diffs but it's a bit awkward, missing
some APIs in the Rust bindings and also `GFile` based unfortunately.
Doing this in Rust is nicer.  The dirdiff code obviously needs
a lot more testing, but I think it's right.

rust/src/dirdiff.rs

@@ -0,0 +1,275 @@
+/*
+ * Copyright (C) Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ */
+
+use anyhow::{Context, Result};
+use openat_ext::OpenatDirExt;
+use serde_derive::{Deserialize, Serialize};
+use std::borrow::Cow;
+use std::collections::BTreeSet;
+use std::convert::TryFrom;
+use std::fmt;
+
+use std::io::Read;
+
+pub(crate) type FileSet = BTreeSet<String>;
+
+/// Diff between two directories.
+#[derive(Debug, Default, Serialize, Deserialize)]
+pub(crate) struct Diff {
+    /// Files that are new in an existing directory
+    pub(crate) added_files: FileSet,
+    /// New directories
+    pub(crate) added_dirs: FileSet,
+    /// Files removed
+    pub(crate) removed_files: FileSet,
+    /// Directories removed (recursively)
+    pub(crate) removed_dirs: FileSet,
+    /// Files that changed (in any way, metadata or content)
+    pub(crate) changed_files: FileSet,
+    /// Directories that changed mode/permissions
+    pub(crate) changed_dirs: FileSet,
+}
+
+impl Diff {
+    #[allow(dead_code)]
+    pub(crate) fn count(&self) -> usize {
+        self.added_files.len()
+            + self.added_dirs.len()
+            + self.removed_files.len()
+            + self.removed_dirs.len()
+            + self.changed_files.len()
+            + self.changed_dirs.len()
+    }
+
+    pub(crate) fn contains(&self, s: &str) -> bool {
+        self.added_files.contains(s)
+            || self.added_dirs.contains(s)
+            || self.removed_files.contains(s)
+            || self.removed_dirs.contains(s)
+            || self.changed_files.contains(s)
+            || self.changed_dirs.contains(s)
+    }
+}
+
+impl fmt::Display for Diff {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "files(added:{} removed:{} changed:{}) dirs(added:{} removed:{} changed:{})",
+            self.added_files.len(),
+            self.removed_files.len(),
+            self.changed_files.len(),
+            self.added_dirs.len(),
+            self.removed_dirs.len(),
+            self.changed_dirs.len()
+        )
+    }
+}
+
+fn file_content_changed(
+    src: &openat::Dir,
+    dest: &openat::Dir,
+    path: &str,
+    expected_len: u64,
+) -> Result<bool> {
+    let mut remaining = expected_len;
+    let mut srcf = std::io::BufReader::new(src.open_file(path)?);
+    let mut destf = std::io::BufReader::new(dest.open_file(path)?);
+    let mut srcbuf = [0; 4096];
+    let mut destbuf = [0; 4096];
+    let bufsize = srcbuf.len();
+    while remaining > 0 {
+        let readlen = std::cmp::min(usize::try_from(remaining).unwrap_or(bufsize), bufsize);
+        let mut srcbuf = &mut srcbuf[0..readlen];
+        let mut destbuf = &mut destbuf[0..readlen];
+        srcf.read_exact(&mut srcbuf)?;
+        destf.read_exact(&mut destbuf)?;
+        if srcbuf != destbuf {
+            return Ok(true);
+        }
+        remaining = remaining
+            .checked_sub(readlen as u64)
+            .expect("file_content_changed: read underflow");
+    }
+    Ok(false)
+}
+
+fn is_changed(
+    src: &openat::Dir,
+    dest: &openat::Dir,
+    path: &str,
+    srcmeta: &openat::Metadata,
+    destmeta: &openat::Metadata,
+) -> Result<bool> {
+    if srcmeta.permissions() != destmeta.permissions() {
+        return Ok(true);
+    }
+    Ok(match srcmeta.simple_type() {
+        openat::SimpleType::File => {
+            if srcmeta.len() != destmeta.len() {
+                true
+            } else {
+                file_content_changed(src, dest, path, srcmeta.len())?
+            }
+        }
+        openat::SimpleType::Symlink => src.read_link(path)? != dest.read_link(path)?,
+        openat::SimpleType::Other => false,
+        openat::SimpleType::Dir => false,
+    })
+}
+
+fn canonicalize_name<'a>(prefix: Option<&str>, name: &'a str) -> Cow<'a, str> {
+    if let Some(prefix) = prefix {
+        Cow::Owned(format!("{}/{}", prefix, name))
+    } else {
+        Cow::Borrowed(name)
+    }
+}
+
+fn diff_recurse(
+    prefix: Option<&str>,
+    src: &openat::Dir,
+    dest: &openat::Dir,
+    diff: &mut Diff,
+) -> Result<()> {
+    let list_prefix = prefix.unwrap_or(".");
+    for entry in src.list_dir(list_prefix)? {
+        let entry = entry?;
+        let name = if let Some(name) = entry.file_name().to_str() {
+            name
+        } else {
+            // For now ignore invalid UTF-8 names
+            continue;
+        };
+        let path = canonicalize_name(prefix, name);
+        let pathp = &*path;
+        let srctype = src.get_file_type(&entry)?;
+        let is_dir = srctype == openat::SimpleType::Dir;
+
+        match dest.metadata_optional(pathp)? {
+            Some(destmeta) => {
+                let desttype = destmeta.simple_type();
+                let changed = if srctype != desttype {
+                    true
+                } else {
+                    let srcmeta = src.metadata(pathp)?;
+                    is_changed(src, dest, pathp, &srcmeta, &destmeta)?
+                };
+                if is_dir {
+                    diff_recurse(Some(pathp), src, dest, diff)?;
+                    if changed {
+                        diff.changed_dirs.insert(path.into_owned());
+                    }
+                } else if changed {
+                    diff.changed_files.insert(path.into_owned());
+                }
+            }
+            None => {
+                if is_dir {
+                    diff.removed_dirs.insert(path.into_owned());
+                } else {
+                    diff.removed_files.insert(path.into_owned());
+                }
+            }
+        }
+    }
+
+    // Iterate over the target (to) directory, and find any
+    // files/directories which were not present in the source.
+    for entry in dest.list_dir(list_prefix)? {
+        let entry = entry?;
+        let name = if let Some(name) = entry.file_name().to_str() {
+            name
+        } else {
+            // For now ignore invalid UTF-8 names
+            continue;
+        };
+        let path = canonicalize_name(prefix, name);
+        if src.metadata_optional(&*path)?.is_some() {
+            continue;
+        }
+        let desttype = dest.get_file_type(&entry)?;
+        if desttype == openat::SimpleType::Dir {
+            diff.added_dirs.insert(path.into_owned());
+        } else {
+            diff.added_files.insert(path.into_owned());
+        }
+    }
+    Ok(())
+}
+
+/// Given two directories, compute the diff between them.
+pub(crate) fn diff(src: &openat::Dir, dest: &openat::Dir) -> Result<Diff> {
+    let mut diff = Diff {
+        ..Default::default()
+    };
+    diff_recurse(None, src, dest, &mut diff).context("Failed to compute fsdiff")?;
+    Ok(diff)
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use std::fs::Permissions;
+    use std::os::unix::fs::PermissionsExt;
+
+    #[test]
+    fn test_diff() -> Result<()> {
+        let td = tempfile::tempdir()?;
+        let td = openat::Dir::open(td.path())?;
+        td.create_dir("a", 0o755)?;
+        td.create_dir("b", 0o755)?;
+        let a = td.sub_dir("a")?;
+        let b = td.sub_dir("b")?;
+        for d in [&a, &b].iter() {
+            d.ensure_dir_all("sub1/sub2", 0o755)?;
+            d.write_file_contents("sub1/subfile", 0o644, "subfile")?;
+            d.ensure_dir_all("sub1/sub3", 0o755)?;
+            d.ensure_dir_all("sub2/sub4", 0o755)?;
+            d.write_file_contents("sub2/subfile2", 0o644, "subfile2")?;
+            d.write_file_contents("somefile", 0o644, "somefile")?;
+            d.symlink("link2root", "/")?;
+            d.symlink("brokenlink", "enoent")?;
+            d.symlink("somelink", "otherlink")?;
+            d.symlink("otherlink", "sub1/sub2")?;
+        }
+        // Initial setup with identical dirs
+        let d = diff(&a, &b)?;
+        assert_eq!(d.count(), 0);
+
+        // Remove a file
+        b.remove_file("somefile")?;
+        let d = diff(&a, &b)?;
+        assert_eq!(d.count(), 1);
+        assert_eq!(d.removed_files.len(), 1);
+
+        // Change a file
+        b.write_file_contents("somefile", 0o644, "somefile2")?;
+        let d = diff(&a, &b)?;
+        assert_eq!(d.count(), 1);
+        assert_eq!(d.changed_files.len(), 1);
+        assert!(d.changed_files.contains("somefile"));
+
+        // Many changes
+        a.write_file_contents("sub1/sub2/subfile1", 0o644, "subfile1")?;
+        a.write_file_contents("sub1/sub2/subfile2", 0o644, "subfile2")?;
+        b.write_file_contents("sub1/someotherfile", 0o644, "somefile3")?;
+        b.remove_file("link2root")?;
+        b.symlink("link2root", "/notroot")?;
+        b.remove_all("sub2")?;
+        a.open_file("sub1/subfile")?
+            .set_permissions(Permissions::from_mode(0o600))?;
+        let d = diff(&a, &b)?;
+        assert_eq!(d.count(), 7);
+        assert_eq!(d.changed_files.len(), 3);
+        assert_eq!(d.removed_files.len(), 2);
+        assert_eq!(d.added_files.len(), 1);
+        assert_eq!(d.removed_dirs.len(), 1);
+        assert!(d.removed_files.contains("sub1/sub2/subfile1"));
+        assert!(d.added_files.contains("sub1/someotherfile"));
+        Ok(())
+    }
+}

rust/src/lib.rs

@@ -146,6 +146,7 @@ mod composepost;
 pub(crate) use composepost::*;
 mod core;
 use crate::core::*;
+mod dirdiff;
 #[cfg(feature = "fedora-integration")]
 mod fedora_integration;
 mod history;

rust/src/live.rs

@@ -184,6 +184,7 @@ fn apply_diff(
 fn update_etc(
     repo: &ostree::Repo,
     diff: &crate::ostree_diff::FileTreeDiff,
+    config_diff: &crate::dirdiff::Diff,
     sepolicy: &ostree::SePolicy,
     commit: &str,
     destdir: &openat::Dir,
@@ -191,12 +192,14 @@ fn update_etc(
     let expected_subpath = "/usr";
     // We stripped both /usr and /etc, we need to readd them both
     // for the checkout.
-    fn filtermap_paths(s: &String) -> Option<(Option<PathBuf>, &Path)> {
-        s.strip_prefix("/etc").map(|p| {
-            let p = Path::new(p).strip_prefix("/").expect("prefix");
-            (Some(Path::new("/usr/etc").join(p)), p)
-        })
-    }
+    let filtermap_paths = |s: &String| -> Option<(Option<PathBuf>, PathBuf)> {
+        s.strip_prefix("/etc/")
+            .filter(|p| !config_diff.contains(p))
+            .map(|p| {
+                let p = Path::new(p);
+                (Some(Path::new("/usr/etc").join(p)), p.into())
+            })
+    };
     // For some reason in Rust the `parent()` of `foo` is just the empty string `""`; we
     // need it to be the self-link `.` path.
     fn canonicalized_parent(p: &Path) -> &Path {
@@ -234,7 +237,7 @@ fn update_etc(
         repo.checkout_at(
             Some(&opts),
             destdir.as_raw_fd(),
-            target,
+            &target,
             commit,
             cancellable,
         )
@@ -245,7 +248,7 @@ fn update_etc(
         repo.checkout_at(
             Some(&opts),
             destdir.as_raw_fd(),
-            canonicalized_parent(target),
+            canonicalized_parent(&target),
             commit,
             cancellable,
         )
@@ -257,7 +260,7 @@ fn update_etc(
         repo.checkout_at(
             Some(&opts),
             destdir.as_raw_fd(),
-            canonicalized_parent(target),
+            canonicalized_parent(&target),
             commit,
             cancellable,
         )
@@ -277,7 +280,7 @@ fn update_etc(
         .filter_map(filtermap_paths)
         .try_for_each(|(_, target)| -> Result<()> {
             destdir
-                .remove_all(target)
+                .remove_all(&target)
                 .with_context(|| format!("Failed to remove {:?}", target))?;
             Ok(())
         })?;
@@ -426,6 +429,7 @@ pub(crate) fn transaction_apply_live(
         .unwrap_or(booted_commit);
     let diff = crate::ostree_diff::diff(repo, source_commit, &target_commit, Some("/usr"))
         .context("Failed computing diff")?;
+    println!("Computed /usr diff: {}", &diff);
 
     let mut state = state.unwrap_or_default();
 
@@ -436,6 +440,15 @@ pub(crate) fn transaction_apply_live(
     state.inprogress = target_commit.to_string();
     write_live_state(&booted, &state)?;
 
+    // Gather the current diff of /etc - we need to avoid changing
+    // any files which are locally modified.
+    let config_diff = {
+        let usretc = &rootfs_dfd.sub_dir("usr/etc")?;
+        let etc = &rootfs_dfd.sub_dir("etc")?;
+        crate::dirdiff::diff(usretc, etc)?
+    };
+    println!("Computed /etc diff: {}", &config_diff);
+
     // The heart of things: updating the overlayfs on /usr
     apply_diff(repo, &diff, &target_commit, &openat::Dir::open("/usr")?)?;
 
@@ -443,6 +456,7 @@ pub(crate) fn transaction_apply_live(
     update_etc(
         repo,
         &diff,
+        &config_diff,
         &sepolicy,
         &target_commit,
         &openat::Dir::open("/etc")?,

rust/src/ostree_diff.rs

@@ -9,6 +9,7 @@ use gio::prelude::*;
 use ostree::RepoFileExt;
 use serde_derive::{Deserialize, Serialize};
 use std::collections::BTreeSet;
+use std::fmt;
 
 /// Like `g_file_query_info()`, but return None if the target doesn't exist.
 fn query_info_optional(
@@ -53,6 +54,21 @@ pub(crate) struct FileTreeDiff {
     pub(crate) changed_dirs: FileSet,
 }
 
+impl fmt::Display for FileTreeDiff {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "files(added:{} removed:{} changed:{}) dirs(added:{} removed:{} changed:{})",
+            self.added_files.len(),
+            self.removed_files.len(),
+            self.changed_files.len(),
+            self.added_dirs.len(),
+            self.removed_dirs.len(),
+            self.changed_dirs.len()
+        )
+    }
+}
+
 fn diff_recurse(
     prefix: &str,
     diff: &mut FileTreeDiff,

tests/vmcheck/test-apply-live.sh

@@ -86,6 +86,7 @@ vm_build_rpm test-livefs-with-etc \
   build 'echo "A config file for %{name}" > %{name}.conf' \
   install 'mkdir -p %{buildroot}/etc
            install %{name}.conf %{buildroot}/etc
+           echo otherconf > %{buildroot}/etc/%{name}-other.conf
            mkdir -p %{buildroot}/etc/%{name}/
            echo subconfig-one > %{buildroot}/etc/%{name}/subconfig-one.conf
            echo subconfig-two > %{buildroot}/etc/%{name}/subconfig-two.conf
@@ -94,6 +95,7 @@ vm_build_rpm test-livefs-with-etc \
            mkdir -p %{buildroot}/etc/opt
            echo file-in-opt-subdir > %{buildroot}/etc/opt/%{name}-opt.conf' \
   files "/etc/%{name}.conf
+         /etc/%{name}-other.conf
          /etc/%{name}/*
          /etc/opt/%{name}*"
 
@@ -111,13 +113,18 @@ vm_build_rpm test-livefs-service \
 vm_cmd rm -rf /etc/test-livefs-with-etc \
               /etc/test-livefs-with-etc.conf \
               /etc/opt/test-livefs-with-etc-opt.conf
+# But test with a modified config file
+vm_cmd echo myconfig \> /etc/test-livefs-with-etc-other.conf
+vm_cmd grep myconfig /etc/test-livefs-with-etc-other.conf
 
 vm_rpmostree install /var/tmp/vmcheck/yumrepo/packages/x86_64/test-livefs-{with-etc,service}-1.0-1.x86_64.rpm
-vm_rpmostree ex livefs
+vm_rpmostree ex apply-live
 vm_cmd rpm -q bar test-livefs-{with-etc,service} > rpmq.txt
 assert_file_has_content rpmq.txt bar-1.0-1 test-livefs-{with-etc,service}-1.0-1
 vm_cmd cat /etc/test-livefs-with-etc.conf > test-livefs-with-etc.conf
 assert_file_has_content test-livefs-with-etc.conf "A config file for test-livefs-with-etc"
+vm_cmd cat /etc/test-livefs-with-etc-other.conf > conf
+assert_file_has_content conf myconfig
 for v in subconfig-one subconfig-two subdir/subconfig-three; do
     vm_cmd cat /etc/test-livefs-with-etc/${v}.conf > test-livefs-with-etc.conf
     assert_file_has_content_literal test-livefs-with-etc.conf $(basename $v)