From 4a44d85e28bd282f53ccf0fa933dd71b8744a229 Mon Sep 17 00:00:00 2001 From: Seiji Aguchi Date: Mon, 5 Aug 2013 15:40:44 -0400 Subject: [PATCH 01/11] Convert stderr message calling error_get_pretty() to error_report() Convert stderr messages calling error_get_pretty() to error_report(). Timestamp is prepended by -msg timstamp option with it. Per Markus's comment below, A conversion from fprintf() to error_report() is always an improvement, regardless of error_get_pretty(). http://marc.info/?l=qemu-devel&m=137513283408601&w=2 But, it is not reasonable to convert them at one time because fprintf() is used everwhere in qemu. So, it should be done step by step with avoiding regression. Signed-off-by: Seiji Aguchi Reviewed-by: Laszlo Ersek Signed-off-by: Luiz Capitulino --- arch_init.c | 4 ++-- hw/char/serial.c | 5 +++-- hw/i386/pc.c | 6 +++--- qemu-char.c | 2 +- target-i386/cpu.c | 2 +- target-ppc/translate_init.c | 3 ++- vl.c | 9 +++++---- 7 files changed, 17 insertions(+), 14 deletions(-) diff --git a/arch_init.c b/arch_init.c index 68a7ab784f..94d45e1d2a 100644 --- a/arch_init.c +++ b/arch_init.c @@ -1125,8 +1125,8 @@ void do_acpitable_option(const QemuOpts *opts) acpi_table_add(opts, &err); if (err) { - fprintf(stderr, "Wrong acpi table provided: %s\n", - error_get_pretty(err)); + error_report("Wrong acpi table provided: %s", + error_get_pretty(err)); error_free(err); exit(1); } diff --git a/hw/char/serial.c b/hw/char/serial.c index 602559254e..a31eb5756a 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -27,6 +27,7 @@ #include "sysemu/char.h" #include "qemu/timer.h" #include "exec/address-spaces.h" +#include "qemu/error-report.h" //#define DEBUG_SERIAL @@ -696,7 +697,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, s->chr = chr; serial_realize_core(s, &err); if (err != NULL) { - fprintf(stderr, "%s\n", error_get_pretty(err)); + error_report("%s", error_get_pretty(err)); error_free(err); exit(1); } @@ -760,7 +761,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space, serial_realize_core(s, &err); if (err != NULL) { - fprintf(stderr, "%s\n", error_get_pretty(err)); + error_report("%s", error_get_pretty(err)); error_free(err); exit(1); } diff --git a/hw/i386/pc.c b/hw/i386/pc.c index e8bc8ce172..3a620a1856 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -978,7 +978,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge) cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), icc_bridge, &error); if (error) { - fprintf(stderr, "%s\n", error_get_pretty(error)); + error_report("%s", error_get_pretty(error)); error_free(error); exit(1); } @@ -1096,8 +1096,8 @@ void pc_acpi_init(const char *default_dsdt) acpi_table_add(opts, &err); if (err) { - fprintf(stderr, "WARNING: failed to load %s: %s\n", filename, - error_get_pretty(err)); + error_report("WARNING: failed to load %s: %s", filename, + error_get_pretty(err)); error_free(err); } g_free(arg); diff --git a/qemu-char.c b/qemu-char.c index 1be1cf676e..5446b8834c 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -3344,7 +3344,7 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in chr = qemu_chr_new_from_opts(opts, init, &err); if (error_is_set(&err)) { - fprintf(stderr, "%s\n", error_get_pretty(err)); + error_report("%s", error_get_pretty(err)); error_free(err); } if (chr && qemu_opt_get_bool(opts, "mux", 0)) { diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 6e3825220d..42c5de034e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1852,7 +1852,7 @@ X86CPU *cpu_x86_init(const char *cpu_model) out: if (error) { - fprintf(stderr, "%s\n", error_get_pretty(error)); + error_report("%s", error_get_pretty(error)); error_free(error); if (cpu != NULL) { object_unref(OBJECT(cpu)); diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 13b290c597..609f797083 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -27,6 +27,7 @@ #include "cpu-models.h" #include "mmu-hash32.h" #include "mmu-hash64.h" +#include "qemu/error-report.h" //#define PPC_DUMP_CPU //#define PPC_DEBUG_SPR @@ -8281,7 +8282,7 @@ PowerPCCPU *cpu_ppc_init(const char *cpu_model) object_property_set_bool(OBJECT(cpu), true, "realized", &err); if (err != NULL) { - fprintf(stderr, "%s\n", error_get_pretty(err)); + error_report("%s", error_get_pretty(err)); error_free(err); object_unref(OBJECT(cpu)); return NULL; diff --git a/vl.c b/vl.c index f422a1cae4..1c283c9fce 100644 --- a/vl.c +++ b/vl.c @@ -2393,7 +2393,7 @@ static int chardev_init_func(QemuOpts *opts, void *opaque) qemu_chr_new_from_opts(opts, NULL, &local_err); if (error_is_set(&local_err)) { - fprintf(stderr, "%s\n", error_get_pretty(local_err)); + error_report("%s", error_get_pretty(local_err)); error_free(local_err); return -1; } @@ -4375,8 +4375,8 @@ int main(int argc, char **argv, char **envp) vnc_display_init(ds); vnc_display_open(ds, vnc_display, &local_err); if (local_err != NULL) { - fprintf(stderr, "Failed to start VNC server on `%s': %s\n", - vnc_display, error_get_pretty(local_err)); + error_report("Failed to start VNC server on `%s': %s", + vnc_display, error_get_pretty(local_err)); error_free(local_err); exit(1); } @@ -4419,7 +4419,8 @@ int main(int argc, char **argv, char **envp) Error *local_err = NULL; qemu_start_incoming_migration(incoming, &local_err); if (local_err) { - fprintf(stderr, "-incoming %s: %s\n", incoming, error_get_pretty(local_err)); + error_report("-incoming %s: %s", incoming, + error_get_pretty(local_err)); error_free(local_err); exit(1); } From d95704341280fc521dc2b16bbbc5858f6647e2c3 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 20 Aug 2013 00:35:33 +0200 Subject: [PATCH 02/11] OptsVisitor: introduce basic list modes We're going to need more state while processing a list of repeated options. This change eliminates "repeated_opts_first" and adds a new state variable: list_mode repeated_opts repeated_opts_first -------------- ------------- ------------------- LM_NONE NULL false LM_STARTED non-NULL true LM_IN_PROGRESS non-NULL false Additionally, it is documented that lookup_scalar() and processed(), both called by opts_type_XXX(), are invalid in LM_STARTED -- generated qapi code calls opts_next_list() to allocate the very first link before trying to parse a scalar into it. List mode restrictions are expressed in positive / inclusive form. Signed-off-by: Laszlo Ersek Tested-by: Wanlong Gao Signed-off-by: Luiz Capitulino --- qapi/opts-visitor.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 174bd8bdb0..29fd1abd95 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -1,7 +1,7 @@ /* * Options Visitor * - * Copyright Red Hat, Inc. 2012 + * Copyright Red Hat, Inc. 2012, 2013 * * Author: Laszlo Ersek * @@ -18,6 +18,15 @@ #include "qapi/visitor-impl.h" +enum ListMode +{ + LM_NONE, /* not traversing a list of repeated options */ + LM_STARTED, /* opts_start_list() succeeded */ + LM_IN_PROGRESS /* opts_next_list() has been called */ +}; + +typedef enum ListMode ListMode; + struct OptsVisitor { Visitor visitor; @@ -35,8 +44,8 @@ struct OptsVisitor /* The list currently being traversed with opts_start_list() / * opts_next_list(). The list must have a struct element type in the * schema, with a single mandatory scalar member. */ + ListMode list_mode; GQueue *repeated_opts; - bool repeated_opts_first; /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does @@ -156,9 +165,11 @@ opts_start_list(Visitor *v, const char *name, Error **errp) OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); /* we can't traverse a list in a list */ - assert(ov->repeated_opts == NULL); + assert(ov->list_mode == LM_NONE); ov->repeated_opts = lookup_distinct(ov, name, errp); - ov->repeated_opts_first = (ov->repeated_opts != NULL); + if (ov->repeated_opts != NULL) { + ov->list_mode = LM_STARTED; + } } @@ -168,10 +179,13 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp) OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); GenericList **link; - if (ov->repeated_opts_first) { - ov->repeated_opts_first = false; + switch (ov->list_mode) { + case LM_STARTED: + ov->list_mode = LM_IN_PROGRESS; link = list; - } else { + break; + + case LM_IN_PROGRESS: { const QemuOpt *opt; opt = g_queue_pop_head(ov->repeated_opts); @@ -180,6 +194,11 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp) return NULL; } link = &(*list)->next; + break; + } + + default: + abort(); } *link = g_malloc0(sizeof **link); @@ -192,14 +211,16 @@ opts_end_list(Visitor *v, Error **errp) { OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); + assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS); ov->repeated_opts = NULL; + ov->list_mode = LM_NONE; } static const QemuOpt * lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp) { - if (ov->repeated_opts == NULL) { + if (ov->list_mode == LM_NONE) { GQueue *list; /* the last occurrence of any QemuOpt takes effect when queried by name @@ -207,6 +228,7 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp) list = lookup_distinct(ov, name, errp); return list ? g_queue_peek_tail(list) : NULL; } + assert(ov->list_mode == LM_IN_PROGRESS); return g_queue_peek_head(ov->repeated_opts); } @@ -214,9 +236,12 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp) static void processed(OptsVisitor *ov, const char *name) { - if (ov->repeated_opts == NULL) { + if (ov->list_mode == LM_NONE) { g_hash_table_remove(ov->unprocessed_opts, name); + return; } + assert(ov->list_mode == LM_IN_PROGRESS); + /* do nothing */ } @@ -365,7 +390,7 @@ opts_start_optional(Visitor *v, bool *present, const char *name, OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); /* we only support a single mandatory scalar field in a list node */ - assert(ov->repeated_opts == NULL); + assert(ov->list_mode == LM_NONE); *present = (lookup_distinct(ov, name, NULL) != NULL); } From d8754f40acb2d30e4735cdcd21a16e7ac29264a3 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 20 Aug 2013 00:35:34 +0200 Subject: [PATCH 03/11] OptsVisitor: introduce list modes for interval flattening The new modes are equal-rank, exclusive alternatives of LM_IN_PROGRESS. Teach opts_next_list(), opts_type_int() and opts_type_uint64() to handle them. Also enumerate explicitly what functions are valid to call in what modes: - opts_next_list() is valid to call while flattening a range, - opts_end_list(): ditto, - lookup_scalar() is invalid to call during flattening; generated qapi traversal code must continue asking for the same kind of signed/unsigned list element until the interval is fully flattened, - processed(): ditto. List mode restrictions are always formulated in positive / inclusive sense. The restrictions for lookup_scalar() and processed() are automatically satisfied by current qapi traversals if the schema to build is compatible with OptsVisitor. The new list modes are not entered yet. Signed-off-by: Laszlo Ersek Tested-by: Wanlong Gao Signed-off-by: Luiz Capitulino --- qapi/opts-visitor.c | 67 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 29fd1abd95..c2a57bd52d 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -22,7 +22,32 @@ enum ListMode { LM_NONE, /* not traversing a list of repeated options */ LM_STARTED, /* opts_start_list() succeeded */ - LM_IN_PROGRESS /* opts_next_list() has been called */ + + LM_IN_PROGRESS, /* opts_next_list() has been called. + * + * Generating the next list link will consume the most + * recently parsed QemuOpt instance of the repeated + * option. + * + * Parsing a value into the list link will examine the + * next QemuOpt instance of the repeated option, and + * possibly enter LM_SIGNED_INTERVAL or + * LM_UNSIGNED_INTERVAL. + */ + + LM_SIGNED_INTERVAL, /* opts_next_list() has been called. + * + * Generating the next list link will consume the most + * recently stored element from the signed interval, + * parsed from the most recent QemuOpt instance of the + * repeated option. This may consume QemuOpt itself + * and return to LM_IN_PROGRESS. + * + * Parsing a value into the list link will store the + * next element of the signed interval. + */ + + LM_UNSIGNED_INTERVAL /* Same as above, only for an unsigned interval. */ }; typedef enum ListMode ListMode; @@ -47,6 +72,15 @@ struct OptsVisitor ListMode list_mode; GQueue *repeated_opts; + /* When parsing a list of repeating options as integers, values of the form + * "a-b", representing a closed interval, are allowed. Elements in the + * range are generated individually. + */ + union { + int64_t s; + uint64_t u; + } range_next, range_limit; + /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does * not survive or escape the OptsVisitor object. @@ -185,6 +219,22 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp) link = list; break; + case LM_SIGNED_INTERVAL: + case LM_UNSIGNED_INTERVAL: + link = &(*list)->next; + + if (ov->list_mode == LM_SIGNED_INTERVAL) { + if (ov->range_next.s < ov->range_limit.s) { + ++ov->range_next.s; + break; + } + } else if (ov->range_next.u < ov->range_limit.u) { + ++ov->range_next.u; + break; + } + ov->list_mode = LM_IN_PROGRESS; + /* range has been completed, fall through in order to pop option */ + case LM_IN_PROGRESS: { const QemuOpt *opt; @@ -211,7 +261,10 @@ opts_end_list(Visitor *v, Error **errp) { OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); - assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS); + assert(ov->list_mode == LM_STARTED || + ov->list_mode == LM_IN_PROGRESS || + ov->list_mode == LM_SIGNED_INTERVAL || + ov->list_mode == LM_UNSIGNED_INTERVAL); ov->repeated_opts = NULL; ov->list_mode = LM_NONE; } @@ -303,6 +356,11 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) long long val; char *endptr; + if (ov->list_mode == LM_SIGNED_INTERVAL) { + *obj = ov->range_next.s; + return; + } + opt = lookup_scalar(ov, name, errp); if (!opt) { return; @@ -328,6 +386,11 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) const QemuOpt *opt; const char *str; + if (ov->list_mode == LM_UNSIGNED_INTERVAL) { + *obj = ov->range_next.u; + return; + } + opt = lookup_scalar(ov, name, errp); if (!opt) { return; From 1e1c555a49175e2298eaa156e008a92d207bf812 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 20 Aug 2013 00:35:35 +0200 Subject: [PATCH 04/11] OptsVisitor: opts_type_int(): recognize intervals when LM_IN_PROGRESS When a well-formed range value, bounded by signed integers, is encountered while processing a repeated option, enter LM_SIGNED_INTERVAL and return the low bound. Signed-off-by: Laszlo Ersek Tested-by: Wanlong Gao Signed-off-by: Luiz Capitulino --- qapi/opts-visitor.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index c2a57bd52d..90be583c5c 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -367,15 +367,37 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) } str = opt->str ? opt->str : ""; + /* we've gotten past lookup_scalar() */ + assert(ov->list_mode == LM_NONE || ov->list_mode == LM_IN_PROGRESS); + errno = 0; val = strtoll(str, &endptr, 0); - if (*str != '\0' && *endptr == '\0' && errno == 0 && INT64_MIN <= val && - val <= INT64_MAX) { - *obj = val; - processed(ov, name); - return; + if (errno == 0 && endptr > str && INT64_MIN <= val && val <= INT64_MAX) { + if (*endptr == '\0') { + *obj = val; + processed(ov, name); + return; + } + if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) { + long long val2; + + str = endptr + 1; + val2 = strtoll(str, &endptr, 0); + if (errno == 0 && endptr > str && *endptr == '\0' && + INT64_MIN <= val2 && val2 <= INT64_MAX && val <= val2) { + ov->range_next.s = val; + ov->range_limit.s = val2; + ov->list_mode = LM_SIGNED_INTERVAL; + + /* as if entering on the top */ + *obj = ov->range_next.s; + return; + } + } } - error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, "an int64 value"); + error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, + (ov->list_mode == LM_NONE) ? "an int64 value" : + "an int64 value or range"); } From 62d090e23fc17e4e60725ead0dff8902f8e66b52 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 20 Aug 2013 00:35:36 +0200 Subject: [PATCH 05/11] OptsVisitor: rebase opts_type_uint64() to parse_uint_full() Simplify the code in preparation for the next patch. Signed-off-by: Laszlo Ersek Tested-by: Wanlong Gao Signed-off-by: Luiz Capitulino --- qapi/opts-visitor.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 90be583c5c..d8f9a0e96e 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -407,6 +407,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); const QemuOpt *opt; const char *str; + unsigned long long val; if (ov->list_mode == LM_UNSIGNED_INTERVAL) { *obj = ov->range_next.u; @@ -417,26 +418,12 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) if (!opt) { return; } - str = opt->str; - if (str != NULL) { - while (isspace((unsigned char)*str)) { - ++str; - } - if (*str != '-' && *str != '\0') { - unsigned long long val; - char *endptr; - - /* non-empty, non-negative subject sequence */ - errno = 0; - val = strtoull(str, &endptr, 0); - if (*endptr == '\0' && errno == 0 && val <= UINT64_MAX) { - *obj = val; - processed(ov, name); - return; - } - } + if (parse_uint_full(str, &val, 0) == 0 && val <= UINT64_MAX) { + *obj = val; + processed(ov, name); + return; } error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, "an uint64 value"); From 581a8a800070500527f6c75e5d6b75134c2b5a9d Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 20 Aug 2013 00:35:37 +0200 Subject: [PATCH 06/11] OptsVisitor: opts_type_uint64(): recognize intervals when LM_IN_PROGRESS When a well-formed range value, bounded by unsigned integers, is encountered while processing a repeated option, enter LM_UNSIGNED_INTERVAL and return the low bound. Signed-off-by: Laszlo Ersek Tested-by: Wanlong Gao Signed-off-by: Luiz Capitulino --- qapi/opts-visitor.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index d8f9a0e96e..d54d373188 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -408,6 +408,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) const QemuOpt *opt; const char *str; unsigned long long val; + char *endptr; if (ov->list_mode == LM_UNSIGNED_INTERVAL) { *obj = ov->range_next.u; @@ -420,13 +421,34 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) } str = opt->str; - if (parse_uint_full(str, &val, 0) == 0 && val <= UINT64_MAX) { - *obj = val; - processed(ov, name); - return; + /* we've gotten past lookup_scalar() */ + assert(ov->list_mode == LM_NONE || ov->list_mode == LM_IN_PROGRESS); + + if (parse_uint(str, &val, &endptr, 0) == 0 && val <= UINT64_MAX) { + if (*endptr == '\0') { + *obj = val; + processed(ov, name); + return; + } + if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) { + unsigned long long val2; + + str = endptr + 1; + if (parse_uint_full(str, &val2, 0) == 0 && + val2 <= UINT64_MAX && val <= val2) { + ov->range_next.u = val; + ov->range_limit.u = val2; + ov->list_mode = LM_UNSIGNED_INTERVAL; + + /* as if entering on the top */ + *obj = ov->range_next.u; + return; + } + } } error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, - "an uint64 value"); + (ov->list_mode == LM_NONE) ? "a uint64 value" : + "a uint64 value or range"); } From 15a849be100b54776bcf63193c3fea598666030f Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 20 Aug 2013 00:35:38 +0200 Subject: [PATCH 07/11] OptsVisitor: don't try to flatten overlong integer ranges Prevent mistyped command line options from incurring high memory and CPU usage at startup. 64K elements in a range should be enough for everyone (TM). The OPTS_VISITOR_RANGE_MAX macro is public so that unit tests can construct corner cases with it. Signed-off-by: Laszlo Ersek Tested-by: Wanlong Gao Signed-off-by: Luiz Capitulino --- include/qapi/opts-visitor.h | 6 ++++++ qapi/opts-visitor.c | 7 +++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h index 5939eeebc7..fd48c14ec8 100644 --- a/include/qapi/opts-visitor.h +++ b/include/qapi/opts-visitor.h @@ -16,6 +16,12 @@ #include "qapi/visitor.h" #include "qemu/option.h" +/* Inclusive upper bound on the size of any flattened range. This is a safety + * (= anti-annoyance) measure; wrong ranges should not cause long startup + * delays nor exhaust virtual memory. + */ +#define OPTS_VISITOR_RANGE_MAX 65536 + typedef struct OptsVisitor OptsVisitor; /* Contrarily to qemu-option.c::parse_option_number(), OptsVisitor's "int" diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index d54d373188..96ed85899d 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -384,7 +384,9 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) str = endptr + 1; val2 = strtoll(str, &endptr, 0); if (errno == 0 && endptr > str && *endptr == '\0' && - INT64_MIN <= val2 && val2 <= INT64_MAX && val <= val2) { + INT64_MIN <= val2 && val2 <= INT64_MAX && val <= val2 && + (val > INT64_MAX - OPTS_VISITOR_RANGE_MAX || + val2 < val + OPTS_VISITOR_RANGE_MAX)) { ov->range_next.s = val; ov->range_limit.s = val2; ov->list_mode = LM_SIGNED_INTERVAL; @@ -435,7 +437,8 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) str = endptr + 1; if (parse_uint_full(str, &val2, 0) == 0 && - val2 <= UINT64_MAX && val <= val2) { + val2 <= UINT64_MAX && val <= val2 && + val2 - val < OPTS_VISITOR_RANGE_MAX) { ov->range_next.u = val; ov->range_limit.u = val2; ov->list_mode = LM_UNSIGNED_INTERVAL; From 99351c8472f76552c059a5dd382860229d647c4f Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 20 Aug 2013 00:35:39 +0200 Subject: [PATCH 08/11] add "test-int128" and "test-bitops" to .gitignore "test-int128" was probably missed in commit 6046c620 ("int128: optimize and add test cases"). "test-bitops" was probably missed in commit 3464700f ("tests: Add test-bitops.c with some sextract tests"). Signed-off-by: Laszlo Ersek Tested-by: Wanlong Gao Signed-off-by: Luiz Capitulino --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 0fe114d251..a8e0f1716b 100644 --- a/.gitignore +++ b/.gitignore @@ -45,7 +45,9 @@ qemu-bridge-helper qemu-monitor.texi vscclient QMP/qmp-commands.txt +test-bitops test-coroutine +test-int128 test-qmp-input-visitor test-qmp-output-visitor test-string-input-visitor From 3953e3a5d34fa7caffc3e32eae4270b2d810d966 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 20 Aug 2013 00:35:40 +0200 Subject: [PATCH 09/11] OptsVisitor: introduce unit tests, with test cases for range flattening According to commit 4f193e34 ("tests: Use qapi-schema-test.json as schema parser test") the "tests/qapi-schema/qapi-schema-test.out" file must be updated as well. Signed-off-by: Laszlo Ersek Tested-by: Wanlong Gao Signed-off-by: Luiz Capitulino --- .gitignore | 1 + tests/Makefile | 6 +- tests/qapi-schema/qapi-schema-test.json | 15 ++ tests/qapi-schema/qapi-schema-test.out | 6 +- tests/test-opts-visitor.c | 275 ++++++++++++++++++++++++ 5 files changed, 300 insertions(+), 3 deletions(-) create mode 100644 tests/test-opts-visitor.c diff --git a/.gitignore b/.gitignore index a8e0f1716b..d2c5c2f6b8 100644 --- a/.gitignore +++ b/.gitignore @@ -48,6 +48,7 @@ QMP/qmp-commands.txt test-bitops test-coroutine test-int128 +test-opts-visitor test-qmp-input-visitor test-qmp-output-visitor test-string-input-visitor diff --git a/tests/Makefile b/tests/Makefile index b0200fd60f..baba9e95ad 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -23,6 +23,8 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF) gcov-files-test-string-input-visitor-y = qapi/string-input-visitor.c check-unit-y += tests/test-string-output-visitor$(EXESUF) gcov-files-test-string-output-visitor-y = qapi/string-output-visitor.c +check-unit-y += tests/test-opts-visitor$(EXESUF) +gcov-files-test-opts-visitor-y = qapi/opts-visitor.c check-unit-y += tests/test-coroutine$(EXESUF) gcov-files-test-coroutine-y = coroutine-$(CONFIG_COROUTINE_BACKEND).c check-unit-y += tests/test-visitor-serialization$(EXESUF) @@ -100,7 +102,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \ tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \ tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \ tests/test-qmp-commands.o tests/test-visitor-serialization.o \ - tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o + tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \ + tests/test-opts-visitor.o test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o @@ -148,6 +151,7 @@ tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qap tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y) qapi-types.o qapi-visit.o libqemuutil.a libqemustub.a tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a +tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a tests/test-mul64$(EXESUF): tests/test-mul64.o libqemuutil.a tests/test-bitops$(EXESUF): tests/test-bitops.o libqemuutil.a diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 4434fa3961..fe5af756c5 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -51,3 +51,18 @@ { 'command': 'user_def_cmd', 'data': {} } { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} } { 'command': 'user_def_cmd2', 'data': {'ud1a': 'UserDefOne', 'ud1b': 'UserDefOne'}, 'returns': 'UserDefTwo' } + +# For testing integer range flattening in opts-visitor. The following schema +# corresponds to the option format: +# +# -userdef i64=3-6,i64=-5--1,u64=2,u16=1,u16=7-12 +# +# For simplicity, this example doesn't use [type=]discriminator nor optargs +# specific to discriminator values. +{ 'type': 'UserDefOptions', + 'data': { + '*i64' : [ 'int' ], + '*u64' : [ 'uint64' ], + '*u16' : [ 'uint16' ], + '*i64x': 'int' , + '*u64x': 'uint64' } } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index fb00344894..3851880de3 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -9,11 +9,13 @@ OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]), OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]), OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]), - OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')])] + OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]), + OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])] ['EnumOne', 'UserDefUnionKind', 'UserDefNativeListUnionKind'] [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]), OrderedDict([('type', 'UserDefOne'), ('data', OrderedDict([('integer', 'int'), ('string', 'str'), ('*enum1', 'EnumOne')]))]), OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string', 'str'), ('dict', OrderedDict([('string', 'str'), ('dict', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]), OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]), OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]), - OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))])] + OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]), + OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])] diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c new file mode 100644 index 0000000000..9f902b597e --- /dev/null +++ b/tests/test-opts-visitor.c @@ -0,0 +1,275 @@ +/* + * Options Visitor unit-tests. + * + * Copyright (C) 2013 Red Hat, Inc. + * + * Authors: + * Laszlo Ersek (based on test-string-output-visitor) + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include + +#include "qemu/config-file.h" /* qemu_add_opts() */ +#include "qemu/option.h" /* qemu_opts_parse() */ +#include "qapi/opts-visitor.h" /* opts_visitor_new() */ +#include "test-qapi-visit.h" /* visit_type_UserDefOptions() */ +#include "qapi/dealloc-visitor.h" /* qapi_dealloc_visitor_new() */ + +static QemuOptsList userdef_opts = { + .name = "userdef", + .head = QTAILQ_HEAD_INITIALIZER(userdef_opts.head), + .desc = { { 0 } } /* validated with OptsVisitor */ +}; + +/* fixture (= glib test case context) and test case manipulation */ + +typedef struct OptsVisitorFixture { + UserDefOptions *userdef; + Error *err; +} OptsVisitorFixture; + + +static void +setup_fixture(OptsVisitorFixture *f, gconstpointer test_data) +{ + const char *opts_string = test_data; + QemuOpts *opts; + OptsVisitor *ov; + + opts = qemu_opts_parse(qemu_find_opts("userdef"), opts_string, 0); + g_assert(opts != NULL); + + ov = opts_visitor_new(opts); + visit_type_UserDefOptions(opts_get_visitor(ov), &f->userdef, NULL, + &f->err); + opts_visitor_cleanup(ov); + qemu_opts_del(opts); +} + + +static void +teardown_fixture(OptsVisitorFixture *f, gconstpointer test_data) +{ + if (f->userdef != NULL) { + QapiDeallocVisitor *dv; + + dv = qapi_dealloc_visitor_new(); + visit_type_UserDefOptions(qapi_dealloc_get_visitor(dv), &f->userdef, + NULL, NULL); + qapi_dealloc_visitor_cleanup(dv); + } + error_free(f->err); +} + + +static void +add_test(const char *testpath, + void (*test_func)(OptsVisitorFixture *f, gconstpointer test_data), + gconstpointer test_data) +{ + g_test_add(testpath, OptsVisitorFixture, test_data, setup_fixture, + test_func, teardown_fixture); +} + +/* test output evaluation */ + +static void +expect_ok(OptsVisitorFixture *f, gconstpointer test_data) +{ + g_assert(f->err == NULL); + g_assert(f->userdef != NULL); +} + + +static void +expect_fail(OptsVisitorFixture *f, gconstpointer test_data) +{ + g_assert(f->err != NULL); + + /* The error message is printed when this test utility is invoked directly + * (ie. without gtester) and the --verbose flag is passed: + * + * tests/test-opts-visitor --verbose + */ + g_test_message("'%s': %s", (const char *)test_data, + error_get_pretty(f->err)); +} + + +static void +test_value(OptsVisitorFixture *f, gconstpointer test_data) +{ + uint64_t magic, bitval; + intList *i64; + uint64List *u64; + uint16List *u16; + + expect_ok(f, test_data); + + magic = 0; + for (i64 = f->userdef->i64; i64 != NULL; i64 = i64->next) { + g_assert(-16 <= i64->value && i64->value < 64-16); + bitval = 1ull << (i64->value + 16); + g_assert((magic & bitval) == 0); + magic |= bitval; + } + g_assert(magic == 0xDEADBEEF); + + magic = 0; + for (u64 = f->userdef->u64; u64 != NULL; u64 = u64->next) { + g_assert(u64->value < 64); + bitval = 1ull << u64->value; + g_assert((magic & bitval) == 0); + magic |= bitval; + } + g_assert(magic == 0xBADC0FFEE0DDF00D); + + magic = 0; + for (u16 = f->userdef->u16; u16 != NULL; u16 = u16->next) { + g_assert(u16->value < 64); + bitval = 1ull << u16->value; + g_assert((magic & bitval) == 0); + magic |= bitval; + } + g_assert(magic == 0xD15EA5E); +} + + +static void +expect_i64_min(OptsVisitorFixture *f, gconstpointer test_data) +{ + expect_ok(f, test_data); + g_assert(f->userdef->has_i64); + g_assert(f->userdef->i64->next == NULL); + g_assert(f->userdef->i64->value == INT64_MIN); +} + + +static void +expect_i64_max(OptsVisitorFixture *f, gconstpointer test_data) +{ + expect_ok(f, test_data); + g_assert(f->userdef->has_i64); + g_assert(f->userdef->i64->next == NULL); + g_assert(f->userdef->i64->value == INT64_MAX); +} + + +static void +expect_zero(OptsVisitorFixture *f, gconstpointer test_data) +{ + expect_ok(f, test_data); + g_assert(f->userdef->has_u64); + g_assert(f->userdef->u64->next == NULL); + g_assert(f->userdef->u64->value == 0); +} + + +static void +expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data) +{ + expect_ok(f, test_data); + g_assert(f->userdef->has_u64); + g_assert(f->userdef->u64->next == NULL); + g_assert(f->userdef->u64->value == UINT64_MAX); +} + +/* test cases */ + +int +main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + qemu_add_opts(&userdef_opts); + + /* Three hexadecimal magic numbers, "dead beef", "bad coffee, odd food" and + * "disease", from + * , were + * converted to binary and dissected into bit ranges. Each magic number is + * going to be recomposed using the lists called "i64", "u64" and "u16", + * respectively. + * + * (Note that these types pertain to the individual bit shift counts, not + * the magic numbers themselves; the intent is to exercise opts_type_int() + * and opts_type_uint64().) + * + * The "i64" shift counts have been decreased by 16 (decimal) in order to + * test negative values as well. Finally, the full list of QemuOpt elements + * has been permuted with "shuf". + * + * Both "i64" and "u64" have some (distinct) single-element ranges + * represented as both "a" and "a-a". "u16" is a special case of "i64" (see + * visit_type_uint16()), so it wouldn't add a separate test in this regard. + */ + + add_test("/visitor/opts/flatten/value", &test_value, + "i64=-1-0,u64=12-16,u64=2-3,i64=-11--9,u64=57,u16=9,i64=5-5," + "u16=1-4,u16=20,u64=63-63,i64=-16--13,u64=50-52,i64=14-15,u16=11," + "i64=7,u16=18,i64=2-3,u16=6,u64=54-55,u64=0,u64=18-20,u64=33-43," + "i64=9-12,u16=26-27,u64=59-61,u16=13-16,u64=29-31,u64=22-23," + "u16=24,i64=-7--3"); + + add_test("/visitor/opts/i64/val1/errno", &expect_fail, + "i64=0x8000000000000000"); + add_test("/visitor/opts/i64/val1/empty", &expect_fail, "i64="); + add_test("/visitor/opts/i64/val1/trailing", &expect_fail, "i64=5z"); + add_test("/visitor/opts/i64/nonlist", &expect_fail, "i64x=5-6"); + add_test("/visitor/opts/i64/val2/errno", &expect_fail, + "i64=0x7fffffffffffffff-0x8000000000000000"); + add_test("/visitor/opts/i64/val2/empty", &expect_fail, "i64=5-"); + add_test("/visitor/opts/i64/val2/trailing", &expect_fail, "i64=5-6z"); + add_test("/visitor/opts/i64/range/empty", &expect_fail, "i64=6-5"); + add_test("/visitor/opts/i64/range/minval", &expect_i64_min, + "i64=-0x8000000000000000--0x8000000000000000"); + add_test("/visitor/opts/i64/range/maxval", &expect_i64_max, + "i64=0x7fffffffffffffff-0x7fffffffffffffff"); + + add_test("/visitor/opts/u64/val1/errno", &expect_fail, "u64=-1"); + add_test("/visitor/opts/u64/val1/empty", &expect_fail, "u64="); + add_test("/visitor/opts/u64/val1/trailing", &expect_fail, "u64=5z"); + add_test("/visitor/opts/u64/nonlist", &expect_fail, "u64x=5-6"); + add_test("/visitor/opts/u64/val2/errno", &expect_fail, + "u64=0xffffffffffffffff-0x10000000000000000"); + add_test("/visitor/opts/u64/val2/empty", &expect_fail, "u64=5-"); + add_test("/visitor/opts/u64/val2/trailing", &expect_fail, "u64=5-6z"); + add_test("/visitor/opts/u64/range/empty", &expect_fail, "u64=6-5"); + add_test("/visitor/opts/u64/range/minval", &expect_zero, "u64=0-0"); + add_test("/visitor/opts/u64/range/maxval", &expect_u64_max, + "u64=0xffffffffffffffff-0xffffffffffffffff"); + + /* Test maximum range sizes. The macro value is open-coded here + * *intentionally*; the test case must use concrete values by design. If + * OPTS_VISITOR_RANGE_MAX is changed, the following values need to be + * recalculated as well. The assert and this comment should help with it. + */ + g_assert(OPTS_VISITOR_RANGE_MAX == 65536); + + /* The unsigned case is simple, a u64-u64 difference can always be + * represented as a u64. + */ + add_test("/visitor/opts/u64/range/max", &expect_ok, "u64=0-65535"); + add_test("/visitor/opts/u64/range/2big", &expect_fail, "u64=0-65536"); + + /* The same cannot be said about an i64-i64 difference. */ + add_test("/visitor/opts/i64/range/max/pos/a", &expect_ok, + "i64=0x7fffffffffff0000-0x7fffffffffffffff"); + add_test("/visitor/opts/i64/range/max/pos/b", &expect_ok, + "i64=0x7ffffffffffeffff-0x7ffffffffffffffe"); + add_test("/visitor/opts/i64/range/2big/pos", &expect_fail, + "i64=0x7ffffffffffeffff-0x7fffffffffffffff"); + add_test("/visitor/opts/i64/range/max/neg/a", &expect_ok, + "i64=-0x8000000000000000--0x7fffffffffff0001"); + add_test("/visitor/opts/i64/range/max/neg/b", &expect_ok, + "i64=-0x7fffffffffffffff--0x7fffffffffff0000"); + add_test("/visitor/opts/i64/range/2big/neg", &expect_fail, + "i64=-0x8000000000000000--0x7fffffffffff0000"); + add_test("/visitor/opts/i64/range/2big/full", &expect_fail, + "i64=-0x8000000000000000-0x7fffffffffffffff"); + + g_test_run(); + return 0; +} From 277acfe8b38de35be8cb6e274678b5a7919c2d44 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 20 Aug 2013 10:58:21 +0800 Subject: [PATCH 10/11] monitor: print the invalid char in error message It's more friendly to print which char is invalid to user, especially when user tries to input a float value and expect the monitor to round it to int. Since we don't round float number when we look for a integer, telling which char is invalid is less confusing. Signed-off-by: Fam Zheng Reviewed-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- monitor.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/monitor.c b/monitor.c index 5dc0aa97f5..da9c9a222b 100644 --- a/monitor.c +++ b/monitor.c @@ -3171,9 +3171,13 @@ static const MonitorDef monitor_defs[] = { { NULL }, }; -static void expr_error(Monitor *mon, const char *msg) +static void expr_error(Monitor *mon, const char *fmt, ...) { - monitor_printf(mon, "%s\n", msg); + va_list ap; + va_start(ap, fmt); + monitor_vprintf(mon, fmt, ap); + monitor_printf(mon, "\n"); + va_end(ap); siglongjmp(expr_env, 1); } @@ -3291,7 +3295,7 @@ static int64_t expr_unary(Monitor *mon) expr_error(mon, "number too large"); } if (pch == p) { - expr_error(mon, "invalid char in expression"); + expr_error(mon, "invalid char '%c' in expression", *p); } pch = p; while (qemu_isspace(*pch)) From 21e0043bada1a24ae2ba6cd0051e104c0cbf9634 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 20 Aug 2013 15:50:15 +0100 Subject: [PATCH 11/11] scripts/qapi.py: Avoid syntax not supported by Python 2.4 The Python "except Foo as x" syntax was only introduced in Python 2.6, but we aim to support Python 2.4 and later. Use the old-style "except Foo, x" syntax instead, thus fixing configure/compile on systems with older Python. Signed-off-by: Peter Maydell Signed-off-by: Luiz Capitulino --- scripts/qapi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 0ebea945bb..1069310f8d 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -161,7 +161,7 @@ class QAPISchema: def parse_schema(fp): try: schema = QAPISchema(fp) - except QAPISchemaError as e: + except QAPISchemaError, e: print >>sys.stderr, e exit(1)