use result of open(2) to check for presence

use result of open(2) to check for presence

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 211 bytes --]

Not that the stat against open race would matter much in this context,
but that simplifies
the code a bit. Also some diagnostics added (why the open failed)

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

[-- Attachment #2: 0001-use-result-of-open-2-to-check-for-presence-of-the-old-config-file.txt --]
[-- Type: text/plain, Size: 1713 bytes --]

Subject: [PATCH] use result of open(2) to check for presence

of the old config file

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>


---

 config.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

b74ea5a71504808112bd4781a69b17f6009104db
diff --git a/config.c b/config.c
index 992e988..8355224 100644
--- a/config.c
+++ b/config.c
@@ -409,8 +409,7 @@ int git_config_set_multivar(const char* 
 	const char* value_regex, int multi_replace)
 {
 	int i;
-	struct stat st;
-	int fd;
+	int fd, in_fd;
 	char* config_filename = strdup(git_path("config"));
 	char* lock_file = strdup(git_path("config.lock"));
 	const char* last_dot = strrchr(key, '.');
@@ -457,9 +456,17 @@ int git_config_set_multivar(const char* 
 	/*
 	 * If .git/config does not exist yet, write a minimal version.
 	 */
-	if (stat(config_filename, &st)) {
+	in_fd = open(config_filename, O_RDONLY);
+	if ( in_fd < 0 ) {
 		free(store.key);
 
+		if ( ENOENT != errno ) {
+			error("opening %s: %s", config_filename,
+			      strerror(errno));
+			close(fd);
+			unlink(lock_file);
+			return 3; /* same as "invalid config file" */
+		}
 		/* if nothing to unset, error out */
 		if (value == NULL) {
 			close(fd);
@@ -471,7 +478,7 @@ int git_config_set_multivar(const char* 
 		store_write_section(fd, key);
 		store_write_pair(fd, key, value);
 	} else{
-		int in_fd;
+		struct stat st;
 		char* contents;
 		int i, copy_begin, copy_end, new_line = 0;
 
@@ -528,7 +535,7 @@ int git_config_set_multivar(const char* 
 			return 5;
 		}
 
-		in_fd = open(config_filename, O_RDONLY, 0666);
+		fstat(in_fd, &st);
 		contents = mmap(NULL, st.st_size, PROT_READ,
 			MAP_PRIVATE, in_fd, 0);
 		close(in_fd);
-- 
1.0.GIT

Re: use result of open(2) to check for presence

[Morten Welinder]

Neat, but perhaps you should check the fstat return code.
It actually can fail, for example if the filesize cannot be represented in the
structure because it is too big.

Morten

Re: use result of open(2) to check for presence

[Alex Riesen]

On 1/5/06, Morten Welinder <mwelinder@gmail.com> wrote:
> Neat, but perhaps you should check the fstat return code.
> It actually can fail, for example if the filesize cannot be represented in the
> structure because it is too big.

Yes, of course. It will be added along with the missing test for mmap
returning MAP_FAILED. Sometime.

Sarcasm aside, how do you get such a situation?

Re: use result of open(2) to check for presence

[Morten Welinder]

Config files always tend to grow, but I do agree that 4GB config files stretches
credibility a bit.

However, empty ones are not unlikely and then mmap will fail.

M.

Re: use result of open(2) to check for presence

[Alex Riesen]

Morten Welinder, Thu, Jan 05, 2006 17:45:27 +0100:
> Config files always tend to grow, but I do agree that 4GB config files stretches
> credibility a bit.
> 
> However, empty ones are not unlikely and then mmap will fail.
> 

Shouldn't be a problem: the code in question does not seem to do
anything for empty files and wont even touch the result of mmap.