git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Rose via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff Hostetler <git@jeffhostetler.com>,
	Seija <doremylover123@gmail.com>
Subject: Re: [PATCH v5] revision: use calloc instead of malloc where possible
Date: Tue, 06 Dec 2022 20:44:11 +0100	[thread overview]
Message-ID: <221206.86k034b98o.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <pull.1390.v5.git.git.1670348301601.gitgitgadget@gmail.com>


On Tue, Dec 06 2022, Rose via GitGitGadget wrote:

> From: Seija <doremylover123@gmail.com>
>
> We can avoid having to call memset by calling calloc directly
>
> Signed-off-by: Seija doremylover123@gmail.com
> ---
>     revision: use calloc instead of malloc where possible
>     
>     We can avoid having to call memset by calling calloc directly
>     
>     Signed-off-by: Seija doremylover123@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1390%2FAtariDreams%2Fcalloc-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1390/AtariDreams/calloc-v5
> Pull-Request: https://github.com/git/git/pull/1390
>
> Range-diff vs v4:
>
>  1:  3cd6b1eab13 ! 1:  8072fa30e4f maintenance: use calloc instead of malloc where possible
>      @@ Metadata
>       Author: Seija <doremylover123@gmail.com>
>       
>        ## Commit message ##
>      -    maintenance: use calloc instead of malloc where possible
>      +    revision: use calloc instead of malloc where possible
>       
>           We can avoid having to call memset by calling calloc directly
>       
>      @@ Commit message
>       
>        ## builtin/pack-redundant.c ##
>       @@ builtin/pack-redundant.c: static inline struct llist_item *llist_item_get(void)
>      - 		new_item = free_nodes;
>      - 		free_nodes = free_nodes->next;
>      - 	} else {
>      --		int i = 1;
>      -+		size_t i = 1;
>      - 		ALLOC_ARRAY(new_item, BLKSIZE);
>      - 		for (; i < BLKSIZE; i++)
>      - 			llist_item_put(&new_item[i]);
>      -@@ builtin/pack-redundant.c: static inline struct llist_item *llist_item_get(void)
>      + 	return new_item;
>      + }
>        
>      - static inline void llist_init(struct llist **list)
>      - {
>      +-static inline void llist_init(struct llist **list)
>      +-{
>       -	*list = xmalloc(sizeof(struct llist));
>       -	(*list)->front = (*list)->back = NULL;
>       -	(*list)->size = 0;
>      -+	CALLOC_ARRAY(*list, 1);
>      - }
>      - 
>      +-}
>      +-
>        static struct llist * llist_copy(struct llist *list)
>      + {
>      + 	struct llist *ret;
>      + 	struct llist_item *new_item, *old_item, *prev;
>      + 
>      +-	llist_init(&ret);
>      ++	CALLOC_ARRAY(ret, 1);
>      + 
>      + 	if ((ret->size = list->size) == 0)
>      + 		return ret;
>      +@@ builtin/pack-redundant.c: static void load_all_objects(void)
>      + 	struct pack_list *pl = local_packs;
>      + 	struct llist_item *hint, *l;
>      + 
>      +-	llist_init(&all_objects);
>      ++	CALLOC_ARRAY(all_objects, 1);
>      + 
>      + 	while (pl) {
>      + 		hint = NULL;
>      +@@ builtin/pack-redundant.c: static void cmp_local_packs(void)
>      + 
>      + 	/* only one packfile */
>      + 	if (!pl->next) {
>      +-		llist_init(&pl->unique_objects);
>      ++		CALLOC_ARRAY(pl->unique_objects, 1);
>      + 		return;
>      + 	}
>      + 
>      +@@ builtin/pack-redundant.c: static struct pack_list * add_pack(struct packed_git *p)
>      + 		return NULL;
>      + 
>      + 	l.pack = p;
>      +-	llist_init(&l.remaining_objects);
>      ++	CALLOC_ARRAY(l.remaining_objects, 1);
>      + 
>      + 	if (open_pack_index(p))
>      + 		return NULL;
>      +@@ builtin/pack-redundant.c: int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
>      + 		scan_alt_odb_packs();
>      + 
>      + 	/* ignore objects given on stdin */
>      +-	llist_init(&ignore);
>      ++	CALLOC_ARRAY(ignore, 1);
>      + 	if (!isatty(0)) {
>      + 		while (fgets(buf, sizeof(buf), stdin)) {
>      + 			oid = xmalloc(sizeof(*oid));
>       
>        ## remote.c ##
>       @@ remote.c: void apply_push_cas(struct push_cas_option *cas,
>      @@ submodule.c: static struct fetch_task *fetch_task_create(struct submodule_parall
>        
>        	task->sub = submodule_from_path(spf->r, treeish_name, path);
>        
>      -
>      - ## xdiff/xutils.c ##
>      -@@ xdiff/xutils.c: void *xdl_cha_alloc(chastore_t *cha) {
>      - 	void *data;
>      - 
>      - 	if (!(ancur = cha->ancur) || ancur->icurr == cha->nsize) {
>      --		if (!(ancur = (chanode_t *) xdl_malloc(sizeof(chanode_t) + cha->nsize))) {
>      -+		if (!(ancur = (chanode_t *) xdl_calloc(1, sizeof(chanode_t) + cha->nsize))) {
>      - 
>      - 			return NULL;
>      - 		}
>      --		ancur->icurr = 0;
>      --		ancur->next = NULL;
>      - 		if (cha->tail)
>      - 			cha->tail->next = ancur;
>      - 		if (!cha->head)
>
>
>  builtin/pack-redundant.c | 17 +++++------------
>  remote.c                 |  4 ++--
>  submodule.c              | 10 +++++-----
>  3 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> index ecd49ca268f..ce5be807cf0 100644
> --- a/builtin/pack-redundant.c
> +++ b/builtin/pack-redundant.c
> @@ -59,19 +59,12 @@ static inline struct llist_item *llist_item_get(void)
>  	return new_item;
>  }
>  
> -static inline void llist_init(struct llist **list)
> -{
> -	*list = xmalloc(sizeof(struct llist));
> -	(*list)->front = (*list)->back = NULL;
> -	(*list)->size = 0;
> -}
> -
>  static struct llist * llist_copy(struct llist *list)
>  {
>  	struct llist *ret;
>  	struct llist_item *new_item, *old_item, *prev;
>  
> -	llist_init(&ret);
> +	CALLOC_ARRAY(ret, 1);
>  
>  	if ((ret->size = list->size) == 0)
>  		return ret;
> @@ -448,7 +441,7 @@ static void load_all_objects(void)
>  	struct pack_list *pl = local_packs;
>  	struct llist_item *hint, *l;
>  
> -	llist_init(&all_objects);
> +	CALLOC_ARRAY(all_objects, 1);
>  
>  	while (pl) {
>  		hint = NULL;
> @@ -475,7 +468,7 @@ static void cmp_local_packs(void)
>  
>  	/* only one packfile */
>  	if (!pl->next) {
> -		llist_init(&pl->unique_objects);
> +		CALLOC_ARRAY(pl->unique_objects, 1);
>  		return;
>  	}
>  
> @@ -512,7 +505,7 @@ static struct pack_list * add_pack(struct packed_git *p)
>  		return NULL;
>  
>  	l.pack = p;
> -	llist_init(&l.remaining_objects);
> +	CALLOC_ARRAY(l.remaining_objects, 1);
>  
>  	if (open_pack_index(p))
>  		return NULL;
> @@ -620,7 +613,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
>  		scan_alt_odb_packs();
>  
>  	/* ignore objects given on stdin */
> -	llist_init(&ignore);
> +	CALLOC_ARRAY(ignore, 1);
>  	if (!isatty(0)) {
>  		while (fgets(buf, sizeof(buf), stdin)) {
>  			oid = xmalloc(sizeof(*oid));
> diff --git a/remote.c b/remote.c
> index 60869beebe7..475a1d18af0 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2741,9 +2741,9 @@ void apply_push_cas(struct push_cas_option *cas,
>  
>  struct remote_state *remote_state_new(void)
>  {
> -	struct remote_state *r = xmalloc(sizeof(*r));
> +	struct remote_state *r;
>  
> -	memset(r, 0, sizeof(*r));
> +	CALLOC_ARRAY(r, 1);
>  
>  	hashmap_init(&r->remotes_hash, remotes_hash_cmp, NULL, 0);
>  	hashmap_init(&r->branches_hash, branches_hash_cmp, NULL, 0);
> diff --git a/submodule.c b/submodule.c
> index 8ac2fca855d..015102a83d6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1458,14 +1458,13 @@ struct fetch_task {
>   */
>  static const struct submodule *get_non_gitmodules_submodule(const char *path)
>  {
> -	struct submodule *ret = NULL;
> +	struct submodule *ret;
>  	const char *name = default_name_or_path(path);
>  
>  	if (!name)
>  		return NULL;
>  
> -	ret = xmalloc(sizeof(*ret));
> -	memset(ret, 0, sizeof(*ret));
> +	CALLOC_ARRAY(ret, 1);
>  	ret->path = name;
>  	ret->name = name;
>  
> @@ -1504,8 +1503,9 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
>  					    const char *path,
>  					    const struct object_id *treeish_name)
>  {
> -	struct fetch_task *task = xmalloc(sizeof(*task));
> -	memset(task, 0, sizeof(*task));
> +	struct fetch_task *task;
> +
> +	CALLOC_ARRAY(task, 1);
>  
>  	task->sub = submodule_from_path(spf->r, treeish_name, path);
>  
>
> base-commit: 2e71cbbddd64695d43383c25c7a054ac4ff86882

This is partially some of the sentiments Junio did in the v4 review
(although I looked at this before reading that).

I think calloc-ing or memset-ing a struct is going in the opposite
direction of where we've been trending, which is to have explicit
initializers. Sometimes it's justified, but in the "pack-redundant.c"
case I think it's probably better & more future-proof to keep it, and
get rid of the odd patter nof passing in a ** to an init function to
have it alloc for us.

I.e. this (there's some stray new free() in there from testing, sorry,
but those are also bugs we should fix...):

	diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
	index ecd49ca268f..af91d643dd2 100644
	--- a/builtin/pack-redundant.c
	+++ b/builtin/pack-redundant.c
	@@ -27,6 +27,7 @@ static struct llist {
	 	struct llist_item *back;
	 	size_t size;
	 } *all_objects; /* all objects which must be present in local packfiles */
	+#define LLIST_INIT { 0 }
	 
	 static struct pack_list {
	 	struct pack_list *next;
	@@ -59,19 +60,18 @@ static inline struct llist_item *llist_item_get(void)
	 	return new_item;
	 }
	 
	-static inline void llist_init(struct llist **list)
	+static inline void llist_init(struct llist *list)
	 {
	-	*list = xmalloc(sizeof(struct llist));
	-	(*list)->front = (*list)->back = NULL;
	-	(*list)->size = 0;
	+	struct llist blank = LLIST_INIT;
	+	memcpy(list, &blank, sizeof(*list));
	 }
	 
	 static struct llist * llist_copy(struct llist *list)
	 {
	-	struct llist *ret;
	+	struct llist *ret = xmalloc(sizeof(struct llist));
	 	struct llist_item *new_item, *old_item, *prev;
	 
	-	llist_init(&ret);
	+	llist_init(ret);
	 
	 	if ((ret->size = list->size) == 0)
	 		return ret;
	@@ -420,6 +420,7 @@ static void minimize(struct pack_list **min)
	 
	 	unique_pack_objects = llist_copy(all_objects);
	 	llist_sorted_difference_inplace(unique_pack_objects, missing);
	+	free(missing);
	 
	 	/* remove unique pack objects from the non_unique packs */
	 	pl = non_unique;
	@@ -447,8 +448,9 @@ static void load_all_objects(void)
	 {
	 	struct pack_list *pl = local_packs;
	 	struct llist_item *hint, *l;
	+	all_objects = xmalloc(sizeof(struct llist));
	 
	-	llist_init(&all_objects);
	+	llist_init(all_objects);
	 
	 	while (pl) {
	 		hint = NULL;
	@@ -475,7 +477,8 @@ static void cmp_local_packs(void)
	 
	 	/* only one packfile */
	 	if (!pl->next) {
	-		llist_init(&pl->unique_objects);
	+		pl->unique_objects = xmalloc(sizeof(struct llist));
	+		llist_init(pl->unique_objects);
	 		return;
	 	}
	 
	@@ -512,7 +515,8 @@ static struct pack_list * add_pack(struct packed_git *p)
	 		return NULL;
	 
	 	l.pack = p;
	-	llist_init(&l.remaining_objects);
	+	l.remaining_objects = xmalloc(sizeof(struct llist));
	+	llist_init(l.remaining_objects);
	 
	 	if (open_pack_index(p))
	 		return NULL;
	@@ -562,7 +566,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
	 	int i;
	 	int i_still_use_this = 0;
	 	struct pack_list *min = NULL, *red, *pl;
	-	struct llist *ignore;
	+	struct llist *ignore = xmalloc(sizeof(struct llist));
	 	struct object_id *oid;
	 	char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */
	 
	@@ -620,7 +624,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
	 		scan_alt_odb_packs();
	 
	 	/* ignore objects given on stdin */
	-	llist_init(&ignore);
	+	llist_init(ignore);
	 	if (!isatty(0)) {
	 		while (fgets(buf, sizeof(buf), stdin)) {
	 			oid = xmalloc(sizeof(*oid));
	@@ -635,6 +639,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
	 		llist_sorted_difference_inplace(pl->remaining_objects, ignore);
	 		pl = pl->next;
	 	}
	+	free(ignore);
	 
	 	cmp_local_packs();

Now, for:

> @@ -1504,8 +1503,9 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
>  					    const char *path,
>  					    const struct object_id *treeish_name)
>  {
> -	struct fetch_task *task = xmalloc(sizeof(*task));
> -	memset(task, 0, sizeof(*task));
> +	struct fetch_task *task;
> +
> +	CALLOC_ARRAY(task, 1);
>  
>  	task->sub = submodule_from_path(spf->r, treeish_name, path);

This is no worse than before, but I think we're just converting here
from one bad pattern to another.

The "struct fetch_task" contains a "struct strvec", for many of our
structs we play it fast and loose with whether you can calloc() it, but
for "struct strbuf", "struct strvec" etc. we have an "empty_strvec" (or
"slopbuf" etc.), so we *really* want those to be properly init'd.

AFAICT we're just lucky that it happens to work with the strvec API in
this case, but if this was a strbuf it could easily segfault etc.

So the better fix here since we're spending review time on it isn't to
move from one memset() equivalent pattern to another, but actually to
start properly initializing this.

Finally, this commit just seems to be all over the place in what it's
changing. We have a bunch of:

	x = xmalloc(...);
	memset(x, 0, ...);

In our tree, it's not clear why these are being picked out in
particular, or what they have to do with each other.

I think that if we're proposing to refactor these doing so with
coccinelle is a much better thing to do. There's a parallel thread about
that over at:
https://lore.kernel.org/git/6694c52b38674859eb0390c7f62da1209a8d8ec3.1670266373.git.me@ttaylorr.com/

  reply	other threads:[~2022-12-06 19:52 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-05 14:48 [PATCH] maintenance: use xcalloc instead of xmalloc where possible Rose via GitGitGadget
2022-12-05 15:01 ` Jeff Hostetler
2022-12-05 15:33 ` [PATCH v2] " Rose via GitGitGadget
2022-12-05 16:01   ` [PATCH v3] " Rose via GitGitGadget
2022-12-05 16:12     ` [PATCH v4] maintenance: use calloc instead of malloc " Rose via GitGitGadget
2022-12-06  5:03       ` Junio C Hamano
2022-12-06 17:38       ` [PATCH v5] revision: " Rose via GitGitGadget
2022-12-06 19:44         ` Ævar Arnfjörð Bjarmason [this message]
2022-12-06 19:53         ` [PATCH v6] " Rose via GitGitGadget
2022-12-07  8:44           ` Bagas Sanjaya

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=221206.86k034b98o.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=doremylover123@gmail.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).