git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Eric Sunshine <sunshine@sunshineco.com>
Date
May 14, 2018, 03:03 UTC
Message-ID
<CAPig+cT1eejizU26mPE+tN+eZR4Wg0jw1DbmShgqRu69EYQutQ@mail.gmail.com>
In-Reply-To
<cover.1526204440.git.martin.agren@gmail.com>
On Sun, May 13, 2018 at 5:58 AM, Martin Ågren <martin.agren@gmail.com> wrote:
Show 26 quoted lines
> On 13 May 2018 at 10:59, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> 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
>> 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.
>
> 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.

Yep, making this a multi-part patch series and updating the commit message of the patch which introduces config_store_data_clear(), as you suggest, makes sense. The patch series could be organized differently -- such as first moving freeing of 'value_regex' into new config_store_data_clear(), then freeing additional fields in later patches -- but I don't think it matters much in practice, so the current organization is likely good enough.

Previous: Martin ÅgrenNext: Martin Ågren
Message 11 of 18 in “config: free resources of `struct config_store_data`”
  1. config: free resources of `struct config_store_data`Martin Ågren, May 13, 2018
  2. Eric SunshineMay 13, 2018
  3. Martin ÅgrenMay 13, 2018
  4. 2/1 config: let `config_store_data_clear()` handle `value_regex`Martin Ågren, May 13, 2018
  5. Eric SunshineMay 14, 2018
  6. regex: do not call `regfree()` if compilation failsMartin Ågren, May 20, 2018
  7. Stefan BellerMay 21, 2018
  8. Eric SunshineMay 22, 2018
  9. Martin ÅgrenMay 22, 2018
  10. 3/1 config: let `config_store_data_clear()` handle `key`Martin Ågren, May 13, 2018
  11. Eric SunshineMay 14, 2018
  12. 0/3 config: free resources of `struct config_store_data`Martin Ågren, May 20, 2018
  13. 1/3 config: free resources of `struct config_store_data`Martin Ågren, May 20, 2018
  14. 2/3 config: let `config_store_data_clear()` handle `value_regex`Martin Ågren, May 20, 2018
  15. 3/3 config: let `config_store_data_clear()` handle `key`Martin Ågren, May 20, 2018
  16. Eric SunshineMay 23, 2018
  17. Eric SunshineMay 23, 2018
  18. Martin ÅgrenMay 13, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.