freebsd-update: handle directories changing to files, too

[emaste]

Summary

Extension of D41893 for directories back to files.

[emaste]

adjust quoting, add -- to rm invocation

[dim]

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.

[emaste]

Upload correct diff

[emaste]

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]

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

[emaste]

Update after f6d37c9ca13f8ab0ef32cf5344daecb8122d1e85

[emaste]

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

2900

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

[emaste]

drop redundant [ -e

[emaste]

test for existence of file before removing

to avoid spurious errors

[dim]

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.

[emaste]

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

[khorben_defora.org]

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)

[emaste]

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

[khorben_defora.org]

In D41945#958461, @emaste wrote:

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)

[emaste]

In D41945#958820, @khorben_defora.org wrote:

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

[khorben_defora.org]

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

2983

True, I did not think of that.

[karels]

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

[dim]

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

[karels]

Thanks! Could you also look at https://reviews.freebsd.org/D42020?