From patchwork Wed Jun 15 17:27:16 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 635993 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rVD7r5Jm1z9s9W for ; Thu, 16 Jun 2016 03:31:04 +1000 (AEST) Received: from localhost ([::1]:43999 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDEec-0001WZ-Mz for incoming@patchwork.ozlabs.org; Wed, 15 Jun 2016 13:31:02 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57432) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDEb7-0006hL-I5 for qemu-devel@nongnu.org; Wed, 15 Jun 2016 13:27:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDEb3-0007Tc-AH for qemu-devel@nongnu.org; Wed, 15 Jun 2016 13:27:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41272) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDEb3-0007TJ-1w for qemu-devel@nongnu.org; Wed, 15 Jun 2016 13:27:21 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C62363710C9; Wed, 15 Jun 2016 17:27:19 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-116-20.ams2.redhat.com [10.36.116.20]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5FHRHSg018827 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 15 Jun 2016 13:27:18 -0400 Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id 9B2EA1138648; Wed, 15 Jun 2016 19:27:16 +0200 (CEST) From: Markus Armbruster To: qemu-devel@nongnu.org Date: Wed, 15 Jun 2016 19:27:16 +0200 Message-Id: <1466011636-6112-4-git-send-email-armbru@redhat.com> In-Reply-To: <1466011636-6112-1-git-send-email-armbru@redhat.com> References: <1466011636-6112-1-git-send-email-armbru@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Wed, 15 Jun 2016 17:27:19 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH 3/3] log: Fix qemu_set_log_filename() error handling X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: pbonzini@redhat.com, alex.bennee@linaro.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" 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 Reviewed-by: Eric Blake --- bsd-user/main.c | 3 ++- include/qemu/log.h | 2 +- linux-user/main.c | 3 ++- monitor.c | 7 ++++++- tests/test-logging.c | 41 ++++++++--------------------------------- trace/control.c | 3 ++- util/log.c | 6 +++--- vl.c | 2 +- 8 files changed, 25 insertions(+), 42 deletions(-) diff --git a/bsd-user/main.c b/bsd-user/main.c index 9f592be..3d6a4f4 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -20,6 +20,7 @@ #include #include +#include "qapi/error.h" #include "qemu.h" #include "qemu/path.h" #include "qemu/help_option.h" @@ -848,7 +849,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 df8d041..8bec6b4 100644 --- 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 f8a8764..1dd8af3 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -22,6 +22,7 @@ #include #include +#include "qapi/error.h" #include "qemu.h" #include "qemu/path.h" #include "qemu/cutils.h" @@ -3846,7 +3847,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 a27e115..29a51bf 100644 --- 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 e043adc..440e75f 100644 --- 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(""); + 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); } -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"); -} -#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 d099f73..e1556a3 100644 --- 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 fcf85c6..32e4160 100644 --- 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 0a42577..71e9623 100644 --- a/vl.c +++ b/vl.c @@ -4058,7 +4058,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) {