{"thread":{"id":"17701","subject":"[PATCH 1/2] code cleanup in sha1_file.c: xmalloc -> xcalloc","startedAt":"2009-02-10T15:29:59Z","lastAt":"2009-02-11T06:43:32Z","messageCount":5,"participants":["Guanqun Lu","Junio C Hamano","Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"104024","messageId":"7v63jipax4.fsf@gitster.siamese.dyndns.org","threadId":"17701","inReplyTo":"1234334612-24907-2-git-send-email-guanqun.lu@gmail.com","subject":"Re: [PATCH 2/2] lockfile: set lk->fd = -1 in 'rollback_lock_file()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-10T15:29:59Z","receivedAt":"2009-02-10T15:29:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Guanqun Lu <guanqun.lu@gmail.com> writes:\n\n> Post-conditions when these functions return successfully:\n>                     lk->fd == -1?    lk->filename[0] == '\\0'?\n> close_lock_file()       yes                 no\n> commit_lock_file()      yes                 yes\n> rollback_lock_file()    no*                 yes\n>\n> [*] This commit changes this 'no' in rollback_lock_file() to 'yes',\n> which achieves more robust and unified interface.\n\nIs there a broken caller this patch fixes?  IOW, is there a codepath that\ncan call rollback and then later notices lk->fd is not negative and uses\nit without checking lk->filename[0] first?\n\n> Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>\n> ---\n>  lockfile.c |    4 +++-\n>  1 files changed, 3 insertions(+), 1 deletions(-)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index 021c337..44e5253 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -243,8 +243,10 @@ int commit_locked_index(struct lock_file *lk)\n>  void rollback_lock_file(struct lock_file *lk)\n>  {\n>  \tif (lk->filename[0]) {\n> -\t\tif (lk->fd >= 0)\n> +\t\tif (lk->fd >= 0) {\n>  \t\t\tclose(lk->fd);\n> +\t\t\tlk->fd = -1;\n> +\t\t}\n>  \t\tunlink(lk->filename);\n>  \t}\n>  \tlk->filename[0] = 0;\n> -- \n> 1.6.1.2.392.gb04d1\n"},{"id":"104033","messageId":"EohcOnsg9EGsisIaM0B0iYUSHt-mz_fzqBCqg27mBU0yXXlkP9baOA@cipher.nrlssc.navy.mil","threadId":"17701","inReplyTo":"1234334612-24907-1-git-send-email-guanqun.lu@gmail.com","subject":"Re: [PATCH 1/2] code cleanup in sha1_file.c: xmalloc -> xcalloc","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-02-10T16:29:14Z","receivedAt":"2009-02-10T16:29:14Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Guanqun Lu wrote:\n> 'xmalloc' followed immediately by 'memset' is replaced\n> with 'xcalloc', and a simple grep in this project seems\n> to show that it's the only place.\n> \n> Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>\n> ---\n>  sha1_file.c |    3 +--\n>  1 files changed, 1 insertions(+), 2 deletions(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 8868b80..93e5fc0 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -858,8 +858,7 @@ unsigned char* use_pack(struct packed_git *p,\n>  \n>  static struct packed_git *alloc_packed_git(int extra)\n>  {\n> -\tstruct packed_git *p = xmalloc(sizeof(*p) + extra);\n> -\tmemset(p, 0, sizeof(*p));\n> +\tstruct packed_git *p = xcalloc(1, sizeof(*p) + extra);\n>  \tp->pack_fd = -1;\n>  \treturn p;\n>  }\n\nI previously scanned through and did this.  I left this one as is\nbecause the extra part is about as large as the sizeof(*p) part.  69 and\n72 bytes respectively on 32-bit.  The extra part is always filled in\nimmediately by callers.  It's only called once for each pack so it's\nnot performance critical, so maybe your patch is worth it since it is\nsimpler?\n\n-brandon\n"},{"id":"104038","messageId":"7v3aemnsxp.fsf@gitster.siamese.dyndns.org","threadId":"17701","inReplyTo":"1234334612-24907-1-git-send-email-guanqun.lu@gmail.com","subject":"Re: [PATCH 1/2] code cleanup in sha1_file.c: xmalloc -> xcalloc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-10T16:43:46Z","receivedAt":"2009-02-10T16:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Guanqun Lu <guanqun.lu@gmail.com> writes:\n\n> 'xmalloc' followed immediately by 'memset' is replaced\n> with 'xcalloc', and a simple grep in this project seems\n> to show that it's the only place.\n\nBut isn't this memset() done only for the initial part of the allocated\narea, not the whole thing?  You are not cleaning up but changing what it\ndoes, if I am reading this code correctly.\n\n> Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>\n> ---\n>  sha1_file.c |    3 +--\n>  1 files changed, 1 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 8868b80..93e5fc0 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -858,8 +858,7 @@ unsigned char* use_pack(struct packed_git *p,\n>  \n>  static struct packed_git *alloc_packed_git(int extra)\n>  {\n> -\tstruct packed_git *p = xmalloc(sizeof(*p) + extra);\n> -\tmemset(p, 0, sizeof(*p));\n> +\tstruct packed_git *p = xcalloc(1, sizeof(*p) + extra);\n>  \tp->pack_fd = -1;\n>  \treturn p;\n>  }\n> -- \n> 1.6.1.2.392.gb04d1\n"},{"id":"104016","messageId":"1234334612-24907-1-git-send-email-guanqun.lu@gmail.com","threadId":"17701","inReplyTo":null,"subject":"[PATCH 1/2] code cleanup in sha1_file.c: xmalloc -> xcalloc","fromName":"Guanqun Lu","fromEmail":"guanqun.lu@gmail.com","sentAt":"2009-02-11T06:43:31Z","receivedAt":"2009-02-11T06:43:31Z","isPatch":true,"sender":{"key":"guanqun.lu@gmail.com","avatar":null},"body":"'xmalloc' followed immediately by 'memset' is replaced\nwith 'xcalloc', and a simple grep in this project seems\nto show that it's the only place.\n\nSigned-off-by: Guanqun Lu <guanqun.lu@gmail.com>\n---\n sha1_file.c |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8868b80..93e5fc0 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -858,8 +858,7 @@ unsigned char* use_pack(struct packed_git *p,\n \n static struct packed_git *alloc_packed_git(int extra)\n {\n-\tstruct packed_git *p = xmalloc(sizeof(*p) + extra);\n-\tmemset(p, 0, sizeof(*p));\n+\tstruct packed_git *p = xcalloc(1, sizeof(*p) + extra);\n \tp->pack_fd = -1;\n \treturn p;\n }\n-- \n1.6.1.2.392.gb04d1\n"},{"id":"104017","messageId":"1234334612-24907-2-git-send-email-guanqun.lu@gmail.com","threadId":"17701","inReplyTo":"1234334612-24907-1-git-send-email-guanqun.lu@gmail.com","subject":"[PATCH 2/2] lockfile: set lk->fd = -1 in 'rollback_lock_file()'","fromName":"Guanqun Lu","fromEmail":"guanqun.lu@gmail.com","sentAt":"2009-02-11T06:43:32Z","receivedAt":"2009-02-11T06:43:32Z","isPatch":true,"sender":{"key":"guanqun.lu@gmail.com","avatar":null},"body":"Post-conditions when these functions return successfully:\n                    lk->fd == -1?    lk->filename[0] == '\\0'?\nclose_lock_file()       yes                 no\ncommit_lock_file()      yes                 yes\nrollback_lock_file()    no*                 yes\n\n[*] This commit changes this 'no' in rollback_lock_file() to 'yes',\nwhich achieves more robust and unified interface.\n\nSigned-off-by: Guanqun Lu <guanqun.lu@gmail.com>\n---\n lockfile.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 021c337..44e5253 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -243,8 +243,10 @@ int commit_locked_index(struct lock_file *lk)\n void rollback_lock_file(struct lock_file *lk)\n {\n \tif (lk->filename[0]) {\n-\t\tif (lk->fd >= 0)\n+\t\tif (lk->fd >= 0) {\n \t\t\tclose(lk->fd);\n+\t\t\tlk->fd = -1;\n+\t\t}\n \t\tunlink(lk->filename);\n \t}\n \tlk->filename[0] = 0;\n-- \n1.6.1.2.392.gb04d1\n"}]}