Page MenuHomeFreeBSD

freebsd-update: handle directories changing to files, too
ClosedPublic

Authored by emaste on Sep 22 2023, 8:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 8, 10:01 AM
Unknown Object (File)
Sat, Dec 7, 3:38 PM
Unknown Object (File)
Tue, Dec 3, 8:54 AM
Unknown Object (File)
Tue, Dec 3, 8:53 AM
Unknown Object (File)
Tue, Dec 3, 8:48 AM
Unknown Object (File)
Tue, Dec 3, 8:47 AM
Unknown Object (File)
Tue, Dec 3, 8:28 AM
Unknown Object (File)
Tue, Dec 3, 8:02 AM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste created this revision.
emaste added a reviewer: gordon.

adjust quoting, add -- to rm invocation

I think this part is OK now. That will at least ensure upgrades work, but not rollbacks.

usr.sbin/freebsd-update/freebsd-update.sh
2918

This could be shortened as:

if [ -e "${BASEDIR}/${FPATH}" ] && [ ${TYPE} = d ] && ! [ -d "${BASEDIR}/${FPATH}" ]; then

since && short-circuits. But it's mostly a matter of preference.

This revision is now accepted and ready to land.Sep 26 2023, 3:21 PM
This revision now requires review to proceed.Sep 26 2023, 3:31 PM

I think this part is OK now. That will at least ensure upgrades work, but not rollbacks.

Oops, I uploaded the wrong diff to this review. D41893 is the review for the upgrade (but not rollback) first part.

emaste planned changes to this revision.EditedSep 27 2023, 7:50 PM

This needs to be rebased and reworked after the slightly different approach taken in f6d37c9ca13f8ab0ef32cf5344daecb8122d1e85.

emaste added inline comments.
usr.sbin/freebsd-update/freebsd-update.sh
2900

Oh I guess -e && -d is redundant, will drop -e

test for existence of file before removing

to avoid spurious errors

usr.sbin/freebsd-update/freebsd-update.sh
2983

If you are deleting directories in dir_conflict using rm -rf I think it is reasonable to delete these offending files with rm -f too, and in that case it is not necessary to do the if [ -e, since rm will shut up and not fail in such cases.

usr.sbin/freebsd-update/freebsd-update.sh
2983

I've switched this to [ -f "${BASEDIR}/${FPATH} ] and [ -L "${BASEDIR}/${FPATH} ] locally now, because we still get an error message:

$ touch not-a-dir
$ rm -f not-a-dir/does-not-exist
rm: not-a-dir/does-not-exist: Not a directory

usr.sbin/freebsd-update/freebsd-update.sh
2918

Or maybe even:

if [ -e "${BASEDIR}/${FPATH}" -a ${TYPE} = d -a ! -d "${BASEDIR}/${FPATH}" ]; then

(it reduces the amount of calls to test through [, although it is a built-in in sh anyway)

usr.sbin/freebsd-update/freebsd-update.sh
2918

That doesn't have the benefit of short-circuiting though (and [ is a built-in anyhow, as you say).

In any case that change is already committed and is independent of this review :)

test for existence of file before removing

to avoid spurious errors

I also generally avoid that (e.g., with rm -f as already suggested here) because otherwise it creates TOCTOU conditions. (Low risk here but you never know)

I also generally avoid that (e.g., with rm -f as already suggested here) because otherwise it creates TOCTOU conditions. (Low risk here but you never know)

rm -f doesn't avoid the error.

$ touch not-a-dir
$ rm -f not-a-dir/does-not-exist
rm: not-a-dir/does-not-exist: Not a directory
usr.sbin/freebsd-update/freebsd-update.sh
2983

True, I did not think of that.

There hasn't been any discussion here for a while; any approvals, or objections?

So I think it's indeed fine now. Maybe we can still get this into 14.0?

This revision is now accepted and ready to land.Oct 17 2023, 4:01 PM