threads / patch / 16324

patchsha1_file: make sure correct error is propagated

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

## tl;dr

9 messages between Nov 14, 2008 and Nov 15, 2008. Diffs are folded; open one to read it.

replies: 8people: 6as markdown or json

Sam Vilain· Nov 14, 2008, 07:19 UTC · lore
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(-)
Show changes to sha1_file.c +1 −1
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
Francis Galiegue· Nov 14, 2008, 07:44 UTC · re: Sam Vilain · lore

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

Le vendredi 14 novembre 2008, Sam Vilain a écrit :
Show 10 quoted lines
> 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
Sam Vilain· Nov 14, 2008, 09:41 UTC · re: Francis Galiegue · lore

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

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 :(
Show changes to sha1_file.c +2 −2
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));
Junio C Hamano· Nov 14, 2008, 19:05 UTC · re: Sam Vilain · lore

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

Sam Vilain <sam@vilain.net> writes:
Show 20 quoted lines
> 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???
Francis Galiegue· Nov 14, 2008, 19:09 UTC · re: Junio C Hamano · lore

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

Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit : [...]

Show 9 quoted lines
> >  	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
Andreas Ericsson· Nov 14, 2008, 19:50 UTC · re: Francis Galiegue · lore

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

Francis Galiegue wrote:
Show 19 quoted lines
> 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
Francis Galiegue· Nov 14, 2008, 20:08 UTC · re: Andreas Ericsson · lore

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

Le Friday 14 November 2008 20:50:09 Andreas Ericsson, vous avez écrit :
Show 24 quoted lines
> 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
Junio C Hamano· Nov 15, 2008, 05:44 UTC · re: Francis Galiegue · lore

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

Francis Galiegue <fg@one2team.com> writes:
Show 19 quoted lines
> 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.

Sam Vilain· Nov 15, 2008, 06:30 UTC · re: Junio C Hamano · lore

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

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.

← back to recent threads