[Qemu-devel] [PATCH 0/3] log: Fix error handling and a memory leak

[Qemu-devel] [PATCH 0/3] log: Fix error handling and a memory leak

[Markus Armbruster]

log.c has no maintainer.  I can take this through my error-next
branch.

Markus Armbruster (3):
  log: Plug memory leak on multiple -dfilter
  log: Fix qemu_set_dfilter_ranges() error reporting
  log: Fix qemu_set_log_filename() error handling

 bsd-user/main.c      |   3 +-
 include/qemu/log.h   |   4 +-
 linux-user/main.c    |   3 +-
 monitor.c            |   7 ++-
 tests/test-logging.c |  88 ++++++++++---------------------------
 trace/control.c      |   3 +-
 util/log.c           | 122 +++++++++++++++++++++++++++------------------------
 vl.c                 |   4 +-
 8 files changed, 104 insertions(+), 130 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/3] log: Plug memory leak on multiple -dfilter

[Markus Armbruster]

-dfilter overwrites any previous filter.  The overwritten filter is
leaked.  Leaks since the beginning (commit 3514552, v2.6.0).  Free it
properly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/log.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/util/log.c b/util/log.c
index 5ad72c1..6f45e0a 100644
--- a/util/log.c
+++ b/util/log.c
@@ -145,9 +145,16 @@ bool qemu_log_in_addr_range(uint64_t addr)
 void qemu_set_dfilter_ranges(const char *filter_spec)
 {
     gchar **ranges = g_strsplit(filter_spec, ",", 0);
+
+    if (debug_regions) {
+        g_array_unref(debug_regions);
+        debug_regions = NULL;
+    }
+
     if (ranges) {
         gchar **next = ranges;
         gchar *r = *next++;
+
         debug_regions = g_array_sized_new(FALSE, FALSE,
                                           sizeof(Range), g_strv_length(ranges));
         while (r) {
-- 
2.5.5

[Qemu-devel] [PATCH 2/3] log: Fix qemu_set_dfilter_ranges() error reporting

[Markus Armbruster]

g_error() is not an acceptable way to report errors to the user:

    $ qemu-system-x86_64 -dfilter 1000+0

    ** (process:17187): ERROR **: Failed to parse range in: 1000+0
    Trace/breakpoint trap (core dumped)

g_assert() isn't, either:

    $ qemu-system-x86_64 -dfilter 1000x+64
    **
    ERROR:/work/armbru/qemu/util/log.c:180:qemu_set_dfilter_ranges: assertion failed: (e == range_op)
    Aborted (core dumped)

Convert qemu_set_dfilter_ranges() to Error.  Rework its deeply nested
control flow.  Touch up the error messages.  Call it with
&error_fatal.

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/log.h   |   2 +-
 tests/test-logging.c |  49 ++++++++--------------
 util/log.c           | 113 ++++++++++++++++++++++++++-------------------------
 vl.c                 |   2 +-
 4 files changed, 75 insertions(+), 91 deletions(-)

diff --git a/include/qemu/log.h b/include/qemu/log.h
index 234fa81..df8d041 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -107,7 +107,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_dfilter_ranges(const char *ranges);
+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/tests/test-logging.c b/tests/test-logging.c
index 5ef5bb8..e043adc 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -27,11 +27,14 @@
 #include "qemu/osdep.h"
 
 #include "qemu-common.h"
-#include "include/qemu/log.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
 
 static void test_parse_range(void)
 {
-    qemu_set_dfilter_ranges("0x1000+0x100");
+    Error *err = NULL;
+
+    qemu_set_dfilter_ranges("0x1000+0x100", &error_abort);
 
     g_assert_false(qemu_log_in_addr_range(0xfff));
     g_assert(qemu_log_in_addr_range(0x1000));
@@ -39,56 +42,40 @@ static void test_parse_range(void)
     g_assert(qemu_log_in_addr_range(0x10ff));
     g_assert_false(qemu_log_in_addr_range(0x1100));
 
-    qemu_set_dfilter_ranges("0x1000-0x100");
+    qemu_set_dfilter_ranges("0x1000-0x100", &error_abort);
 
     g_assert_false(qemu_log_in_addr_range(0x1001));
     g_assert(qemu_log_in_addr_range(0x1000));
     g_assert(qemu_log_in_addr_range(0x0f01));
     g_assert_false(qemu_log_in_addr_range(0x0f00));
 
-    qemu_set_dfilter_ranges("0x1000..0x1100");
+    qemu_set_dfilter_ranges("0x1000..0x1100", &error_abort);
 
     g_assert_false(qemu_log_in_addr_range(0xfff));
     g_assert(qemu_log_in_addr_range(0x1000));
     g_assert(qemu_log_in_addr_range(0x1100));
     g_assert_false(qemu_log_in_addr_range(0x1101));
 
-    qemu_set_dfilter_ranges("0x1000..0x1000");
+    qemu_set_dfilter_ranges("0x1000..0x1000", &error_abort);
 
     g_assert_false(qemu_log_in_addr_range(0xfff));
     g_assert(qemu_log_in_addr_range(0x1000));
     g_assert_false(qemu_log_in_addr_range(0x1001));
 
-    qemu_set_dfilter_ranges("0x1000+0x100,0x2100-0x100,0x3000..0x3100");
+    qemu_set_dfilter_ranges("0x1000+0x100,0x2100-0x100,0x3000..0x3100",
+                            &error_abort);
     g_assert(qemu_log_in_addr_range(0x1050));
     g_assert(qemu_log_in_addr_range(0x2050));
     g_assert(qemu_log_in_addr_range(0x3050));
+
+    qemu_set_dfilter_ranges("0x1000+onehundred", &err);
+    error_free_or_abort(&err);
+
+    qemu_set_dfilter_ranges("0x1000+0", &err);
+    error_free_or_abort(&err);
 }
 
 #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
-static void test_parse_invalid_range_subprocess(void)
-{
-    qemu_set_dfilter_ranges("0x1000+onehundred");
-}
-static void test_parse_invalid_range(void)
-{
-    g_test_trap_subprocess("/logging/parse_invalid_range/subprocess", 0, 0);
-    g_test_trap_assert_failed();
-    g_test_trap_assert_stdout("");
-    g_test_trap_assert_stderr("*Failed to parse range in: 0x1000+onehundred\n");
-}
-static void test_parse_zero_range_subprocess(void)
-{
-    qemu_set_dfilter_ranges("0x1000+0");
-}
-static void test_parse_zero_range(void)
-{
-    g_test_trap_subprocess("/logging/parse_zero_range/subprocess", 0, 0);
-    g_test_trap_assert_failed();
-    g_test_trap_assert_stdout("");
-    g_test_trap_assert_stderr("*Failed to parse range in: 0x1000+0\n");
-}
-
 /* 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.
@@ -126,10 +113,6 @@ int main(int argc, char **argv)
 
     g_test_add_func("/logging/parse_range", test_parse_range);
 #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
-    g_test_add_func("/logging/parse_invalid_range/subprocess", test_parse_invalid_range_subprocess);
-    g_test_add_func("/logging/parse_invalid_range", test_parse_invalid_range);
-    g_test_add_func("/logging/parse_zero_range/subprocess", test_parse_zero_range_subprocess);
-    g_test_add_func("/logging/parse_zero_range", test_parse_zero_range);
     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);
diff --git a/util/log.c b/util/log.c
index 6f45e0a..fcf85c6 100644
--- a/util/log.c
+++ b/util/log.c
@@ -22,6 +22,7 @@
 #include "qemu/log.h"
 #include "qemu/range.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "trace/control.h"
 
@@ -142,75 +143,75 @@ bool qemu_log_in_addr_range(uint64_t addr)
 }
 
 
-void qemu_set_dfilter_ranges(const char *filter_spec)
+void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp)
 {
     gchar **ranges = g_strsplit(filter_spec, ",", 0);
+    int i;
 
     if (debug_regions) {
         g_array_unref(debug_regions);
         debug_regions = NULL;
     }
 
-    if (ranges) {
-        gchar **next = ranges;
-        gchar *r = *next++;
+    debug_regions = g_array_sized_new(FALSE, FALSE,
+                                      sizeof(Range), g_strv_length(ranges));
+    for (i = 0; ranges[i]; i++) {
+        const char *r = ranges[i];
+        const char *range_op, *r2, *e;
+        uint64_t r1val, r2val;
+        struct Range range;
 
-        debug_regions = g_array_sized_new(FALSE, FALSE,
-                                          sizeof(Range), g_strv_length(ranges));
-        while (r) {
-            char *range_op = strstr(r, "-");
-            char *r2 = range_op ? range_op + 1 : NULL;
-            if (!range_op) {
-                range_op = strstr(r, "+");
-                r2 = range_op ? range_op + 1 : NULL;
-            }
-            if (!range_op) {
-                range_op = strstr(r, "..");
-                r2 = range_op ? range_op + 2 : NULL;
-            }
-            if (range_op) {
-                const char *e = NULL;
-                uint64_t r1val, r2val;
-
-                if ((qemu_strtoull(r, &e, 0, &r1val) == 0) &&
-                    (qemu_strtoull(r2, NULL, 0, &r2val) == 0) &&
-                    r2val > 0) {
-                    struct Range range;
-
-                    g_assert(e == range_op);
+        range_op = strstr(r, "-");
+        r2 = range_op ? range_op + 1 : NULL;
+        if (!range_op) {
+            range_op = strstr(r, "+");
+            r2 = range_op ? range_op + 1 : NULL;
+        }
+        if (!range_op) {
+            range_op = strstr(r, "..");
+            r2 = range_op ? range_op + 2 : NULL;
+        }
+        if (!range_op) {
+            error_setg(errp, "Bad range specifier");
+            goto out;
+        }
 
-                    switch (*range_op) {
-                    case '+':
-                    {
-                        range.begin = r1val;
-                        range.end = r1val + (r2val - 1);
-                        break;
-                    }
-                    case '-':
-                    {
-                        range.end = r1val;
-                        range.begin = r1val - (r2val - 1);
-                        break;
-                    }
-                    case '.':
-                        range.begin = r1val;
-                        range.end = r2val;
-                        break;
-                    default:
-                        g_assert_not_reached();
-                    }
-                    g_array_append_val(debug_regions, range);
+        if (qemu_strtoull(r, &e, 0, &r1val)
+            || e != range_op) {
+            error_setg(errp, "Invalid number to the left of %.*s",
+                       (int)(r2 - range_op), range_op);
+            goto out;
+        }
+        if (qemu_strtoull(r2, NULL, 0, &r2val)) {
+            error_setg(errp, "Invalid number to the right of %.*s",
+                       (int)(r2 - range_op), range_op);
+            goto out;
+        }
+        if (r2val == 0) {
+            error_setg(errp, "Invalid range");
+            goto out;
+        }
 
-                } else {
-                    g_error("Failed to parse range in: %s", r);
-                }
-            } else {
-                g_error("Bad range specifier in: %s", r);
-            }
-            r = *next++;
+        switch (*range_op) {
+        case '+':
+            range.begin = r1val;
+            range.end = r1val + (r2val - 1);
+            break;
+        case '-':
+            range.end = r1val;
+            range.begin = r1val - (r2val - 1);
+            break;
+        case '.':
+            range.begin = r1val;
+            range.end = r2val;
+            break;
+        default:
+            g_assert_not_reached();
         }
-        g_strfreev(ranges);
+        g_array_append_val(debug_regions, range);
     }
+out:
+    g_strfreev(ranges);
 }
 
 /* fflush() the log file */
diff --git a/vl.c b/vl.c
index 45eff56..0a42577 100644
--- a/vl.c
+++ b/vl.c
@@ -3345,7 +3345,7 @@ int main(int argc, char **argv, char **envp)
                 log_file = optarg;
                 break;
             case QEMU_OPTION_DFILTER:
-                qemu_set_dfilter_ranges(optarg);
+                qemu_set_dfilter_ranges(optarg, &error_fatal);
                 break;
             case QEMU_OPTION_s:
                 add_device_config(DEV_GDB, "tcp::" DEFAULT_GDBSTUB_PORT);
-- 
2.5.5

[Qemu-devel] [PATCH 3/3] log: Fix qemu_set_log_filename() error handling

[Markus Armbruster]

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>
---
 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 <machine/trap.h>
 #include <sys/mman.h>
 
+#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 <sys/syscall.h>
 #include <sys/resource.h>
 
+#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) {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/3] log: Plug memory leak on multiple -dfilter

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

On 06/15/2016 11:27 AM, Markus Armbruster wrote:
> -dfilter overwrites any previous filter.  The overwritten filter is
> leaked.  Leaks since the beginning (commit 3514552, v2.6.0).  Free it
> properly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/log.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] log: Fix qemu_set_dfilter_ranges() error reporting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

On 06/15/2016 11:27 AM, Markus Armbruster wrote:
> g_error() is not an acceptable way to report errors to the user:
> 
>     $ qemu-system-x86_64 -dfilter 1000+0
> 
>     ** (process:17187): ERROR **: Failed to parse range in: 1000+0
>     Trace/breakpoint trap (core dumped)
> 
> g_assert() isn't, either:
> 
>     $ qemu-system-x86_64 -dfilter 1000x+64
>     **
>     ERROR:/work/armbru/qemu/util/log.c:180:qemu_set_dfilter_ranges: assertion failed: (e == range_op)
>     Aborted (core dumped)

I see you're trying to improve my range.h patches, and got dragged into
this stuff.

> 
> Convert qemu_set_dfilter_ranges() to Error.  Rework its deeply nested
> control flow.  Touch up the error messages.  Call it with
> &error_fatal.
> 
> This also permits testing without a subprocess, so do that.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qemu/log.h   |   2 +-
>  tests/test-logging.c |  49 ++++++++--------------
>  util/log.c           | 113 ++++++++++++++++++++++++++-------------------------
>  vl.c                 |   2 +-
>  4 files changed, 75 insertions(+), 91 deletions(-)
> 

> +    qemu_set_dfilter_ranges("0x1000+onehundred", &err);
> +    error_free_or_abort(&err);
> +
> +    qemu_set_dfilter_ranges("0x1000+0", &err);
> +    error_free_or_abort(&err);
>  }
>  

Maybe also worth testing "0x" and "0x1000+0x" for being invalid?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/3] log: Fix qemu_set_log_filename() error handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

