# [PATCH 2/2] Use a strbuf for building up section header and key/value pair strings. 8 messages from 2007-12-14 to 2007-12-14. Participants: Kristian Høgsberg, Johannes Schindelin, Johannes Sixt, Junio C Hamano. Thread: https://gitlist.dev/t/11293 ## Kristian Høgsberg, 2007-12-14 19:22 Subject: config.c fixes Message-ID: <1197660157-24109-1-git-send-email-krh@redhat.com> URL: https://gitlist.dev/e/1197660157-24109-1-git-send-email-krh%40redhat.com ``` 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 ``` ## Kristian Høgsberg, 2007-12-14 19:22 Subject: [PATCH 1/2] Fix config lockfile handling. Message-ID: <1197660157-24109-2-git-send-email-krh@redhat.com> URL: https://gitlist.dev/e/1197660157-24109-2-git-send-email-krh%40redhat.com In-Reply-To: <1197660157-24109-1-git-send-email-krh@redhat.com> ``` 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 --- 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 ``` ## Kristian Høgsberg, 2007-12-14 19:22 Subject: [PATCH 2/2] Use a strbuf for building up section header and key/value pair strings. Message-ID: <1197660157-24109-3-git-send-email-krh@redhat.com> URL: https://gitlist.dev/e/1197660157-24109-3-git-send-email-krh%40redhat.com In-Reply-To: <1197660157-24109-2-git-send-email-krh@redhat.com> ``` Avoids horrible 1-byte write(2) calls and cleans up the logic a bit. Signed-off-by: Kristian Høgsberg --- 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 ``` ## Johannes Schindelin, 2007-12-14 19:29 Subject: Re: [PATCH 1/2] Fix config lockfile handling. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0712141928240.27959%40racer.site In-Reply-To: <1197660157-24109-2-git-send-email-krh@redhat.com> ``` 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 ``` ## Johannes Sixt, 2007-12-14 19:32 Subject: Re: [PATCH 1/2] Fix config lockfile handling. Message-ID: <20071214193216.13C795A873@dx.sixt.local> URL: https://gitlist.dev/e/20071214193216.13C795A873%40dx.sixt.local In-Reply-To: <1197660157-24109-2-git-send-email-krh@redhat.com> ``` 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 ``` ## Junio C Hamano, 2007-12-14 20:07 Subject: Re: [PATCH 1/2] Fix config lockfile handling. Message-ID: <7vmysdqbui.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmysdqbui.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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. ``` ## Johannes Schindelin, 2007-12-14 20:15 Subject: Re: [PATCH 1/2] Fix config lockfile handling. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0712142015240.27959%40racer.site In-Reply-To: <7vmysdqbui.fsf@gitster.siamese.dyndns.org> ``` Hi, On Fri, 14 Dec 2007, Junio C Hamano wrote: > Johannes Schindelin 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 ``` ## Junio C Hamano, 2007-12-14 21:18 Subject: Re: [PATCH 1/2] Fix config lockfile handling. Message-ID: <7vwsrhotzt.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwsrhotzt.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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. ```