[PULL 00/32] Miscellaneous patches for 2020-04-29

[PULL 00/32] Miscellaneous patches for 2020-04-29

[Markus Armbruster]

The following changes since commit fdd76fecdde1ad444ff4deb7f1c4f7e4a1ef97d6:

  Update version for v5.0.0 release (2020-04-28 17:46:57 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-misc-2020-04-29

for you to fetch changes up to 8ef3a4be27efccd791d05e74b7b17d918f511a76:

  qemu-option: pass NULL rather than 0 to the id of qemu_opts_set() (2020-04-29 08:01:52 +0200)

----------------------------------------------------------------
Miscellaneous patches for 2020-04-29

* Fix CLI option parsing corner cases
* Fix bugs on (unlikely) error paths
* Fix undefined behavior for silly option arguments
* Tidy up bamboo and sam460ex reporting of funny memory sizes
* Clean up error API violations
* A bit of refactoring here and there

----------------------------------------------------------------
Markus Armbruster (30):
      tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()
      qemu-options: Factor out get_opt_name_value() helper
      qemu-option: Fix sloppy recognition of "id=..." after ",,"
      qemu-option: Fix has_help_option()'s sloppy parsing
      test-qemu-opts: Simplify test_has_help_option() after bug fix
      qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
      qemu-img: Factor out accumulate_options() helper
      qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite
      qemu-img: Reject broken -o ""
      cryptodev: Fix cryptodev_builtin_cleanup() error API violation
      block/file-posix: Fix check_cache_dropped() error handling
      cpus: Fix configure_icount() error API violation
      cpus: Proper range-checking for -icount shift=N
      arm/virt: Fix virt_machine_device_plug_cb() error API violation
      fdc: Fix fallback=auto error handling
      bochs-display: Fix vgamem=SIZE error handling
      virtio-net: Fix duplex=... and speed=... error handling
      xen/pt: Fix flawed conversion to realize()
      io: Fix qio_channel_socket_close() error handling
      migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
      tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff
      qga: Fix qmp_guest_get_memory_blocks() error handling
      qga: Fix qmp_guest_suspend_{disk, ram}() error handling
      sam460ex: Suppress useless warning on -m 32 and -m 64
      smbus: Fix spd_data_generate() error API violation
      bamboo, sam460ex: Tidy up error message for unsupported RAM size
      smbus: Fix spd_data_generate() for number of banks > 2
      Makefile: Drop unused, broken target recurse-fuzz
      fuzz: Simplify how we compute available machines and types
      libqos: Give get_machine_allocator() internal linkage

Masahiro Yamada (1):
      qemu-option: pass NULL rather than 0 to the id of qemu_opts_set()

Philippe Mathieu-Daudé (1):
      various: Remove suspicious '\' character outside of #define in C code

 Makefile                          |   1 -
 include/hw/i2c/smbus_eeprom.h     |   2 +-
 include/qemu/option.h             |   1 -
 tests/qtest/libqos/qos_external.h |  10 +-
 backends/cryptodev-builtin.c      |  10 +-
 block/file-posix.c                |   5 +-
 block/replication.c               |   4 +-
 block/vhdx.c                      |   8 +-
 cpus.c                            |  52 ++++++----
 dump/dump.c                       |   2 +-
 hw/arm/virt.c                     |   4 +-
 hw/block/fdc.c                    |   1 +
 hw/display/bochs-display.c        |   6 +-
 hw/i2c/smbus_eeprom.c             |  32 +-----
 hw/mips/mips_fulong2e.c           |  10 +-
 hw/net/virtio-net.c               |   7 +-
 hw/ppc/ppc4xx_devs.c              |   8 +-
 hw/ppc/sam460ex.c                 |  13 +--
 hw/riscv/sifive_u.c               |   2 +-
 hw/scsi/scsi-disk.c               |   2 +-
 hw/sd/sdhci.c                     |   2 +-
 hw/xen/xen_pt.c                   |  12 +--
 io/channel-socket.c               |   5 +-
 migration/colo.c                  |   8 +-
 qemu-img.c                        |  87 +++++++++-------
 qga/commands-posix.c              |   3 +
 qga/commands-win32.c              |  14 +++
 softmmu/vl.c                      |  10 +-
 target/i386/cpu.c                 |  18 ++--
 target/microblaze/cpu.c           |  14 +--
 target/ppc/translate_init.inc.c   |   4 +-
 tests/qtest/fuzz/qos_fuzz.c       |  34 ++----
 tests/qtest/libqos/qos_external.c |  72 +++++--------
 tests/qtest/qos-test.c            |  29 ++++--
 tests/test-logging.c              |   4 +-
 tests/test-qemu-opts.c            |  46 ++++++++-
 util/qemu-option.c                | 210 +++++++++++++++++++-------------------
 37 files changed, 391 insertions(+), 361 deletions(-)

-- 
2.21.1

[PULL 01/32] various: Remove suspicious '\' character outside of #define in C code

[Markus Armbruster]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

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 <marcandre.lureau@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200412223619.11284-2-f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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) |
-- 
2.21.1

