git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Randal L. Schwartz]

gcc -o sha1_file.o -c -g -O2 -Wall -I/usr/local/include -L/usr/local/lib -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1 -DSHA1_HEADER='<openssl/sha.h>' sha1_file.c
sha1_file.c: In function `move_temp_to_file':
sha1_file.c:1247: error: `ENOTSUP' undeclared (first use in this function)
sha1_file.c:1247: error: (Each undeclared identifier is reported only once
sha1_file.c:1247: error: for each function it appears in.)
gmake: *** [sha1_file.o] Error 1


-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Randal L. Schwartz]

>>>>> "Randal" == Randal L Schwartz <merlyn@stonehenge.com> writes:

Randal> gcc -o sha1_file.o -c -g -O2 -Wall -I/usr/local/include -L/usr/local/lib -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1 -DSHA1_HEADER='<openssl/sha.h>' sha1_file.c
Randal> sha1_file.c: In function `move_temp_to_file':
Randal> sha1_file.c:1247: error: `ENOTSUP' undeclared (first use in this function)
Randal> sha1_file.c:1247: error: (Each undeclared identifier is reported only once
Randal> sha1_file.c:1247: error: for each function it appears in.)
Randal> gmake: *** [sha1_file.o] Error 1

got it... a bit messy, but here it is:

Subject: [PATCH] fix for openbsd

---

 sha1_file.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

applies-to: b16bae041dfd1c1081873f2e88a5f82858fb2051
28dcc3eaf13856585a81a24f1b4393032f825053
diff --git a/sha1_file.c b/sha1_file.c
index 7fdc469..1079ca5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1244,7 +1244,11 @@ int move_temp_to_file(const char *tmpfil
 		 * When this succeeds, we just return 0. We have nothing
 		 * left to unlink.
 		 */
-		if ((ret == EXDEV || ret == ENOTSUP) && !rename(tmpfile, filename))
+		if ((ret == EXDEV
+#ifdef ENOTSUP
+                     || ret == ENOTSUP
+#endif
+		     ) && !rename(tmpfile, filename))
 			return 0;
 		ret = errno;
 	}
---
0.99.8.GIT



-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Johannes Schindelin]

Hi,

On Wed, 26 Oct 2005, Randal L. Schwartz wrote:

> Randal> sha1_file.c:1247: error: `ENOTSUP' undeclared (first use in this 
> function)

I thought ENOTSUP is POSIX standard?

What does your man page for link() say? If it is not supported, what value 
will be in errno?

Ciao,
Dscho

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Randal L. Schwartz]

>>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Johannes> Hi,
Johannes> On Wed, 26 Oct 2005, Randal L. Schwartz wrote:

Randal> sha1_file.c:1247: error: `ENOTSUP' undeclared (first use in this 
>> function)

Johannes> I thought ENOTSUP is POSIX standard?

Johannes> What does your man page for link() say? If it is not supported, what value 
Johannes> will be in errno?

EOPNOTSUPP

Probably a BSD thing.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Johannes Schindelin]

Hi,

On Wed, 26 Oct 2005, Randal L. Schwartz wrote:

> >>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> Johannes> What does your man page for link() say? If it is not 
> Johannes> supported, what value will be in errno?
> 
> EOPNOTSUPP

How about this, then:

---

Subject: [PATCH] Support EOPNOTSUPP of OpenBSD

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

applies-to: 01ba9ea9d9dfba5ae94c32691ac8d9fdd859fccf
eaebda7362215aa0c7349e9971848c0535782a40
diff --git a/Makefile b/Makefile
index 4877abb..5cb7194 100644
--- a/Makefile
+++ b/Makefile
@@ -211,6 +211,7 @@ ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	PLATFORM_DEFINES += -I/usr/local/include -L/usr/local/lib
+	PLATFORM_DEFINES += -DENOTSUP=EOPNOTSUPP
 endif
 ifneq (,$(findstring arm,$(uname_M)))
 	ARM_SHA1 = YesPlease
