Re: [PATCH 2/2] bugreport: slightly better memory management
From: Junio C Hamano <hidden>
Date: 2021-09-07 21:57:06
Carlo Marcelo Arenas Belón [off-list ref] writes:
238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16) introduces an UNLEAK for a strbuf that contains the buffer that gets flushed to disk earlier, instead of simply cleaning the buffer. do so, and while at it, move the free() call for another temporary string closer to its creator, so it is easier to keep track of.
'do so' -> 'Do so'.
quoted hunk
Signed-off-by: Carlo Marcelo Arenas Belón <redacted> --- builtin/bugreport.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)diff --git a/builtin/bugreport.c b/builtin/bugreport.c index 17042381c3..a9bedde1e8 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c@@ -152,6 +152,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) strbuf_addstr(&report_path, "git-bugreport-"); strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0); strbuf_addstr(&report_path, ".txt"); + free(prefixed_filename);
Correct, but it can be raised even further. We can free it after we addstr to report_path. I looked at the existing callers of prefix_filename(), hoping that many of them might take it in a strbuf they have, in which case we may be able to expose an alternative interface to take a caller supplied strbuf to clean this one up. But it seems this is the only one, so "store in a temporary, strbuf_addstr it, and immediately free the temporary" here would be good.
quoted hunk
switch (safe_create_leading_directories(report_path.buf)) { case SCLD_OK:@@ -181,6 +182,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) die_errno(_("unable to write to %s"), report_path.buf); close(report); + strbuf_release(&buffer);
We are done with the strbuf once write_in_full() returns, but this is close enough.
quoted hunk
@@ -191,8 +193,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) fprintf(stderr, _("Created new report at '%s'.\n"), user_relative_path); - free(prefixed_filename); - UNLEAK(buffer); UNLEAK(report_path); return !!launch_editor(report_path.buf, NULL, NULL);
Having reviewed all, I am not sure if my reaction is "good, now we are cleaner" or "meh, for the same reason why report_path can be left alive, it is fine to leave buffer alive, too".