{"thread":{"id":"11297","subject":"[PATCH] Fix config lockfile handling.","startedAt":"2007-12-14T20:59:56Z","lastAt":"2007-12-17T16:24:43Z","messageCount":5,"participants":["Kristian Høgsberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"63201","messageId":"1197665998-32386-1-git-send-email-krh@redhat.com","threadId":"11297","inReplyTo":null,"subject":"config.c fixes, take 2","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-14T20:59:56Z","receivedAt":"2007-12-14T20:59:56Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"Hi,\n\nHere's a follow-up series that allocates the lock_file on the heap.\nAlso, git_config_rename_section() did an extra close too so I added\na fix for that in this series.\n\ncheers,\nKristian\n"},{"id":"63200","messageId":"1197665998-32386-2-git-send-email-krh@redhat.com","threadId":"11297","inReplyTo":"1197665998-32386-1-git-send-email-krh@redhat.com","subject":"[PATCH] Fix config lockfile handling.","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-14T20:59:57Z","receivedAt":"2007-12-14T20:59:57Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"When we commit or roll back the lock file the fd is automatically closed,\nso don't do that again.\n\nSigned-off-by: Kristian Høgsberg \n---\n config.c | 17 +++--------------\n 1 files changed, 3 insertions(+), 14 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 49d2b42..2e52b17 100644\n--- a/config.c\n+++ b/config.c\n@@ -751,7 +751,7 @@ int git_config_set_multivar(const char* key, const char* value,\n \tconst char* value_regex, int multi_replace)\n {\n \tint i, dot;\n-\tint fd = -1, in_fd;\n+\tint fd, in_fd;\n \tint ret;\n \tchar* config_filename;\n \tstruct lock_file *lock = NULL;\n@@ -955,26 +955,15 @@ int git_config_set_multivar(const char* key, const char* value,\n \t\tmunmap(contents, contents_sz);\n \t}\n \n-\tif (close(fd) || commit_lock_file(lock) < 0) {\n+\tif (commit_lock_file(lock) < 0) {\n \t\tfprintf(stderr, \"Cannot commit config file!\\n\");\n \t\tret = 4;\n \t\tgoto out_free;\n \t}\n \n-\t/* fd is closed, so don't try to close it below. */\n-\tfd = -1;\n-\t/*\n-\t * lock is committed, so don't try to roll it back below.\n-\t * NOTE: Since lockfile.c keeps a linked list of all created\n-\t * lock_file structures, it isn't safe to free(lock). It's\n-\t * better to just leave it hanging around.\n-\t */\n-\tlock = NULL;\n \tret = 0;\n \n out_free:\n-\tif (0 <= fd)\n-\t\tclose(fd);\n \tif (lock)\n \t\trollback_lock_file(lock);\n \tfree(config_filename);\n@@ -1072,7 +1061,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)\n \t}\n \tfclose(config_file);\n unlock_and_out:\n-\tif (close(out_fd) || commit_lock_file(lock) < 0)\n+\tif (commit_lock_file(lock) < 0)\n \t\t\tret = error(\"Cannot commit config file!\");\n out:\n \tfree(config_filename);\n-- \n1.5.3.4\n"},{"id":"63204","messageId":"1197665998-32386-3-git-send-email-krh@redhat.com","threadId":"11297","inReplyTo":"1197665998-32386-2-git-send-email-krh@redhat.com","subject":"[PATCH] Use a strbuf for building up section header and key/value pair strings.","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-14T20:59:58Z","receivedAt":"2007-12-14T20:59:58Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"Avoids horrible 1-byte write(2) calls and cleans up the logic a bit.\n\nSigned-off-by: Kristian Høgsberg \n---\n config.c | 91 ++++++++++++++++++++++++++------------------------------------\n 1 files changed, 38 insertions(+), 53 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2e52b17..4c8e15d 100644\n--- a/config.c\n+++ b/config.c\n@@ -610,46 +610,36 @@ static int write_error(void)\n \n static int store_write_section(int fd, const char* key)\n {\n-\tconst char *dot = strchr(key, '.');\n-\tint len1 = store.baselen, len2 = -1;\n+\tconst char *dot;\n+\tint i, success;\n+\tstruct strbuf sb;\n \n-\tdot = strchr(key, '.');\n+\tstrbuf_init(&sb, 0);\n+\tdot = memchr(key, '.', store.baselen);\n \tif (dot) {\n-\t\tint dotlen = dot - key;\n-\t\tif (dotlen < len1) {\n-\t\t\tlen2 = len1 - dotlen - 1;\n-\t\t\tlen1 = dotlen;\n+\t\tstrbuf_addf(&sb, \"[%.*s \\\"\", dot - key, key);\n+\t\tfor (i = dot - key + 1; i < store.baselen; i++) {\n+\t\t\tif (key[i] == '\"')\n+\t\t\t\tstrbuf_addch(&sb, '\\\\');\n+\t\t\tstrbuf_addch(&sb, key[i]);\n \t\t}\n+\t\tstrbuf_addstr(&sb, \"\\\"]\\n\");\n+\t} else {\n+\t\tstrbuf_addf(&sb, \"[%.*s]\\n\", store.baselen, key);\n \t}\n \n-\tif (write_in_full(fd, \"[\", 1) != 1 ||\n-\t write_in_full(fd, key, len1) != len1)\n-\t\treturn 0;\n-\tif (len2 >= 0) {\n-\t\tif (write_in_full(fd, \" \\\"\", 2) != 2)\n-\t\t\treturn 0;\n-\t\twhile (--len2 >= 0) {\n-\t\t\tunsigned char c = *++dot;\n-\t\t\tif (c == '\"')\n-\t\t\t\tif (write_in_full(fd, \"\\\\\", 1) != 1)\n-\t\t\t\t\treturn 0;\n-\t\t\tif (write_in_full(fd, &c, 1) != 1)\n-\t\t\t\treturn 0;\n-\t\t}\n-\t\tif (write_in_full(fd, \"\\\"\", 1) != 1)\n-\t\t\treturn 0;\n-\t}\n-\tif (write_in_full(fd, \"]\\n\", 2) != 2)\n-\t\treturn 0;\n+\tsuccess = write_in_full(fd, sb.buf, sb.len) == sb.len;\n+\tstrbuf_release(&sb);\n \n-\treturn 1;\n+\treturn success;\n }\n \n static int store_write_pair(int fd, const char* key, const char* value)\n {\n-\tint i;\n-\tint length = strlen(key+store.baselen+1);\n-\tint quote = 0;\n+\tint i, success;\n+\tint length = strlen(key + store.baselen + 1);\n+\tconst char *quote = \"\";\n+\tstruct strbuf sb;\n \n \t/*\n \t * Check to see if the value needs to be surrounded with a dq pair.\n@@ -659,43 +649,38 @@ static int store_write_pair(int fd, const char* key, const char* value)\n \t * configuration parser.\n \t */\n \tif (value[0] == ' ')\n-\t\tquote = 1;\n+\t\tquote = \"\\\"\";\n \tfor (i = 0; value[i]; i++)\n \t\tif (value[i] == ';' || value[i] == '#')\n-\t\t\tquote = 1;\n-\tif (i && value[i-1] == ' ')\n-\t\tquote = 1;\n+\t\t\tquote = \"\\\"\";\n+\tif (i && value[i - 1] == ' ')\n+\t\tquote = \"\\\"\";\n+\n+\tstrbuf_init(&sb, 0);\n+\tstrbuf_addf(&sb, \"\\t%.*s = %s\", \n+\t\t length, key + store.baselen + 1, quote);\n \n-\tif (write_in_full(fd, \"\\t\", 1) != 1 ||\n-\t write_in_full(fd, key+store.baselen+1, length) != length ||\n-\t write_in_full(fd, \" = \", 3) != 3)\n-\t\treturn 0;\n-\tif (quote && write_in_full(fd, \"\\\"\", 1) != 1)\n-\t\treturn 0;\n \tfor (i = 0; value[i]; i++)\n \t\tswitch (value[i]) {\n \t\tcase '\\n':\n-\t\t\tif (write_in_full(fd, \"\\\\n\", 2) != 2)\n-\t\t\t\treturn 0;\n+\t\t\tstrbuf_addstr(&sb, \"\\\\n\");\n \t\t\tbreak;\n \t\tcase '\\t':\n-\t\t\tif (write_in_full(fd, \"\\\\t\", 2) != 2)\n-\t\t\t\treturn 0;\n+\t\t\tstrbuf_addstr(&sb, \"\\\\t\");\n \t\t\tbreak;\n \t\tcase '\"':\n \t\tcase '\\\\':\n-\t\t\tif (write_in_full(fd, \"\\\\\", 1) != 1)\n-\t\t\t\treturn 0;\n+\t\t\tstrbuf_addch(&sb, '\\\\');\n \t\tdefault:\n-\t\t\tif (write_in_full(fd, value+i, 1) != 1)\n-\t\t\t\treturn 0;\n+\t\t\tstrbuf_addch(&sb, value[i]);\n \t\t\tbreak;\n \t\t}\n-\tif (quote && write_in_full(fd, \"\\\"\", 1) != 1)\n-\t\treturn 0;\n-\tif (write_in_full(fd, \"\\n\", 1) != 1)\n-\t\treturn 0;\n-\treturn 1;\n+\tstrbuf_addf(&sb, \"%s\\n\", quote);\n+\n+\tsuccess = write_in_full(fd, sb.buf, sb.len) == sb.len;\n+\tstrbuf_release(&sb);\n+\n+\treturn success;\n }\n \n static ssize_t find_beginning_of_line(const char* contents, size_t size,\n-- \n1.5.3.4\n"},{"id":"63213","messageId":"7vfxy5os60.fsf@gitster.siamese.dyndns.org","threadId":"11297","inReplyTo":"1197665998-32386-2-git-send-email-krh@redhat.com","subject":"Re: [PATCH] Fix config lockfile handling.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T21:57:59Z","receivedAt":"2007-12-14T21:57:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristian Høgsberg writes:\n\n> When we commit or roll back the lock file the fd is automatically closed,\n> so don't do that again.\n\nWith your change, we do not check the return status from close(2)\nanymore, which means that we may have run out of diskspace without\nnoticing and renamed the incomplete file into the real place. Oops?\n\nAt least the original code wouldn't have had that problem.\n\nThe right fix in the longer term would be to check the return value from\nthe close(2) in commit_lock_file(), but it currently does not check on\npurpose, because the callers may have already closed the fd.\n"},{"id":"63439","messageId":"1197908683.8688.0.camel@hinata.boston.redhat.com","threadId":"11297","inReplyTo":"7vfxy5os60.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix config lockfile handling.","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-17T16:24:43Z","receivedAt":"2007-12-17T16:24:43Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"\nOn Fri, 2007-12-14 at 13:57 -0800, Junio C Hamano wrote:\n> Kristian Høgsberg writes:\n> \n> > When we commit or roll back the lock file the fd is automatically closed,\n> > so don't do that again.\n> \n> With your change, we do not check the return status from close(2)\n> anymore, which means that we may have run out of diskspace without\n> noticing and renamed the incomplete file into the real place. Oops?\n\nYou're right of course. Ok, so lets just stick with the current config\nfile handling for 1.5.4, it's not seriously broken.\n\nKristian\n"}]}