---
0.99.8.GIT

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Randal L. Schwartz]

>>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Johannes> Hi,
Johannes> On Wed, 26 Oct 2005, Randal L. Schwartz wrote:

>> >>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
Johannes> What does your man page for link() say? If it is not 
Johannes> supported, what value will be in errno?
>> 
>> EOPNOTSUPP

Interesting... google for "ENOTSUPP EOPNOTSUPP".  Apparently
hits like this:

<http://sources.redhat.com/ml/bug-glibc/2002-08/msg00013.html>

shows that on some platforms, they may have both.  This is not as
easy as it looks.  Maybe define an extra || condition to handle either
or both?

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Jan Harkes]

On Wed, Oct 26, 2005 at 08:30:39AM -0700, Randal L. Schwartz wrote:
> >> >>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Johannes> What does your man page for link() say? If it is not 
> Johannes> supported, what value will be in errno?
> >> 
> >> EOPNOTSUPP
> 
> Interesting... google for "ENOTSUPP EOPNOTSUPP".  Apparently
> hits like this:
> 
> <http://sources.redhat.com/ml/bug-glibc/2002-08/msg00013.html>
> 
> shows that on some platforms, they may have both.  This is not as
> easy as it looks.  Maybe define an extra || condition to handle either
> or both?

It looks like glibc assumes that the kernel doesn't have ENOTSUP and
defines it as EOPNOTSUPP in <bits/errno.h>. So even if the kernel does
return different values, any glibc linked application probably wouldn't
be able to test for the ENOTSUP error.

Checking the SuSv2 manpage for link(2), it doesn't mention either of
these error codes. But does mention EPERM as a valid error when the
filesystem doesn't support hardlinks.
    http://www.opengroup.org/onlinepubs/007908799/xsh/link.html

My local manual page for link(2) agrees with that definition,

	EPERM  oldpath is a directory.

	EPERM  The  filesystem  containing oldpath and newpath does not support
	       the creation of hard links.

I also took a look at the Linux 2.4/2.6 kernel sources, and msdos, fat
and vfat all do not set a function for 'dir->i_op->link'. In that case,
vfs_link() will return EPERM.

So I don't know where ENOTSUP/EOPNOTSUPP would be coming from. From the
few places I looked, I don't think the original patch can be right. Not
sure yet if I want to change Coda's return value to EPERM as well, the
thing is that we do support hardlinks, just not between different
directories, but I guess EXDEV isn't really accurate either.

Jan

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Johannes Schindelin]

Hi,

On Wed, 26 Oct 2005, Jan Harkes wrote:

> Checking the SuSv2 manpage for link(2), it doesn't mention either of
> these error codes. But does mention EPERM as a valid error when the
> filesystem doesn't support hardlinks.
>     http://www.opengroup.org/onlinepubs/007908799/xsh/link.html
> 
> My local manual page for link(2) agrees with that definition,
> 
> 	EPERM  oldpath is a directory.
> 
> 	EPERM  The  filesystem  containing oldpath and newpath does not support
> 	       the creation of hard links.

On my iBook, connecting a USB stick (FAT formatted), link() sets 
errno=ENOTSUP. I checked the man page, and erroneously thought that they 
got it right.

*turns his eyes helplessly to the sky*

Ciao,
Dscho

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Jan Harkes]

On Wed, Oct 26, 2005 at 12:15:52PM -0400, Jan Harkes wrote:
> Checking the SuSv2 manpage for link(2), it doesn't mention either of
> these error codes. But does mention EPERM as a valid error when the
> filesystem doesn't support hardlinks.
>     http://www.opengroup.org/onlinepubs/007908799/xsh/link.html

Re-reading this, I seem to have misinterpreted the text. EPERM is
returned when the filesystem doesn't allow hard linking a directory.
I guess the closest error would actually be EMLINK, assuming that a
filesystem that doesn't support hardlinks would have LINK_MAX set to 1.

    Linux returns EPERM
    Coda returns EXDEV
    BSDs seem to return EOPNOTSUPP.
    MacOS X/Darwin doesn't mention it in their manpages.

