core: Don't rm-rf directories when doing overrides
It's very normal for base packages to own directories with dependent packages installing files there. Doing an rm-rf for directories was just wrong. Concretely this fixes an `override replace ./systemd-*.rpm`. librpm is also pretty conservative here (for good reason) and just ignores `ENOTEMPTY`, so let's match that. I opted to split things up so we remove not-directories in a first pass, then remove all directories we can in the second. This should maximize our chances of removing what we can in a scenario where e.g. two co-dependent packages install files to a directory one of them owns. Closes: https://github.com/projectatomic/rpm-ostree/issues/1340 Closes: #1341 Approved by: jlebon
This commit is contained in:
Colin Walters
Atomic Bot
parent
4c3d9cdbce
commit
e34f17fc3e
@ -2802,11 +2802,31 @@ build_rootfs_usrlinks (RpmOstreeContext *self,
|
||||
return TRUE;
|
||||
}
|
||||
|
||||
/* Order by decreasing string length, used by package removals/replacements */
|
||||
static int
|
||||
compare_strlen (const void *a,
|
||||
const void *b,
|
||||
void * data)
|
||||
{
|
||||
size_t a_len = strlen (a);
|
||||
size_t b_len = strlen (b);
|
||||
if (a_len == b_len)
|
||||
return 0;
|
||||
else if (a_len > b_len)
|
||||
return -1;
|
||||
return 1;
|
||||
}
|
||||
|
||||
/* Given a single package, remove its files (non-directories), unless they're
|
||||
* included in @files_skip. Add its directories to @dirs_to_remove, which
|
||||
* are handled in a second pass.
|
||||
*/
|
||||
static gboolean
|
||||
delete_package_from_root (RpmOstreeContext *self,
|
||||
rpmte pkg,
|
||||
int rootfs_dfd,
|
||||
GHashTable *files_skip,
|
||||
GSequence *dirs_to_remove,
|
||||
GCancellable *cancellable,
|
||||
GError **error)
|
||||
{
|
||||
@ -2854,8 +2874,51 @@ delete_package_from_root (RpmOstreeContext *self,
|
||||
if (!g_str_has_prefix (fn, "usr/"))
|
||||
continue;
|
||||
|
||||
if (!glnx_shutil_rm_rf_at (rootfs_dfd, fn, cancellable, error))
|
||||
return FALSE;
|
||||
/* Delete files first, we'll handle directories next */
|
||||
if (S_ISDIR (mode))
|
||||
g_sequence_insert_sorted (dirs_to_remove, g_strdup (fn), compare_strlen, NULL);
|
||||
else
|
||||
{
|
||||
if (unlinkat (rootfs_dfd, fn, 0) < 0)
|
||||
{
|
||||
if (errno != ENOENT)
|
||||
return glnx_throw_errno_prefix (error, "unlinkat(%s)", fn);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return TRUE;
|
||||
}
|
||||
|
||||
/* Process the directories which we were queued up by
|
||||
* delete_package_from_root(). We ignore non-empty directories
|
||||
* since it's valid for a package being replaced to own a directory
|
||||
* to which dependent packages install files (e.g. systemd).
|
||||
*/
|
||||
static gboolean
|
||||
handle_package_deletion_directories (int rootfs_dfd,
|
||||
GSequence *dirs_to_remove,
|
||||
GCancellable *cancellable,
|
||||
GError **error)
|
||||
{
|
||||
/* We want to perform a bottom-up removal of directories. This is implemented
|
||||
* by having a sequence of filenames sorted by decreasing length. A filename
|
||||
* of greater length must be a subdirectory, or unrelated.
|
||||
*
|
||||
* If a directory isn't empty, that's fine - a package may own
|
||||
* the directory, but it's valid for other packages to put content under it.
|
||||
* Particularly while we're doing a replacement.
|
||||
*/
|
||||
GSequenceIter *iter = g_sequence_get_begin_iter (dirs_to_remove);
|
||||
while (!g_sequence_iter_is_end (iter))
|
||||
{
|
||||
const char *fn = g_sequence_get (iter);
|
||||
if (unlinkat (rootfs_dfd, fn, AT_REMOVEDIR) < 0)
|
||||
{
|
||||
if (!G_IN_SET (errno, ENOENT, EEXIST, ENOTEMPTY))
|
||||
return glnx_throw_errno_prefix (error, "rmdir(%s)", fn);
|
||||
}
|
||||
iter = g_sequence_iter_next (iter);
|
||||
}
|
||||
|
||||
return TRUE;
|
||||
@ -3776,6 +3839,7 @@ rpmostree_context_assemble (RpmOstreeContext *self,
|
||||
&files_skip_delete, cancellable, error))
|
||||
return FALSE;
|
||||
|
||||
g_autoptr(GSequence) dirs_to_remove = g_sequence_new (g_free);
|
||||
for (guint i = 0; i < n_rpmts_elements; i++)
|
||||
{
|
||||
rpmte te = rpmtsElement (ordering_ts, i);
|
||||
@ -3786,11 +3850,16 @@ rpmostree_context_assemble (RpmOstreeContext *self,
|
||||
g_assert_cmpint (type, ==, TR_REMOVED);
|
||||
|
||||
if (!delete_package_from_root (self, te, tmprootfs_dfd, files_skip_delete,
|
||||
cancellable, error))
|
||||
dirs_to_remove, cancellable, error))
|
||||
return FALSE;
|
||||
n_rpmts_done++;
|
||||
rpmostree_output_progress_n_items ("Building filesystem", n_rpmts_done, n_rpmts_elements);
|
||||
}
|
||||
g_clear_pointer (&files_skip_delete, g_hash_table_unref);
|
||||
|
||||
if (!handle_package_deletion_directories (tmprootfs_dfd, dirs_to_remove, cancellable, error))
|
||||
return FALSE;
|
||||
g_clear_pointer (&dirs_to_remove, g_sequence_free);
|
||||
|
||||
for (guint i = 0; i < n_rpmts_elements; i++)
|
||||
{
|
||||
|
||||
114
tests/vmcheck/test-override-replace-2.sh
Executable file
114
tests/vmcheck/test-override-replace-2.sh
Executable file
@ -0,0 +1,114 @@
|
||||
#!/bin/bash
|
||||
#
|
||||
# Copyright (C) 2017 Red Hat, Inc.
|
||||
#
|
||||
# This library is free software; you can redistribute it and/or
|
||||
# modify it under the terms of the GNU Lesser General Public
|
||||
# License as published by the Free Software Foundation; either
|
||||
# version 2 of the License, or (at your option) any later version.
|
||||
#
|
||||
# This library is distributed in the hope that it will be useful,
|
||||
# but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
|
||||
# Lesser General Public License for more details.
|
||||
#
|
||||
# You should have received a copy of the GNU Lesser General Public
|
||||
# License along with this library; if not, write to the
|
||||
# Free Software Foundation, Inc., 59 Temple Place - Suite 330,
|
||||
# Boston, MA 02111-1307, USA.
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
. ${commondir}/libtest.sh
|
||||
. ${commondir}/libvm.sh
|
||||
|
||||
set -x
|
||||
|
||||
YUMREPO=/tmp/vmcheck/yumrepo/packages/x86_64
|
||||
|
||||
vm_assert_status_jq \
|
||||
'.deployments[0]["base-checksum"]|not' \
|
||||
'.deployments[0]["pending-base-checksum"]|not' \
|
||||
'.deployments[0]["base-local-replacements"]|length == 0' \
|
||||
'.deployments[0]["requested-base-local-replacements"]|length == 0'
|
||||
|
||||
build_rpms() {
|
||||
local ver=${1:-1}
|
||||
foodir=/usr/lib/foo-base
|
||||
br_foodir='%{buildroot}'${foodir}
|
||||
vm_build_rpm foo-base \
|
||||
version ${ver} \
|
||||
files "%dir ${foodir}
|
||||
${foodir}/foo-base-1.txt
|
||||
${foodir}/foo-base-2.txt" \
|
||||
install "mkdir -p ${br_foodir} && \
|
||||
echo foo-1-${ver} > ${br_foodir}/foo-base-1.txt && \
|
||||
echo foo-2-${ver} > ${br_foodir}/foo-base-2.txt"
|
||||
bardir=${foodir}/bar
|
||||
br_bardir='%{buildroot}'${bardir}
|
||||
vm_build_rpm bar-base \
|
||||
version ${ver} \
|
||||
files "%dir ${bardir}
|
||||
${bardir}/bar.txt" \
|
||||
install "mkdir -p ${br_bardir} && echo bar-${ver} > ${br_bardir}/bar.txt"
|
||||
for x in 1 2; do
|
||||
vm_build_rpm foo-ext-${x} \
|
||||
version ${ver} \
|
||||
requires "foo-base >= ${ver}" \
|
||||
files "${foodir}/foo-ext-${x}-1.txt
|
||||
${foodir}/foo-ext-${x}-2.txt" \
|
||||
install "mkdir -p ${br_foodir} && \
|
||||
for y in 1 2; do echo foo-ext-${ver}-\${y} > ${br_foodir}/foo-ext-${x}-\${y}.txt; done"
|
||||
vm_build_rpm bar-ext-${x} \
|
||||
version ${ver} \
|
||||
requires "bar-base >= ${ver}" \
|
||||
files "${bardir}/bar-ext-${x}-1.txt
|
||||
${bardir}/bar-ext-${x}-2.txt" \
|
||||
install "mkdir -p ${br_bardir} && \
|
||||
for y in 1 2; do echo bar-ext-${ver}-\${y} > ${br_bardir}/bar-ext-${x}-\${y}.txt; done"
|
||||
done
|
||||
}
|
||||
build_rpms
|
||||
|
||||
vm_rpmostree install {foo,bar}-ext-{1,2}
|
||||
vm_cmd ostree refs $(vm_get_deployment_info 0 checksum) \
|
||||
--create vmcheck_tmp/with_foo_and_bar
|
||||
vm_rpmostree cleanup -p
|
||||
|
||||
# upgrade to new commit with foo in the base layer
|
||||
vm_cmd ostree commit -b vmcheck --tree=ref=vmcheck_tmp/with_foo_and_bar
|
||||
vm_rpmostree upgrade
|
||||
vm_reboot
|
||||
echo "ok setup"
|
||||
|
||||
# Replace *just* the base parts for
|
||||
# https://github.com/projectatomic/rpm-ostree/issues/1340
|
||||
ver=2
|
||||
build_rpms ${ver}
|
||||
prev_root=$(vm_get_deployment_root 0)
|
||||
vm_cmd /bin/sh -c "cd ${prev_root} && find . -print" > prev-list.txt
|
||||
vm_rpmostree override replace $YUMREPO/{foo,bar}-base-${ver}-1.x86_64.rpm
|
||||
vm_assert_status_jq \
|
||||
'.deployments[0]["base-local-replacements"]|length == 2' \
|
||||
'.deployments[0]["requested-base-local-replacements"]|length == 2'
|
||||
new_root=$(vm_get_deployment_root 0)
|
||||
vm_cmd /bin/sh -c "cd ${new_root} && find . -print" > new-list.txt
|
||||
diff -u {prev,new}-list.txt
|
||||
rm -f *-list.txt
|
||||
vm_rpmostree cleanup -p
|
||||
echo "ok override replace base"
|
||||
|
||||
# Now replace both
|
||||
ver=3
|
||||
build_rpms ${ver}
|
||||
prev_root=$(vm_get_deployment_root 0)
|
||||
vm_cmd /bin/sh -c "cd ${prev_root} && find . -print" > prev-list.txt
|
||||
vm_rpmostree override replace $YUMREPO/{foo,bar}-base-${ver}-1.x86_64.rpm $YUMREPO/{foo,bar}-ext-{1,2}-${ver}-1.x86_64.rpm
|
||||
vm_assert_status_jq \
|
||||
'.deployments[0]["base-local-replacements"]|length == 6' \
|
||||
'.deployments[0]["requested-base-local-replacements"]|length == 6'
|
||||
new_root=$(vm_get_deployment_root 0)
|
||||
vm_cmd /bin/sh -c "cd ${new_root} && find . -print" > new-list.txt
|
||||
diff -u {prev,new}-list.txt
|
||||
rm -f *-list.txt
|
||||
echo "ok override replace both"
|
||||
Loading…
Reference in New Issue
Block a user