[v7,8/9] qemu-log: support simple pid substitution for logs

Commit Message

Alex Bennée Feb. 22, 2016, 3:59 p.m. UTC

When debugging stuff that occurs over several forks it would be useful
not to keep overwriting the one logfile you've set-up. This allows a
simple %d to be included once in the logfile parameter which is
substituted with getpid().

As the test cases involve checking user output they need
g_test_trap_subprocess() support. As a result they are currently skipped
on Travis builds due to the older glib involved.

Signed-off-by: Alex Bennée <[e-mail]>
Reviewed-by: Leandro Dorileo <[e-mail]>
Reviewed-by: Aurelien Jarno <[e-mail]>

---
v5
  - add another r-b
v7
  - simpler error check as suggested by Eric Blake
  - don't g_error, just error_report (so we don't crash from monitor)
  - add some unit tests
---
 tests/test-logging.c | 39 +++++++++++++++++++++++++++++++++++++++
 util/log.c           | 22 ++++++++++++++++++++--
 2 files changed, 59 insertions(+), 2 deletions(-)

Comments

Richard Henderson Feb. 26, 2016, 8:21 p.m. UTC | #1

On 02/22/2016 07:59 AM, Alex Bennée wrote:
> When debugging stuff that occurs over several forks it would be useful
> not to keep overwriting the one logfile you've set-up. This allows a
> simple %d to be included once in the logfile parameter which is
> substituted with getpid().
> 
> As the test cases involve checking user output they need
> g_test_trap_subprocess() support. As a result they are currently skipped
> on Travis builds due to the older glib involved.
> 
> Signed-off-by: Alex Bennée <[e-mail]>
> Reviewed-by: Leandro Dorileo <[e-mail]>
> Reviewed-by: Aurelien Jarno <[e-mail]>
> 
> ---
> v5
>   - add another r-b
> v7
>   - simpler error check as suggested by Eric Blake
>   - don't g_error, just error_report (so we don't crash from monitor)
>   - add some unit tests
> ---
>  tests/test-logging.c | 39 +++++++++++++++++++++++++++++++++++++++
>  util/log.c           | 22 ++++++++++++++++++++--
>  2 files changed, 59 insertions(+), 2 deletions(-)


Reviewed-by: Richard Henderson <[e-mail]>


r~

Patch

diff --git a/tests/test-logging.c b/tests/test-logging.c
index c2dae49..2bcad92 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -62,14 +62,53 @@  static void test_parse_range(void)
     g_assert(qemu_log_in_addr_range(0x1050));
     g_assert(qemu_log_in_addr_range(0x2050));
     g_assert(qemu_log_in_addr_range(0x3050));
+}
 
+/* As the only real failure from a bad log filename path spec is
+ * reporting to the user we have to use the g_test_trap_subprocess
+ * mechanism and check no errors reported on stderr.
+ */
+#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
+static void test_parse_path(void)
+{
+    /* All these should work without issue */
+    if (g_test_subprocess()) {
+        qemu_set_log_filename("/tmp/qemu.log");
+        qemu_set_log_filename("/tmp/qemu-%d.log");
+        qemu_set_log_filename("/tmp/qemu.log.%d");
+        return;
+    }
+
+    g_test_trap_subprocess (NULL, 0, 0);
+    g_test_trap_assert_passed();
+    g_test_trap_assert_stdout("");
+    g_test_trap_assert_stderr("");
 }
 
+static void test_parse_invalid_path(void)
+{
+    if (g_test_subprocess()) {
+        qemu_set_log_filename("/tmp/qemu-%d%d.log");
+        return;
+    }
+
+    g_test_trap_subprocess (NULL, 0, 0);
+    g_test_trap_assert_passed();
+    g_test_trap_assert_stdout("");
+    g_test_trap_assert_stderr("Bad logfile format: /tmp/qemu-%d%d.log\n");
+}
+#endif /* CONFIG_HAS_GLIB_SUBPROCESS_TESTS */
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
     g_test_add_func("/logging/parse_range", test_parse_range);
 
+#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
+    g_test_add_func("/logging/parse_path", test_parse_path);
+    g_test_add_func("/logging/parse_invalid_path", test_parse_invalid_path);
+#endif
+
     return g_test_run();
 }
diff --git a/util/log.c b/util/log.c
index bd4a660..0ab856a 100644
--- a/util/log.c
+++ b/util/log.c
@@ -21,6 +21,7 @@ 
 #include "qemu-common.h"
 #include "qemu/log.h"
 #include "qemu/range.h"
+#include "qemu/error-report.h"
 #include "trace/control.h"
 
 static char *logfilename;
@@ -77,11 +78,28 @@  void do_qemu_set_log(int log_flags, bool use_own_buffers)
         qemu_log_close();
     }
 }
-
+/*
+ * Allow the user to include %d in their logfile which will be
+ * substituted with the current PID. This is useful for debugging many
+ * nested linux-user tasks but will result in lots of logs.
+ */
 void qemu_set_log_filename(const char *filename)
 {
+    char *pidstr;
     g_free(logfilename);
-    logfilename = g_strdup(filename);
+
+    pidstr = strstr(filename, "%");
+    if (pidstr) {
+        /* We only accept one %d, no other format strings */
+        if (pidstr[1] != 'd' || strchr(pidstr + 2, '%')) {
+            error_report("Bad logfile format: %s", filename);
+            logfilename = NULL;
+        } else {
+            logfilename = g_strdup_printf(filename, getpid());
+        }
+    } else {
+        logfilename = g_strdup(filename);
+    }
     qemu_log_close();
     qemu_set_log(qemu_loglevel);
 }