[PULL 02/32] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Markus Armbruster]

The two turn out to be inconsistent for "a,b,,help".  Test case
marked /* BUG */.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200415074927.19897-2-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 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);
-- 
2.21.1

[PULL 03/32] qemu-options: Factor out get_opt_name_value() helper

[Markus Armbruster]

The next commits will put it to use.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
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);
 }
 
 /**
-- 
2.21.1

[PULL 04/32] qemu-option: Fix sloppy recognition of "id=..." after ", , "

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
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
-- 
2.21.1

[PULL 05/32] qemu-option: Fix has_help_option()'s sloppy parsing

[Markus Armbruster]

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 <armbru@redhat.com>
Message-Id: <20200415074927.19897-5-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 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
-- 
2.21.1

[PULL 06/32] test-qemu-opts: Simplify test_has_help_option() after bug fix

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
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);
     }
 }
-- 
2.21.1

[PULL 07/32] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()

[Markus Armbruster]

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 <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
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 {
-- 
2.21.1

[PULL 08/32] qemu-img: Factor out accumulate_options() helper

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
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)) {
-- 
2.21.1

[PULL 09/32] qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite

[Markus Armbruster]

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 <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
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) {
-- 
2.21.1

[PULL 10/32] qemu-img: Reject broken -o ""

[Markus Armbruster]

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 <armbru@redhat.com>
Message-Id: <20200415074927.19897-10-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 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;
     }
 
-- 
2.21.1

[PULL 11/32] cryptodev: Fix cryptodev_builtin_cleanup() error API violation

[Markus Armbruster]

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 <arei.gonglei@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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);
         }
     }
 
-- 
2.21.1

[PULL 12/32] block/file-posix: Fix check_cache_dropped() error handling

[Markus Armbruster]

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 <stefanha@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
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) {
-- 
2.21.1

[PULL 13/32] cpus: Fix configure_icount() error API violation

[Markus Armbruster]

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 <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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;
-- 
2.21.1

[PULL 14/32] cpus: Proper range-checking for -icount shift=N

[Markus Armbruster]

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 <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
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;
         }
-- 
2.21.1

[PULL 15/32] arm/virt: Fix virt_machine_device_plug_cb() error API violation

[Markus Armbruster]

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 <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
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);
     }
 }
 
-- 
2.21.1

[PULL 16/32] fdc: Fix fallback=auto error handling

[Markus Armbruster]

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 <jsnow@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
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 */
-- 
2.21.1

[PULL 17/32] bochs-display: Fix vgamem=SIZE error handling

[Markus Armbruster]

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 <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200422130719.28225-8-armbru@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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,
-- 
2.21.1

[PULL 18/32] virtio-net: Fix duplex=... and speed=... error handling

[Markus Armbruster]

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" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200422130719.28225-9-armbru@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 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);
     }
 
-- 
2.21.1

[PULL 19/32] xen/pt: Fix flawed conversion to realize()

[Markus Armbruster]

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 <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Paul Durrant <paul@xen.org>
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;
-- 
2.21.1

[PULL 20/32] io: Fix qio_channel_socket_close() error handling

[Markus Armbruster]

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é <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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;
-- 
2.21.1

[PULL 21/32] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling

[Markus Armbruster]

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 <chen.zhang@intel.com>
Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Zhang Chen <chen.zhang@intel.com>
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);
 }
-- 
2.21.1

[PULL 22/32] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Markus Armbruster]

Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200422130719.28225-13-armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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);
 
-- 
2.21.1

