threads / patch / 17701

patch, 2 partscode cleanup in sha1_file.c: xmalloc -> xcalloc

Subject: [PATCH 1/2] code cleanup in sha1_file.c: xmalloc -> xcalloc

## tl;dr

5 messages between Feb 10, 2009 and Feb 11, 2009. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Guanqun Lu· Feb 11, 2009, 06:43 UTC · lore

'xmalloc' followed immediately by 'memset' is replaced with 'xcalloc', and a simple grep in this project seems to show that it's the only place.

Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>
---
 sha1_file.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

Show changes to sha1_file.c +1 −2

diff --git a/sha1_file.c b/sha1_file.c
index 8868b80..93e5fc0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -858,8 +858,7 @@ unsigned char* use_pack(struct packed_git *p,
 
 static struct packed_git *alloc_packed_git(int extra)
 {
-	struct packed_git *p = xmalloc(sizeof(*p) + extra);
-	memset(p, 0, sizeof(*p));
+	struct packed_git *p = xcalloc(1, sizeof(*p) + extra);
 	p->pack_fd = -1;
 	return p;
 }

-- 
1.6.1.2.392.gb04d1

Brandon Casey· Feb 10, 2009, 16:29 UTC · re: Guanqun Lu · lore

Re: [PATCH 1/2] code cleanup in sha1_file.c: xmalloc -> xcalloc

Guanqun Lu wrote:

Show 23 quoted lines

> 'xmalloc' followed immediately by 'memset' is replaced
> with 'xcalloc', and a simple grep in this project seems
> to show that it's the only place.
> 
> Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>
> ---
>  sha1_file.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index 8868b80..93e5fc0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -858,8 +858,7 @@ unsigned char* use_pack(struct packed_git *p,
>  
>  static struct packed_git *alloc_packed_git(int extra)
>  {
> -	struct packed_git *p = xmalloc(sizeof(*p) + extra);
> -	memset(p, 0, sizeof(*p));
> +	struct packed_git *p = xcalloc(1, sizeof(*p) + extra);
>  	p->pack_fd = -1;
>  	return p;
>  }

I previously scanned through and did this. I left this one as is because the extra part is about as large as the sizeof(*p) part. 69 and 72 bytes respectively on 32-bit. The extra part is always filled in immediately by callers. It's only called once for each pack so it's not performance critical, so maybe your patch is worth it since it is simpler?

-brandon

Junio C Hamano· Feb 10, 2009, 16:43 UTC · re: Guanqun Lu · lore

Re: [PATCH 1/2] code cleanup in sha1_file.c: xmalloc -> xcalloc

Guanqun Lu <guanqun.lu@gmail.com> writes:

> 'xmalloc' followed immediately by 'memset' is replaced
> with 'xcalloc', and a simple grep in this project seems
> to show that it's the only place.

But isn't this memset() done only for the initial part of the allocated area, not the whole thing? You are not cleaning up but changing what it does, if I am reading this code correctly.

Show 21 quoted lines

> Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>
> ---
>  sha1_file.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 8868b80..93e5fc0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -858,8 +858,7 @@ unsigned char* use_pack(struct packed_git *p,
>  
>  static struct packed_git *alloc_packed_git(int extra)
>  {
> -	struct packed_git *p = xmalloc(sizeof(*p) + extra);
> -	memset(p, 0, sizeof(*p));
> +	struct packed_git *p = xcalloc(1, sizeof(*p) + extra);
>  	p->pack_fd = -1;
>  	return p;
>  }
> -- 
> 1.6.1.2.392.gb04d1

Guanqun Lu· Feb 11, 2009, 06:43 UTC · re: Guanqun Lu · lore

[PATCH 2/2] lockfile: set lk->fd = -1 in 'rollback_lock_file()'

Post-conditions when these functions return successfully:
                    lk->fd == -1?    lk->filename[0] == '\0'?
close_lock_file()       yes                 no
commit_lock_file()      yes                 yes
rollback_lock_file()    no*                 yes

[*] This commit changes this 'no' in rollback_lock_file() to 'yes', which achieves more robust and unified interface.

Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>
---
 lockfile.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

Show changes to lockfile.c +3 −1

diff --git a/lockfile.c b/lockfile.c
index 021c337..44e5253 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -243,8 +243,10 @@ int commit_locked_index(struct lock_file *lk)
 void rollback_lock_file(struct lock_file *lk)
 {
 	if (lk->filename[0]) {
-		if (lk->fd >= 0)
+		if (lk->fd >= 0) {
 			close(lk->fd);
+			lk->fd = -1;
+		}
 		unlink(lk->filename);
 	}
 	lk->filename[0] = 0;

-- 
1.6.1.2.392.gb04d1

Junio C Hamano· Feb 10, 2009, 15:29 UTC · re: Guanqun Lu · lore

Re: [PATCH 2/2] lockfile: set lk->fd = -1 in 'rollback_lock_file()'

Guanqun Lu <guanqun.lu@gmail.com> writes:

Show 8 quoted lines

> Post-conditions when these functions return successfully:
>                     lk->fd == -1?    lk->filename[0] == '\0'?
> close_lock_file()       yes                 no
> commit_lock_file()      yes                 yes
> rollback_lock_file()    no*                 yes
>
> [*] This commit changes this 'no' in rollback_lock_file() to 'yes',
> which achieves more robust and unified interface.

Is there a broken caller this patch fixes? IOW, is there a codepath that can call rollback and then later notices lk->fd is not negative and uses it without checking lk->filename[0] first?

Show 23 quoted lines

> Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>
> ---
>  lockfile.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index 021c337..44e5253 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -243,8 +243,10 @@ int commit_locked_index(struct lock_file *lk)
>  void rollback_lock_file(struct lock_file *lk)
>  {
>  	if (lk->filename[0]) {
> -		if (lk->fd >= 0)
> +		if (lk->fd >= 0) {
>  			close(lk->fd);
> +			lk->fd = -1;
> +		}
>  		unlink(lk->filename);
>  	}
>  	lk->filename[0] = 0;
> -- 
> 1.6.1.2.392.gb04d1