Page MenuHomeFreeBSD

freebsd-update: handle file->directory change on upgrade
ClosedPublic

Authored by emaste on Sep 17 2023, 1:49 PM.
Tags
None
Referenced Files
F99527331: D41893.id127788.diff
Thu, Oct 10, 12:06 PM
Unknown Object (File)
Wed, Oct 9, 2:10 PM
Unknown Object (File)
Mon, Oct 7, 2:11 PM
Unknown Object (File)
Fri, Oct 4, 2:27 PM
Unknown Object (File)
Fri, Oct 4, 5:44 AM
Unknown Object (File)
Wed, Oct 2, 3:31 PM
Unknown Object (File)
Mon, Sep 30, 6:23 PM
Unknown Object (File)
Mon, Sep 30, 1:26 PM

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

emaste created this revision.
emaste added subscribers: cperciva, releng.
usr.sbin/freebsd-update/freebsd-update.sh
2912
dim added inline comments.
usr.sbin/freebsd-update/freebsd-update.sh
2914

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.

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

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?

In D41893#955383, @dim wrote:

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

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.

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
fi

only 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.

This revision is now accepted and ready to land.Sep 22 2023, 8:00 PM
This revision now requires review to proceed.Sep 22 2023, 8:02 PM
fernape added inline comments.
usr.sbin/freebsd-update/freebsd-update.sh
2914

This should probably be between double quotes to avoid problems should BASEDIR or FPATH have a space somewhere.

rm -f "${BASEDIR}/${FPATH}"

quote rm arg, also add -- to avoid issues with pathological $BASEDIR

emaste retitled this revision from draft patch for freebsd-update to handle directory <-> file swap to freebsd-update: handle file->directory change on upgrade.Sep 25 2023, 6:19 PM
In D41893#955748, @dim wrote:

...

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?

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
2912

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
In D41893#955383, @dim wrote:

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

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.

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.

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
2912

Would it make sense to first check if the entry in INDEX is a directory?

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.)

This revision is now accepted and ready to land.Sep 26 2023, 3:36 PM

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 revision now requires review to proceed.Sep 27 2023, 1:40 PM

This has enough guard rails that even someone doing something incredibly silly (BASEDIR set to "-r /" would be relatively protected. LGTM.

This revision is now accepted and ready to land.Sep 27 2023, 2:18 PM