From 78ee6bd04821847036a805cb4bdd46464e1d3098 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Mon, 13 Apr 2020 00:35:56 +0200 Subject: [PATCH 01/32] various: Remove suspicious '\' character outside of #define in C code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the following coccinelle warnings: $ spatch --sp-file --verbose-parsing ... \ scripts/coccinelle/remove_local_err.cocci ... SUSPICIOUS: a \ character appears outside of a #define at ./target/ppc/translate_init.inc.c:5213 SUSPICIOUS: a \ character appears outside of a #define at ./target/ppc/translate_init.inc.c:5261 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:166 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:167 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:169 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:170 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:171 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:172 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:173 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5787 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5789 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5800 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5801 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5802 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5804 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5805 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5806 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:6329 SUSPICIOUS: a \ character appears outside of a #define at ./hw/sd/sdhci.c:1133 SUSPICIOUS: a \ character appears outside of a #define at ./hw/scsi/scsi-disk.c:3081 SUSPICIOUS: a \ character appears outside of a #define at ./hw/net/virtio-net.c:1529 SUSPICIOUS: a \ character appears outside of a #define at ./hw/riscv/sifive_u.c:468 SUSPICIOUS: a \ character appears outside of a #define at ./dump/dump.c:1895 SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2209 SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2215 SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2221 SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2222 SUSPICIOUS: a \ character appears outside of a #define at ./block/replication.c:172 SUSPICIOUS: a \ character appears outside of a #define at ./block/replication.c:173 Reviewed-by: Marc-André Lureau Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20200412223619.11284-2-f4bug@amsat.org> Reviewed-by: Alistair Francis Acked-by: David Gibson Signed-off-by: Markus Armbruster --- block/replication.c | 4 ++-- block/vhdx.c | 8 ++++---- dump/dump.c | 2 +- hw/net/virtio-net.c | 2 +- hw/riscv/sifive_u.c | 2 +- hw/scsi/scsi-disk.c | 2 +- hw/sd/sdhci.c | 2 +- target/i386/cpu.c | 18 +++++++++--------- target/microblaze/cpu.c | 14 +++++++------- target/ppc/translate_init.inc.c | 4 ++-- 10 files changed, 29 insertions(+), 29 deletions(-) diff --git a/block/replication.c b/block/replication.c index da013c2041..971f0fe266 100644 --- a/block/replication.c +++ b/block/replication.c @@ -172,8 +172,8 @@ static void replication_child_perm(BlockDriverState *bs, BdrvChild *c, if ((bs->open_flags & (BDRV_O_INACTIVE | BDRV_O_RDWR)) == BDRV_O_RDWR) { *nperm |= BLK_PERM_WRITE; } - *nshared = BLK_PERM_CONSISTENT_READ \ - | BLK_PERM_WRITE \ + *nshared = BLK_PERM_CONSISTENT_READ + | BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED; return; } diff --git a/block/vhdx.c b/block/vhdx.c index 33e57cd656..e16fdc2f2d 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -2206,20 +2206,20 @@ static QemuOptsList vhdx_create_opts = { .name = VHDX_BLOCK_OPT_BLOCK_SIZE, .type = QEMU_OPT_SIZE, .def_value_str = stringify(0), - .help = "Block Size; min 1MB, max 256MB. " \ + .help = "Block Size; min 1MB, max 256MB. " "0 means auto-calculate based on image size." }, { .name = BLOCK_OPT_SUBFMT, .type = QEMU_OPT_STRING, - .help = "VHDX format type, can be either 'dynamic' or 'fixed'. "\ + .help = "VHDX format type, can be either 'dynamic' or 'fixed'. " "Default is 'dynamic'." }, { .name = VHDX_BLOCK_OPT_ZERO, .type = QEMU_OPT_BOOL, - .help = "Force use of payload blocks of type 'ZERO'. "\ - "Non-standard, but default. Do not set to 'off' when "\ + .help = "Force use of payload blocks of type 'ZERO'. " + "Non-standard, but default. Do not set to 'off' when " "using 'qemu-img convert' with subformat=dynamic." }, { NULL } diff --git a/dump/dump.c b/dump/dump.c index 22ed1d3b0d..248ea06370 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -1892,7 +1892,7 @@ static void dump_process(DumpState *s, Error **errp) result = qmp_query_dump(NULL); /* should never fail */ assert(result); - qapi_event_send_dump_completed(result, !!local_err, (local_err ? \ + qapi_event_send_dump_completed(result, !!local_err, (local_err ? error_get_pretty(local_err) : NULL)); qapi_free_DumpQueryResult(result); diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index a46e3b37a7..eddfa7f923 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1526,7 +1526,7 @@ static void virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain, + sizeof(struct eth_header)); unit->ip = ip6; unit->ip_plen = &(ip6->ip6_ctlun.ip6_un1.ip6_un1_plen); - unit->tcp = (struct tcp_header *)(((uint8_t *)unit->ip)\ + unit->tcp = (struct tcp_header *)(((uint8_t *)unit->ip) + sizeof(struct ip6_header)); unit->tcp_hdrlen = (htons(unit->tcp->th_offset_flags) & 0xF000) >> 10; diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 56351c4faa..998666c91f 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -465,7 +465,7 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) object_property_add_bool(obj, "start-in-flash", sifive_u_get_start_in_flash, sifive_u_set_start_in_flash, NULL); object_property_set_description(obj, "start-in-flash", - "Set on to tell QEMU's ROM to jump to " \ + "Set on to tell QEMU's ROM to jump to " "flash. Otherwise QEMU will jump to DRAM", NULL); } diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 1c0cb63a6f..e5bcd0baf8 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -3078,7 +3078,7 @@ static const TypeInfo scsi_cd_info = { #ifdef __linux__ static Property scsi_block_properties[] = { - DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf), \ + DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf), DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk), DEFINE_PROP_BOOL("share-rw", SCSIDiskState, qdev.conf.share_rw, false), DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0), diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index de63ffb037..70531ad360 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1130,7 +1130,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) /* Limit block size to the maximum buffer size */ if (extract32(s->blksize, 0, 12) > s->buf_maxsz) { - qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than " \ + qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than " "the maximum buffer 0x%x", __func__, s->blksize, s->buf_maxsz); diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 90ffc5f3b1..9c256ab159 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -5784,9 +5784,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, host_cpuid(index, 0, eax, ebx, ecx, edx); break; } - *eax = (L1_DTLB_2M_ASSOC << 24) | (L1_DTLB_2M_ENTRIES << 16) | \ + *eax = (L1_DTLB_2M_ASSOC << 24) | (L1_DTLB_2M_ENTRIES << 16) | (L1_ITLB_2M_ASSOC << 8) | (L1_ITLB_2M_ENTRIES); - *ebx = (L1_DTLB_4K_ASSOC << 24) | (L1_DTLB_4K_ENTRIES << 16) | \ + *ebx = (L1_DTLB_4K_ASSOC << 24) | (L1_DTLB_4K_ENTRIES << 16) | (L1_ITLB_4K_ASSOC << 8) | (L1_ITLB_4K_ENTRIES); *ecx = encode_cache_cpuid80000005(env->cache_info_amd.l1d_cache); *edx = encode_cache_cpuid80000005(env->cache_info_amd.l1i_cache); @@ -5797,13 +5797,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, host_cpuid(index, 0, eax, ebx, ecx, edx); break; } - *eax = (AMD_ENC_ASSOC(L2_DTLB_2M_ASSOC) << 28) | \ - (L2_DTLB_2M_ENTRIES << 16) | \ - (AMD_ENC_ASSOC(L2_ITLB_2M_ASSOC) << 12) | \ + *eax = (AMD_ENC_ASSOC(L2_DTLB_2M_ASSOC) << 28) | + (L2_DTLB_2M_ENTRIES << 16) | + (AMD_ENC_ASSOC(L2_ITLB_2M_ASSOC) << 12) | (L2_ITLB_2M_ENTRIES); - *ebx = (AMD_ENC_ASSOC(L2_DTLB_4K_ASSOC) << 28) | \ - (L2_DTLB_4K_ENTRIES << 16) | \ - (AMD_ENC_ASSOC(L2_ITLB_4K_ASSOC) << 12) | \ + *ebx = (AMD_ENC_ASSOC(L2_DTLB_4K_ASSOC) << 28) | + (L2_DTLB_4K_ENTRIES << 16) | + (AMD_ENC_ASSOC(L2_ITLB_4K_ASSOC) << 12) | (L2_ITLB_4K_ENTRIES); encode_cache_cpuid80000006(env->cache_info_amd.l2_cache, cpu->enable_l3_cache ? @@ -6326,7 +6326,7 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp) */ env->features[w] |= x86_cpu_get_supported_feature_word(w, cpu->migratable) & - ~env->user_features[w] & \ + ~env->user_features[w] & ~feature_word_info[w].no_autoenable_flags; } } diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index a2c2f271df..c9cf2364ca 100644 --- a/target/microblaze/cpu.c +++ b/target/microblaze/cpu.c @@ -163,14 +163,14 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp) qemu_init_vcpu(cs); - env->pvr.regs[0] = PVR0_USE_EXC_MASK \ - | PVR0_USE_ICACHE_MASK \ + env->pvr.regs[0] = PVR0_USE_EXC_MASK + | PVR0_USE_ICACHE_MASK | PVR0_USE_DCACHE_MASK; - env->pvr.regs[2] = PVR2_D_OPB_MASK \ - | PVR2_D_LMB_MASK \ - | PVR2_I_OPB_MASK \ - | PVR2_I_LMB_MASK \ - | PVR2_FPU_EXC_MASK \ + env->pvr.regs[2] = PVR2_D_OPB_MASK + | PVR2_D_LMB_MASK + | PVR2_I_OPB_MASK + | PVR2_I_LMB_MASK + | PVR2_FPU_EXC_MASK | 0; version = cpu->cfg.version ? cpu->cfg.version : DEFAULT_CPU_VERSION; diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c index e853164a86..fd763e588e 100644 --- a/target/ppc/translate_init.inc.c +++ b/target/ppc/translate_init.inc.c @@ -5210,7 +5210,7 @@ POWERPC_FAMILY(e5500)(ObjectClass *oc, void *data) PPC_FLOAT_STFIWX | PPC_WAIT | PPC_MEM_TLBSYNC | PPC_TLBIVAX | PPC_MEM_SYNC | PPC_64B | PPC_POPCNTB | PPC_POPCNTWD; - pcc->insns_flags2 = PPC2_BOOKE206 | PPC2_PRCNTL | PPC2_PERM_ISA206 | \ + pcc->insns_flags2 = PPC2_BOOKE206 | PPC2_PRCNTL | PPC2_PERM_ISA206 | PPC2_FP_CVT_S64; pcc->msr_mask = (1ull << MSR_CM) | (1ull << MSR_GS) | @@ -5258,7 +5258,7 @@ POWERPC_FAMILY(e6500)(ObjectClass *oc, void *data) PPC_FLOAT_STFIWX | PPC_WAIT | PPC_MEM_TLBSYNC | PPC_TLBIVAX | PPC_MEM_SYNC | PPC_64B | PPC_POPCNTB | PPC_POPCNTWD | PPC_ALTIVEC; - pcc->insns_flags2 = PPC2_BOOKE206 | PPC2_PRCNTL | PPC2_PERM_ISA206 | \ + pcc->insns_flags2 = PPC2_BOOKE206 | PPC2_PRCNTL | PPC2_PERM_ISA206 | PPC2_FP_CVT_S64 | PPC2_ATOMIC_ISA206; pcc->msr_mask = (1ull << MSR_CM) | (1ull << MSR_GS) | From 32c2dcf5e87bf05153df92e49de75f72d4466e4e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:19 +0200 Subject: [PATCH 02/32] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt() The two turn out to be inconsistent for "a,b,,help". Test case marked /* BUG */. Signed-off-by: Markus Armbruster Message-Id: <20200415074927.19897-2-armbru@redhat.com> Reviewed-by: Eric Blake --- tests/test-qemu-opts.c | 44 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index ef96e84aed..88a3e7bdf4 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -728,6 +728,49 @@ static void test_opts_parse_size(void) qemu_opts_reset(&opts_list_02); } +static void test_has_help_option(void) +{ + static const struct { + const char *params; + /* expected value of has_help_option() */ + bool expect_has_help_option; + /* expected value of qemu_opt_has_help_opt() with implied=false */ + bool expect_opt_has_help_opt; + /* expected value of qemu_opt_has_help_opt() with implied=true */ + bool expect_opt_has_help_opt_implied; + } test[] = { + { "help", true, true, false }, + { "?", true, true, false }, + { "helpme", false, false, false }, + { "?me", false, false, false }, + { "a,help", true, true, true }, + { "a,?", true, true, true }, + { "a=0,help,b", true, true, true }, + { "a=0,?,b", true, true, true }, + { "help,b=1", true, true, false }, + { "?,b=1", true, true, false }, + { "a,b,,help", false /* BUG */, true, true }, + { "a,b,,?", false /* BUG */, true, true }, + }; + int i; + QemuOpts *opts; + + for (i = 0; i < ARRAY_SIZE(test); i++) { + g_assert_cmpint(has_help_option(test[i].params), + ==, test[i].expect_has_help_option); + opts = qemu_opts_parse(&opts_list_03, test[i].params, false, + &error_abort); + g_assert_cmpint(qemu_opt_has_help_opt(opts), + ==, test[i].expect_opt_has_help_opt); + qemu_opts_del(opts); + opts = qemu_opts_parse(&opts_list_03, test[i].params, true, + &error_abort); + g_assert_cmpint(qemu_opt_has_help_opt(opts), + ==, test[i].expect_opt_has_help_opt_implied); + qemu_opts_del(opts); + } +} + static void append_verify_list_01(QemuOptDesc *desc, bool with_overlapping) { int i = 0; @@ -990,6 +1033,7 @@ int main(int argc, char *argv[]) g_test_add_func("/qemu-opts/opts_parse/bool", test_opts_parse_bool); g_test_add_func("/qemu-opts/opts_parse/number", test_opts_parse_number); g_test_add_func("/qemu-opts/opts_parse/size", test_opts_parse_size); + g_test_add_func("/qemu-opts/has_help_option", test_has_help_option); g_test_add_func("/qemu-opts/append_to_null", test_opts_append_to_null); g_test_add_func("/qemu-opts/append", test_opts_append); g_test_add_func("/qemu-opts/to_qdict/basic", test_opts_to_qdict_basic); From 6129803b55553b90805aa5012077b21c6c6eacdc Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:20 +0200 Subject: [PATCH 03/32] qemu-options: Factor out get_opt_name_value() helper The next commits will put it to use. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Message-Id: <20200415074927.19897-3-armbru@redhat.com> --- util/qemu-option.c | 102 +++++++++++++++++++++++++-------------------- 1 file changed, 56 insertions(+), 46 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index 97172b5eaa..f08f4bc458 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -805,61 +805,71 @@ void qemu_opts_print(QemuOpts *opts, const char *separator) } } +static const char *get_opt_name_value(const char *params, + const char *firstname, + char **name, char **value) +{ + const char *p, *pe, *pc; + + pe = strchr(params, '='); + pc = strchr(params, ','); + + if (!pe || (pc && pc < pe)) { + /* found "foo,more" */ + if (firstname) { + /* implicitly named first option */ + *name = g_strdup(firstname); + p = get_opt_value(params, value); + } else { + /* option without value, must be a flag */ + p = get_opt_name(params, name, ','); + if (strncmp(*name, "no", 2) == 0) { + memmove(*name, *name + 2, strlen(*name + 2) + 1); + *value = g_strdup("off"); + } else { + *value = g_strdup("on"); + } + } + } else { + /* found "foo=bar,more" */ + p = get_opt_name(params, name, '='); + assert(*p == '='); + p++; + p = get_opt_value(p, value); + } + + assert(!*p || *p == ','); + if (*p == ',') { + p++; + } + return p; +} + static void opts_do_parse(QemuOpts *opts, const char *params, const char *firstname, bool prepend, bool *invalidp, Error **errp) { - char *option = NULL; - char *value = NULL; - const char *p,*pe,*pc; Error *local_err = NULL; + char *option, *value; + const char *p; - for (p = params; *p != '\0'; p++) { - pe = strchr(p, '='); - pc = strchr(p, ','); - if (!pe || (pc && pc < pe)) { - /* found "foo,more" */ - if (p == params && firstname) { - /* implicitly named first option */ - option = g_strdup(firstname); - p = get_opt_value(p, &value); - } else { - /* option without value, probably a flag */ - p = get_opt_name(p, &option, ','); - if (strncmp(option, "no", 2) == 0) { - memmove(option, option+2, strlen(option+2)+1); - value = g_strdup("off"); - } else { - value = g_strdup("on"); - } - } - } else { - /* found "foo=bar,more" */ - p = get_opt_name(p, &option, '='); - assert(*p == '='); - p++; - p = get_opt_value(p, &value); - } - if (strcmp(option, "id") != 0) { - /* store and parse */ - opt_set(opts, option, value, prepend, invalidp, &local_err); - value = NULL; - if (local_err) { - error_propagate(errp, local_err); - goto cleanup; - } - } - if (*p != ',') { - break; + for (p = params; *p;) { + p = get_opt_name_value(p, firstname, &option, &value); + firstname = NULL; + + if (!strcmp(option, "id")) { + g_free(option); + g_free(value); + continue; } + + opt_set(opts, option, value, prepend, invalidp, &local_err); g_free(option); - g_free(value); - option = value = NULL; + if (local_err) { + error_propagate(errp, local_err); + return; + } } - - cleanup: - g_free(option); - g_free(value); } /** From 933d1527785fe839300459abb486905094d192a7 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:21 +0200 Subject: [PATCH 04/32] qemu-option: Fix sloppy recognition of "id=..." after ",," Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Message-Id: <20200415074927.19897-4-armbru@redhat.com> --- tests/test-qemu-opts.c | 4 ++-- util/qemu-option.c | 27 +++++++++++++++++++-------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 88a3e7bdf4..8ff97268d8 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -500,10 +500,10 @@ static void test_opts_parse(void) g_assert(!opts); /* TODO Cover .merge_lists = true */ - /* Buggy ID recognition */ + /* Buggy ID recognition (fixed) */ opts = qemu_opts_parse(&opts_list_03, "x=,,id=bar", false, &error_abort); g_assert_cmpuint(opts_count(opts), ==, 1); - g_assert_cmpstr(qemu_opts_id(opts), ==, "bar"); /* BUG */ + g_assert(!qemu_opts_id(opts)); g_assert_cmpstr(qemu_opt_get(opts, "x"), ==, ",id=bar"); /* Anti-social ID */ diff --git a/util/qemu-option.c b/util/qemu-option.c index f08f4bc458..d2956082bd 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -872,6 +872,24 @@ static void opts_do_parse(QemuOpts *opts, const char *params, } } +static char *opts_parse_id(const char *params) +{ + const char *p; + char *name, *value; + + for (p = params; *p;) { + p = get_opt_name_value(p, NULL, &name, &value); + if (!strcmp(name, "id")) { + g_free(name); + return value; + } + g_free(name); + g_free(value); + } + + return NULL; +} + /** * Store options parsed from @params into @opts. * If @firstname is non-null, the first key=value in @params may omit @@ -889,20 +907,13 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, bool *invalidp, Error **errp) { const char *firstname; - char *id = NULL; - const char *p; + char *id = opts_parse_id(params); QemuOpts *opts; Error *local_err = NULL; assert(!permit_abbrev || list->implied_opt_name); firstname = permit_abbrev ? list->implied_opt_name : NULL; - if (strncmp(params, "id=", 3) == 0) { - get_opt_value(params + 3, &id); - } else if ((p = strstr(params, ",id=")) != NULL) { - get_opt_value(p + 4, &id); - } - /* * This code doesn't work for defaults && !list->merge_lists: when * params has no id=, and list has an element with !opts->id, it From 80a94855737622436a9b5cd25315b9c80d7e3ffa Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:22 +0200 Subject: [PATCH 05/32] qemu-option: Fix has_help_option()'s sloppy parsing has_help_option() uses its own parser. It's inconsistent with qemu_opts_parse(), as demonstrated by test-qemu-opts case /qemu-opts/has_help_option. Fix by reusing the common parser. Signed-off-by: Markus Armbruster Message-Id: <20200415074927.19897-5-armbru@redhat.com> Reviewed-by: Eric Blake --- tests/test-qemu-opts.c | 4 ++-- util/qemu-option.c | 39 +++++++++++++++++++-------------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 8ff97268d8..77c944c4aa 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -749,8 +749,8 @@ static void test_has_help_option(void) { "a=0,?,b", true, true, true }, { "help,b=1", true, true, false }, { "?,b=1", true, true, false }, - { "a,b,,help", false /* BUG */, true, true }, - { "a,b,,?", false /* BUG */, true, true }, + { "a,b,,help", true, true, true }, + { "a,b,,?", true, true, true }, }; int i; QemuOpts *opts; diff --git a/util/qemu-option.c b/util/qemu-option.c index d2956082bd..0abf26b61f 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value, *ret = size; } -bool has_help_option(const char *param) -{ - const char *p = param; - bool result = false; - - while (*p && !result) { - char *value; - - p = get_opt_value(p, &value); - if (*p) { - p++; - } - - result = is_help_option(value); - g_free(value); - } - - return result; -} - bool is_valid_option_list(const char *p) { char *value = NULL; @@ -890,6 +870,25 @@ static char *opts_parse_id(const char *params) return NULL; } +bool has_help_option(const char *params) +{ + const char *p; + char *name, *value; + bool ret; + + for (p = params; *p;) { + p = get_opt_name_value(p, NULL, &name, &value); + ret = is_help_option(name); + g_free(name); + g_free(value); + if (ret) { + return true; + } + } + + return false; +} + /** * Store options parsed from @params into @opts. * If @firstname is non-null, the first key=value in @params may omit From 59d27ebc446baec4a972cc146df732910b5aa2de Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:23 +0200 Subject: [PATCH 06/32] test-qemu-opts: Simplify test_has_help_option() after bug fix Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Message-Id: <20200415074927.19897-6-armbru@redhat.com> --- tests/test-qemu-opts.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 77c944c4aa..2a0f42a09b 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -732,41 +732,39 @@ static void test_has_help_option(void) { static const struct { const char *params; - /* expected value of has_help_option() */ - bool expect_has_help_option; /* expected value of qemu_opt_has_help_opt() with implied=false */ - bool expect_opt_has_help_opt; + bool expect; /* expected value of qemu_opt_has_help_opt() with implied=true */ - bool expect_opt_has_help_opt_implied; + bool expect_implied; } test[] = { - { "help", true, true, false }, - { "?", true, true, false }, - { "helpme", false, false, false }, - { "?me", false, false, false }, - { "a,help", true, true, true }, - { "a,?", true, true, true }, - { "a=0,help,b", true, true, true }, - { "a=0,?,b", true, true, true }, - { "help,b=1", true, true, false }, - { "?,b=1", true, true, false }, - { "a,b,,help", true, true, true }, - { "a,b,,?", true, true, true }, + { "help", true, false }, + { "?", true, false }, + { "helpme", false, false }, + { "?me", false, false }, + { "a,help", true, true }, + { "a,?", true, true }, + { "a=0,help,b", true, true }, + { "a=0,?,b", true, true }, + { "help,b=1", true, false }, + { "?,b=1", true, false }, + { "a,b,,help", true, true }, + { "a,b,,?", true, true }, }; int i; QemuOpts *opts; for (i = 0; i < ARRAY_SIZE(test); i++) { g_assert_cmpint(has_help_option(test[i].params), - ==, test[i].expect_has_help_option); + ==, test[i].expect); opts = qemu_opts_parse(&opts_list_03, test[i].params, false, &error_abort); g_assert_cmpint(qemu_opt_has_help_opt(opts), - ==, test[i].expect_opt_has_help_opt); + ==, test[i].expect); qemu_opts_del(opts); opts = qemu_opts_parse(&opts_list_03, test[i].params, true, &error_abort); g_assert_cmpint(qemu_opt_has_help_opt(opts), - ==, test[i].expect_opt_has_help_opt_implied); + ==, test[i].expect_implied); qemu_opts_del(opts); } } From 56a9efa199a603b77e7f2bd0e84e11e897bf7473 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:24 +0200 Subject: [PATCH 07/32] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily() When opts_parse() sets @invalidp to true, qemu_opts_parse_noisily() uses has_help_option() to decide whether to print help. This parses the input string a second time. Easy to avoid: replace @invalidp by @help_wanted. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20200415074927.19897-7-armbru@redhat.com> --- util/qemu-option.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index 0abf26b61f..2d0d24ee27 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -519,7 +519,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name) } static void opt_set(QemuOpts *opts, const char *name, char *value, - bool prepend, bool *invalidp, Error **errp) + bool prepend, bool *help_wanted, Error **errp) { QemuOpt *opt; const QemuOptDesc *desc; @@ -529,8 +529,8 @@ static void opt_set(QemuOpts *opts, const char *name, char *value, if (!desc && !opts_accepts_any(opts)) { g_free(value); error_setg(errp, QERR_INVALID_PARAMETER, name); - if (invalidp) { - *invalidp = true; + if (help_wanted && is_help_option(name)) { + *help_wanted = true; } return; } @@ -827,7 +827,7 @@ static const char *get_opt_name_value(const char *params, static void opts_do_parse(QemuOpts *opts, const char *params, const char *firstname, bool prepend, - bool *invalidp, Error **errp) + bool *help_wanted, Error **errp) { Error *local_err = NULL; char *option, *value; @@ -843,7 +843,7 @@ static void opts_do_parse(QemuOpts *opts, const char *params, continue; } - opt_set(opts, option, value, prepend, invalidp, &local_err); + opt_set(opts, option, value, prepend, help_wanted, &local_err); g_free(option); if (local_err) { error_propagate(errp, local_err); @@ -903,7 +903,7 @@ void qemu_opts_do_parse(QemuOpts *opts, const char *params, static QemuOpts *opts_parse(QemuOptsList *list, const char *params, bool permit_abbrev, bool defaults, - bool *invalidp, Error **errp) + bool *help_wanted, Error **errp) { const char *firstname; char *id = opts_parse_id(params); @@ -928,7 +928,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, return NULL; } - opts_do_parse(opts, params, firstname, defaults, invalidp, &local_err); + opts_do_parse(opts, params, firstname, defaults, help_wanted, &local_err); if (local_err) { error_propagate(errp, local_err); qemu_opts_del(opts); @@ -964,11 +964,11 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params, { Error *err = NULL; QemuOpts *opts; - bool invalidp = false; + bool help_wanted = false; - opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err); + opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err); if (err) { - if (invalidp && has_help_option(params)) { + if (help_wanted) { qemu_opts_print_help(list, true); error_free(err); } else { From 6d2b5cbafb8fb4bb3563cbf698b3a0903a993d7a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:25 +0200 Subject: [PATCH 08/32] qemu-img: Factor out accumulate_options() helper Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Message-Id: <20200415074927.19897-8-armbru@redhat.com> --- qemu-img.c | 59 +++++++++++++++++++++--------------------------------- 1 file changed, 23 insertions(+), 36 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 821cbf610e..d36b21b758 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -223,6 +223,25 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts) return true; } +static int accumulate_options(char **options, char *optarg) +{ + char *new_options; + + if (!is_valid_option_list(optarg)) { + error_report("Invalid option list: %s", optarg); + return -1; + } + + if (!*options) { + *options = g_strdup(optarg); + } else { + new_options = g_strdup_printf("%s,%s", *options, optarg); + g_free(*options); + *options = new_options; + } + return 0; +} + static QemuOptsList qemu_source_opts = { .name = "source", .implied_opt_name = "file", @@ -482,17 +501,9 @@ static int img_create(int argc, char **argv) fmt = optarg; break; case 'o': - if (!is_valid_option_list(optarg)) { - error_report("Invalid option list: %s", optarg); + if (accumulate_options(&options, optarg) < 0) { goto fail; } - if (!options) { - options = g_strdup(optarg); - } else { - char *old_options = options; - options = g_strdup_printf("%s,%s", options, optarg); - g_free(old_options); - } break; case 'q': quiet = true; @@ -2127,17 +2138,9 @@ static int img_convert(int argc, char **argv) s.compressed = true; break; case 'o': - if (!is_valid_option_list(optarg)) { - error_report("Invalid option list: %s", optarg); + if (accumulate_options(&options, optarg) < 0) { goto fail_getopt; } - if (!options) { - options = g_strdup(optarg); - } else { - char *old_options = options; - options = g_strdup_printf("%s,%s", options, optarg); - g_free(old_options); - } break; case 'l': if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) { @@ -3953,18 +3956,10 @@ static int img_amend(int argc, char **argv) help(); break; case 'o': - if (!is_valid_option_list(optarg)) { - error_report("Invalid option list: %s", optarg); + if (accumulate_options(&options, optarg) < 0) { ret = -1; goto out_no_progress; } - if (!options) { - options = g_strdup(optarg); - } else { - char *old_options = options; - options = g_strdup_printf("%s,%s", options, optarg); - g_free(old_options); - } break; case 'f': fmt = optarg; @@ -4855,17 +4850,9 @@ static int img_measure(int argc, char **argv) out_fmt = optarg; break; case 'o': - if (!is_valid_option_list(optarg)) { - error_report("Invalid option list: %s", optarg); + if (accumulate_options(&options, optarg) < 0) { goto out; } - if (!options) { - options = g_strdup(optarg); - } else { - char *old_options = options; - options = g_strdup_printf("%s,%s", options, optarg); - g_free(old_options); - } break; case 'l': if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) { From 80c710cb06ff40b45de033e4352528b3adcd2de9 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:26 +0200 Subject: [PATCH 09/32] qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely join multiple parameter strings separated by ',' like this: g_strdup_printf("%s,%s", params1, params2); How it does that is anything but obvious. A close reading of the code reveals that it fails exactly when its argument starts with ',' or ends with an odd number of ','. Makes sense, actually, because when the argument starts with ',', a separating ',' preceding it would get escaped, and when it ends with an odd number of ',', a separating ',' following it would get escaped. Move it to qemu-img.c and rewrite it the obvious way. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20200415074927.19897-9-armbru@redhat.com> --- include/qemu/option.h | 1 - qemu-img.c | 26 ++++++++++++++++++++++++++ util/qemu-option.c | 22 ---------------------- 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/include/qemu/option.h b/include/qemu/option.h index 844587cab3..eb4097889d 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -33,7 +33,6 @@ const char *get_opt_value(const char *p, char **value); void parse_option_size(const char *name, const char *value, uint64_t *ret, Error **errp); bool has_help_option(const char *param); -bool is_valid_option_list(const char *param); enum QemuOptType { QEMU_OPT_STRING = 0, /* no parsing (use string as-is) */ diff --git a/qemu-img.c b/qemu-img.c index d36b21b758..cc51db7ed4 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts) return true; } +/* + * Is @optarg safe for accumulate_options()? + * It is when multiple of them can be joined together separated by ','. + * To make that work, @optarg must not start with ',' (or else a + * separating ',' preceding it gets escaped), and it must not end with + * an odd number of ',' (or else a separating ',' following it gets + * escaped). + */ +static bool is_valid_option_list(const char *optarg) +{ + size_t len = strlen(optarg); + size_t i; + + if (optarg[0] == ',') { + return false; + } + + for (i = len; i > 0 && optarg[i - 1] == ','; i--) { + } + if ((len - i) % 2) { + return false; + } + + return true; +} + static int accumulate_options(char **options, char *optarg) { char *new_options; diff --git a/util/qemu-option.c b/util/qemu-option.c index 2d0d24ee27..9542988183 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -165,28 +165,6 @@ void parse_option_size(const char *name, const char *value, *ret = size; } -bool is_valid_option_list(const char *p) -{ - char *value = NULL; - bool result = false; - - while (*p) { - p = get_opt_value(p, &value); - if ((*p && !*++p) || - (!*value || *value == ',')) { - goto out; - } - - g_free(value); - value = NULL; - } - - result = true; -out: - g_free(value); - return result; -} - static const char *opt_type_to_string(enum QemuOptType type) { switch (type) { From f62514b3def5fb2acbef64d0e053c0c31fa45aff Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Apr 2020 09:49:27 +0200 Subject: [PATCH 10/32] qemu-img: Reject broken -o "" qemu-img create, convert, amend, and measure use accumulate_options() to merge multiple -o options. This is broken for -o "": $ qemu-img create -f qcow2 -o backing_file=a -o "" -o backing_fmt=raw,size=1M new.qcow2 qemu-img: warning: Could not verify backing image. This may become an error in future versions. Could not open 'a,backing_fmt=raw': No such file or directory Formatting 'new.qcow2', fmt=qcow2 size=1048576 backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ qemu-img info new.qcow2 image: new.qcow2 file format: qcow2 virtual size: 1 MiB (1048576 bytes) disk size: 196 KiB cluster_size: 65536 --> backing file: a,backing_fmt=raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false Merging these three -o the obvious way is wrong, because it results in an unwanted ',' escape: backing_file=a,,backing_fmt=raw,size=1M ~~ We could silently drop -o "", but Kevin asked me to reject it instead. Signed-off-by: Markus Armbruster Message-Id: <20200415074927.19897-10-armbru@redhat.com> Reviewed-by: Eric Blake --- qemu-img.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index cc51db7ed4..a2369766f0 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -229,14 +229,16 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts) * To make that work, @optarg must not start with ',' (or else a * separating ',' preceding it gets escaped), and it must not end with * an odd number of ',' (or else a separating ',' following it gets - * escaped). + * escaped), or be empty (or else a separating ',' preceding it can + * escape a separating ',' following it). + * */ static bool is_valid_option_list(const char *optarg) { size_t len = strlen(optarg); size_t i; - if (optarg[0] == ',') { + if (!optarg[0] || optarg[0] == ',') { return false; } From 2a340b67bd0a702e6d2102691cb2cb2177b7e210 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:06 +0200 Subject: [PATCH 11/32] cryptodev: Fix cryptodev_builtin_cleanup() error API violation The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. cryptodev_builtin_cleanup() passes @errp to cryptodev_builtin_sym_close_session() in a loop. Harmless, because cryptodev_builtin_sym_close_session() can't actually fail. Fix it anyway. Cc: Gonglei Signed-off-by: Markus Armbruster Message-Id: <20200422130719.28225-2-armbru@redhat.com> --- backends/cryptodev-builtin.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c index c8ae3b9742..14316333fe 100644 --- a/backends/cryptodev-builtin.c +++ b/backends/cryptodev-builtin.c @@ -282,12 +282,7 @@ static int cryptodev_builtin_sym_close_session( CryptoDevBackendBuiltin *builtin = CRYPTODEV_BACKEND_BUILTIN(backend); - if (session_id >= MAX_NUM_SESSIONS || - builtin->sessions[session_id] == NULL) { - error_setg(errp, "Cannot find a valid session id: %" PRIu64 "", - session_id); - return -1; - } + assert(session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]); qcrypto_cipher_free(builtin->sessions[session_id]->cipher); g_free(builtin->sessions[session_id]); @@ -356,8 +351,7 @@ static void cryptodev_builtin_cleanup( for (i = 0; i < MAX_NUM_SESSIONS; i++) { if (builtin->sessions[i] != NULL) { - cryptodev_builtin_sym_close_session( - backend, i, 0, errp); + cryptodev_builtin_sym_close_session(backend, i, 0, &error_abort); } } From 77ed971b9d96a288e497509054e1e59493ffd1fc Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:07 +0200 Subject: [PATCH 12/32] block/file-posix: Fix check_cache_dropped() error handling The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. check_cache_dropped() calls error_setg() in a loop. It fails to break the loop in one instance. If a subsequent iteration error_setg()s again, it trips error_setv()'s assertion. Fix it to break the loop. Fixes: 31be8a2a97ecba7d31a82932286489cac318e9e9 Cc: Stefan Hajnoczi Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20200422130719.28225-3-armbru@redhat.com> --- block/file-posix.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/file-posix.c b/block/file-posix.c index 7e19bbff5f..094e3b0212 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2691,10 +2691,13 @@ static void check_cache_dropped(BlockDriverState *bs, Error **errp) vec_end = DIV_ROUND_UP(length, page_size); for (i = 0; i < vec_end; i++) { if (vec[i] & 0x1) { - error_setg(errp, "page cache still in use!"); break; } } + if (i < vec_end) { + error_setg(errp, "page cache still in use!"); + break; + } } if (window) { From abc9bf69a66a11499a801ff545b8fe7adbb3a04c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:08 +0200 Subject: [PATCH 13/32] cpus: Fix configure_icount() error API violation The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. configure_icount() is wrong that way. Harmless, because its @errp is always &error_abort or &error_fatal. Just as wrong (and just as harmless): when it fails, it can still update global state. Fix all that. Cc: Paolo Bonzini Signed-off-by: Markus Armbruster Message-Id: <20200422130719.28225-4-armbru@redhat.com> --- cpus.c | 51 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/cpus.c b/cpus.c index ef441bdf62..1b542b37f9 100644 --- a/cpus.c +++ b/cpus.c @@ -797,40 +797,49 @@ void cpu_ticks_init(void) void configure_icount(QemuOpts *opts, Error **errp) { - const char *option; + const char *option = qemu_opt_get(opts, "shift"); + bool sleep = qemu_opt_get_bool(opts, "sleep", true); + bool align = qemu_opt_get_bool(opts, "align", false); + long time_shift = -1; char *rem_str = NULL; - option = qemu_opt_get(opts, "shift"); - if (!option) { - if (qemu_opt_get(opts, "align") != NULL) { - error_setg(errp, "Please specify shift option when using align"); - } + if (!option && qemu_opt_get(opts, "align")) { + error_setg(errp, "Please specify shift option when using align"); return; } - icount_sleep = qemu_opt_get_bool(opts, "sleep", true); + if (align && !sleep) { + error_setg(errp, "align=on and sleep=off are incompatible"); + return; + } + + if (strcmp(option, "auto") != 0) { + errno = 0; + time_shift = strtol(option, &rem_str, 0); + if (errno != 0 || *rem_str != '\0' || !strlen(option)) { + error_setg(errp, "icount: Invalid shift value"); + return; + } + } else if (icount_align_option) { + error_setg(errp, "shift=auto and align=on are incompatible"); + return; + } else if (!icount_sleep) { + error_setg(errp, "shift=auto and sleep=off are incompatible"); + return; + } + + icount_sleep = sleep; if (icount_sleep) { timers_state.icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT, icount_timer_cb, NULL); } - icount_align_option = qemu_opt_get_bool(opts, "align", false); + icount_align_option = align; - if (icount_align_option && !icount_sleep) { - error_setg(errp, "align=on and sleep=off are incompatible"); - } - if (strcmp(option, "auto") != 0) { - errno = 0; - timers_state.icount_time_shift = strtol(option, &rem_str, 0); - if (errno != 0 || *rem_str != '\0' || !strlen(option)) { - error_setg(errp, "icount: Invalid shift value"); - } + if (time_shift >= 0) { + timers_state.icount_time_shift = time_shift; use_icount = 1; return; - } else if (icount_align_option) { - error_setg(errp, "shift=auto and align=on are incompatible"); - } else if (!icount_sleep) { - error_setg(errp, "shift=auto and sleep=off are incompatible"); } use_icount = 2; From 9ec374a781c34daa6e70fcd885ef30e090cc2384 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:09 +0200 Subject: [PATCH 14/32] cpus: Proper range-checking for -icount shift=N timers_state.icount_time_shift must be in [0,63] to avoid undefined behavior when shifting by it, e.g. in cpu_icount_to_ns(). icount_adjust() clamps it to [0,MAX_ICOUNT_SHIFT], with MAX_ICOUNT_SHIFT = 10. configure_icount() doesn't. Fix that. Fixes: a8bfac37085c3372366d722f131a7e18d664ee4d Cc: Paolo Bonzini Signed-off-by: Markus Armbruster Message-Id: <20200422130719.28225-5-armbru@redhat.com> --- cpus.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cpus.c b/cpus.c index 1b542b37f9..5670c96bcf 100644 --- a/cpus.c +++ b/cpus.c @@ -25,6 +25,7 @@ #include "qemu/osdep.h" #include "qemu-common.h" #include "qemu/config-file.h" +#include "qemu/cutils.h" #include "migration/vmstate.h" #include "monitor/monitor.h" #include "qapi/error.h" @@ -801,7 +802,6 @@ void configure_icount(QemuOpts *opts, Error **errp) bool sleep = qemu_opt_get_bool(opts, "sleep", true); bool align = qemu_opt_get_bool(opts, "align", false); long time_shift = -1; - char *rem_str = NULL; if (!option && qemu_opt_get(opts, "align")) { error_setg(errp, "Please specify shift option when using align"); @@ -814,9 +814,8 @@ void configure_icount(QemuOpts *opts, Error **errp) } if (strcmp(option, "auto") != 0) { - errno = 0; - time_shift = strtol(option, &rem_str, 0); - if (errno != 0 || *rem_str != '\0' || !strlen(option)) { + if (qemu_strtol(option, NULL, 0, &time_shift) < 0 + || time_shift < 0 || time_shift > MAX_ICOUNT_SHIFT) { error_setg(errp, "icount: Invalid shift value"); return; } From 0fbddcecf23dcc311b42d9faf87dc2656dadd62a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:10 +0200 Subject: [PATCH 15/32] arm/virt: Fix virt_machine_device_plug_cb() error API violation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. virt_machine_device_plug_cb() passes @errp to cryptodev_builtin_sym_close_session() in a loop. Harmless, because cryptodev_builtin_sym_close_session() can't actually fail. Fix by dropping its Error ** parameter. Cc: Peter Maydell Cc: qemu-arm@nongnu.org Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200422130719.28225-6-armbru@redhat.com> --- hw/arm/virt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 7dc96abf72..cca5316256 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1186,7 +1186,7 @@ static void create_smmu(const VirtMachineState *vms, g_free(node); } -static void create_virtio_iommu_dt_bindings(VirtMachineState *vms, Error **errp) +static void create_virtio_iommu_dt_bindings(VirtMachineState *vms) { const char compat[] = "virtio,pci-iommu"; uint16_t bdf = vms->virtio_iommu_bdf; @@ -2118,7 +2118,7 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev, vms->iommu = VIRT_IOMMU_VIRTIO; vms->virtio_iommu_bdf = pci_get_bdf(pdev); - create_virtio_iommu_dt_bindings(vms, errp); + create_virtio_iommu_dt_bindings(vms); } } From 07a978ef376cde544cca564ec225a8eb79ff2d54 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:11 +0200 Subject: [PATCH 16/32] fdc: Fix fallback=auto error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fdctrl_realize_common() rejects fallback=auto. Used by devices "isa-fdc", "sysbus-fdc", "SUNW,fdtwo". The error handling is broken: $ qemu-system-x86_64 -nodefaults -device isa-fdc,fallback=auto,driveA=fd0 -drive if=none,id=fd0 ** ERROR:/work/armbru/qemu/hw/block/fdc.c:434:pick_drive_type: assertion failed: (drv->drive != FLOPPY_DRIVE_TYPE_AUTO) Aborted (core dumped) Cause: fdctrl_realize_common() neglects to bail out after setting the error. Fix that. Fixes: a73275dd6fc3bfda33165bebc28e0c33c20cb0a0 Cc: John Snow Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200422130719.28225-7-armbru@redhat.com> --- hw/block/fdc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 33bc9e2f92..9628cc171e 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -2615,6 +2615,7 @@ static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) { error_setg(errp, "Cannot choose a fallback FDrive type of 'auto'"); + return; } /* Fill 'command_to_handler' lookup table */ From ee29f6e92d987bd7f37e3a0002dd839f8431adb1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:12 +0200 Subject: [PATCH 17/32] bochs-display: Fix vgamem=SIZE error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bochs_display_realize() rejects out-of-range vgamem. The error handling is broken: $ qemu-system-x86_64 -S -display none -monitor stdio QEMU 4.2.93 monitor - type 'help' for more information (qemu) device_add bochs-display,vgamem=1 Error: bochs-display: video memory too small (qemu) device_add bochs-display,vgamem=1 RAMBlock "0000:00:04.0/bochs-display-vram" already registered, abort! Aborted (core dumped) Cause: bochs_display_realize() neglects to bail out after setting the error. Fix that. Fixes: 765c94290863eef1fc4a67819d452cc13b7854a1 Cc: Gerd Hoffmann Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200422130719.28225-8-armbru@redhat.com> Reviewed-by: Gerd Hoffmann --- hw/display/bochs-display.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c index 70eb619ef4..e763a0a72d 100644 --- a/hw/display/bochs-display.c +++ b/hw/display/bochs-display.c @@ -267,16 +267,18 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp) Object *obj = OBJECT(dev); int ret; - s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s); - if (s->vgamem < 4 * MiB) { error_setg(errp, "bochs-display: video memory too small"); + return; } if (s->vgamem > 256 * MiB) { error_setg(errp, "bochs-display: video memory too big"); + return; } s->vgamem = pow2ceil(s->vgamem); + s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s); + memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem, &error_fatal); memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s, From 843c4cfcf445fc3d6458ff31136c44e03dda8866 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:13 +0200 Subject: [PATCH 18/32] virtio-net: Fix duplex=... and speed=... error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit virtio_net_device_realize() rejects invalid duplex and speed values. The error handling is broken: $ ../qemu/bld-sani/x86_64-softmmu/qemu-system-x86_64 -S -display none -monitor stdio QEMU 4.2.93 monitor - type 'help' for more information (qemu) device_add virtio-net,duplex=x Error: 'duplex' must be 'half' or 'full' (qemu) c ================================================================= ==15654==ERROR: AddressSanitizer: heap-use-after-free on address 0x62e000014590 at pc 0x560b75c8dc13 bp 0x7fffdf1a6950 sp 0x7fffdf1a6940 READ of size 8 at 0x62e000014590 thread T0 #0 0x560b75c8dc12 in object_dynamic_cast_assert /work/armbru/qemu/qom/object.c:826 #1 0x560b74c38ac0 in virtio_vmstate_change /work/armbru/qemu/hw/virtio/virtio.c:3210 #2 0x560b74d9765e in vm_state_notify /work/armbru/qemu/softmmu/vl.c:1271 #3 0x560b7494ba72 in vm_prepare_start /work/armbru/qemu/cpus.c:2156 #4 0x560b7494bacd in vm_start /work/armbru/qemu/cpus.c:2162 #5 0x560b75a7d890 in qmp_cont /work/armbru/qemu/monitor/qmp-cmds.c:160 #6 0x560b75a8d70a in hmp_cont /work/armbru/qemu/monitor/hmp-cmds.c:1043 #7 0x560b75a799f2 in handle_hmp_command /work/armbru/qemu/monitor/hmp.c:1082 [...] 0x62e000014590 is located 33168 bytes inside of 42288-byte region [0x62e00000c400,0x62e000016930) freed by thread T1 here: #0 0x7feadd39491f in __interceptor_free (/lib64/libasan.so.5+0x10d91f) #1 0x7feadcebcd7c in g_free (/lib64/libglib-2.0.so.0+0x55d7c) #2 0x560b75c8fd40 in object_unref /work/armbru/qemu/qom/object.c:1128 #3 0x560b7498a625 in memory_region_unref /work/armbru/qemu/memory.c:1762 #4 0x560b74999fa4 in do_address_space_destroy /work/armbru/qemu/memory.c:2788 #5 0x560b762362fc in call_rcu_thread /work/armbru/qemu/util/rcu.c:283 #6 0x560b761c8884 in qemu_thread_start /work/armbru/qemu/util/qemu-thread-posix.c:519 #7 0x7fead9be34bf in start_thread (/lib64/libpthread.so.0+0x84bf) previously allocated by thread T0 here: #0 0x7feadd394d18 in __interceptor_malloc (/lib64/libasan.so.5+0x10dd18) #1 0x7feadcebcc88 in g_malloc (/lib64/libglib-2.0.so.0+0x55c88) #2 0x560b75c8cf8a in object_new /work/armbru/qemu/qom/object.c:699 #3 0x560b75010ad9 in qdev_device_add /work/armbru/qemu/qdev-monitor.c:654 #4 0x560b750120c2 in qmp_device_add /work/armbru/qemu/qdev-monitor.c:805 #5 0x560b75012c1b in hmp_device_add /work/armbru/qemu/qdev-monitor.c:905 [...] ==15654==ABORTING Cause: virtio_net_device_realize() neglects to bail out after setting the error. Fix that. Fixes: 9473939ed7addcaaeb8fde5c093918fb7fa0919c Cc: "Michael S. Tsirkin" Cc: Jason Wang Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200422130719.28225-9-armbru@redhat.com> Acked-by: Michael S. Tsirkin --- hw/net/virtio-net.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index eddfa7f923..65bb6886c7 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -2947,6 +2947,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp) n->net_conf.duplex = DUPLEX_FULL; } else { error_setg(errp, "'duplex' must be 'half' or 'full'"); + return; } n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX); } else { @@ -2955,7 +2956,9 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp) if (n->net_conf.speed < SPEED_UNKNOWN) { error_setg(errp, "'speed' must be between 0 and INT_MAX"); - } else if (n->net_conf.speed >= 0) { + return; + } + if (n->net_conf.speed >= 0) { n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX); } From c61d1d9e4976d127a73bec14376d8e828491f62c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:14 +0200 Subject: [PATCH 19/32] xen/pt: Fix flawed conversion to realize() The conversion of xen_pt_initfn() to xen_pt_realize() blindly replaced XEN_PT_ERR() by error_setg(). Several error conditions that did not fail xen_pt_initfn() now fail xen_pt_realize(). Unsurprisingly, the cleanup on these errors looks highly suspicious. Revert the inappropriate replacements. Fixes: 5a11d0f7549e24a10e178a9dc8ff5e698031d9a6 Cc: Stefano Stabellini Cc: Anthony Perard Cc: Paul Durrant Cc: xen-devel@lists.xenproject.org Signed-off-by: Markus Armbruster Reviewed-by: Paul Durrant Message-Id: <20200422130719.28225-10-armbru@redhat.com> --- hw/xen/xen_pt.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index b91082cb8b..81d5ad8da7 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -858,8 +858,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); if (rc < 0) { - error_setg_errno(errp, errno, "Mapping machine irq %u to" - " pirq %i failed", machine_irq, pirq); + XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n", + machine_irq, pirq, errno); /* Disable PCI intx assertion (turn on bit10 of devctl) */ cmd |= PCI_COMMAND_INTX_DISABLE; @@ -880,8 +880,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) PCI_SLOT(d->devfn), e_intx); if (rc < 0) { - error_setg_errno(errp, errno, "Binding of interrupt %u failed", - e_intx); + XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n", + e_intx, errno); /* Disable PCI intx assertion (turn on bit10 of devctl) */ cmd |= PCI_COMMAND_INTX_DISABLE; @@ -889,8 +889,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) if (xen_pt_mapped_machine_irq[machine_irq] == 0) { if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) { - error_setg_errno(errp, errno, "Unmapping of machine" - " interrupt %u failed", machine_irq); + XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!" + " (err: %d)\n", machine_irq, errno); } } s->machine_irq = 0; From fdceb4ab4d7ba920bb094ab477df4221261b291e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:15 +0200 Subject: [PATCH 20/32] io: Fix qio_channel_socket_close() error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. qio_channel_socket_close() passes @errp first to socket_listen_cleanup(), and then, if closesocket() fails, to error_setg_errno(). If socket_listen_cleanup() failed, this will trip the assertion in error_setv(). Fix by ignoring a second error. Fixes: 73564c407caedf992a1c688b5fea776a8b56ba2a Cc: Daniel P. Berrangé Signed-off-by: Markus Armbruster Reviewed-by: Daniel P. Berrangé Message-Id: <20200422130719.28225-11-armbru@redhat.com> --- io/channel-socket.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/io/channel-socket.c b/io/channel-socket.c index b74f5b92a0..e1b4667087 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -704,6 +704,7 @@ qio_channel_socket_close(QIOChannel *ioc, { QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc); int rc = 0; + Error *err = NULL; if (sioc->fd != -1) { #ifdef WIN32 @@ -715,8 +716,8 @@ qio_channel_socket_close(QIOChannel *ioc, if (closesocket(sioc->fd) < 0) { sioc->fd = -1; - error_setg_errno(errp, errno, - "Unable to close socket"); + error_setg_errno(&err, errno, "Unable to close socket"); + error_propagate(errp, err); return -1; } sioc->fd = -1; From 735527e17907cca330ec369a13566fab2ef9adf6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:16 +0200 Subject: [PATCH 21/32] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. qmp_xen_colo_do_checkpoint() passes @errp first to replication_do_checkpoint_all(), and then to colo_notify_filters_event(). If both fail, this will trip the assertion in error_setv(). Similar code in secondary_vm_do_failover() calls colo_notify_filters_event() only after replication_do_checkpoint_all() succeeded. Do the same here. Fixes: 0e8818f023616677416840d6ddc880db8de3c967 Cc: Zhang Chen Cc: zhanghailiang Signed-off-by: Markus Armbruster Reviewed-by: zhanghailiang Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Zhang Chen Message-Id: <20200422130719.28225-12-armbru@redhat.com> --- migration/colo.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/migration/colo.c b/migration/colo.c index a54ac84f41..1b3493729b 100644 --- a/migration/colo.c +++ b/migration/colo.c @@ -263,7 +263,13 @@ ReplicationStatus *qmp_query_xen_replication_status(Error **errp) void qmp_xen_colo_do_checkpoint(Error **errp) { - replication_do_checkpoint_all(errp); + Error *err = NULL; + + replication_do_checkpoint_all(&err); + if (err) { + error_propagate(errp, err); + return; + } /* Notify all filters of all NIC to do checkpoint */ colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp); } From 131889924bb31a6a371ed9300efb2036edd6b40b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:17 +0200 Subject: [PATCH 22/32] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a Signed-off-by: Markus Armbruster Message-Id: <20200422130719.28225-13-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- tests/test-logging.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test-logging.c b/tests/test-logging.c index 6387e4933f..8580b82420 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -73,10 +73,10 @@ static void test_parse_range(void) g_assert(qemu_log_in_addr_range(UINT64_MAX)); g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1)); - qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err); + qemu_set_dfilter_ranges("0..0xffffffffffffffff", &error_abort); g_assert(qemu_log_in_addr_range(0)); g_assert(qemu_log_in_addr_range(UINT64_MAX)); - + qemu_set_dfilter_ranges("2..1", &err); error_free_or_abort(&err); From 4155c998b6f7618aad0e67c34c2a4aea841114cf Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:18 +0200 Subject: [PATCH 23/32] qga: Fix qmp_guest_get_memory_blocks() error handling The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. qmp_guest_get_memory_blocks() passes &local_err to transfer_memory_block() in a loop. If this fails in more than one iteration, it can trip error_setv()'s assertion. Fix it to break the loop. Cc: Michael Roth Signed-off-by: Markus Armbruster Message-Id: <20200422130719.28225-14-armbru@redhat.com> Reviewed-by: Eric Blake --- qga/commands-posix.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index a52af0315f..ae1348dc8f 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -2518,6 +2518,9 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp) mem_blk->phys_index = strtoul(&de->d_name[6], NULL, 10); mem_blk->has_can_offline = true; /* lolspeak ftw */ transfer_memory_block(mem_blk, true, NULL, &local_err); + if (local_err) { + break; + } entry = g_malloc0(sizeof *entry); entry->value = mem_blk; From 51bd45816615bdb22a31a86f02d601ce536034e3 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:07:19 +0200 Subject: [PATCH 24/32] qga: Fix qmp_guest_suspend_{disk, ram}() error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err first to check_suspend_mode(), then to acquire_privilege(), then to execute_async(). Continuing after errors here can only end in tears. For instance, we risk tripping error_setv()'s assertion. Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69 Fixes: f54603b6aa765514b2519e74114a2f417759d727 Cc: Michael Roth Signed-off-by: Markus Armbruster Message-Id: <20200422130719.28225-15-armbru@redhat.com> Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé --- qga/commands-win32.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 9717a8d52d..5ba56327dd 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp) *mode = GUEST_SUSPEND_MODE_DISK; check_suspend_mode(*mode, &local_err); + if (local_err) { + goto out; + } acquire_privilege(SE_SHUTDOWN_NAME, &local_err); + if (local_err) { + goto out; + } execute_async(do_suspend, mode, &local_err); +out: if (local_err) { error_propagate(errp, local_err); g_free(mode); @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp) *mode = GUEST_SUSPEND_MODE_RAM; check_suspend_mode(*mode, &local_err); + if (local_err) { + goto out; + } acquire_privilege(SE_SHUTDOWN_NAME, &local_err); + if (local_err) { + goto out; + } execute_async(do_suspend, mode, &local_err); +out: if (local_err) { error_propagate(errp, local_err); g_free(mode); From fc0cfc1decb5457058d7cbfeb7ae26ad5d6ecba4 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:48:12 +0200 Subject: [PATCH 25/32] sam460ex: Suppress useless warning on -m 32 and -m 64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requesting 32 or 64 MiB of RAM with the sam460ex machine type produces a useless warning: qemu-system-ppc: warning: Memory size is too small for SDRAM type, adjusting type This is because sam460ex_init() asks spd_data_generate() for DDR2, which is impossible, so spd_data_generate() corrects it to DDR. The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD EEPROM creation". Make sam460ex_init() pass the correct SDRAM type to get rid of the warning. Signed-off-by: Markus Armbruster Message-Id: <20200422134815.1584-2-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- hw/ppc/sam460ex.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c index 898453cf30..1e3eaac0db 100644 --- a/hw/ppc/sam460ex.c +++ b/hw/ppc/sam460ex.c @@ -335,7 +335,8 @@ static void sam460ex_init(MachineState *machine) dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]); i2c = PPC4xx_I2C(dev)->bus; /* SPD EEPROM on RAM module */ - spd_data = spd_data_generate(DDR2, ram_sizes[0], &err); + spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2, + ram_sizes[0], &err); if (err) { warn_report_err(err); } From f26740c61a57f1a1556f79a49c6863479fe5aa6b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:48:13 +0200 Subject: [PATCH 26/32] smbus: Fix spd_data_generate() error API violation The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. spd_data_generate() can pass @errp to error_setg() more than once when it adjusts both memory size and type. Harmless, because no caller passes anything that needs adjusting. Until the previous commit, sam460ex passed types that needed adjusting, but not sizes. spd_data_generate()'s contract is rather awkward: If everything's fine, return non-null and don't set an error. Else, if memory size or type need adjusting, return non-null and set an error describing the adjustment. Else, return null and set an error reporting why no data can be generated. Its callers treat the error as a warning even when null is returned. They don't create the "smbus-eeprom" device then. Suspicious. Since the previous commit, only "everything's fine" can actually happen. Drop the unused code and simplify the callers. This gets rid of the error API violation. Signed-off-by: Markus Armbruster Message-Id: <20200422134815.1584-3-armbru@redhat.com> --- hw/i2c/smbus_eeprom.c | 30 ++++-------------------------- hw/mips/mips_fulong2e.c | 10 ++-------- hw/ppc/sam460ex.c | 12 +++--------- include/hw/i2c/smbus_eeprom.h | 2 +- 4 files changed, 10 insertions(+), 44 deletions(-) diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index 5adf3b15b5..07fbbf87f1 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -195,8 +195,7 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, } /* Generate SDRAM SPD EEPROM data describing a module of type and size */ -uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size, - Error **errp) +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size) { uint8_t *spd; uint8_t nbanks; @@ -222,29 +221,10 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size, g_assert_not_reached(); } size = ram_size >> 20; /* work in terms of megabytes */ - if (size < 4) { - error_setg(errp, "SDRAM size is too small"); - return NULL; - } sz_log2 = 31 - clz32(size); size = 1U << sz_log2; - if (ram_size > size * MiB) { - error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, " - "truncating to %u MB", ram_size, size); - } - if (sz_log2 < min_log2) { - error_setg(errp, - "Memory size is too small for SDRAM type, adjusting type"); - if (size >= 32) { - type = DDR; - min_log2 = 5; - max_log2 = 12; - } else { - type = SDR; - min_log2 = 2; - max_log2 = 9; - } - } + assert(ram_size == size * MiB); + assert(sz_log2 >= min_log2); nbanks = 1; while (sz_log2 > max_log2 && nbanks < 8) { @@ -252,9 +232,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size, nbanks++; } - if (size > (1ULL << sz_log2) * nbanks) { - error_setg(errp, "Memory size is too big for SDRAM, truncating"); - } + assert(size == (1ULL << sz_log2) * nbanks); /* split to 2 banks if possible to avoid a bug in MIPS Malta firmware */ if (nbanks == 1 && sz_log2 > min_log2) { diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c index 5040afd581..ef02d54b33 100644 --- a/hw/mips/mips_fulong2e.c +++ b/hw/mips/mips_fulong2e.c @@ -297,7 +297,6 @@ static void mips_fulong2e_init(MachineState *machine) MemoryRegion *bios = g_new(MemoryRegion, 1); long bios_size; uint8_t *spd_data; - Error *err = NULL; int64_t kernel_entry; PCIBus *pci_bus; ISABus *isa_bus; @@ -377,13 +376,8 @@ static void mips_fulong2e_init(MachineState *machine) } /* Populate SPD eeprom data */ - spd_data = spd_data_generate(DDR, machine->ram_size, &err); - if (err) { - warn_report_err(err); - } - if (spd_data) { - smbus_eeprom_init_one(smbus, 0x50, spd_data); - } + spd_data = spd_data_generate(DDR, machine->ram_size); + smbus_eeprom_init_one(smbus, 0x50, spd_data); mc146818_rtc_init(isa_bus, 2000, NULL); diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c index 1e3eaac0db..42a8c9fb7f 100644 --- a/hw/ppc/sam460ex.c +++ b/hw/ppc/sam460ex.c @@ -292,7 +292,6 @@ static void sam460ex_init(MachineState *machine) SysBusDevice *sbdev; struct boot_info *boot_info; uint8_t *spd_data; - Error *err = NULL; int success; cpu = POWERPC_CPU(cpu_create(machine->cpu_type)); @@ -336,14 +335,9 @@ static void sam460ex_init(MachineState *machine) i2c = PPC4xx_I2C(dev)->bus; /* SPD EEPROM on RAM module */ spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2, - ram_sizes[0], &err); - if (err) { - warn_report_err(err); - } - if (spd_data) { - spd_data[20] = 4; /* SO-DIMM module */ - smbus_eeprom_init_one(i2c, 0x50, spd_data); - } + ram_sizes[0]); + spd_data[20] = 4; /* SO-DIMM module */ + smbus_eeprom_init_one(i2c, 0x50, spd_data); /* RTC */ i2c_create_slave(i2c, "m41t80", 0x68); diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h index 15e2151b50..68b0063ab6 100644 --- a/include/hw/i2c/smbus_eeprom.h +++ b/include/hw/i2c/smbus_eeprom.h @@ -31,6 +31,6 @@ void smbus_eeprom_init(I2CBus *bus, int nb_eeprom, const uint8_t *eeprom_spd, int size); enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; -uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, Error **errp); +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size); #endif From 0f1eddf5ed7e35d8d16dfa042ced1107c762cec4 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:48:14 +0200 Subject: [PATCH 27/32] bamboo, sam460ex: Tidy up error message for unsupported RAM size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improve $ ppc-softmmu/qemu-system-ppc -M sam460ex -m 4096 qemu-system-ppc: Max 1 banks of 2048 ,1024 ,512 ,256 ,128 ,64 ,32 MB DIMM/bank supported qemu-system-ppc: Possible valid RAM size: 2048 to qemu-system-ppc: at most 1 bank of 2048, 1024, 512, 256, 128, 64, 32 MiB each supported Possible valid RAM size: 1024 MiB Signed-off-by: Markus Armbruster Message-Id: <20200422134815.1584-4-armbru@redhat.com> Reviewed-by: BALATON Zoltan Reviewed-by: Philippe Mathieu-Daudé --- hw/ppc/ppc4xx_devs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c index 3376c43ff5..f1651e04d9 100644 --- a/hw/ppc/ppc4xx_devs.c +++ b/hw/ppc/ppc4xx_devs.c @@ -716,11 +716,11 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks, for (i = 0; sdram_bank_sizes[i]; i++) { g_string_append_printf(s, "%" PRIi64 "%s", sdram_bank_sizes[i] / MiB, - sdram_bank_sizes[i + 1] ? " ," : ""); + sdram_bank_sizes[i + 1] ? ", " : ""); } - error_report("Max %d banks of %s MB DIMM/bank supported", - nr_banks, s->str); - error_report("Possible valid RAM size: %" PRIi64, + error_report("at most %d bank%s of %s MiB each supported", + nr_banks, nr_banks == 1 ? "" : "s", s->str); + error_printf("Possible valid RAM size: %" PRIi64 " MiB \n", used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB); g_string_free(s, true); From 32c82f0eaf9919cb0268d18d86a94bfd7ff5d1b2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:48:15 +0200 Subject: [PATCH 28/32] smbus: Fix spd_data_generate() for number of banks > 2 spd_data_generate() splits @ram_size bytes into @nbanks RAM banks of 1 << sz_log2 MiB each, like this: size = ram_size >> 20; /* work in terms of megabytes */ [...] nbanks = 1; while (sz_log2 > max_log2 && nbanks < 8) { sz_log2--; nbanks++; } Each iteration halves the size of a bank, and increments the number of banks. Wrong: it should double the number of banks. The bug goes back all the way to commit b296b664ab "smbus: Add a helper to generate SPD EEPROM data". It can't bite because spd_data_generate()'s current users pass only @ram_size that result in *zero* iterations: machine RAM size #banks type bank size fulong2e 256 MiB 1 DDR 256 MiB sam460ex 2048 MiB 1 DDR2 2048 MiB 1024 MiB 1 DDR2 1024 MiB 512 MiB 1 DDR2 512 MiB 256 MiB 1 DDR2 256 MiB 128 MiB 1 SDR 128 MiB 64 MiB 1 SDR 64 MiB 32 MiB 1 SDR 32 MiB Apply the obvious, minimal fix. I admit I'm tempted to rip out the unused (and obviously untested) feature instead, because YAGNI. Note that this is not the final result, as spd_data_generate() next increases #banks from 1 to 2 if possible. This is done "to avoid a bug in MIPS Malta firmware". We don't even use this function with machine type malta. *Shrug* Signed-off-by: Markus Armbruster Message-Id: <20200422134815.1584-5-armbru@redhat.com> --- hw/i2c/smbus_eeprom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index 07fbbf87f1..e199fc8678 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -229,7 +229,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size) nbanks = 1; while (sz_log2 > max_log2 && nbanks < 8) { sz_log2--; - nbanks++; + nbanks *= 2; } assert(size == (1ULL << sz_log2) * nbanks); From 14b6ce68cc8658be107247544e9a287ed17aaf76 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 24 Apr 2020 09:11:40 +0200 Subject: [PATCH 29/32] Makefile: Drop unused, broken target recurse-fuzz Target recurse-fuzz depends on pc-bios/optionrom/fuzz, which can't be made. It's not used anywhere. Added in commit c621dc3e01c, looks like cargo cult. Delete. Signed-off-by: Markus Armbruster Message-Id: <20200424071142.3525-2-armbru@redhat.com> Reviewed-by: Alexander Bulekov --- Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/Makefile b/Makefile index 8a9113e666..34275f57c9 100644 --- a/Makefile +++ b/Makefile @@ -582,7 +582,6 @@ $(ROM_DIRS_RULES): .PHONY: recurse-all recurse-clean recurse-install recurse-all: $(addsuffix /all, $(TARGET_DIRS) $(ROM_DIRS)) -recurse-fuzz: $(addsuffix /fuzz, $(TARGET_DIRS) $(ROM_DIRS)) recurse-clean: $(addsuffix /clean, $(TARGET_DIRS) $(ROM_DIRS)) recurse-install: $(addsuffix /install, $(TARGET_DIRS)) $(addsuffix /install, $(TARGET_DIRS)): all From a56f3cdbdf328d95398c70432bba638a6b3f63fa Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 24 Apr 2020 09:11:41 +0200 Subject: [PATCH 30/32] fuzz: Simplify how we compute available machines and types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apply_to_qlist(), apply_to_node() work with QObjects. This is designed for use by tests/qtest/qos-test.c, which gets the data in that form via QMP. Goes back to commit fc281c8020 "tests: qgraph API for the qtest driver framework". Commit 275ab39d86 "fuzz: add support for qos-assisted fuzz targets" added another user: qtest/fuzz/qos_fuzz.c. To get the data as QObjects, it uses qmp_marshal_query_machines() and qmp_marshal_qom_list_types(). All this code is rather cumbersome. Switch to working with generated QAPI types instead: * Replace apply_to_qlist() & friends by machines_apply_to_node() and types_apply_to_node(). * Have qos_fuzz.c use qmp_query_machines() and qmp_qom_list_types() instead. * Have qos_test.c convert from QObject to the QAPI types. Signed-off-by: Markus Armbruster Message-Id: <20200424071142.3525-3-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alexander Bulekov --- tests/qtest/fuzz/qos_fuzz.c | 34 ++++----------- tests/qtest/libqos/qos_external.c | 70 +++++++++++-------------------- tests/qtest/libqos/qos_external.h | 8 +++- tests/qtest/qos-test.c | 29 +++++++++---- 4 files changed, 59 insertions(+), 82 deletions(-) diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c index af28c92866..87eadb0889 100644 --- a/tests/qtest/fuzz/qos_fuzz.c +++ b/tests/qtest/fuzz/qos_fuzz.c @@ -36,7 +36,6 @@ #include "qapi/qapi-commands-machine.h" #include "qapi/qapi-commands-qom.h" -#include "qapi/qmp/qlist.h" void *fuzz_qos_obj; @@ -45,34 +44,19 @@ QGuestAllocator *fuzz_qos_alloc; static const char *fuzz_target_name; static char **fuzz_path_vec; -/* - * Replaced the qmp commands with direct qmp_marshal calls. - * Probably there is a better way to do this - */ static void qos_set_machines_devices_available(void) { - QDict *req = qdict_new(); - QObject *response; - QDict *args = qdict_new(); - QList *lst; + MachineInfoList *mach_info; + ObjectTypeInfoList *type_info; - qmp_marshal_query_machines(NULL, &response, &error_abort); - lst = qobject_to(QList, response); - apply_to_qlist(lst, true); + mach_info = qmp_query_machines(&error_abort); + machines_apply_to_node(mach_info); + qapi_free_MachineInfoList(mach_info); - qobject_unref(response); - - - qdict_put_str(req, "execute", "qom-list-types"); - qdict_put_str(args, "implements", "device"); - qdict_put_bool(args, "abstract", true); - qdict_put_obj(req, "arguments", (QObject *) args); - - qmp_marshal_qom_list_types(args, &response, &error_abort); - lst = qobject_to(QList, response); - apply_to_qlist(lst, false); - qobject_unref(response); - qobject_unref(req); + type_info = qmp_qom_list_types(true, "device", true, true, + &error_abort); + types_apply_to_node(type_info); + qapi_free_ObjectTypeInfoList(type_info); } static char **current_path; diff --git a/tests/qtest/libqos/qos_external.c b/tests/qtest/libqos/qos_external.c index 398556dde0..c707dac3b9 100644 --- a/tests/qtest/libqos/qos_external.c +++ b/tests/qtest/libqos/qos_external.c @@ -29,62 +29,40 @@ #include "libqos/qgraph_internal.h" #include "libqos/qos_external.h" - - -void apply_to_node(const char *name, bool is_machine, bool is_abstract) +static void machine_apply_to_node(const char *name) { - char *machine_name = NULL; - if (is_machine) { - const char *arch = qtest_get_arch(); - machine_name = g_strconcat(arch, "/", name, NULL); - name = machine_name; + char *machine_name = g_strconcat(qtest_get_arch(), "/", name, NULL); + + qos_graph_node_set_availability(machine_name, true); + g_free(machine_name); +} + +void machines_apply_to_node(MachineInfoList *mach_info) +{ + MachineInfoList *tail; + + for (tail = mach_info; tail; tail = tail->next) { + machine_apply_to_node(tail->value->name); + if (tail->value->alias) { + machine_apply_to_node(tail->value->alias); + } } +} + +static void type_apply_to_node(const char *name, bool is_abstract) +{ qos_graph_node_set_availability(name, true); if (is_abstract) { qos_delete_cmd_line(name); } - g_free(machine_name); } -/** - * apply_to_qlist(): using QMP queries QEMU for a list of - * machines and devices available, and sets the respective node - * as true. If a node is found, also all its produced and contained - * child are marked available. - * - * See qos_graph_node_set_availability() for more info - */ -void apply_to_qlist(QList *list, bool is_machine) +void types_apply_to_node(ObjectTypeInfoList *type_info) { - const QListEntry *p; - const char *name; - bool abstract; - QDict *minfo; - QObject *qobj; - QString *qstr; - QBool *qbool; + ObjectTypeInfoList *tail; - for (p = qlist_first(list); p; p = qlist_next(p)) { - minfo = qobject_to(QDict, qlist_entry_obj(p)); - qobj = qdict_get(minfo, "name"); - qstr = qobject_to(QString, qobj); - name = qstring_get_str(qstr); - - qobj = qdict_get(minfo, "abstract"); - if (qobj) { - qbool = qobject_to(QBool, qobj); - abstract = qbool_get_bool(qbool); - } else { - abstract = false; - } - - apply_to_node(name, is_machine, abstract); - qobj = qdict_get(minfo, "alias"); - if (qobj) { - qstr = qobject_to(QString, qobj); - name = qstring_get_str(qstr); - apply_to_node(name, is_machine, abstract); - } + for (tail = type_info; tail; tail = tail->next) { + type_apply_to_node(tail->value->name, tail->value->abstract); } } diff --git a/tests/qtest/libqos/qos_external.h b/tests/qtest/libqos/qos_external.h index 7b44930c55..f63388cb30 100644 --- a/tests/qtest/libqos/qos_external.h +++ b/tests/qtest/libqos/qos_external.h @@ -20,8 +20,12 @@ #define QOS_EXTERNAL_H #include "libqos/qgraph.h" -void apply_to_node(const char *name, bool is_machine, bool is_abstract); -void apply_to_qlist(QList *list, bool is_machine); +#include "libqos/malloc.h" +#include "qapi/qapi-types-machine.h" +#include "qapi/qapi-types-qom.h" + +void machines_apply_to_node(MachineInfoList *mach_info); +void types_apply_to_node(ObjectTypeInfoList *type_info); QGuestAllocator *get_machine_allocator(QOSGraphObject *obj); void *allocate_objects(QTestState *qts, char **path, QGuestAllocator **p_alloc); diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c index ad193f43a5..3062a13557 100644 --- a/tests/qtest/qos-test.c +++ b/tests/qtest/qos-test.c @@ -19,11 +19,12 @@ #include "qemu/osdep.h" #include #include "libqtest-single.h" +#include "qapi/error.h" #include "qapi/qmp/qdict.h" -#include "qapi/qmp/qbool.h" -#include "qapi/qmp/qstring.h" #include "qemu/module.h" -#include "qapi/qmp/qlist.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-machine.h" +#include "qapi/qapi-visit-qom.h" #include "libqos/malloc.h" #include "libqos/qgraph.h" #include "libqos/qgraph_internal.h" @@ -51,13 +52,20 @@ static void qos_set_machines_devices_available(void) { QDict *response; QDict *args = qdict_new(); - QList *list; + QObject *ret; + Visitor *v; + MachineInfoList *mach_info; + ObjectTypeInfoList *type_info; qtest_start("-machine none"); response = qmp("{ 'execute': 'query-machines' }"); - list = qdict_get_qlist(response, "return"); + ret = qdict_get(response, "return"); - apply_to_qlist(list, true); + v = qobject_input_visitor_new(ret); + visit_type_MachineInfoList(v, NULL, &mach_info, &error_abort); + visit_free(v); + machines_apply_to_node(mach_info); + qapi_free_MachineInfoList(mach_info); qobject_unref(response); @@ -66,10 +74,13 @@ static void qos_set_machines_devices_available(void) response = qmp("{'execute': 'qom-list-types'," " 'arguments': %p }", args); - g_assert(qdict_haskey(response, "return")); - list = qdict_get_qlist(response, "return"); + ret = qdict_get(response, "return"); - apply_to_qlist(list, false); + v = qobject_input_visitor_new(ret); + visit_type_ObjectTypeInfoList(v, NULL, &type_info, &error_abort); + visit_free(v); + types_apply_to_node(type_info); + qapi_free_ObjectTypeInfoList(type_info); qtest_end(); qobject_unref(response); From f6528054f2ef7aab5c70664419b982b50142c25a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 24 Apr 2020 09:11:42 +0200 Subject: [PATCH 31/32] libqos: Give get_machine_allocator() internal linkage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Markus Armbruster Message-Id: <20200424071142.3525-4-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- tests/qtest/libqos/qos_external.c | 2 +- tests/qtest/libqos/qos_external.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/qtest/libqos/qos_external.c b/tests/qtest/libqos/qos_external.c index c707dac3b9..9f5180e18d 100644 --- a/tests/qtest/libqos/qos_external.c +++ b/tests/qtest/libqos/qos_external.c @@ -66,7 +66,7 @@ void types_apply_to_node(ObjectTypeInfoList *type_info) } } -QGuestAllocator *get_machine_allocator(QOSGraphObject *obj) +static QGuestAllocator *get_machine_allocator(QOSGraphObject *obj) { return obj->get_driver(obj, "memory"); } diff --git a/tests/qtest/libqos/qos_external.h b/tests/qtest/libqos/qos_external.h index f63388cb30..72d7f91707 100644 --- a/tests/qtest/libqos/qos_external.h +++ b/tests/qtest/libqos/qos_external.h @@ -18,7 +18,6 @@ #ifndef QOS_EXTERNAL_H #define QOS_EXTERNAL_H -#include "libqos/qgraph.h" #include "libqos/malloc.h" #include "qapi/qapi-types-machine.h" @@ -26,7 +25,6 @@ void machines_apply_to_node(MachineInfoList *mach_info); void types_apply_to_node(ObjectTypeInfoList *type_info); -QGuestAllocator *get_machine_allocator(QOSGraphObject *obj); void *allocate_objects(QTestState *qts, char **path, QGuestAllocator **p_alloc); #endif From 8ef3a4be27efccd791d05e74b7b17d918f511a76 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Mon, 27 Apr 2020 09:57:04 +0900 Subject: [PATCH 32/32] qemu-option: pass NULL rather than 0 to the id of qemu_opts_set() The second argument 'id' is a pointer. Pass NULL rather than 0. Signed-off-by: Masahiro Yamada Message-Id: <20200427005704.2475782-1-masahiroy@kernel.org> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- softmmu/vl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..afd2615fb3 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -3059,19 +3059,19 @@ void qemu_init(int argc, char **argv, char **envp) } break; case QEMU_OPTION_kernel: - qemu_opts_set(qemu_find_opts("machine"), 0, "kernel", optarg, + qemu_opts_set(qemu_find_opts("machine"), NULL, "kernel", optarg, &error_abort); break; case QEMU_OPTION_initrd: - qemu_opts_set(qemu_find_opts("machine"), 0, "initrd", optarg, + qemu_opts_set(qemu_find_opts("machine"), NULL, "initrd", optarg, &error_abort); break; case QEMU_OPTION_append: - qemu_opts_set(qemu_find_opts("machine"), 0, "append", optarg, + qemu_opts_set(qemu_find_opts("machine"), NULL, "append", optarg, &error_abort); break; case QEMU_OPTION_dtb: - qemu_opts_set(qemu_find_opts("machine"), 0, "dtb", optarg, + qemu_opts_set(qemu_find_opts("machine"), NULL, "dtb", optarg, &error_abort); break; case QEMU_OPTION_cdrom: @@ -3182,7 +3182,7 @@ void qemu_init(int argc, char **argv, char **envp) } break; case QEMU_OPTION_bios: - qemu_opts_set(qemu_find_opts("machine"), 0, "firmware", optarg, + qemu_opts_set(qemu_find_opts("machine"), NULL, "firmware", optarg, &error_abort); break; case QEMU_OPTION_singlestep: