Commit bd6fee9f authored by Markus Armbruster's avatar Markus Armbruster
Browse files

log: Fix qemu_set_dfilter_ranges() error reporting 



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: default avatarMarkus Armbruster <armbru@redhat.com>
Message-Id: <1466011636-6112-3-git-send-email-armbru@redhat.com>
Reviewed-by: default avatarEric Blake <eblake@redhat.com>
parent 2ec62fae

include/qemu/log.h

+1 −1
Original line number Diff line number Diff line
@@ -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);

tests/test-logging.c

+16 −33
Original line number Diff line number Diff line
@@ -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));

}



#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");

    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

/* 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);

util/log.c

+57 −56
Original line number Diff line number Diff line
@@ -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,24 +143,26 @@ 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));

        while (r) {

            char *range_op = strstr(r, "-");

            char *r2 = range_op ? range_op + 1 : NULL;

    for (i = 0; ranges[i]; i++) {

        const char *r = ranges[i];

        const char *range_op, *r2, *e;

        uint64_t r1val, r2val;

        struct Range range;



        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;
@@ -168,30 +171,36 @@ void qemu_set_dfilter_ranges(const char *filter_spec) 
            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;

        if (!range_op) {

            error_setg(errp, "Bad range specifier");

            goto out;

        }



                    g_assert(e == range_op);

        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;

        }



        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;
@@ -200,18 +209,10 @@ void qemu_set_dfilter_ranges(const char *filter_spec) 
            g_assert_not_reached();

        }

        g_array_append_val(debug_regions, range);



                } else {

                    g_error("Failed to parse range in: %s", r);

                }

            } else {

                g_error("Bad range specifier in: %s", r);

            }

            r = *next++;

    }

out:

    g_strfreev(ranges);

}

}



/* fflush() the log file */

void qemu_log_flush(void)

vl.c

+1 −1
Original line number Diff line number Diff line
@@ -3339,7 +3339,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);