Simply falling back on rename when there is any error, as Linus
suggested, seems like a pretty good solution right now. Rename should
give the same set of errors except when the link target already existed.

Jan


diff --git a/sha1_file.c b/sha1_file.c
index e456799..0eba58c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1234,15 +1234,14 @@ int move_temp_to_file(const char *tmpfil
 	int ret = link_temp_to_file(tmpfile, filename);
 	if (ret) {
 		/*
-		 * Coda hack - coda doesn't like cross-directory links,
-		 * so we fall back to a rename, which will mean that it
-		 * won't be able to check collisions, but that's not a
-		 * big deal.
+		 * The file system may not support hardlinks, so we fall back
+		 * to a rename. We won't be able to check for collisions, but
+		 * that's not a big deal.
 		 *
 		 * When this succeeds, we just return 0. We have nothing
 		 * left to unlink.
 		 */
-		if (ret == EXDEV && !rename(tmpfile, filename))
+		if (ret && ret != EEXIST && !rename(tmpfile, filename))
 			return 0;
 	}
 	unlink(tmpfile);

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Johannes Schindelin]

Hi,

On Wed, 26 Oct 2005, Jan Harkes wrote:

> -		if (ret == EXDEV && !rename(tmpfile, filename))
> +		if (ret && ret != EEXIST && !rename(tmpfile, filename))

We don't need to test for ret, as we're already in a "if (ret)" block.  
What would happen if ret==EEXIST, and we try to rename() nevertheless? It 
should fail, too, right?

Ciao,
Dscho

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Linus Torvalds]

On Wed, 26 Oct 2005, Johannes Schindelin wrote:
> 
> On Wed, 26 Oct 2005, Jan Harkes wrote:
> 
> > -		if (ret == EXDEV && !rename(tmpfile, filename))
> > +		if (ret && ret != EEXIST && !rename(tmpfile, filename))
> 
> We don't need to test for ret, as we're already in a "if (ret)" block.  
> What would happen if ret==EEXIST, and we try to rename() nevertheless? It 
> should fail, too, right?

No, the rename will succeed and overwrite the existing file.

Which is strictly correct, but it's the wrong thing if we ever want to do 
collission detection.

So I'd actually prefer something like this patch instead.

		Linus

---
diff --git a/sha1_file.c b/sha1_file.c
index 7fdc469..642f00d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1232,19 +1232,20 @@ static int link_temp_to_file(const char 
 int move_temp_to_file(const char *tmpfile, char *filename)
 {
 	int ret = link_temp_to_file(tmpfile, filename);
-	if (ret) {
-		/*
-		 * Coda hack - coda doesn't like cross-directory links,
-		 * so we fall back to a rename, which will mean that it
-		 * won't be able to check collisions, but that's not a
-		 * big deal.
-		 *
-		 * The same holds for FAT formatted media.
-		 *
-		 * When this succeeds, we just return 0. We have nothing
-		 * left to unlink.
-		 */
-		if ((ret == EXDEV || ret == ENOTSUP) && !rename(tmpfile, filename))
+
+	/*
+	 * Coda hack - coda doesn't like cross-directory links,
+	 * so we fall back to a rename, which will mean that it
+	 * won't be able to check collisions, but that's not a
+	 * big deal.
+	 *
+	 * The same holds for FAT formatted media.
+	 *
+	 * When this succeeds, we just return 0. We have nothing
+	 * left to unlink.
+	 */
+	if (ret && ret != EEXIST) {
+		if (!rename(tmpfile, filename))
 			return 0;
 		ret = errno;
 	}

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Linus Torvalds]

On Wed, 26 Oct 2005, Randal L. Schwartz wrote:
> 
> got it... a bit messy, but here it is:
> 
> Subject: [PATCH] fix for openbsd

I think we might as well do it for any error, and skip the test of the 
actual errno entirely.

		Linus

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Johannes Schindelin]

Hi,

On Wed, 26 Oct 2005, Linus Torvalds wrote:

> On Wed, 26 Oct 2005, Randal L. Schwartz wrote:
> > 
> > got it... a bit messy, but here it is:
> > 
> > Subject: [PATCH] fix for openbsd
> 
> I think we might as well do it for any error, and skip the test of the 
> actual errno entirely.

Hah! I beat you at your own game! (I already sent out a totally untested 
patch ;-)

Ciao,
Dscho

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Horst von Brand]

