log: Fix qemu_set_log_filename() error handling

When qemu_set_log_filename() detects an invalid file name, it reports
an error, closes the log file (if any), and starts logging to stderr
(unless daemonized or nothing is being logged).

This is wrong.  Asking for an invalid log file on the command line
should be fatal.  Asking for one in the monitor should fail without
messing up an existing logfile.

Fix by converting qemu_set_log_filename() to Error.  Pass it
&error_fatal, except for hmp_logfile report errors.

This also permits testing without a subprocess, so do that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1466011636-6112-4-git-send-email-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

diff --git a/bsd-user/main.c b/bsd-user/main.c
index abe9a26f9bc4657e406f3017accaebc8660b875d..4819b9ec63339cb19cd4a4b2f6c9e0f218b1deca 100644 (file)
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 #include <machine/trap.h>
 
+#include "qapi/error.h"
 #include "qemu.h"
 #include "qemu/path.h"
 #include "qemu/help_option.h"
@@ -847,7 +848,7 @@ int main(int argc, char **argv)
 
     /* init debug */
     qemu_log_needs_buffers();
-    qemu_set_log_filename(log_file);
+    qemu_set_log_filename(log_file, &error_fatal);
     if (log_mask) {
         int mask;
 

diff --git a/include/qemu/log.h b/include/qemu/log.h
index df8d041854dc7f15b7ce00791bc0250c1c7993de..8bec6b403930c5de6d5ae8be24d90c4715144847 100644 (file)
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -106,7 +106,7 @@ extern const QEMULogItem qemu_log_items[];
 
 void qemu_set_log(int log_flags);
 void qemu_log_needs_buffers(void);
-void qemu_set_log_filename(const char *filename);
+void qemu_set_log_filename(const char *filename, Error **errp);
 void qemu_set_dfilter_ranges(const char *ranges, Error **errp);
 bool qemu_log_in_addr_range(uint64_t addr);
 int qemu_str_to_log_mask(const char *str);

diff --git a/linux-user/main.c b/linux-user/main.c
index b9a4e0ea45acf951eaa022b1b6c456291fded9d8..358ed01e96ea6138a2dbce44aea3ad3e77085f43 100644 (file)
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -21,6 +21,7 @@
 #include <sys/syscall.h>
 #include <sys/resource.h>
 
+#include "qapi/error.h"
 #include "qemu.h"
 #include "qemu/path.h"
 #include "qemu/cutils.h"
@@ -3845,7 +3846,7 @@ static void handle_arg_log(const char *arg)
 
 static void handle_arg_log_filename(const char *arg)
 {
-    qemu_set_log_filename(arg);
+    qemu_set_log_filename(arg, &error_fatal);
 }
 
 static void handle_arg_set_env(const char *arg)

diff --git a/monitor.c b/monitor.c
index a5d054b03960aac235e930b35aeaf3d6b0858212..6f960f13a6059c39713df5ca0562cfc5b8bb0882 100644 (file)
--- a/monitor.c
+++ b/monitor.c
@@ -1111,7 +1111,12 @@ void qmp_client_migrate_info(const char *protocol, const char *hostname,
 
 static void hmp_logfile(Monitor *mon, const QDict *qdict)
 {
-    qemu_set_log_filename(qdict_get_str(qdict, "filename"));
+    Error *err = NULL;
+
+    qemu_set_log_filename(qdict_get_str(qdict, "filename"), &err);
+    if (err) {
+        error_report_err(err);
+    }
 }
 
 static void hmp_log(Monitor *mon, const QDict *qdict)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index e043adc8f8dcaf257d52829c933801e6d4f57c0e..440e75f5dbc6ce737350d33fa770882a183494b4 100644 (file)
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -75,49 +75,24 @@ static void test_parse_range(void)
     error_free_or_abort(&err);
 }
 
-#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
-/* 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.
- */
-static void test_parse_path_subprocess(void)
-{
-    /* All these should work without issue */
-    qemu_set_log_filename("/tmp/qemu.log");
-    qemu_set_log_filename("/tmp/qemu-%d.log");
-    qemu_set_log_filename("/tmp/qemu.log.%d");
-}
 static void test_parse_path(void)
 {
-    g_test_trap_subprocess ("/logging/parse_path/subprocess", 0, 0);
-    g_test_trap_assert_passed();
-    g_test_trap_assert_stdout("");
-    g_test_trap_assert_stderr("");
-}
-static void test_parse_invalid_path_subprocess(void)
-{
-    qemu_set_log_filename("/tmp/qemu-%d%d.log");
-}
-static void test_parse_invalid_path(void)
-{
-    g_test_trap_subprocess ("/logging/parse_invalid_path/subprocess", 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");
+    Error *err = NULL;
+
+    qemu_set_log_filename("/tmp/qemu.log", &error_abort);
+    qemu_set_log_filename("/tmp/qemu-%d.log", &error_abort);
+    qemu_set_log_filename("/tmp/qemu.log.%d", &error_abort);
+
+    qemu_set_log_filename("/tmp/qemu-%d%d.log", &err);
+    error_free_or_abort(&err);
 }
-#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_path/subprocess", test_parse_path_subprocess);
-    g_test_add_func("/logging/parse_invalid_path", test_parse_invalid_path);
-    g_test_add_func("/logging/parse_invalid_path/subprocess", test_parse_invalid_path_subprocess);
-#endif
 
     return g_test_run();
 }

diff --git a/trace/control.c b/trace/control.c
index d099f735d50b7516ee25934f52d991150ac208d0..e1556a35704ee81d6dc0437777cbed7c782fb8c2 100644 (file)
--- a/trace/control.c
+++ b/trace/control.c
@@ -19,6 +19,7 @@
 #ifdef CONFIG_TRACE_LOG
 #include "qemu/log.h"
 #endif
+#include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "monitor/monitor.h"
 
@@ -187,7 +188,7 @@ void trace_init_file(const char *file)
      * only applies to the simple backend; use "-D" for the log backend.
      */
     if (file) {
-        qemu_set_log_filename(file);
+        qemu_set_log_filename(file, &error_fatal);
     }
 #else
     if (file) {

diff --git a/util/log.c b/util/log.c
index fcf85c6253b3ec7bbcc40e50a3fe4d31814be81e..32e416051cdf695e1471e9e34ff27aa2bdd7a0f5 100644 (file)
--- a/util/log.c
+++ b/util/log.c
@@ -103,7 +103,7 @@ void qemu_log_needs_buffers(void)
  * 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)
+void qemu_set_log_filename(const char *filename, Error **errp)
 {
     char *pidstr;
     g_free(logfilename);
@@ -112,8 +112,8 @@ void qemu_set_log_filename(const char *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;
+            error_setg(errp, "Bad logfile format: %s", filename);
+            return;
         } else {
             logfilename = g_strdup_printf(filename, getpid());
         }

diff --git a/vl.c b/vl.c
index 749c4212342c52e20c0e4a7137fc64b131b92b73..c85833a63c28ea9481e9ac2a59670a97efc2c548 100644 (file)
--- a/vl.c
+++ b/vl.c
@@ -4054,7 +4054,7 @@ int main(int argc, char **argv, char **envp)
     /* Open the logfile at this point and set the log mask if necessary.
      */
     if (log_file) {
-        qemu_set_log_filename(log_file);
+        qemu_set_log_filename(log_file, &error_fatal);
     }
 
     if (log_mask) {