On Sun, Sep 26, 2021 at 02:35:07PM +0200, Stefan Sperling wrote:
> On Sun, Sep 26, 2021 at 02:17:59PM +0200, Stefan Sperling wrote:
> > On Sun, Sep 26, 2021 at 02:05:33PM +0200, Stefan Sperling wrote:
> > > On Sun, Sep 26, 2021 at 01:57:49PM +0200, Stefan Sperling wrote:
> > > > I suspect it is not intentional.
> > > > 
> > > > It looks like EMLINK from open(symlink, O_NOFOLLOW) comes from this change:
> > > > https://reviews.freebsd.org/D29323
> > > > (I don't have a freebsd system to test with, so I might be wrong.)
> > > 
> > > I made a mistake and did no go back far enough in history.
> > > 
> > > Turns out this behaviour was introduced in FreeBSD way back in 1998:
> > > https://cgit.freebsd.org/src/commit/?id=100ceca222a193e5d374a3dc9782287785dd9b61
> > > 
> > > No rationale given for the switch from ELOOP to EMLINK.
> > 
> > It does not look like there is any hope for this to change:
> > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214633
> 
> I think we should just give up and admit that this aspect of UNIX
> was never unified. It's ugly but reflects reality rather than
> hiding the problem under a not-really-portable POSIX rug.
> 
> ok?

Oops, I forgot to run regression tests before sending this out.

I got one case wrong where the check was for == ELOOP instead of != ELOOP.
Fixed here.

diff 5267b9e4960076d7de62633b9f5f1dcdb6594b33 /home/stsp/src/got
blob - 66b864fee620072787f5c6149732f3c409f4f0cf
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -4394,7 +4394,7 @@ print_diff(void *arg, unsigned char status, unsigned c
 		if (dirfd != -1) {
 			fd = openat(dirfd, de_name, O_RDONLY | O_NOFOLLOW);
 			if (fd == -1) {
-				if (errno != ELOOP) {
+				if (!got_err_open_nofollow_on_symlink()) { 
 					err = got_error_from_errno2("openat",
 					    abspath);
 					goto done;
@@ -4407,7 +4407,7 @@ print_diff(void *arg, unsigned char status, unsigned c
 		} else {
 			fd = open(abspath, O_RDONLY | O_NOFOLLOW);
 			if (fd == -1) {
-				if (errno != ELOOP) {
+				if (!got_err_open_nofollow_on_symlink()) {
 					err = got_error_from_errno2("open",
 					    abspath);
 					goto done;
blob - f87c06cad9b95c07cc3e6ce8f0e0f733de70a1a6
file + include/got_error.h
--- include/got_error.h
+++ include/got_error.h
@@ -427,3 +427,9 @@ const struct got_error *got_error_path(const char *, i
  * additional arguments.
 */
 const struct got_error *got_error_fmt(int, const char *, ...);
+
+/*
+ * Check whether open(2) with O_NOFOLLOW failed on a symlink.
+ * This must be called directly after open(2) because it uses errno!
+ */
+int got_err_open_nofollow_on_symlink(void);
blob - 72e00488bedc8ea49b3e20af7eb76ec82839bfa3
file + lib/error.c
--- lib/error.c
+++ lib/error.c
@@ -258,3 +258,15 @@ got_error_fmt(int code, const char *fmt, ...)
 
 	abort();
 }
+
+int
+got_err_open_nofollow_on_symlink(void)
+{
+	/*
+	 * Check whether open(2) with O_NOFOLLOW failed on a symlink.
+	 * Posix mandates ELOOP and OpenBSD follows it. Others return
+	 * different error codes. We carry this workaround to help the
+	 * portable version a little.
+	 */
+	return (errno == ELOOP || errno == EMLINK || errno == EFTYPE);
+}
blob - d2c6246c748ba52478619a448ec02bf61c9e9729
file + lib/object_create.c
--- lib/object_create.c
+++ lib/object_create.c
@@ -129,7 +129,7 @@ got_object_blob_file_create(struct got_object_id **id,
 
 	fd = open(ondisk_path, O_RDONLY | O_NOFOLLOW);
 	if (fd == -1) {
-		if (errno != ELOOP)
+		if (!got_err_open_nofollow_on_symlink())
 			return got_error_from_errno2("open", ondisk_path);
 
 		if (lstat(ondisk_path, &sb) == -1) {
blob - 904fb23ceb80ef07dc958d9908801822c83de3f3
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -1284,7 +1284,7 @@ replace_existing_symlink(int *did_something, const cha
 	 */
 	fd = open(ondisk_path, O_RDWR | O_EXCL | O_NOFOLLOW);
 	if (fd == -1) {
-		if (errno != ELOOP)
+		if (!got_err_open_nofollow_on_symlink())
 			return got_error_from_errno2("open", ondisk_path);
 
 		/* We are updating an existing on-disk symlink. */
@@ -1781,9 +1781,10 @@ get_file_status(unsigned char *status, struct stat *sb
 		}
 	} else {
 		fd = open(abspath, O_RDONLY | O_NOFOLLOW);
-		if (fd == -1 && errno != ENOENT && errno != ELOOP)
+		if (fd == -1 && errno != ENOENT &&
+		    !got_err_open_nofollow_on_symlink())
 			return got_error_from_errno2("open", abspath);
-		else if (fd == -1 && errno == ELOOP) {
+		else if (fd == -1 && got_err_open_nofollow_on_symlink()) {
 			if (lstat(abspath, sb) == -1)
 				return got_error_from_errno2("lstat", abspath);
 		} else if (fd == -1 || fstat(fd, sb) == -1) {
@@ -3767,7 +3768,7 @@ worktree_status(struct got_worktree *worktree, const c
 	fd = open(ondisk_path, O_RDONLY | O_NOFOLLOW | O_DIRECTORY);
 	if (fd == -1) {
 		if (errno != ENOTDIR && errno != ENOENT && errno != EACCES &&
-		    errno != ELOOP)
+		    !got_err_open_nofollow_on_symlink())
 			err = got_error_from_errno2("open", ondisk_path);
 		else {
 			if (!no_ignores) {
@@ -4473,7 +4474,7 @@ create_patched_content(char **path_outfile, int revers
 	if (dirfd2 != -1) {
 		fd2 = openat(dirfd2, de_name2, O_RDONLY | O_NOFOLLOW);
 		if (fd2 == -1) {
-			if (errno != ELOOP) {
+			if (!got_err_open_nofollow_on_symlink()) {
 				err = got_error_from_errno2("openat", path2);
 				goto done;
 			}
@@ -4487,7 +4488,7 @@ create_patched_content(char **path_outfile, int revers
 	} else {
 		fd2 = open(path2, O_RDONLY | O_NOFOLLOW);
 		if (fd2 == -1) {
-			if (errno != ELOOP) {
+			if (!got_err_open_nofollow_on_symlink()) {
 				err = got_error_from_errno2("open", path2);
 				goto done;
 			}