Re: [PATCH] config: free resources of `struct config_store_data`

From: Martin Ågren <martin.agren@gmail.com>
Date: May 13, 2018, 09:58 UTC
Message-ID: <cover.1526204440.git.martin.agren@gmail.com>
In-Reply-To: <CAPig+cThT3VVw75VF0wuF-yR-xbnfNOxhviYGZLAksF8HFuOGw@mail.gmail.com>

On 13 May 2018 at 10:59, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 10 quoted lines

> On Sun, May 13, 2018 at 4:23 AM Martin Ågren <martin.agren@gmail.com> wrote:
>
>> Introduce and use a small helper function `config_store_data_clear()` to
>> plug these leaks. This should be safe. The memory tracked here is config
>> parser events. Once the users (`git_config_set_multivar_in_file_gently()`
>> and `git_config_copy_or_rename_section_in_file()` at the moment) are
>> done, no-one should be holding on to a pointer into this memory.
>
> A newcomer to this code (such as myself) might legitimately wonder why
> store->key and store->value_regex are not also being cleaned up by this

Good point. I was only concerned by the members that no-one took responsibility for.

Show 5 quoted lines

> function. An examination of the relevant code reveals that those structure
> members are manually (and perhaps tediously) freed already by
> git_config_set_multivar_in_file_gently(), however, if
> config_store_data_clear() was responsible for freeing them, then
> git_config_set_multivar_in_file_gently() could be made a bit less noisy.

Yes, that's true.

> On the other hand, if there's actually a good reason for
>   config_store_data_clear() not freeing those members, then perhaps it
> deserves an in-code comment explaining why they are exempt.

Not any good reason that I can think of, other than "history". But to be clear, a day ago I was as much of a newcomer in this part of the code as you are. Johannes is the one who might have the most up-to-date understanding of this.

How about the following two patches as patches 2/3 and 3/3? I would also need to mention in the commit message of this patch (1/3) that the function will soon learn to clean up more members.

I could of course squash the three patches into one, but there is some minor trickery involved, like we can't reuse a pointer in one place, but need to xstrdup it.

Thank you for your comments. I'd be very interested in your thoughts on this.

Martin

Martin Ågren (2):
  config: let `config_store_data_clear()` handle `value_regex`
  config: let `config_store_data_clear()` handle `key`

 config.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

-- 
2.17.0.583.g9a75a153ac