config.c fixes

config.c fixes

[Kristian Høgsberg]

Hi,

While strace'ing builtin-clone I saw this horror:

...
write(3, "remote", 6)                   = 6
write(3, " = ", 3)                      = 3
write(3, "o", 1)                        = 1
write(3, "r", 1)                        = 1
write(3, "i", 1)                        = 1
write(3, "g", 1)                        = 1
write(3, "i", 1)                        = 1
write(3, "n", 1)                        = 1
write(3, "\n", 1)                       = 1
munmap(0xb7f72000, 102)                 = 0
close(3)                                = 0
close(3)                                = -1 EBADF (Bad file descriptor)
...

That's just terrible.  And hey, it turns out the code is terrible too.
I think the best solution is to just parse up the entire config file
up front and keep it in a data structure, make the changes and then
write it all out at the end.  That makes it cheap to make a series of
changes too:

	config = open_config(file);
	set_key(config, key1, value1);
	set_key(config, key2, value2);
	set_key(config, key3, value3);
	commit_config(config);

Or something.  Anyway, for 1.5.4 I wrote the following two patches that
fixes the 1-bytes writes and the double close.

 config.c |  113 +++++++++++++++++++++++--------------------------------------
 1 files changed, 43 insertions(+), 70 deletions(-)

cheers,
Kristian

[PATCH 1/2] Fix config lockfile handling.

[Kristian Høgsberg]

When we commit or roll back the lock file the fd is automatically closed,
so don't do that again.  Also, just keep the lock on the stack.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 config.c |   22 +++++-----------------
 1 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/config.c b/config.c