On 06/15/2016 11:27 AM, Markus Armbruster wrote:
> 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>
> ---
>  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 <machine/trap.h>
>  #include <sys/mman.h>
>  
> +#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);

Commit race between this patch and Denis' series:

https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg00515.html

Whoever goes in second will have a build failure until all the new uses
are also converted.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] log: Fix qemu_set_dfilter_ranges() error reporting

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/15/2016 11:27 AM, Markus Armbruster wrote:
>> g_error() is not an acceptable way to report errors to the user:
>> 
>>     $ qemu-system-x86_64 -dfilter 1000+0
>> 
>>     ** (process:17187): ERROR **: Failed to parse range in: 1000+0
>>     Trace/breakpoint trap (core dumped)
>> 
>> g_assert() isn't, either:
>> 
>>     $ qemu-system-x86_64 -dfilter 1000x+64
>>     **
>>     ERROR:/work/armbru/qemu/util/log.c:180:qemu_set_dfilter_ranges: assertion failed: (e == range_op)
>>     Aborted (core dumped)
>
> I see you're trying to improve my range.h patches, and got dragged into
> this stuff.

Yup, except I'd call it "build upon", not "improve".

>> Convert qemu_set_dfilter_ranges() to Error.  Rework its deeply nested
>> control flow.  Touch up the error messages.  Call it with
>> &error_fatal.
>> 
>> This also permits testing without a subprocess, so do that.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qemu/log.h   |   2 +-
>>  tests/test-logging.c |  49 ++++++++--------------
>>  util/log.c           | 113 ++++++++++++++++++++++++++-------------------------
>>  vl.c                 |   2 +-
>>  4 files changed, 75 insertions(+), 91 deletions(-)
>> 
>
>> +    qemu_set_dfilter_ranges("0x1000+onehundred", &err);
>> +    error_free_or_abort(&err);
>> +
>> +    qemu_set_dfilter_ranges("0x1000+0", &err);
>> +    error_free_or_abort(&err);
>>  }
>>  
>
> Maybe also worth testing "0x" and "0x1000+0x" for being invalid?

The former would add a test for the error handling of
qemu_set_dfilter_ranges()'s other use of qemu_strtoull().  If it wasn't
worth testing before my patch...  But I can add a test case anyway, if I
need to respin.

The latter would basically test an additional error path within
qemu_strtoull().

>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!