Re: [PATCH] mmap error handling

[Pavel Roskin <proski@gnu.org>]

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 <proski@gnu.org>

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