| Message ID | patch-3.3-062fb3f454e-20220607T154520Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote API: fix -fanalyzer-spotted freeing issue | expand |
Thanks for sending out this series :) I feel a little guilty leaving behind this much junk. Coincidentally, I was already planning on revisiting my previous work to clean up debt/mistakes, so this was a good lesson in what to look out for. The previous patches look good to me, and I have no comments on those. I have a style question on this patch (more for my own learning than a suggestion on this patch), but it also looks good to me. Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > In this case however we do not use the "struct remote", so the > FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static > variables into per-repository struct, 2021-11-17) can be replaced with > free(). Did you mean *reuse* the "struct remote"? > The API was also odd in that remote_state_new() would xmalloc() for us, > but the user had to free() it themselves, let's instead change the > behavior to have the destructor free() what we malloc() in the > constructer. > > In this case this appears to have been done for consistency with > repo_clear(), let's instead have repo_clear() handle the NULL-ing of > its "remote_state", and not attempt to reset the structure in remote.c Yes, this was specifically for consistency reasons. We'll have to read on to see whether or not this patch remains consistent with repo_clear()... > static void add_merge(struct branch *branch, const char *name) > @@ -2720,12 +2720,12 @@ void remote_state_clear(struct remote_state *remote_state) > > for (i = 0; i < remote_state->remotes_nr; i++) > remote_clear(remote_state->remotes[i]); > - FREE_AND_NULL(remote_state->remotes); > - remote_state->remotes_alloc = 0; > - remote_state->remotes_nr = 0; > + free(remote_state->remotes); > > hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent); > hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent); > + > + free(remote_state); > } Now that remote_state_clear() free()-s the "struct remote_state", should we rename it to something like "remote_state_free()"? There's some precedent for this in the other repo_clear() 'destructors'... > diff --git a/remote.h b/remote.h > index dd4402436f1..d91b2b29373 100644 > --- a/remote.h > +++ b/remote.h > @@ -54,9 +54,17 @@ struct remote_state { > int initialized; > }; > > -void remote_state_clear(struct remote_state *remote_state); > +/** > + * xmalloc() a "struct remote_state" and initialize it. The resulting > + * data should be free'd with remote_state_clear(). > + */ > struct remote_state *remote_state_new(void); > > +/** > + * free() the structure returned by remote_state_new(). > + */ > +void remote_state_clear(struct remote_state *remote_state); > + > struct remote { > struct hashmap_entry ent; > > diff --git a/repository.c b/repository.c > index 5d166b692c8..0a6df6937e4 100644 > --- a/repository.c > +++ b/repository.c > @@ -292,7 +292,7 @@ void repo_clear(struct repository *repo) > > if (repo->remote_state) { > remote_state_clear(repo->remote_state); > - FREE_AND_NULL(repo->remote_state); > + repo->remote_state = NULL; > } > > repo_clear_path_cache(&repo->cached_paths); I suppose the question of whether or not to free() in the 'destructor' depends on whether we expect the struct to be reusable? I don't expect that "struct remote_state" needs to be reused, so free()-ing it is ok to me. The API is not _that_ odd though ;) As you noted, my initial use of FREE_AND_NULL() is for consistency reasons with the rest of repo_clear(), which looks like this: if (repo->config) { git_configset_clear(repo->config); FREE_AND_NULL(repo->config); } if (repo->submodule_cache) { submodule_cache_free(repo->submodule_cache); repo->submodule_cache = NULL; } if (repo->index) { discard_index(repo->index); if (repo->index != &the_index) FREE_AND_NULL(repo->index); } if (repo->promisor_remote_config) { promisor_remote_clear(repo->promisor_remote_config); FREE_AND_NULL(repo->promisor_remote_config); } if (repo->remote_state) { remote_state_clear(repo->remote_state); - FREE_AND_NULL(repo->remote_state); + repo->remote_state = NULL; } promisor_remote_clear(), discard_index(), and git_configset_clear() don't free() the struct, so it makes sense for them to use FREE_AND_NULL(). AFAICT, these structs are meant to be reused, so it makes sense that we "clear" it without freeing the struct pointer itself. On the other hand, submodule_cache_free() _does_ free() the struct, and so we just use "= NULL". I noticed that this uses the verb "free", and not "clear". So now that remote_state_clear() *does* free() the struct, it is perfectly fine to use "= NULL" here as well, though it uses the verb "clear". I'm not sure if we have a style around clear/free. Feel free to ignore if there isn't one.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change the buggy "remote_clear()" function to stop pretending to to be "buggy" -> ""; "to to" -> "to". Over-using FREE_AND_NULL() is indeed irritating, but as long as we are not reusing the struct whose member are getting freed, there is no "bug" per-se, no? > able to zero out a "struct remote". Setting "url" and "pushurl" to > NULL results in an invalid state unless the corresponding "url_nr" and > "pushurl_nr" are also set to zero. > > In this case however we do not use the "struct remote", so the "use" -> "reuse", with comma around "however" on both sides, probably. > FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static > variables into per-repository struct, 2021-11-17) can be replaced with > free(). Yay. > The API was also odd in that remote_state_new() would xmalloc() for us, > but the user had to free() it themselves, let's instead change the > behavior to have the destructor free() what we malloc() in the > constructer. I am not sure if remote_clear() that does not free the remote goes well with remote_state_clear() that does free remote_state. As long as it is clear whose responsibility to release resources is, we can go either way, but helper functions with similar names having quite different resource maintainance semantics looks like a potential source of confusion to me. > In this case this appears to have been done for consistency with > repo_clear(), let's instead have repo_clear() handle the NULL-ing of > its "remote_state", and not attempt to reset the structure in remote.c > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > remote.c | 14 +++++++------- > remote.h | 10 +++++++++- > repository.c | 2 +- > 3 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/remote.c b/remote.c > index 0b243b090d9..c6ce04dacb7 100644 > --- a/remote.c > +++ b/remote.c > @@ -147,15 +147,15 @@ static void remote_clear(struct remote *remote) > > for (i = 0; i < remote->url_nr; i++) > free((char *)remote->url[i]); > - FREE_AND_NULL(remote->url); > + free(remote->url); > > for (i = 0; i < remote->pushurl_nr; i++) > free((char *)remote->pushurl[i]); > - FREE_AND_NULL(remote->pushurl); > + free(remote->pushurl); > free((char *)remote->receivepack); > free((char *)remote->uploadpack); > - FREE_AND_NULL(remote->http_proxy); > - FREE_AND_NULL(remote->http_proxy_authmethod); > + free(remote->http_proxy); > + free(remote->http_proxy_authmethod); > } > > static void add_merge(struct branch *branch, const char *name) > @@ -2720,12 +2720,12 @@ void remote_state_clear(struct remote_state *remote_state) > > for (i = 0; i < remote_state->remotes_nr; i++) > remote_clear(remote_state->remotes[i]); > - FREE_AND_NULL(remote_state->remotes); > - remote_state->remotes_alloc = 0; > - remote_state->remotes_nr = 0; > + free(remote_state->remotes); > > hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent); > hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent); > + > + free(remote_state); > } > > /* > diff --git a/remote.h b/remote.h > index dd4402436f1..d91b2b29373 100644 > --- a/remote.h > +++ b/remote.h > @@ -54,9 +54,17 @@ struct remote_state { > int initialized; > }; > > -void remote_state_clear(struct remote_state *remote_state); > +/** > + * xmalloc() a "struct remote_state" and initialize it. The resulting > + * data should be free'd with remote_state_clear(). > + */ > struct remote_state *remote_state_new(void); > > +/** > + * free() the structure returned by remote_state_new(). > + */ > +void remote_state_clear(struct remote_state *remote_state); > + > struct remote { > struct hashmap_entry ent; > > diff --git a/repository.c b/repository.c > index 5d166b692c8..0a6df6937e4 100644 > --- a/repository.c > +++ b/repository.c > @@ -292,7 +292,7 @@ void repo_clear(struct repository *repo) > > if (repo->remote_state) { > remote_state_clear(repo->remote_state); > - FREE_AND_NULL(repo->remote_state); > + repo->remote_state = NULL; > } > > repo_clear_path_cache(&repo->cached_paths);
Glen Choo <chooglen@google.com> writes: > I suppose the question of whether or not to free() in the 'destructor' > depends on whether we expect the struct to be reusable? I don't expect > that "struct remote_state" needs to be reused, so free()-ing it is ok to > me. > > The API is not _that_ odd though ;) As you noted, my initial use of > FREE_AND_NULL() is for consistency reasons with the rest of > repo_clear(), which looks like this: > > if (repo->config) { > git_configset_clear(repo->config); > FREE_AND_NULL(repo->config); So git_configset_clear() does clear but does not free. > } > > if (repo->submodule_cache) { > submodule_cache_free(repo->submodule_cache); submodule_cache_free() does (probably clear and) free. > repo->submodule_cache = NULL; > } > > if (repo->index) { > discard_index(repo->index); discard_index() does not free. > if (repo->index != &the_index) > FREE_AND_NULL(repo->index); > } > > if (repo->promisor_remote_config) { > promisor_remote_clear(repo->promisor_remote_config); promisor_remote_clear() does not free. > FREE_AND_NULL(repo->promisor_remote_config); > } > > if (repo->remote_state) { > remote_state_clear(repo->remote_state); > - FREE_AND_NULL(repo->remote_state); > + repo->remote_state = NULL; > } > > promisor_remote_clear(), discard_index(), and git_configset_clear() > don't free() the struct, so it makes sense for them to use > FREE_AND_NULL(). AFAICT, these structs are meant to be reused, so it > makes sense that we "clear" it without freeing the struct pointer > itself. > > On the other hand, submodule_cache_free() _does_ free() the struct, and > so we just use "= NULL". I noticed that this uses the verb "free", and > not "clear". > > So now that remote_state_clear() *does* free() the struct, it is > perfectly fine to use "= NULL" here as well, though it uses the verb > "clear". > > I'm not sure if we have a style around clear/free. Feel free to ignore > if there isn't one. It does bother me. Changing _clear() that did not free the container resource to free it, without changing the name to free or release or whatever, smells like leaving a source of confusion for future developers.
diff --git a/remote.c b/remote.c index 0b243b090d9..c6ce04dacb7 100644 --- a/remote.c +++ b/remote.c @@ -147,15 +147,15 @@ static void remote_clear(struct remote *remote) for (i = 0; i < remote->url_nr; i++) free((char *)remote->url[i]); - FREE_AND_NULL(remote->url); + free(remote->url); for (i = 0; i < remote->pushurl_nr; i++) free((char *)remote->pushurl[i]); - FREE_AND_NULL(remote->pushurl); + free(remote->pushurl); free((char *)remote->receivepack); free((char *)remote->uploadpack); - FREE_AND_NULL(remote->http_proxy); - FREE_AND_NULL(remote->http_proxy_authmethod); + free(remote->http_proxy); + free(remote->http_proxy_authmethod); } static void add_merge(struct branch *branch, const char *name) @@ -2720,12 +2720,12 @@ void remote_state_clear(struct remote_state *remote_state) for (i = 0; i < remote_state->remotes_nr; i++) remote_clear(remote_state->remotes[i]); - FREE_AND_NULL(remote_state->remotes); - remote_state->remotes_alloc = 0; - remote_state->remotes_nr = 0; + free(remote_state->remotes); hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent); hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent); + + free(remote_state); } /* diff --git a/remote.h b/remote.h index dd4402436f1..d91b2b29373 100644 --- a/remote.h +++ b/remote.h @@ -54,9 +54,17 @@ struct remote_state { int initialized; }; -void remote_state_clear(struct remote_state *remote_state); +/** + * xmalloc() a "struct remote_state" and initialize it. The resulting + * data should be free'd with remote_state_clear(). + */ struct remote_state *remote_state_new(void); +/** + * free() the structure returned by remote_state_new(). + */ +void remote_state_clear(struct remote_state *remote_state); + struct remote { struct hashmap_entry ent; diff --git a/repository.c b/repository.c index 5d166b692c8..0a6df6937e4 100644 --- a/repository.c +++ b/repository.c @@ -292,7 +292,7 @@ void repo_clear(struct repository *repo) if (repo->remote_state) { remote_state_clear(repo->remote_state); - FREE_AND_NULL(repo->remote_state); + repo->remote_state = NULL; } repo_clear_path_cache(&repo->cached_paths);
Change the buggy "remote_clear()" function to stop pretending to to be able to zero out a "struct remote". Setting "url" and "pushurl" to NULL results in an invalid state unless the corresponding "url_nr" and "pushurl_nr" are also set to zero. In this case however we do not use the "struct remote", so the FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static variables into per-repository struct, 2021-11-17) can be replaced with free(). The API was also odd in that remote_state_new() would xmalloc() for us, but the user had to free() it themselves, let's instead change the behavior to have the destructor free() what we malloc() in the constructer. In this case this appears to have been done for consistency with repo_clear(), let's instead have repo_clear() handle the NULL-ing of its "remote_state", and not attempt to reset the structure in remote.c Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 14 +++++++------- remote.h | 10 +++++++++- repository.c | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-)