core: Maintain /usr/etc as /etc when running scripts #1592
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Still testing this, but not yet marking WIP since I'm hoping it's close to ready. |
cgwalters
force-pushed
the
unpriv-prep
branch
2 times, most recently
from
292bb72 to
22f8132
Compare
|
OK, this is updated now. I am still wavering on
|
|
☔ The latest upstream changes (presumably 2e3d4b7) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased 🏄♂️ |
jlebon
reviewed
Oct 3, 2018
There was a problem hiding this comment.
Understanding how all the various paths are affected every time we make changes to how we handle /etc is painful. I feel like we need some function annotations or something.
It's tempting to basically maintain /usr/etc as /etc all
the way from immediately after checkout to just before commit.
This would make the layering and the unified core paths more similar, right? I think I'd be in favour of that.
src/libpriv/rpmostree-core.c
Outdated
| @@ -3876,6 +3878,16 @@ rpmostree_context_assemble (RpmOstreeContext *self, | |||
| &var_lib_rpm_statedir, error)) | |||
| return FALSE; | |||
|
|
|||
| if (!glnx_fstatat_allow_noent (tmprootfs_dfd, "usr/etc", NULL, 0, error)) | |||
There was a problem hiding this comment.
Aside: at this point, we rely so much on this test that we should make sure to error out upfront during a treecompose if some package somehow created /usr/etc before we started post-processing.
There was a problem hiding this comment.
Why are we checking this here though? When is it not already /usr/etc at this point of the process? Is this essentially the unified core path vs the layering path?
There was a problem hiding this comment.
What I hit is tests/check/test-ucontainer.sh failing, because it creates a rootfs that has just a foo package which is basically empty. So there's no RPMs that include files in /etc.
Admittedly this is a pathological case today...but you know some people are arguing for an empty /etc.
Yeah but...it'd be a fairly invasive patch. Here I targeted just the codepaths around running scripts. I think my vote would be to do that patch, but later. |
|
OK, patch looks good though,
Can you unpack this a bit? Even with |
Yep, exactly. |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 4, 2018
In preparation for running in default Docker permissions where we can `chroot()` and `makedev()` but not e.g. create bind mounts, move `/usr/etc` to `/etc` when running scripts. The script processing is also entangled with our passwd/group file handling, so change those functions called from the core too. It's tempting to basically maintain `/usr/etc` as `/etc` all the way from immediately after checkout to just before commit. We can't change how we do imports now; perhaps importing RPMs into ostree as `usr/etc` was just a mistake in retrospect, but oh well. Closes: #1592 Approved by: jlebon
|
💔 Test failed - status-atomicjenkins |
|
bot, retest this please |
|
@rh-atomic-bot retry |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 5, 2018
In preparation for running in default Docker permissions where we can `chroot()` and `makedev()` but not e.g. create bind mounts, move `/usr/etc` to `/etc` when running scripts. The script processing is also entangled with our passwd/group file handling, so change those functions called from the core too. It's tempting to basically maintain `/usr/etc` as `/etc` all the way from immediately after checkout to just before commit. We can't change how we do imports now; perhaps importing RPMs into ostree as `usr/etc` was just a mistake in retrospect, but oh well. Closes: #1592 Approved by: jlebon
|
💔 Test failed - status-atomicjenkins |
|
Whee! Big pile of |
jlebon
reviewed
Oct 9, 2018
src/libpriv/rpmostree-passwd-util.c
Outdated
| /* And now the inverse: /usr/etc/passwd -> /usr/lib/passwd */ | ||
| if (!glnx_renameat (rootfs_dfd, glnx_strjoina ("usr/etc/", file), | ||
| /* And now the inverse: /etc/passwd -> /usr/lib/passwd */ | ||
| if (!glnx_renameat (rootfs_dfd, glnx_strjoina ("etc/", file), | ||
| rootfs_dfd, glnx_strjoina ("usr/lib/", file), error)) | ||
| return FALSE; | ||
| /* /usr/etc/passwd.rpmostreesave -> /usr/etc/passwd */ |
There was a problem hiding this comment.
Minor: that comment needs updating.
In preparation for running in default Docker permissions where we can `chroot()` and `makedev()` but not e.g. create bind mounts, move `/usr/etc` to `/etc` when running scripts. The script processing is also entangled with our passwd/group file handling, so change those functions called from the core too. It's tempting to basically maintain `/usr/etc` as `/etc` all the way from immediately after checkout to just before commit. We can't change how we do imports now; perhaps importing RPMs into ostree as `usr/etc` was just a mistake in retrospect, but oh well.
cgwalters
force-pushed
the
unpriv-prep
branch
from
6a43af1 to
48b36f6
Compare
|
Stashing the followup patch here: From fbd89fd25b36df70566339c2e82a23964af219aa Mon Sep 17 00:00:00 2001
From: Colin Walters <[email protected]>
Date: Tue, 9 Oct 2018 20:07:45 -0400
Subject: [PATCH] wip
---
src/libpriv/rpmostree-core.c | 32 ++++----------
src/libpriv/rpmostree-passwd-util.c | 2 +-
src/libpriv/rpmostree-postprocess.c | 68 +++++++----------------------
3 files changed, 25 insertions(+), 77 deletions(-)
diff --git a/src/libpriv/rpmostree-core.c b/src/libpriv/rpmostree-core.c
index 6bdd9aec..b471f071 100644
--- a/src/libpriv/rpmostree-core.c
+++ b/src/libpriv/rpmostree-core.c
@@ -3811,20 +3811,16 @@ rpmostree_context_assemble (RpmOstreeContext *self,
gboolean skip_sanity_check = FALSE;
g_variant_dict_lookup (self->spec->dict, "skip-sanity-check", "b", &skip_sanity_check);
- if (!glnx_fstatat_allow_noent (tmprootfs_dfd, "usr/etc", NULL, 0, error))
+ /* Ensure there's an /etc; a truly minimal tree might not have it, but
+ * we want to rely on its presence.
+ */
+ if (!glnx_shutil_mkdir_p_at (tmprootfs_dfd, "etc", 0755, cancellable, error))
return FALSE;
- gboolean renamed_etc = (errno == 0);
- if (renamed_etc)
- {
- /* In general now, we place contents in /etc when running scripts */
- if (!glnx_renameat (tmprootfs_dfd, "usr/etc", tmprootfs_dfd, "etc", error))
- return FALSE;
- /* But leave a compat symlink, as we used to bind mount, so scripts
- * could still use that too.
- */
- if (symlinkat ("../etc", tmprootfs_dfd, "usr/etc") < 0)
- return glnx_throw_errno_prefix (error, "symlinkat");
- }
+ /* Leave a compat symlink in /usr/etc, as we used to bind mount, so scripts
+ * could still use that too.
+ */
+ if (symlinkat ("../etc", tmprootfs_dfd, "usr/etc") < 0)
+ return glnx_throw_errno_prefix (error, "symlinkat");
/* NB: we're not running scripts right now for removals, so this is only for overlays and
* replacements */
@@ -4025,16 +4021,6 @@ rpmostree_context_assemble (RpmOstreeContext *self,
return FALSE;
}
- /* Undo the /etc move above */
- if (renamed_etc)
- {
- /* Remove the symlink and swap back */
- if (!glnx_unlinkat (tmprootfs_dfd, "usr/etc", 0, error))
- return FALSE;
- if (!glnx_renameat (tmprootfs_dfd, "etc", tmprootfs_dfd, "usr/etc", error))
- return FALSE;
- }
-
/* And clean up var/tmp, we don't want it in commits */
if (!glnx_shutil_rm_rf_at (tmprootfs_dfd, "var/tmp", cancellable, error))
return FALSE;
diff --git a/src/libpriv/rpmostree-passwd-util.c b/src/libpriv/rpmostree-passwd-util.c
index 03c0ed57..64d52f3f 100644
--- a/src/libpriv/rpmostree-passwd-util.c
+++ b/src/libpriv/rpmostree-passwd-util.c
@@ -1265,7 +1265,7 @@ rpmostree_passwd_complete_rpm_layering (int rootfs_dfd,
if (!glnx_renameat (rootfs_dfd, glnx_strjoina ("etc/", file),
rootfs_dfd, glnx_strjoina ("usr/lib/", file), error))
return FALSE;
- /* /usr/etc/passwd.rpmostreesave -> /usr/etc/passwd */
+ /* /etc/passwd.rpmostreesave -> /etc/passwd */
if (!glnx_renameat (rootfs_dfd, glnx_strjoina ("etc/", file, ".rpmostreesave"),
rootfs_dfd, glnx_strjoina ("etc/", file), error))
return FALSE;
diff --git a/src/libpriv/rpmostree-postprocess.c b/src/libpriv/rpmostree-postprocess.c
index 89c64c63..4ab1576d 100644
--- a/src/libpriv/rpmostree-postprocess.c
+++ b/src/libpriv/rpmostree-postprocess.c
@@ -63,15 +63,6 @@ run_bwrap_mutably (int rootfs_fd,
GCancellable *cancellable,
GError **error)
{
- /* For scripts, it's /etc, not /usr/etc */
- if (!glnx_renameat (rootfs_fd, "usr/etc", rootfs_fd, "etc", error))
- return FALSE;
- /* But leave a compat symlink, as we used to bind mount, so scripts
- * could still use that too.
- */
- if (symlinkat ("../etc", rootfs_fd, "usr/etc") < 0)
- return glnx_throw_errno_prefix (error, "symlinkat");
-
RpmOstreeBwrapMutability mut =
unified_core_mode ? RPMOSTREE_BWRAP_MUTATE_ROFILES : RPMOSTREE_BWRAP_MUTATE_FREELY;
g_autoptr(RpmOstreeBwrap) bwrap = rpmostree_bwrap_new (rootfs_fd, mut, error);
@@ -99,12 +90,6 @@ run_bwrap_mutably (int rootfs_fd,
if (!rpmostree_bwrap_run (bwrap, cancellable, error))
return FALSE;
- /* Remove the symlink and swap back */
- if (!glnx_unlinkat (rootfs_fd, "usr/etc", 0, error))
- return FALSE;
- if (!glnx_renameat (rootfs_fd, "etc", rootfs_fd, "usr/etc", error))
- return FALSE;
-
return TRUE;
}
@@ -284,11 +269,11 @@ process_kernel_and_initramfs (int rootfs_dfd,
* on systemd itself having set up the machine id from its %post,
* so we need to read it. We'll reset the machine ID after this.
*/
- { glnx_autofd int fd = openat (rootfs_dfd, "usr/etc/machine-id", O_RDONLY | O_CLOEXEC);
+ { glnx_autofd int fd = openat (rootfs_dfd, "etc/machine-id", O_RDONLY | O_CLOEXEC);
if (fd < 0)
{
if (errno != ENOENT)
- return glnx_throw_errno_prefix (error, "openat(usr/etc/machine-id)");
+ return glnx_throw_errno_prefix (error, "openat(etc/machine-id)");
}
else
{
@@ -388,14 +373,14 @@ process_kernel_and_initramfs (int rootfs_dfd,
* systemd-219-9.fc22) but it is correctly populated if the file is there.
*/
g_print ("Creating empty machine-id\n");
- if (!glnx_file_replace_contents_at (rootfs_dfd, "usr/etc/machine-id", (guint8*)"", 0,
+ if (!glnx_file_replace_contents_at (rootfs_dfd, "etc/machine-id", (guint8*)"", 0,
GLNX_FILE_REPLACE_NODATASYNC,
cancellable, error))
return FALSE;
}
else
{
- (void) unlinkat (rootfs_dfd, "usr/etc/machine-id", 0);
+ (void) unlinkat (rootfs_dfd, "etc/machine-id", 0);
}
/* Run dracut with our chosen arguments (commonly at least --no-hostonly) */
@@ -825,7 +810,7 @@ replace_nsswitch (int dfd,
GError **error)
{
g_autofree char *nsswitch_contents =
- glnx_file_get_contents_utf8_at (dfd, "usr/etc/nsswitch.conf", NULL,
+ glnx_file_get_contents_utf8_at (dfd, "etc/nsswitch.conf", NULL,
cancellable, error);
if (!nsswitch_contents)
return FALSE;
@@ -835,7 +820,7 @@ replace_nsswitch (int dfd,
if (!new_nsswitch_contents)
return FALSE;
- if (!glnx_file_replace_contents_at (dfd, "usr/etc/nsswitch.conf",
+ if (!glnx_file_replace_contents_at (dfd, "etc/nsswitch.conf",
(guint8*)new_nsswitch_contents, -1,
GLNX_FILE_REPLACE_NODATASYNC,
cancellable, error))
@@ -852,7 +837,7 @@ rpmostree_rootfs_fixup_selinux_store_root (int rootfs_dfd,
GCancellable *cancellable,
GError **error)
{
- const char *semanage_path = "usr/etc/selinux/semanage.conf";
+ const char *semanage_path = "etc/selinux/semanage.conf";
/* Check if the config file exists; if not, do nothing silently */
if (!glnx_fstatat_allow_noent (rootfs_dfd, semanage_path, NULL, AT_SYMLINK_NOFOLLOW, error))
@@ -916,7 +901,7 @@ postprocess_selinux_policy_store_location (int rootfs_dfd,
if (!rpmostree_rootfs_fixup_selinux_store_root (rootfs_dfd, cancellable, error))
return FALSE;
- etc_policy_location = glnx_strjoina ("usr/etc/selinux/", name);
+ etc_policy_location = glnx_strjoina ("etc/selinux/", name);
if (!glnx_opendirat (rootfs_dfd, etc_policy_location, TRUE, &etc_selinux_dfd, error))
return FALSE;
@@ -978,7 +963,7 @@ rpmostree_postprocess_final (int rootfs_dfd,
error))
return FALSE;
- g_print ("Migrating /usr/etc/passwd to /usr/lib/\n");
+ g_print ("Migrating /etc/passwd to /usr/lib/\n");
if (!rpmostree_passwd_migrate_except_root (rootfs_dfd, RPM_OSTREE_PASSWD_MIGRATE_PASSWD, NULL,
cancellable, error))
return FALSE;
@@ -990,7 +975,7 @@ rpmostree_postprocess_final (int rootfs_dfd,
preserve_groups_set = _rpmostree_jsonutil_jsarray_strings_to_set (etc_group_members);
}
- g_print ("Migrating /usr/etc/group to /usr/lib/\n");
+ g_print ("Migrating /etc/group to /usr/lib/\n");
if (!rpmostree_passwd_migrate_except_root (rootfs_dfd, RPM_OSTREE_PASSWD_MIGRATE_GROUP,
preserve_groups_set,
cancellable, error))
@@ -1253,7 +1238,7 @@ cleanup_selinux_lockfiles (int rootfs_fd,
GCancellable *cancellable,
GError **error)
{
- if (!glnx_fstatat_allow_noent (rootfs_fd, "usr/etc/selinux", NULL, 0, error))
+ if (!glnx_fstatat_allow_noent (rootfs_fd, "etc/selinux", NULL, 0, error))
return FALSE;
if (errno == ENOENT)
@@ -1261,8 +1246,8 @@ cleanup_selinux_lockfiles (int rootfs_fd,
/* really we only have to do this for the active policy, but let's scan all the dirs */
g_auto(GLnxDirFdIterator) dfd_iter = { 0, };
- if (!glnx_dirfd_iterator_init_at (rootfs_fd, "usr/etc/selinux", FALSE, &dfd_iter, error))
- return glnx_prefix_error (error, "Opening /usr/etc/selinux");
+ if (!glnx_dirfd_iterator_init_at (rootfs_fd, "etc/selinux", FALSE, &dfd_iter, error))
+ return glnx_prefix_error (error, "Opening /etc/selinux");
while (TRUE)
{
@@ -1494,9 +1479,6 @@ rpmostree_treefile_postprocessing (int rootfs_fd,
g_assert (treefile_rs);
g_assert (treefile);
- if (!rename_if_exists (rootfs_fd, "etc", rootfs_fd, "usr/etc", error))
- return FALSE;
-
gboolean machineid_compat = TRUE;
if (!_rpmostree_jsonutil_object_get_optional_boolean_member (treefile, "machineid-compat",
&machineid_compat, error))
@@ -1518,10 +1500,10 @@ rpmostree_treefile_postprocessing (int rootfs_fd,
{
glnx_autofd int multiuser_wants_dfd = -1;
- if (!glnx_shutil_mkdir_p_at (rootfs_fd, "usr/etc/systemd/system/multi-user.target.wants", 0755,
+ if (!glnx_shutil_mkdir_p_at (rootfs_fd, "etc/systemd/system/multi-user.target.wants", 0755,
cancellable, error))
return FALSE;
- if (!glnx_opendirat (rootfs_fd, "usr/etc/systemd/system/multi-user.target.wants", TRUE,
+ if (!glnx_opendirat (rootfs_fd, "etc/systemd/system/multi-user.target.wants", TRUE,
&multiuser_wants_dfd, error))
return FALSE;
@@ -1594,12 +1576,6 @@ rpmostree_treefile_postprocessing (int rootfs_fd,
else
len = 0;
- /* Put /etc back for backwards compatibility */
- if (len > 0)
- {
- if (!rename_if_exists (rootfs_fd, "usr/etc", rootfs_fd, "etc", error))
- return FALSE;
- }
/* Process the remove-files element */
for (guint i = 0; i < len; i++)
{
@@ -1616,12 +1592,6 @@ rpmostree_treefile_postprocessing (int rootfs_fd,
if (!glnx_shutil_rm_rf_at (rootfs_fd, val, cancellable, error))
return FALSE;
}
- if (len > 0)
- {
- /* And put /etc back to /usr/etc */
- if (!rename_if_exists (rootfs_fd, "etc", rootfs_fd, "usr/etc", error))
- return FALSE;
- }
/* This works around a potential issue with libsolv if we go down the
* rpmostree_get_pkglist_for_root() path. Though rpm has been using the
@@ -1639,10 +1609,6 @@ rpmostree_treefile_postprocessing (int rootfs_fd,
if (symlinkat ("../../" RPMOSTREE_RPMDB_LOCATION, rootfs_fd, "var/lib/rpm") < 0)
return glnx_throw_errno_prefix (error, "symlinkat(%s)", "var/lib/rpm");
- /* Take care of /etc for these bits */
- if (!rename_if_exists (rootfs_fd, "usr/etc", rootfs_fd, "etc", error))
- return FALSE;
-
if (json_object_has_member (treefile, "remove-from-packages"))
{
remove = json_object_get_array_member (treefile, "remove-from-packages");
@@ -1729,10 +1695,6 @@ rpmostree_treefile_postprocessing (int rootfs_fd,
}
}
- /* Backwards compatibility */
- if (!rename_if_exists (rootfs_fd, "etc", rootfs_fd, "usr/etc", error))
- return FALSE;
-
/* Copy in additional files before postprocessing */
if (!copy_additional_files (rootfs_fd, treefile_rs, treefile, cancellable, error))
return FALSE;
--
2.17.1
|
|
⚡ Test exempted: pull fully rebased and already tested. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In preparation for running in default Docker permissions where
we can
chroot()andmakedev()but not e.g. create bind mounts,move
/usr/etcto/etcwhen running scripts.The script processing is also entangled with our passwd/group
file handling, so change those functions called from the core too.
It's tempting to basically maintain
/usr/etcas/etcallthe way from immediately after checkout to just before commit.
We can't change how we do imports now; perhaps importing
RPMs into ostree as
usr/etcwas just a mistake in retrospect,but oh well.