[PATCH] sha1_file: make sure correct error is propagated

[PATCH] sha1_file: make sure correct error is propagated

[Sam Vilain]

From: Sam Vilain <samv@maia.lan>

In the case that a object directory exists, but is not writable, the
code path that tries to create it is followed and the returned errno
and path that of the directory tried to be created.  The resultant
error message is confusing.

So, if the mkstemp() fails with EPERM, don't try to create the
directory - return straight away.

Signed-off-by: Sam Vilain <sam@vilain.net>
---
 sha1_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index ab2b520..7662330 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2231,7 +2231,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
 	memcpy(buffer, filename, dirlen);
 	strcpy(buffer + dirlen, "tmp_obj_XXXXXX");
 	fd = mkstemp(buffer);
-	if (fd < 0 && dirlen) {
+	if (fd < 0 && dirlen && (errno != EPERM)) {
 		/* Make sure the directory exists */
 		memcpy(buffer, filename, dirlen);
 		buffer[dirlen-1] = 0;
-- 
debian.1.5.6.1

Re: [PATCH] sha1_file: make sure correct error is propagated

[Francis Galiegue]

Le vendredi 14 novembre 2008, Sam Vilain a écrit :
> From: Sam Vilain <samv@maia.lan>
> 
> In the case that a object directory exists, but is not writable, the
> code path that tries to create it is followed and the returned errno
> and path that of the directory tried to be created.  The resultant
> error message is confusing.
> 
> So, if the mkstemp() fails with EPERM, don't try to create the
> directory - return straight away.
> 

Are you sure you didn't mean EACCESS?

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH] sha1_file: make sure correct error is propagated

[Sam Vilain]

On Fri, 2008-11-14 at 08:44 +0100, Francis Galiegue wrote:
> > So, if the mkstemp() fails with EPERM, don't try to create the
> > directory - return straight away.
> Are you sure you didn't mean EACCESS?

Ah, you're right there.  Well, maybe this one should be as well:

Subject: sha1_file: accept EACCESS as equivalent to EPERM

This was testing for 'Operation not permitted' rather than any kind
of 'Permission Denied' error; prefer EACCESS.
    
Signed-off-by: Sam Vilain <sam@vilain.net>
--
  Sorry for the inevitable wrapping/whitespace fail :(

diff --git a/sha1_file.c b/sha1_file.c
index 7662330..cd422e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2231,7 +2231,7 @@ static int create_tmpfile(char *buffer, size_t
bufsiz, const char *filename)
 	memcpy(buffer, filename, dirlen);
 	strcpy(buffer + dirlen, "tmp_obj_XXXXXX");
 	fd = mkstemp(buffer);
-	if (fd < 0 && dirlen && (errno != EPERM)) {
+	if (fd < 0 && dirlen && (errno != EACCESS)) {
 		/* Make sure the directory exists */
 		memcpy(buffer, filename, dirlen);
 		buffer[dirlen-1] = 0;
@@ -2257,7 +2257,7 @@ static int write_loose_object(const unsigned char
*sha1, char *hdr, int hdrlen,
 	filename = sha1_file_name(sha1);
 	fd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);
 	if (fd < 0) {
-		if (errno == EPERM)
+		if (errno == EACCESS || errno == EPERM)
 			return error("insufficient permission for adding an object to
repository database %s\n", get_object_directory());
 		else
 			return error("unable to create temporary sha1 filename %s: %s\n",
tmpfile, strerror(errno));

Re: [PATCH] sha1_file: make sure correct error is propagated

[Junio C Hamano]

Sam Vilain <sam@vilain.net> writes:

> Subject: sha1_file: accept EACCESS as equivalent to EPERM
>
> This was testing for 'Operation not permitted' rather than any kind
> of 'Permission Denied' error; prefer EACCESS.
>     
> Signed-off-by: Sam Vilain <sam@vilain.net>
> --
>   Sorry for the inevitable wrapping/whitespace fail :(
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 7662330..cd422e6 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2231,7 +2231,7 @@ static int create_tmpfile(char *buffer, size_t
> bufsiz, const char *filename)
>  	memcpy(buffer, filename, dirlen);
>  	strcpy(buffer + dirlen, "tmp_obj_XXXXXX");
>  	fd = mkstemp(buffer);
> -	if (fd < 0 && dirlen && (errno != EPERM)) {
> +	if (fd < 0 && dirlen && (errno != EACCESS)) {

Is this accepting the two as equivalents???

Re: [PATCH] sha1_file: make sure correct error is propagated

[Francis Galiegue]

Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit :
[...]
> >  	fd = mkstemp(buffer);
> > -	if (fd < 0 && dirlen && (errno != EPERM)) {
> > +	if (fd < 0 && dirlen && (errno != EACCESS)) {
>
> Is this accepting the two as equivalents???
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Well, looking at mkdir(2), it says:

       EPERM  The file system containing pathname does not support the 
creation of directories.

Hmm, err... git would fail at an earlier point anyway, wouldn't it? Even git 
init would fail there.

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH] sha1_file: make sure correct error is propagated

[Andreas Ericsson]

Francis Galiegue wrote:
> Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit :
> [...]
>>>  	fd = mkstemp(buffer);
>>> -	if (fd < 0 && dirlen && (errno != EPERM)) {
>>> +	if (fd < 0 && dirlen && (errno != EACCESS)) {
>> Is this accepting the two as equivalents???
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Well, looking at mkdir(2), it says:
> 
>        EPERM  The file system containing pathname does not support the 
> creation of directories.
> 
> Hmm, err... git would fail at an earlier point anyway, wouldn't it? Even git 
> init would fail there.
> 

Not necessarily. .git could be mounted erroneously from via a networked
filesystem but without write permissions. Yes, other things would fail
then too, but both EPERM and EACCESS are valid and possible return codes.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] sha1_file: make sure correct error is propagated

[Francis Galiegue]

Le Friday 14 November 2008 20:50:09 Andreas Ericsson, vous avez écrit :
> Francis Galiegue wrote:
> > Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit :
> > [...]
> >
> >>>  	fd = mkstemp(buffer);
> >>> -	if (fd < 0 && dirlen && (errno != EPERM)) {
> >>> +	if (fd < 0 && dirlen && (errno != EACCESS)) {
> >>
> >> Is this accepting the two as equivalents???
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe git" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> > Well, looking at mkdir(2), it says:
> >
> >        EPERM  The file system containing pathname does not support the
> > creation of directories.
> >
> > Hmm, err... git would fail at an earlier point anyway, wouldn't it? Even
> > git init would fail there.
>
> Not necessarily. .git could be mounted erroneously from via a networked
> filesystem but without write permissions. 

In which case EACCESS would be returned anyway. There is quite a difference 
between EACCESS (Permission denied) and EPERM (operation not permitted).

Basically, my understanding is that mkdir() will only return EPERM if the 
underlying filesystem can not even CREATE directories on the filesystem. So, 
unless you are doing very bizarre things with your git repository, I cannot 
see how you can even trigger an EPERM unless you asked for it.

> Yes, other things would fail 
> then too, but both EPERM and EACCESS are valid and possible return codes.

And so is ENOSPC, and so is EIO, and so is... It's endless. I think focus 
should be made on the most common ones, and EACCESS _is_ such one. Others 
just aren't.

This is why I suggested replacing EPERM with EACCESS in the first place: 
EACCESS is by far the most common error code you will get (even root will get 
that on a read-only filesystem, not EPERM).

-- 
fge

Re: [PATCH] sha1_file: make sure correct error is propagated

[Junio C Hamano]

Francis Galiegue <fg@one2team.com> writes:

> Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit :
> [...]
>> >  	fd = mkstemp(buffer);
>> > -	if (fd < 0 && dirlen && (errno != EPERM)) {
>> > +	if (fd < 0 && dirlen && (errno != EACCESS)) {
>>
>> Is this accepting the two as equivalents???
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Well, looking at mkdir(2), it says:
>
>        EPERM  The file system containing pathname does not support the 
> creation of directories.
>
> Hmm, err... git would fail at an earlier point anyway, wouldn't it? Even git 
> init would fail there.

Actually, POSIX does not even talk about EPERM for mkdir(2), but that was
not my point.  The code does something different from what the proposed
commit log message talks about.  That was what bothered me.

Re: [PATCH] sha1_file: make sure correct error is propagated

[Sam Vilain]

On Fri, 2008-11-14 at 21:44 -0800, Junio C Hamano wrote:
> Actually, POSIX does not even talk about EPERM for mkdir(2), but that was
> not my point.  The code does something different from what the proposed
> commit log message talks about.  That was what bothered me.

My wording was a little terse and confusing.  Here's a new one;

Subject: sha1_file.c: resolve confusion EACCESS vs EPERM

EPERM or 'Operation not permitted' is an unlikely error from
mkstemp(); test for EACCESS 'Access Denied' instead.  Make the
special branch which prints the error to the user nicely also
understand EACCESS.