Re: [PATCH] mmap error handling

[Pavel Roskin]

Hi, Linus!

On Thu, 2005-07-28 at 17:30 -0700, Linus Torvalds wrote:
 _always_ save the value of errno before doing any other calls. Even 
 successful calls are perfectly allowed to change errno.

OK.  Fixed patch below.

Signed-off-by: Pavel Roskin [EMAIL PROTECTED]

diff --git a/diff.c b/diff.c
--- a/diff.c
+++ b/diff.c
@@ -377,8 +377,10 @@ int diff_populate_filespec(struct diff_f
if (fd  0)
goto err_empty;
s-data = mmap(NULL, s-size, PROT_READ, MAP_PRIVATE, fd, 0);
-   s-should_munmap = 1;
close(fd);
+   if (s-data == MAP_FAILED)
+   goto err_empty;
+   s-should_munmap = 1;
}
else {
char type[20];
diff --git a/diffcore-order.c b/diffcore-order.c
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -28,7 +28,7 @@ static void prepare_order(const char *or
}
map = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
close(fd);
-   if (-1 == (int)(long)map)
+   if (map == MAP_FAILED)
return;
endp = map + st.st_size;
for (pass = 0; pass  2; pass++) {
diff --git a/local-pull.c b/local-pull.c
--- a/local-pull.c
+++ b/local-pull.c
@@ -54,7 +54,7 @@ int fetch(unsigned char *sha1)
}
map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, ifd, 0);
close(ifd);
-   if (-1 == (int)(long)map) {
+   if (map == MAP_FAILED) {
fprintf(stderr, cannot mmap %s\n, filename);
return -1;
}
diff --git a/read-cache.c b/read-cache.c
--- a/read-cache.c
+++ b/read-cache.c
@@ -392,7 +392,7 @@ int read_cache(void)
return (errno == ENOENT) ? 0 : error(open failed);
 
size = 0; // avoid gcc warning
-   map = (void *)-1;
+   map = MAP_FAILED;
if (!fstat(fd, st)) {
size = st.st_size;
errno = EINVAL;
@@ -400,7 +400,7 @@ int read_cache(void)
map = mmap(NULL, size, PROT_READ | PROT_WRITE, 
MAP_PRIVATE, fd, 0);
}
close(fd);
-   if (-1 == (int)(long)map)
+   if (map == MAP_FAILED)
return error(mmap failed);
 
hdr = map;
diff --git a/rev-cache.c b/rev-cache.c
--- a/rev-cache.c
+++ b/rev-cache.c
@@ -212,11 +212,9 @@ int read_rev_cache(const char *path, FIL
return -1;
}
map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-   if (map == MAP_FAILED) {
-   close(fd);
-   return -1;
-   }
close(fd);
+   if (map == MAP_FAILED)
+   return -1;
 
memset(last_sha1, 0, 20);
ofs = 0;
diff --git a/sha1_file.c b/sha1_file.c
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -518,7 +518,7 @@ static void *map_sha1_file_internal(cons
}
map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
close(fd);
-   if (-1 == (int)(long)map)
+   if (map == MAP_FAILED)
return NULL;
*size = st.st_size;
return map;
@@ -1363,7 +1363,7 @@ int index_fd(unsigned char *sha1, int fd
if (size)
buf = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
close(fd);
-   if ((int)(long)buf == -1)
+   if (buf == MAP_FAILED)
return -1;
 
if (!type)
diff --git a/test-delta.c b/test-delta.c
--- a/test-delta.c
+++ b/test-delta.c
@@ -41,6 +41,7 @@ int main(int argc, char *argv[])
from_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (from_buf == MAP_FAILED) {
perror(argv[2]);
+   close(fd);
return 1;
}
close(fd);
@@ -54,6 +55,7 @@ int main(int argc, char *argv[])
data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (data_buf == MAP_FAILED) {
perror(argv[3]);
+   close(fd);
return 1;
}
close(fd);
diff --git a/tools/mailsplit.c b/tools/mailsplit.c
--- a/tools/mailsplit.c
+++ b/tools/mailsplit.c
@@ -116,8 +116,9 @@ int main(int argc, char **argv)
}
size = st.st_size;
map = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-   if (-1 == (int)(long)map) {
+   if (map == MAP_FAILED) {
perror(mmap);
+   close(fd);
exit(1);
}
close(fd);


-- 
Regards,
Pavel Roskin

-
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmap error handling

[Morten Welinder]

 I have verified that successful close() after failed mmap() won't reset
 the output of perror() to Success.

Does $standard guarantee that?

In general, successful libc calls can set errno to whatever they
please, except zero.  And they sometimes do.  This follows from
C99.

Morten
-
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmap error handling

[Linus Torvalds]

On Thu, 28 Jul 2005, Morten Welinder wrote:

  I have verified that successful close() after failed mmap() won't reset
  the output of perror() to Success.
 
 Does $standard guarantee that?
 
 In general, successful libc calls can set errno to whatever they
 please, except zero.  And they sometimes do.  This follows from
 C99.

Indeed. 

_always_ save the value of errno before doing any other calls. Even 
successful calls are perfectly allowed to change errno.

close() may not _normally_ change errno, but the fact is, not only can 
close sometimes return an error, but it could validly have some debugging 
wrapper that does logging, and change errno because of that.

Yeah, we'd be better off if UNIX had always used the linux kernel practice
of hiding errno as a negative return value (and the IS_ERR() test for
pointers), but hey, crud happens to the best of us.

Linus
-
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html