[PULL 23/32] qga: Fix qmp_guest_get_memory_blocks() error handling

[Markus Armbruster]

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 <mdroth@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200422130719.28225-14-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 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;
-- 
2.21.1

[PULL 24/32] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Markus Armbruster]

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 <mdroth@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200422130719.28225-15-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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);
-- 
2.21.1

[PULL 25/32] sam460ex: Suppress useless warning on -m 32 and -m 64

[Markus Armbruster]

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 <armbru@redhat.com>
Message-Id: <20200422134815.1584-2-armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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);
     }
-- 
2.21.1

[PULL 26/32] smbus: Fix spd_data_generate() error API violation

[Markus Armbruster]

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 <armbru@redhat.com>
Message-Id: <20200422134815.1584-3-armbru@redhat.com>
---
 include/hw/i2c/smbus_eeprom.h |  2 +-
 hw/i2c/smbus_eeprom.c         | 30 ++++--------------------------
 hw/mips/mips_fulong2e.c       | 10 ++--------
 hw/ppc/sam460ex.c             | 12 +++---------
 4 files changed, 10 insertions(+), 44 deletions(-)

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
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);
 
-- 
2.21.1

[PULL 27/32] bamboo, sam460ex: Tidy up error message for unsupported RAM size

[Markus Armbruster]

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 <armbru@redhat.com>
Message-Id: <20200422134815.1584-4-armbru@redhat.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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);
-- 
2.21.1

[PULL 28/32] smbus: Fix spd_data_generate() for number of banks > 2

[Markus Armbruster]

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 <armbru@redhat.com>
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);
-- 
2.21.1

[PULL 29/32] Makefile: Drop unused, broken target recurse-fuzz

[Markus Armbruster]

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 <armbru@redhat.com>
Message-Id: <20200424071142.3525-2-armbru@redhat.com>
Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
---
 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
-- 
2.21.1

[PULL 30/32] fuzz: Simplify how we compute available machines and types

[Markus Armbruster]

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 <armbru@redhat.com>
Message-Id: <20200424071142.3525-3-armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/libqos/qos_external.h |  8 +++-
 tests/qtest/fuzz/qos_fuzz.c       | 34 ++++-----------
 tests/qtest/libqos/qos_external.c | 70 +++++++++++--------------------
 tests/qtest/qos-test.c            | 29 +++++++++----
 4 files changed, 59 insertions(+), 82 deletions(-)

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/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/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 <getopt.h>
 #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);
-- 
2.21.1

[PULL 31/32] libqos: Give get_machine_allocator() internal linkage

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200424071142.3525-4-armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/libqos/qos_external.h | 2 --
 tests/qtest/libqos/qos_external.c | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

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
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");
 }
-- 
2.21.1

[PULL 32/32] qemu-option: pass NULL rather than 0 to the id of qemu_opts_set()

[Markus Armbruster]

From: Masahiro Yamada <masahiroy@kernel.org>

The second argument 'id' is a pointer. Pass NULL rather than 0.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Message-Id: <20200427005704.2475782-1-masahiroy@kernel.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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:
-- 
2.21.1

Re: [PULL 00/32] Miscellaneous patches for 2020-04-29

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200429072048.29963-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200429072048.29963-1-armbru@redhat.com
Subject: [PULL 00/32] Miscellaneous patches for 2020-04-29
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
bbf2736 qemu-option: pass NULL rather than 0 to the id of qemu_opts_set()
3736e38 libqos: Give get_machine_allocator() internal linkage
40a5fe7 fuzz: Simplify how we compute available machines and types
08bd8b8 Makefile: Drop unused, broken target recurse-fuzz
96b5543 smbus: Fix spd_data_generate() for number of banks > 2
a2b3bfe bamboo, sam460ex: Tidy up error message for unsupported RAM size
8cc9661 smbus: Fix spd_data_generate() error API violation
c6b2e22 sam460ex: Suppress useless warning on -m 32 and -m 64
8676301 qga: Fix qmp_guest_suspend_{disk, ram}() error handling
f2ba556 qga: Fix qmp_guest_get_memory_blocks() error handling
1e52928 tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff
42be747 migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
69a4ebd io: Fix qio_channel_socket_close() error handling
21ca33c xen/pt: Fix flawed conversion to realize()
0397c8f virtio-net: Fix duplex=... and speed=... error handling
c849702 bochs-display: Fix vgamem=SIZE error handling
4734313 fdc: Fix fallback=auto error handling
d40f549 arm/virt: Fix virt_machine_device_plug_cb() error API violation
669233d cpus: Proper range-checking for -icount shift=N
33e02df cpus: Fix configure_icount() error API violation
5e67774 block/file-posix: Fix check_cache_dropped() error handling
670e190 cryptodev: Fix cryptodev_builtin_cleanup() error API violation
3e5b9b0 qemu-img: Reject broken -o ""
f08bae7 qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite
536796f qemu-img: Factor out accumulate_options() helper
eb08b54 qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
f866f2c test-qemu-opts: Simplify test_has_help_option() after bug fix
42681dc qemu-option: Fix has_help_option()'s sloppy parsing
4cf9436 qemu-option: Fix sloppy recognition of "id=..." after ", , "
9b629d5 qemu-options: Factor out get_opt_name_value() helper
a85727e tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()
fc0770d various: Remove suspicious '\' character outside of #define in C code