index 49d2b42..0725563 100644
--- a/config.c
+++ b/config.c
@@ -754,7 +754,7 @@ int git_config_set_multivar(const char* key, const char* value,
 	int fd = -1, in_fd;
 	int ret;
 	char* config_filename;
-	struct lock_file *lock = NULL;
+	struct lock_file lock;
 	const char* last_dot = strrchr(key, '.');
 
 	config_filename = getenv(CONFIG_ENVIRONMENT);
@@ -811,8 +811,7 @@ int git_config_set_multivar(const char* key, const char* value,
 	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	lock = xcalloc(sizeof(struct lock_file), 1);
-	fd = hold_lock_file_for_update(lock, config_filename, 0);
+	fd = hold_lock_file_for_update(&lock, config_filename, 0);
 	if (fd < 0) {
 		fprintf(stderr, "could not lock config file\n");
 		free(store.key);
@@ -955,28 +954,17 @@ int git_config_set_multivar(const char* key, const char* value,
 		munmap(contents, contents_sz);
 	}
 
-	if (close(fd) || commit_lock_file(lock) < 0) {
+	if (commit_lock_file(&lock) < 0) {
 		fprintf(stderr, "Cannot commit config file!\n");
 		ret = 4;
 		goto out_free;
 	}
 
-	/* fd is closed, so don't try to close it below. */
-	fd = -1;
-	/*
-	 * lock is committed, so don't try to roll it back below.
-	 * NOTE: Since lockfile.c keeps a linked list of all created
-	 * lock_file structures, it isn't safe to free(lock).  It's
-	 * better to just leave it hanging around.
-	 */
-	lock = NULL;
 	ret = 0;
 
 out_free:
-	if (0 <= fd)
-		close(fd);
-	if (lock)
-		rollback_lock_file(lock);
+	/* If we already committed the lock file, the rollback is a no-op. */
+	rollback_lock_file(&lock);
 	free(config_filename);
 	return ret;
 
-- 
1.5.3.4

[PATCH 2/2] Use a strbuf for building up section header and key/value pair strings.

[Kristian Høgsberg]

Avoids horrible 1-byte write(2) calls and cleans up the logic a bit.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 config.c |   91 ++++++++++++++++++++++++++------------------------------------
 1 files changed, 38 insertions(+), 53 deletions(-)

diff --git a/config.c b/config.c
index 0725563..3392bc4 100644
--- a/config.c
+++ b/config.c
@@ -610,46 +610,36 @@ static int write_error(void)
 
 static int store_write_section(int fd, const char* key)
 {
-	const char *dot = strchr(key, '.');
-	int len1 = store.baselen, len2 = -1;
+	const char *dot;
+	int i, success;
+	struct strbuf sb;
 
-	dot = strchr(key, '.');
+	strbuf_init(&sb, 0);
+	dot = memchr(key, '.', store.baselen);
 	if (dot) {
-		int dotlen = dot - key;
-		if (dotlen < len1) {
-			len2 = len1 - dotlen - 1;
-			len1 = dotlen;
+		strbuf_addf(&sb, "[%.*s \"", dot - key, key);
+		for (i = dot - key + 1; i < store.baselen; i++) {
+			if (key[i] == '"')
+				strbuf_addch(&sb, '\\');
+			strbuf_addch(&sb, key[i]);
 		}
+		strbuf_addstr(&sb, "\"]\n");
+	} else {
+		strbuf_addf(&sb, "[%.*s]\n", store.baselen, key);
 	}
 
-	if (write_in_full(fd, "[", 1) != 1 ||
-	    write_in_full(fd, key, len1) != len1)
-		return 0;
-	if (len2 >= 0) {
-		if (write_in_full(fd, " \"", 2) != 2)
-			return 0;
-		while (--len2 >= 0) {
-			unsigned char c = *++dot;
-			if (c == '"')
-				if (write_in_full(fd, "\\", 1) != 1)
-					return 0;
-			if (write_in_full(fd, &c, 1) != 1)
-				return 0;
-		}
-		if (write_in_full(fd, "\"", 1) != 1)
-			return 0;
-	}
-	if (write_in_full(fd, "]\n", 2) != 2)
-		return 0;
+	success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+	strbuf_release(&sb);
 
-	return 1;
+	return success;
 }
 
 static int store_write_pair(int fd, const char* key, const char* value)
 {
-	int i;
-	int length = strlen(key+store.baselen+1);
-	int quote = 0;
+	int i, success;
+	int length = strlen(key + store.baselen + 1);
+	const char *quote = "";
+	struct strbuf sb;
 
 	/*
 	 * Check to see if the value needs to be surrounded with a dq pair.
@@ -659,43 +649,38 @@ static int store_write_pair(int fd, const char* key, const char* value)
 	 * configuration parser.
 	 */
 	if (value[0] == ' ')
-		quote = 1;
+		quote = "\"";
 	for (i = 0; value[i]; i++)
 		if (value[i] == ';' || value[i] == '#')
-			quote = 1;
-	if (i && value[i-1] == ' ')
-		quote = 1;
+			quote = "\"";
+	if (i && value[i - 1] == ' ')
+		quote = "\"";
+
+	strbuf_init(&sb, 0);
+	strbuf_addf(&sb, "\t%.*s = %s", 
+		    length, key + store.baselen + 1, quote);
 
-	if (write_in_full(fd, "\t", 1) != 1 ||
-	    write_in_full(fd, key+store.baselen+1, length) != length ||
-	    write_in_full(fd, " = ", 3) != 3)
-		return 0;
-	if (quote && write_in_full(fd, "\"", 1) != 1)
-		return 0;
 	for (i = 0; value[i]; i++)
 		switch (value[i]) {
 		case '\n':
-			if (write_in_full(fd, "\\n", 2) != 2)
-				return 0;
+			strbuf_addstr(&sb, "\\n");
 			break;
 		case '\t':
-			if (write_in_full(fd, "\\t", 2) != 2)
-				return 0;
+			strbuf_addstr(&sb, "\\t");
 			break;
 		case '"':
 		case '\\':
-			if (write_in_full(fd, "\\", 1) != 1)
-				return 0;
+			strbuf_addch(&sb, '\\');
 		default:
-			if (write_in_full(fd, value+i, 1) != 1)
-				return 0;
+			strbuf_addch(&sb, value[i]);
 			break;
 		}
-	if (quote && write_in_full(fd, "\"", 1) != 1)
-		return 0;
-	if (write_in_full(fd, "\n", 1) != 1)
-		return 0;
-	return 1;
+	strbuf_addf(&sb, "%s\n", quote);
+
+	success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+	strbuf_release(&sb);
+
+	return success;
 }
 
 static ssize_t find_beginning_of_line(const char* contents, size_t size,
-- 
1.5.3.4

config.c fixes

[Kristian Høgsberg]

Hi,

[ This should have gone out as a cover letter for the patch series
  but some how I didn't manage to make that work ]

While strace'ing builtin-clone I saw this horror:

...
write(3, "remote", 6)                   = 6
write(3, " = ", 3)                      = 3
write(3, "o", 1)                        = 1
write(3, "r", 1)                        = 1
write(3, "i", 1)                        = 1
write(3, "g", 1)                        = 1
write(3, "i", 1)                        = 1
write(3, "n", 1)                        = 1
write(3, "\n", 1)                       = 1
munmap(0xb7f72000, 102)                 = 0
close(3)                                = 0
close(3)                                = -1 EBADF (Bad file descriptor)
...

That's just terrible.  And hey, it turns out the code is terrible too.
I think the best solution is to just parse up the entire config file
up front and keep it in a data structure, make the changes and then
write it all out at the end.  That makes it cheap to make a series of
changes too:

	config = open_config(file);
	set_key(config, key1, value1);
	set_key(config, key2, value2);
	set_key(config, key3, value3);
	commit_config(config);

Or something.  Anyway, for 1.5.4 I wrote the following two patches that
fixes the 1-bytes writes and the double close.

 config.c |  113 +++++++++++++++++++++++--------------------------------------
 1 files changed, 43 insertions(+), 70 deletions(-)

cheers,
Kristian

Re: [PATCH 1/2] Fix config lockfile handling.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 240 bytes --]

Hi,

On Fri, 14 Dec 2007, Kristian Høgsberg wrote:

> -	struct lock_file *lock = NULL;
> +	struct lock_file lock;

AFAICT this cannot work.  At least not reliably.  An atexit() handler will 
access all (even closed) lockfiles.

Ciao,
Dscho

Re: [PATCH 1/2] Fix config lockfile handling.

[Johannes Sixt]

Kristian Høgsberg wrote:

> When we commit or roll back the lock file the fd is automatically closed,
> so don't do that again.  Also, just keep the lock on the stack.

IIRC, locks are accessed from atexit(), e.g. during a die(). So you must not
put one on the stack.

-- Hannes

Re: config.c fixes

[Junio C Hamano]

Kristian Høgsberg <krh@redhat.com> writes:

> While strace'ing builtin-clone I saw this horror:
> I think the best solution is to just parse up the entire config file
> up front and keep it in a data structure, make the changes and then
> write it all out at the end.

Yeah, that was what I suggested a few times when other people have done
config writing side, but without successfully getting past their skulls
(it is not Linus's nor my code).  It's about time somebody started to
clean up that mess.

The timing is a bit unfortunate, though.  I would have preferred to have
a week or so to cook this in 'next' before merging it part of -rc0.

Re: [PATCH 1/2] Fix config lockfile handling.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Fri, 14 Dec 2007, Kristian Høgsberg wrote:
>
>> -	struct lock_file *lock = NULL;
>> +	struct lock_file lock;
>
> AFAICT this cannot work.  At least not reliably.  An atexit() handler will 
> access all (even closed) lockfiles.

Correct.  It cannot be on the stack.

Re: [PATCH 1/2] Fix config lockfile handling.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 481 bytes --]

Hi,

On Fri, 14 Dec 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Fri, 14 Dec 2007, Kristian Høgsberg wrote:
> >
> >> -	struct lock_file *lock = NULL;
> >> +	struct lock_file lock;
> >
> > AFAICT this cannot work.  At least not reliably.  An atexit() handler 
> > will access all (even closed) lockfiles.
> 
> Correct.  It cannot be on the stack.

Note that this behaviour will be another obstacle to libification.

Ciao,
Dscho

Re: config.c fixes

[Kristian Høgsberg]

On Fri, 2007-12-14 at 11:43 -0800, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
> 
> > While strace'ing builtin-clone I saw this horror:
> > I think the best solution is to just parse up the entire config file
> > up front and keep it in a data structure, make the changes and then
> > write it all out at the end.
> 
> Yeah, that was what I suggested a few times when other people have done
> config writing side, but without successfully getting past their skulls
> (it is not Linus's nor my code).  It's about time somebody started to
> clean up that mess.
> 
> The timing is a bit unfortunate, though.  I would have preferred to have
> a week or so to cook this in 'next' before merging it part of -rc0.

Right, what I was describing above was more of a long term thing.  I
sent two patches more suitable for 1.5.4 that fixes the double close and
the 1-byte writes in a less intrusive way.  Except as the Johanneses
point out, I can't use the lock as a local variable, but must allocate
so the atexit handler doesn't break.  If it has to be allocated, the API
should help/enforce that.  I've sent out a couple of new patches that
fixes this.

Kristian

Re: [PATCH 1/2] Fix config lockfile handling.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > AFAICT this cannot work.  At least not reliably.  An atexit() handler 
>> > will access all (even closed) lockfiles.
>> 
>> Correct.  It cannot be on the stack.
>
> Note that this behaviour will be another obstacle to libification.

By your definition of "obstacle", there is no work at all in
libification if the system is obstacle free.

Libification is all about removing run-once-and-exit and atexit() is
just a part of it.

I think we can do this step-by-step by first introducing a new function
"get_lockfile()" that takes a list of active lockfiles (perhaps that
would be a part of "client context" thing in the library) and gives back
a lockfile structure allocated on heap, registers it to the list of
lockfiles that need to be eventually cleaned up, and another function
"rollback_lockfiles()" that take the list of lockfiles in the "client
context" and rolls them all back.  Once there is such a "client contex",
in the current unlibified "main" routines can then declare a global
client context, obtain and use lockfiles for that context, and directly
call rollback_lockfiles() from the atexit() handler.

But that is all post 1.5.4.