Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] GMainLoop object leaks when `perf_gtk__error` is called re-entrantly due to overwriting the global loop pointer.
- [Low] Missing explicit POSIX libc header inclusions (<stdio.h>, <stdarg.h>) causes musl compilation failures.
--
commit 8571088e70ee1febaf2f0ceafacdfc25aeb4ea31
Author: Matt Turner [off-list ref]
tools: port perf ui from GTK 2 to GTK 4
This commit ports the perf GTK UI from GTK 2 to GTK 4. It updates widget
creation, packing, and event loop logic to account for GTK 4 API changes,
such as removing GtkContainer and gtk_main_quit().
quoted hunk ↗ jump to hunk
diff --git a/tools/perf/ui/gtk/progress.c b/tools/perf/ui/gtk/progress.c
index eea6fcde518a8..28a4b486360de 100644
--- a/tools/perf/ui/gtk/progress.c
+++ b/tools/perf/ui/gtk/progress.c
@@ -3,47 +3,62 @@
#include "gtk.h"
#include "../progress.h"
+#include <linux/compiler.h>
[Severity: Low]
This is a pre-existing issue, but can we explicitly include <stdio.h> here?
The snprintf function is used further down in gtk_ui_progress__update(),
and relying on implicit inclusion can cause build failures on systems using
the musl C library.
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/tools/perf/ui/gtk/util.c b/tools/perf/ui/gtk/util.c
index c47f5c387838c..5096888b7c160 100644
--- a/tools/perf/ui/gtk/util.c
+++ b/tools/perf/ui/gtk/util.c
@@ -4,6 +4,7 @@
#include <stdlib.h>
#include <string.h>
+#include <linux/compiler.h>
#include <linux/zalloc.h>
[Severity: Low]
Could we also explicitly include <stdarg.h> and <stdio.h> in this file?
The patch introduces the use of va_copy and va_end below, which requires
<stdarg.h>. Using vasprintf, fprintf, and vfprintf without explicitly
including <stdio.h> can cause musl compilation failures.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -28,27 +29,60 @@ int perf_gtk__deactivate_context(struct perf_gtk_context **ctx)
[ ... ]
quoted hunk ↗ jump to hunk
static int perf_gtk__error(const char *format, va_list args)
{
char *msg;
GtkWidget *dialog;
+ va_list args_copy;
+ va_copy(args_copy, args);
if (!perf_gtk__is_active_context(pgctx) ||
- vasprintf(&msg, format, args) < 0) {
+ vasprintf(&msg, format, args_copy) < 0) {
+ va_end(args_copy);
fprintf(stderr, "Error:\n");
vfprintf(stderr, format, args);
fprintf(stderr, "\n");
return -1;
}
+ va_end(args_copy);
dialog = gtk_message_dialog_new_with_markup(GTK_WINDOW(pgctx->main_window),
GTK_DIALOG_DESTROY_WITH_PARENT,
GTK_MESSAGE_ERROR,
GTK_BUTTONS_CLOSE,
"<b>Error</b>\n\n%s", msg);
- gtk_dialog_run(GTK_DIALOG(dialog));
- gtk_widget_destroy(dialog);
+ /*
+ * "response" only fires when a button is clicked; DESTROY_WITH_PARENT
+ * destroys the dialog directly without it. Quit from "destroy"
+ * instead, which fires either way, so the nested loop below can't
+ * outlive the dialog and hang.
+ */
+ perf_gtk__error_loop = g_main_loop_new(NULL, FALSE);
[Severity: Medium]
What happens if perf_gtk__error() is called re-entrantly while another error
dialog is already open?
If a signal or callback triggers another error while the first dialog's event
loop is running, the global perf_gtk__error_loop pointer will be overwritten
by this new loop.
quoted hunk ↗ jump to hunk
+ g_signal_connect(dialog, "response",
+ G_CALLBACK(perf_gtk__dialog_response), NULL);
+ g_signal_connect_swapped(dialog, "destroy",
+ G_CALLBACK(g_main_loop_quit), perf_gtk__error_loop);
+
+ gtk_widget_set_visible(dialog, TRUE);
+ g_main_loop_run(perf_gtk__error_loop);
+ g_clear_pointer(&perf_gtk__error_loop, g_main_loop_unref);
[Severity: Medium]
When the inner dialog closes, it unrefs its loop and sets the global pointer to
NULL. When the outer dialog subsequently closes, g_clear_pointer() will see a
NULL pointer and fail to unref the outer loop, resulting in a memory leak of
the GMainLoop object. Is there a way to store the loop pointer locally or track
nested loops to prevent this?
quoted hunk ↗ jump to hunk
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-perf-gtk2-v2-0-3eccff053cd1@gmail.com?part=1