=== OUTPUT BEGIN ===
1/32 Checking commit fc0770db08c1 (various: Remove suspicious '\' character outside of #define in C code)
2/32 Checking commit a85727e34525 (tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt())
WARNING: Block comments use a leading /* on a separate line
#44: FILE: tests/test-qemu-opts.c:752:
+        { "a,b,,help", false /* BUG */, true, true },

WARNING: Block comments use a leading /* on a separate line
#45: FILE: tests/test-qemu-opts.c:753:
+        { "a,b,,?", false /* BUG */, true, true },

total: 0 errors, 2 warnings, 56 lines checked

Patch 2/32 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/32 Checking commit 9b629d54ce8a (qemu-options: Factor out get_opt_name_value() helper)
4/32 Checking commit 4cf9436358f5 (qemu-option: Fix sloppy recognition of "id=..." after ", , ")
5/32 Checking commit 42681dc0fb50 (qemu-option: Fix has_help_option()'s sloppy parsing)
6/32 Checking commit f866f2c224ff (test-qemu-opts: Simplify test_has_help_option() after bug fix)
7/32 Checking commit eb08b546645f (qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily())
8/32 Checking commit 536796f458ef (qemu-img: Factor out accumulate_options() helper)
9/32 Checking commit f08bae71a126 (qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite)
ERROR: suspect code indent for conditional statements (4, 4)
#63: FILE: qemu-img.c:243:
+    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
+    }

total: 1 errors, 0 warnings, 67 lines checked

Patch 9/32 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

10/32 Checking commit 3e5b9b0b2c62 (qemu-img: Reject broken -o "")
ERROR: trailing whitespace
#51: FILE: qemu-img.c:234:
+ * $

total: 1 errors, 0 warnings, 18 lines checked

Patch 10/32 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

11/32 Checking commit 670e190178ba (cryptodev: Fix cryptodev_builtin_cleanup() error API violation)
12/32 Checking commit 5e677748f96e (block/file-posix: Fix check_cache_dropped() error handling)
13/32 Checking commit 33e02df34d16 (cpus: Fix configure_icount() error API violation)
ERROR: consider using qemu_strtol in preference to strtol
#59: FILE: cpus.c:818:
+        time_shift = strtol(option, &rem_str, 0);

total: 1 errors, 0 warnings, 70 lines checked

Patch 13/32 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

