Details
- Reviewers
cperciva kevans gordon dim emaste - Group Reviewers
releng - Commits
- rG0eb6c273622d: freebsd-update: handle file -> directory on upgrade
rGcfb624d7e250: freebsd-update: handle file -> directory on upgrade
rG274281864fc0: freebsd-update: handle file -> directory on upgrade
rG774cc6348a50: freebsd-update: handle file -> directory on upgrade
rG4719202d8cff: freebsd-update: handle file -> directory on upgrade
rGbc412215646c: freebsd-update: handle file -> directory on upgrade
rGf6d37c9ca13f: freebsd-update: handle file -> directory on upgrade
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| usr.sbin/freebsd-update/freebsd-update.sh | ||
|---|---|---|
| 2913 | ||
| usr.sbin/freebsd-update/freebsd-update.sh | ||
|---|---|---|
| 2915 | So this will definitely kill off any customizations a user has in such a directory, which should become a file? It's probably tricky to check here, but maybe there needs to be some sort of prompt? | |
With this patch, updated a vultr VM from 13.2 to FreeBSD vultr.guest 14.0-BETA2 FreeBSD 14.0-BETA2 #0 releng/14.0-n265096-dfd44f2f0143: Fri Sep 15 05:46:35 UTC 2023 root@releng1.nyi.freebsd.org:/usr/obj/usr/src/amd64.amd64/sys/GENERIC amd64
Attempting to rollback:
# sh freebsd-update.sh rollback Uninstalling updates...install: ///lib/casper/libcap_dns.so.2: No such file or directory install: ///lib/casper/libcap_fileargs.so.1: No such file or directory install: ///lib/casper/libcap_grp.so.1: No such file or directory install: ///lib/casper/libcap_net.so.1: No such file or directory install: ///lib/casper/libcap_pwd.so.1: No such file or directory install: ///lib/casper/libcap_sysctl.so.2: No such file or directory install: ///lib/casper/libcap_syslog.so.1: No such file or directory rm: ///usr/include/c++/v1/__string/extern_template_lists.h: Not a directory rm: ///usr/include/c++/v1/__string/char_traits.h: Not a directory done.
I think it might have removed the /lib/casper directory, but has not yet re-created it at this point? But I also don't know where it gets the old libcap*.so files from.
rm: ///usr/include/c++/v1/__string/extern_template_lists.h: Not a directory rm: ///usr/include/c++/v1/__string/char_traits.h: Not a directory done.
I think it has rm -rf'd the __string directory already at this point, and has already installed the file __string, and then after the whole thing it still tries to individually rm the __string/*.h files. And then you obviously get "Not a directory".
So if you do the rm -rf step for a directory in the early stage, you should probably remove the files in that directory from the "to be deleted later" list, somehow?
It looks like /lib/casper was in fact created, but the libcap* files weren't saved away somehow. But I think this issue is not directly related to my patch here and needs more investigation.
rm: ///usr/include/c++/v1/__string/extern_template_lists.h: Not a directory rm: ///usr/include/c++/v1/__string/char_traits.h: Not a directory done.
I think it has rm -rf'd the __string directory already at this point, and has already installed the file __string, and then after the whole thing it still tries to individually rm the __string/*.h files. And then you obviously get "Not a directory".
Yes, this is precisely the problem. To do this properly I guess we would need to track the files we're removing, and then skip them in install_delete.
maybe leave the directory->file case out for now and just handle the case that affects 13.2->14.0, so that people can apply that part to a 13.2 system and participate in 14 beta testing?
if [ -e ${BASEDIR}/${FPATH} ]; then
if [ ${TYPE} = d ] && ! [ -d ${BASEDIR}/${FPATH} ]; then
rm -f ${BASEDIR}/${FPATH}
fi
fionly handle file->directory for now
proposed commit message:
freebsd-update: handle file -> directory on upgrade Upgrading from FreeBSD 13.2 to 14.0 failed with install: ///usr/include/c++/v1/__string exists but is not a directory because __string changed from a file to a directory with an LLVM upgrade. Now, remove the existing file when the type conflicts. Note that this is only an interim fix to facilitate upgrades from 13.2 for 14.0 BETA testing. This change does not handle the directory -> file case and further work is needed.
...
maybe leave the directory->file case out for now and just handle the case that affects 13.2->14.0, so that people can apply that part to a 13.2 system and participate in 14 beta testing?
if [ -e ${BASEDIR}/${FPATH} ]; then if [ ${TYPE} = d ] && ! [ -d ${BASEDIR}/${FPATH} ]; then rm -f ${BASEDIR}/${FPATH} fi fi
I'm not sure if a rollback will work at all, then? Since it has to first completely remove the /usr/include/c++/v1/__string/ directory before it can reinstall the /usr/include/c++/v1/__string file?
I'm not sure if a rollback will work at all, then?
Correct. Right now both upgrade (to 14.0-BETAx) and rollback would be broken, this change will fix only upgrade.
| usr.sbin/freebsd-update/freebsd-update.sh | ||
|---|---|---|
| 2915 | This should probably be between double quotes to avoid problems should BASEDIR or FPATH have a space somewhere. rm -f "${BASEDIR}/${FPATH}" | |
For what I understand from the code, in the case of a rollback, installation of "old" files (say 13.2 files, i.e __string file) come first, removal of the "new" files (say 14.0 files, i.e __string/ directory) comes later. From rollback_files() function:
# Deal with files which are neither kernel nor shared library
grep -vE '^/boot/' $1/INDEX-OLD |
grep -vE '/lib/.*\.so\.[0-9]+\|' > INDEX-OLD
grep -vE '^/boot/' $1/INDEX-NEW |
grep -vE '/lib/.*\.so\.[0-9]+\|' > INDEX-NEW
install_from_index INDEX-OLD || return 1 <---
install_delete INDEX-NEW INDEX-OLD || return 1 <---So where the __string directory from 14.0 was deleted?
@emaste do you happen to have the INDEX-NEW and INDEX-OLD files?
| usr.sbin/freebsd-update/freebsd-update.sh | ||
|---|---|---|
| 2913 | Would it make sense to first check if the entry in INDEX is a directory? /var/db/freebsd-update/install.i3wHbM# cut -f2 -d'|' INDEX-NEW | sort | uniq -c 23 L 41 d 9625 f In general the dir/files ration seems to be low. Since most of the files will exist (most files are upgraded, a few removed and some newly installed), we could save many path existence checks. While here, add a message to show instances of this problem. This might be removed in the final version: # A file may change to a directory on upgrade (PR273661).
# If that happens rm the file first so that install can create
# the directory in its place.
if [ ${TYPE} = d ]; then
if [ -e "${BASEDIR}/${FPATH}" ] && ! [ -d "${BASEDIR}/${FPATH}" ]; then
echo "Warn: file ${BASEDIR}/${FPATH} has become a directory"
rm -f -- "${BASEDIR}/${FPATH}"
fi
fi | |
I thought of removing all the file entries in INDEX that start with the directory to be converted to a file in the rollback (/usr/include/c++/v1/__string/) but the INDEX-NEW and INDEX-OLD file are used several times and I'm afraid changing them in the middle of an upgrade might render them unusable. If the point is to skip them (can't delete them because they no longer exist), why not do that by default?
maybe leave the directory->file case out for now and just handle the case that affects 13.2->14.0, so that people can apply that part to a 13.2 system and participate in 14 beta testing?
if [ -e ${BASEDIR}/${FPATH} ]; then if [ ${TYPE} = d ] && ! [ -d ${BASEDIR}/${FPATH} ]; then rm -f ${BASEDIR}/${FPATH} fi fi
The companion review D41945 handles the directory->file case, but there are still pre-existing issues with rollback that are tracked in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=273950.
| usr.sbin/freebsd-update/freebsd-update.sh | ||
|---|---|---|
| 2913 |
For this review it would, but the followup in D41945 will add file/symlink tests. | |
I think this part is OK now. That will at least ensure upgrades work, but not rollbacks.
(As noted in another review, the two ifs could be merged together if preferred, but it's not functionally necessary. Another style thing would be to move these checks into the case d) and case f) parts, but again that is more a matter of style.)
Move test into case d) to simplify and avoid extra stat calls (since we are initially addressing only the upgrade file/link -> directory case).
This has enough guard rails that even someone doing something incredibly silly (BASEDIR set to "-r /" would be relatively protected. LGTM.