From b62e82be06a97ce48d1363bb28e8215ab2487a20 Mon Sep 17 00:00:00 2001 From: Dmitry Frolov Date: Wed, 6 Nov 2024 11:04:36 +0300 Subject: [PATCH 01/10] parallels: fix possible int overflow The sum "cluster_index + count" may overflow uint32_t. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Dmitry Frolov Message-ID: <20241106080521.219255-2-frolov@swemel.ru> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/parallels.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 9205a0864f..071b6dcaf8 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -184,11 +184,11 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap, BDRVParallelsState *s = bs->opaque; uint32_t cluster_index = host_cluster_index(s, off); unsigned long next_used; - if (cluster_index + count > bitmap_size) { + if ((uint64_t)cluster_index + count > bitmap_size) { return -E2BIG; } next_used = find_next_bit(bitmap, bitmap_size, cluster_index); - if (next_used < cluster_index + count) { + if (next_used < (uint64_t)cluster_index + count) { return -EBUSY; } bitmap_set(bitmap, cluster_index, count); From 757dbafe115e4f33782ac59812f37997f5b5ce6c Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 1 Nov 2024 13:36:57 -0400 Subject: [PATCH 02/10] iotests: reflow ReproducibleTestRunner arguments Trivial reflow to let the type names breathe. (I need to add a longer type name.) Signed-off-by: John Snow Message-ID: <20241101173700.965776-2-jsnow@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/iotests.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index ea48af4a7b..673bbcd356 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -1614,10 +1614,13 @@ class ReproducibleStreamWrapper: self.stream.write(arg) class ReproducibleTestRunner(unittest.TextTestRunner): - def __init__(self, stream: Optional[TextIO] = None, - resultclass: Type[unittest.TestResult] = - ReproducibleTestResult, - **kwargs: Any) -> None: + def __init__( + self, + stream: Optional[TextIO] = None, + resultclass: Type[unittest.TestResult] = + ReproducibleTestResult, + **kwargs: Any + ) -> None: rstream = ReproducibleStreamWrapper(stream or sys.stdout) super().__init__(stream=rstream, # type: ignore descriptions=True, From d808888429dbdd308e7348cb112ece06d287b87a Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 1 Nov 2024 13:36:58 -0400 Subject: [PATCH 03/10] iotests: correct resultclass type in ReproducibleTestRunner I have a vague memory that I suggested this base class to Vladimir and said "Maybe someday it will break, and I'll just fix it then." Guess that's today. Fixes various mypy errors in the "make check-tox" python test for at least Python3.8; seemingly requires a fairly modern mypy and/or Python base version to trigger. Signed-off-by: John Snow Message-ID: <20241101173700.965776-3-jsnow@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/iotests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 673bbcd356..19817c7353 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -1617,7 +1617,7 @@ class ReproducibleTestRunner(unittest.TextTestRunner): def __init__( self, stream: Optional[TextIO] = None, - resultclass: Type[unittest.TestResult] = + resultclass: Type[unittest.TextTestResult] = ReproducibleTestResult, **kwargs: Any ) -> None: From 4c600fdcd49c5661b658c325100dcd7754b0a479 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 1 Nov 2024 13:36:59 -0400 Subject: [PATCH 04/10] python: disable too-many-positional-arguments warning Newest versions of pylint complain about specifically positional arguments in addition to too many in general. We already disable the general case, so silence this new warning too. Signed-off-by: John Snow Message-ID: <20241101173700.965776-4-jsnow@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- python/setup.cfg | 1 + tests/qemu-iotests/pylintrc | 1 + 2 files changed, 2 insertions(+) diff --git a/python/setup.cfg b/python/setup.cfg index 3b4e2cc550..cf5af7e664 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -142,6 +142,7 @@ ignore_missing_imports = True disable=consider-using-f-string, consider-using-with, too-many-arguments, + too-many-positional-arguments, too-many-function-args, # mypy handles this with less false positives. too-many-instance-attributes, no-member, # mypy also handles this better. diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc index 05b75ee59b..c5f4833e45 100644 --- a/tests/qemu-iotests/pylintrc +++ b/tests/qemu-iotests/pylintrc @@ -13,6 +13,7 @@ disable=invalid-name, no-else-return, too-few-public-methods, too-many-arguments, + too-many-positional-arguments, too-many-branches, too-many-lines, too-many-locals, From 05fd7214d8262878ac4d8bf8909fa315f1f589a6 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 1 Nov 2024 13:37:00 -0400 Subject: [PATCH 05/10] python: silence pylint raising-non-exception error As of (at least) pylint 3.3.1, this code trips pylint up into believing we are raising something other than an Exception. We are not: the first two values may indeed be "None", but the last and final value must by definition be a SystemExit exception. Signed-off-by: John Snow Message-ID: <20241101173700.965776-5-jsnow@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- python/scripts/mkvenv.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/scripts/mkvenv.py b/python/scripts/mkvenv.py index f2526af0a0..8ac5b0b2a0 100644 --- a/python/scripts/mkvenv.py +++ b/python/scripts/mkvenv.py @@ -379,6 +379,9 @@ def make_venv( # pylint: disable=too-many-arguments try: builder.create(str(env_dir)) except SystemExit as exc: + # pylint 3.3 bug: + # pylint: disable=raising-non-exception, raise-missing-from + # Some versions of the venv module raise SystemExit; *nasty*! # We want the exception that prompted it. It might be a subprocess # error that has output we *really* want to see. From 5102f9df4a9a7adfbd902f9515c3f8f53dba288e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 19 Nov 2024 13:03:53 +0100 Subject: [PATCH 06/10] qdev: Fix set_pci_devfn() to visit option only once pci_devfn properties accept either a string or an integer as input. To implement this, set_pci_devfn() first tries to visit the option as a string, and if that fails, it visits it as an integer instead. While the QemuOpts visitor happens to accept this, it is invalid according to the visitor interface. QObject input visitors run into an assertion failure when this is done. QObject input visitors are used with the JSON syntax version of -device on the command line: $ ./qemu-system-x86_64 -enable-kvm -M q35 -device pcie-pci-bridge,id=pci.1,bus=pcie.0 -blockdev null-co,node-name=disk -device '{ "driver": "virtio-blk-pci", "drive": "disk", "id": "virtio-disk0", "bus": "pci.1", "addr": 1 }' qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143: QObject *qobject_input_try_get_object(QObjectInputVisitor *, const char *, _Bool): Assertion `removed' failed. The proper way to accept both strings and integers is using the alternate mechanism, which tells us the type of the input before it's visited. With this information, we can directly visit it as the right type. This fixes set_pci_devfn() by using the alternate mechanism. Cc: qemu-stable@nongnu.org Reported-by: Peter Maydell Signed-off-by: Kevin Wolf Message-ID: <20241119120353.57812-1-kwolf@redhat.com> Acked-by: Paolo Bonzini Reviewed-by: Markus Armbruster Signed-off-by: Kevin Wolf --- hw/core/qdev-properties-system.c | 54 +++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index a61c5ee6dd..22ea1ed358 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -816,39 +816,57 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { Property *prop = opaque; + g_autofree GenericAlternate *alt; int32_t value, *ptr = object_field_prop_ptr(obj, prop); unsigned int slot, fn, n; - char *str; + g_autofree char *str = NULL; - if (!visit_type_str(v, name, &str, NULL)) { + if (!visit_start_alternate(v, name, &alt, sizeof(*alt), errp)) { + return; + } + + switch (alt->type) { + case QTYPE_QSTRING: + if (!visit_type_str(v, name, &str, errp)) { + goto out; + } + + if (sscanf(str, "%x.%x%n", &slot, &fn, &n) != 2) { + fn = 0; + if (sscanf(str, "%x%n", &slot, &n) != 1) { + goto invalid; + } + } + if (str[n] != '\0' || fn > 7 || slot > 31) { + goto invalid; + } + *ptr = slot << 3 | fn; + break; + + case QTYPE_QNUM: if (!visit_type_int32(v, name, &value, errp)) { - return; + goto out; } if (value < -1 || value > 255) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null", "a value between -1 and 255"); - return; + goto out; } *ptr = value; - return; + break; + + default: + error_setg(errp, "Invalid parameter type for '%s', expected int or str", + name ? name : "null"); + goto out; } - if (sscanf(str, "%x.%x%n", &slot, &fn, &n) != 2) { - fn = 0; - if (sscanf(str, "%x%n", &slot, &n) != 1) { - goto invalid; - } - } - if (str[n] != '\0' || fn > 7 || slot > 31) { - goto invalid; - } - *ptr = slot << 3 | fn; - g_free(str); - return; + goto out; invalid: error_set_from_qdev_prop_error(errp, EINVAL, obj, name, str); - g_free(str); +out: + visit_end_alternate(v, (void **) &alt); } static int print_pci_devfn(Object *obj, Property *prop, char *dest, From 770de685353e8c495ad4773fbd4bc0db997e4dfd Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 22 Nov 2024 23:40:42 +0100 Subject: [PATCH 07/10] tests/avocado/hotplug_blk: Fix addr in device_add command pci_devfn properties accept both integer and string values, but integer 1 and string '1' have different meanings: The integer value means device 0, function 1 whereas the string value '1' is short for '1.0' and means device 1, function 0. This test wants the string version so that the device actually becomes visible for the guest. device_add hides the problem because it goes through QemuOpts, which turns all properties into strings - this is a QEMU bug that we want to fix, but that cancelled out the bug in this test. Fix the test first so that device_add can be fixed afterwards. Signed-off-by: Kevin Wolf Message-ID: <20241122224042.149258-1-kwolf@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Kevin Wolf --- tests/avocado/hotplug_blk.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/avocado/hotplug_blk.py b/tests/avocado/hotplug_blk.py index d55ded1c1d..b36bca02ec 100644 --- a/tests/avocado/hotplug_blk.py +++ b/tests/avocado/hotplug_blk.py @@ -33,7 +33,7 @@ class HotPlug(LinuxTest): 'drive': 'disk', 'id': 'virtio-disk0', 'bus': 'pci.1', - 'addr': 1 + 'addr': '1', } self.assert_no_vda() From be93fd53723cbdca675bd9ed112dae5cabbe1e91 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 27 Aug 2024 15:27:50 -0400 Subject: [PATCH 08/10] qdev-monitor: avoid QemuOpts in QMP device_add MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The QMP device_add monitor command converts the QDict arguments to QemuOpts and then back again to QDict. This process only supports scalar types. Device properties like virtio-blk-pci's iothread-vq-mapping (an array of objects) are silently dropped by qemu_opts_from_qdict() during the QemuOpts conversion even though QAPI is capable of validating them. As a result, hotplugging virtio-blk-pci devices with the iothread-vq-mapping property does not work as expected (the property is ignored). Get rid of the QemuOpts conversion in qmp_device_add() and call qdev_device_add_from_qdict() with from_json=true. Using the QMP command's QDict arguments directly allows non-scalar properties. The HMP is also adjusted since qmp_device_add()'s now expects properly typed JSON arguments and cannot be used from HMP anymore. Move the code that was previously in qmp_device_add() (with QemuOpts conversion and from_json=false) into hmp_device_add() so that its behavior is unchanged. This patch changes the behavior of QMP device_add but not HMP device_add. QMP clients that sent incorrectly typed device_add QMP commands no longer work. This is a breaking change but clients should be using the correct types already. See the netdev_add QAPIfication in commit db2a380c8457 for similar reasoning and object-add in commit 9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false for the time being. Markus helped me figure this out and even provided a draft patch. The code ended up very close to what he suggested. Suggested-by: Markus Armbruster Cc: Daniel P. Berrangé Signed-off-by: Stefan Hajnoczi Message-ID: <20240827192751.948633-2-stefanha@redhat.com> Reviewed-by: Daniel P. Berrangé Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- system/qdev-monitor.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index 4c09b38ffb..03ae610649 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -856,18 +856,9 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict) void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) { - QemuOpts *opts; DeviceState *dev; - opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp); - if (!opts) { - return; - } - if (!monitor_cur_is_qmp() && qdev_device_help(opts)) { - qemu_opts_del(opts); - return; - } - dev = qdev_device_add(opts, errp); + dev = qdev_device_add_from_qdict(qdict, true, errp); if (!dev) { /* * Drain all pending RCU callbacks. This is done because @@ -879,9 +870,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) * to the user */ drain_call_rcu(); - - qemu_opts_del(opts); - return; } object_unref(OBJECT(dev)); } @@ -1018,8 +1006,34 @@ void qmp_device_sync_config(const char *id, Error **errp) void hmp_device_add(Monitor *mon, const QDict *qdict) { Error *err = NULL; + QemuOpts *opts; + DeviceState *dev; - qmp_device_add((QDict *)qdict, NULL, &err); + opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &err); + if (!opts) { + goto out; + } + if (qdev_device_help(opts)) { + qemu_opts_del(opts); + return; + } + dev = qdev_device_add(opts, &err); + if (!dev) { + /* + * Drain all pending RCU callbacks. This is done because + * some bus related operations can delay a device removal + * (in this case this can happen if device is added and then + * removed due to a configuration error) + * to a RCU callback, but user might expect that this interface + * will finish its job completely once qmp command returns result + * to the user + */ + drain_call_rcu(); + + qemu_opts_del(opts); + } + object_unref(dev); +out: hmp_handle_error(mon, err); } From 11bf1d6aa06138e93b274e942d6992af63ffc510 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 27 Aug 2024 15:27:51 -0400 Subject: [PATCH 09/10] vl: use qmp_device_add() in qemu_create_cli_devices() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qemu_create_cli_devices() should use qmp_device_add() to match the behavior of the QMP monitor. A comment explained that libvirt changes implementing strict CLI syntax were needed. Peter Krempa has confirmed that modern libvirt uses the same JSON for -device (CLI) and device_add (QMP). Go ahead and use qmp_device_add(). Cc: Peter Krempa Reviewed-by: Markus Armbruster Signed-off-by: Stefan Hajnoczi Message-ID: <20240827192751.948633-3-stefanha@redhat.com> Reviewed-by: Daniel P. Berrangé Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- system/vl.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/system/vl.c b/system/vl.c index 3bb8f2db9a..54998fdbc7 100644 --- a/system/vl.c +++ b/system/vl.c @@ -2653,17 +2653,11 @@ static void qemu_create_cli_devices(void) qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, &error_fatal); QTAILQ_FOREACH(opt, &device_opts, next) { - DeviceState *dev; + QObject *ret_data = NULL; + loc_push_restore(&opt->loc); - /* - * TODO Eventually we should call qmp_device_add() here to make sure it - * behaves the same, but QMP still has to accept incorrectly typed - * options until libvirt is fixed and we want to be strict on the CLI - * from the start, so call qdev_device_add_from_qdict() directly for - * now. - */ - dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal); - object_unref(OBJECT(dev)); + qmp_device_add(opt->opts, &ret_data, &error_fatal); + assert(ret_data == NULL); /* error_fatal aborts */ loc_pop(&opt->loc); } rom_reset_order_override(); From fbdea3d6c13d5a75895c287a004c6f1a6bf6c164 Mon Sep 17 00:00:00 2001 From: Jakub Jelen Date: Wed, 13 Nov 2024 12:55:23 +0000 Subject: [PATCH 10/10] ssh: Do not switch session to non-blocking mode The libssh does not handle non-blocking mode in SFTP correctly. The driver code already changes the mode to blocking for the SFTP initialization, but for some reason changes to non-blocking mode. This used to work accidentally until libssh in 0.11 branch merged the patch to avoid infinite looping in case of network errors: https://gitlab.com/libssh/libssh-mirror/-/merge_requests/498 Since then, the ssh driver in qemu fails to read files over SFTP as the first SFTP messages exchanged after switching the session to non-blocking mode return SSH_AGAIN, but that message is lost int the SFTP internals and interpretted as SSH_ERROR, which is returned to the caller: https://gitlab.com/libssh/libssh-mirror/-/issues/280 This is indeed an issue in libssh that we should address in the long term, but it will require more work on the internals. For now, the SFTP is not supported in non-blocking mode. Fixes: https://gitlab.com/libssh/libssh-mirror/-/issues/280 Signed-off-by: Jakub Jelen Signed-off-by: Richard W.M. Jones Message-ID: <20241113125526.2495731-1-rjones@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/ssh.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index 9f8140bcb6..b9f33ec739 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -866,9 +866,6 @@ static int ssh_open(BlockDriverState *bs, QDict *options, int bdrv_flags, goto err; } - /* Go non-blocking. */ - ssh_set_blocking(s->session, 0); - if (s->attrs->type == SSH_FILEXFER_TYPE_REGULAR) { bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; }