14/32 Checking commit 669233d94449 (cpus: Proper range-checking for -icount shift=N)
15/32 Checking commit d40f549786cd (arm/virt: Fix virt_machine_device_plug_cb() error API violation)
16/32 Checking commit 473431395c4a (fdc: Fix fallback=auto error handling)
17/32 Checking commit c849702a667f (bochs-display: Fix vgamem=SIZE error handling)
18/32 Checking commit 0397c8fa48c7 (virtio-net: Fix duplex=... and speed=... error handling)
19/32 Checking commit 21ca33c29fec (xen/pt: Fix flawed conversion to realize())
20/32 Checking commit 69a4ebdcc79c (io: Fix qio_channel_socket_close() error handling)
21/32 Checking commit 42be7477a307 (migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling)
22/32 Checking commit 1e5292888756 (tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff)
23/32 Checking commit f2ba556c242f (qga: Fix qmp_guest_get_memory_blocks() error handling)
24/32 Checking commit 867630155baf (qga: Fix qmp_guest_suspend_{disk, ram}() error handling)
25/32 Checking commit c6b2e225677b (sam460ex: Suppress useless warning on -m 32 and -m 64)
26/32 Checking commit 8cc9661b4025 (smbus: Fix spd_data_generate() error API violation)
27/32 Checking commit a2b3bfe4b4cf (bamboo, sam460ex: Tidy up error message for unsupported RAM size)
ERROR: unnecessary whitespace before a quoted newline
#40: FILE: hw/ppc/ppc4xx_devs.c:723:
+        error_printf("Possible valid RAM size: %" PRIi64 " MiB \n",

total: 1 errors, 0 warnings, 15 lines checked

Patch 27/32 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

28/32 Checking commit 96b55431cda5 (smbus: Fix spd_data_generate() for number of banks > 2)
29/32 Checking commit 08bd8b81657a (Makefile: Drop unused, broken target recurse-fuzz)
30/32 Checking commit 40a5fe78ba42 (fuzz: Simplify how we compute available machines and types)
31/32 Checking commit 3736e38b637e (libqos: Give get_machine_allocator() internal linkage)
32/32 Checking commit bbf27368e6dd (qemu-option: pass NULL rather than 0 to the id of qemu_opts_set())
WARNING: line over 80 characters
#48: FILE: softmmu/vl.c:3185:
+                qemu_opts_set(qemu_find_opts("machine"), NULL, "firmware", optarg,

total: 0 errors, 1 warnings, 31 lines checked

Patch 32/32 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200429072048.29963-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PULL 12/32] block/file-posix: Fix check_cache_dropped() error handling

[Stefan Hajnoczi]

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

On Wed, Apr 29, 2020 at 09:20:28AM +0200, Markus Armbruster wrote:
> 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 <stefanha@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20200422130719.28225-3-armbru@redhat.com>
> ---
>  block/file-posix.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Thanks for fixing this bug!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL 00/32] Miscellaneous patches for 2020-04-29

[Peter Maydell]

On Wed, 29 Apr 2020 at 08:23, Markus Armbruster <armbru@redhat.com> wrote:
>
> The following changes since commit fdd76fecdde1ad444ff4deb7f1c4f7e4a1ef97d6:
>
>   Update version for v5.0.0 release (2020-04-28 17:46:57 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-misc-2020-04-29
>
> for you to fetch changes up to 8ef3a4be27efccd791d05e74b7b17d918f511a76:
>
>   qemu-option: pass NULL rather than 0 to the id of qemu_opts_set() (2020-04-29 08:01:52 +0200)
>
> ----------------------------------------------------------------
> Miscellaneous patches for 2020-04-29
>
> * Fix CLI option parsing corner cases
> * Fix bugs on (unlikely) error paths
> * Fix undefined behavior for silly option arguments
> * Tidy up bamboo and sam460ex reporting of funny memory sizes
> * Clean up error API violations
> * A bit of refactoring here and there


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

Re: [PULL 13/32] cpus: Fix configure_icount() error API violation

[Peter Maydell]

On Wed, 29 Apr 2020 at 08:34, Markus Armbruster <armbru@redhat.com> wrote:
>
> 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.

Hi; Coverity complains about this change (CID 1428754):
>
>  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;

Previously, if option was NULL we would always take this early
exit. Now we only take the exit if option is NULL and the
qemu_opt_get() returns true, so in some cases execution
can continue through the function with a NULL option...

>      }
>
> -    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) {

...but here we pass option to strcmp(), which is wrong if it
can be NULL.

thanks
-- PMM

Re: [PULL 13/32] cpus: Fix configure_icount() error API violation

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 29 Apr 2020 at 08:34, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> 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.
>
> Hi; Coverity complains about this change (CID 1428754):
>>
>>  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;
>
> Previously, if option was NULL we would always take this early
> exit. Now we only take the exit if option is NULL and the
> qemu_opt_get() returns true, so in some cases execution
> can continue through the function with a NULL option...
>
>>      }
>>
>> -    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) {
>
> ...but here we pass option to strcmp(), which is wrong if it
> can be NULL.

Right.  I'll post a fix.  Thank you!