Randal L. Schwartz <merlyn@stonehenge.com> wrote:
> >>>>> "Randal" == Randal L Schwartz <merlyn@stonehenge.com> writes:
> 
> Randal> gcc -o sha1_file.o -c -g -O2 -Wall -I/usr/local/include -L/usr/local/lib -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1 -DSHA1_HEADER='<openssl/sha.h>' sha1_file.c
> Randal> sha1_file.c: In function `move_temp_to_file':
> Randal> sha1_file.c:1247: error: `ENOTSUP' undeclared (first use in this function)
> Randal> sha1_file.c:1247: error: (Each undeclared identifier is reported only once
> Randal> sha1_file.c:1247: error: for each function it appears in.)
> Randal> gmake: *** [sha1_file.o] Error 1
> 
> got it... a bit messy, but here it is:
> 
> Subject: [PATCH] fix for openbsd
> 
> ---
> 
>  sha1_file.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> applies-to: b16bae041dfd1c1081873f2e88a5f82858fb2051
> 28dcc3eaf13856585a81a24f1b4393032f825053
> diff --git a/sha1_file.c b/sha1_file.c
> index 7fdc469..1079ca5 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1244,7 +1244,11 @@ int move_temp_to_file(const char *tmpfil
>  		 * When this succeeds, we just return 0. We have nothing
>  		 * left to unlink.
>  		 */
> -		if ((ret == EXDEV || ret == ENOTSUP) && !rename(tmpfile, filename))
> +		if ((ret == EXDEV
> +#ifdef ENOTSUP
> +                     || ret == ENOTSUP
> +#endif
> +		     ) && !rename(tmpfile, filename))

This is EVIL.... why not just:

  #ifndef ENOTSUP
  #define ENOTSUP EXDEV
  #endif

someplace where it doesn't hurt the eyes? (The EXDEV makes the ENOTSUPP
fold away, I presume; not that it should really matter).
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Randal L. Schwartz]

>>>>> "Horst" == Horst von Brand <vonbrand@inf.utfsm.cl> writes:

Horst> This is EVIL.... why not just:

Horst>   #ifndef ENOTSUP
Horst>   #define ENOTSUP EXDEV
Horst>   #endif

I'd consider that a worse hack.  It affects any place where EXDEV
is not really equivalent to ENOTSUP.

It's cleaner to test for the ones that work, building up an OR'ed expression.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Junio C Hamano]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

> It's cleaner to test for the ones that work, building up an OR'ed expression.

You are right.  In this case the only thing we care about is if
it failed because of EEXIST, so checking that and trying to fall
back on rename() otherwise seems cleanest, as Linus and others
suggests.

I swallowed Linus version.  Thanks for reporting (and also ctype
fix the other day).

Re: git 565ebbf79f61873042c22a7126d002c104e056f4 broken on OpenBSD

[Randal L. Schwartz]

>>>>> "Junio" == Junio C Hamano <junkio@cox.net> writes:

Junio> I swallowed Linus version.  Thanks for reporting (and also ctype
Junio> fix the other day).

I apologize that I don't immediately post the patch, but generally
I figure that maybe Smarter People Than Me might recognize the issue
before I can get around to figuring out what broke.

After all, my C is rusty.  My Perl is much better. :)

Also, I'm learning how git works, to generate diffs and patches, etc.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!