-
-
Notifications
You must be signed in to change notification settings - Fork 54
Minor Issues Found in fopen() patch #349
Description
Minor Issues Found in fopen() patch
1. logToFileFilename still uses raw fopen()
main.c:950:
logFile = fopen(logToFileFilename, "w");logToFileFilename is set from optarg at main.c:824 (the --log option), which comes from argv. After the PR's argv UTF-8 conversion those strings are in UTF-8, but fopen() on Windows still interprets them as system-codepage. A user who passes --log /path/with/ünïcödé/log.txt would get a silent open failure even with this PR applied. This should be g_fopen().
If this call were converted, the #include <glib/gstdio.h> added to main.c would be justified (currently the include is not needed — see issue 2).
2. #include <glib/gstdio.h> in main.c is premature
The PR adds <glib/gstdio.h> to main.c, but main.c does not call g_fopen() anywhere in the patched version. g_locale_to_utf8() is declared in <glib.h>, which is already pulled in transitively. The include is harmless but signals intent that wasn't followed through — see issue 1.
3. #ifdef G_OS_WIN32 vs. #ifdef WIN32 inconsistency
The new argv conversion block uses G_OS_WIN32:
#ifdef G_OS_WIN32
for (i = 0; i < argc; i++) { … }
#endifThe surrounding code in main.c consistently uses WIN32:
# ifdef WIN32
bind_textdomain_codeset(PACKAGE, "UTF-8");
# endif#ifdef WIN32
screenRenderInfo.renderType = GERBV_RENDER_TYPE_CAIRO_NORMAL;
#endifBoth macros resolve on Windows in this build (GLib defines G_OS_WIN32; WIN32 is compiler-defined), so there is no functional difference. Cosmetic inconsistency only — the new block should use #ifdef WIN32 to match the file's existing style.
4. argv mutation produces a Valgrind-visible leak (by design)
gchar *utf8_arg = g_locale_to_utf8(argv[i], -1, NULL, NULL, NULL);
if (utf8_arg) {
argv[i] = utf8_arg;
}The original argv[i] pointer (owned by the C runtime) is overwritten and cannot be freed. The new utf8_arg allocation is placed into argv[] and also never freed. This is intentional — these strings must live for the process lifetime — but Valgrind would report them as definite leaks. Since the G_OS_WIN32 guard means this block never runs on Linux (where the Valgrind tests run), there is no practical impact on the test suite. Worth noting in a code comment.
No show stoppers, I merge this with referencing this in an issue for future update.
Originally posted by @spe-ciellt in #318 (comment)