[PATCH v2 7/7] travis.yml: Enable builds on arm64, ppc64le and s390x
Travis recently added the possibility to test on these architectures,
too, so let's enable them in our travis.yml file to extend our test
coverage.
Unfortunately, the libssh in this Ubuntu version (bionic) is in a pretty
unusable Frankenstein state and libspice-server-dev is not available here,
so we can not use the global list of packages to install, but have to
provide individual package lists instead.
Also, some of the iotests crash when using "dist: bionic" on arm64
and ppc64le, thus these two builders have to use "dist: xenial" until
the problem is understood / fixed.
Signed-off-by: Thomas Huth
---
.travis.yml | 86 +
1 file changed, 86 insertions(+)
diff --git a/.travis.yml b/.travis.yml
index 445b0646c1..0e6458b0af 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -354,6 +354,92 @@ matrix:
- TEST_CMD="make -j3 check-tcg V=1"
- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
+- arch: arm64
+ dist: xenial
+ addons:
+apt_packages:
+ - libaio-dev
+ - libattr1-dev
+ - libbrlapi-dev
+ - libcap-ng-dev
+ - libgcrypt20-dev
+ - libgnutls28-dev
+ - libgtk-3-dev
+ - libiscsi-dev
+ - liblttng-ust-dev
+ - libncurses5-dev
+ - libnfs-dev
+ - libnss3-dev
+ - libpixman-1-dev
+ - libpng-dev
+ - librados-dev
+ - libsdl2-dev
+ - libseccomp-dev
+ - liburcu-dev
+ - libusb-1.0-0-dev
+ - libvdeplug-dev
+ - libvte-2.91-dev
+ env:
+- TEST_CMD="make check check-tcg V=1"
+- CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS}"
+
+- arch: ppc64le
+ dist: xenial
+ addons:
+apt_packages:
+ - libaio-dev
+ - libattr1-dev
+ - libbrlapi-dev
+ - libcap-ng-dev
+ - libgcrypt20-dev
+ - libgnutls28-dev
+ - libgtk-3-dev
+ - libiscsi-dev
+ - liblttng-ust-dev
+ - libncurses5-dev
+ - libnfs-dev
+ - libnss3-dev
+ - libpixman-1-dev
+ - libpng-dev
+ - librados-dev
+ - libsdl2-dev
+ - libseccomp-dev
+ - liburcu-dev
+ - libusb-1.0-0-dev
+ - libvdeplug-dev
+ - libvte-2.91-dev
+ env:
+- TEST_CMD="make check check-tcg V=1"
+- CONFIG="--disable-containers
--target-list=${MAIN_SOFTMMU_TARGETS},ppc64le-linux-user"
+
+- arch: s390x
+ dist: bionic
+ addons:
+apt_packages:
+ - libaio-dev
+ - libattr1-dev
+ - libbrlapi-dev
+ - libcap-ng-dev
+ - libgcrypt20-dev
+ - libgnutls28-dev
+ - libgtk-3-dev
+ - libiscsi-dev
+ - liblttng-ust-dev
+ - libncurses5-dev
+ - libnfs-dev
+ - libnss3-dev
+ - libpixman-1-dev
+ - libpng-dev
+ - librados-dev
+ - libsdl2-dev
+ - libseccomp-dev
+ - liburcu-dev
+ - libusb-1.0-0-dev
+ - libvdeplug-dev
+ - libvte-2.91-dev
+ env:
+- TEST_CMD="make check check-tcg V=1"
+- CONFIG="--disable-containers
--target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
# Release builds
# The make-release script expect a QEMU version, so our tag must start
with a 'v'.
--
2.18.1
[PATCH v2 0/7] Enable Travis builds on arm64, ppc64le and s390x
Travis recently added build hosts for arm64, ppc64le and s390x, so this is a welcome addition to our Travis testing matrix. Unfortunately, the builds are running in quite restricted LXD containers there, for example it is not possible to create huge files there (even if they are just sparse), and certain system calls are blocked. So we have to change some tests first to stop them failing in such environments. v2: - Added "make check-tcg" and Alex' patch to disable cross-containers - Explicitely set "dist: xenial" for arm64 and ppc64le since some iotests are crashing on bionic on these hosts. - Dropped "libcap-dev" from the package list since it will be replaced by libcapng-dev soon. Alex Bennée (1): configure: allow disable of cross compilation containers Thomas Huth (6): iotests: Provide a function for checking the creation of huge files iotests: Skip test 060 if it is not possible to create large files iotests: Skip test 079 if it is not possible to create large files tests/hd-geo-test: Skip test when images can not be created tests/test-util-filemonitor: Skip test on non-x86 Travis containers travis.yml: Enable builds on arm64, ppc64le and s390x .travis.yml | 86 +++ configure | 8 +++- tests/hd-geo-test.c | 12 - tests/qemu-iotests/005| 5 +- tests/qemu-iotests/060| 3 ++ tests/qemu-iotests/079| 3 ++ tests/qemu-iotests/220| 6 +-- tests/qemu-iotests/common.rc | 10 tests/tcg/configure.sh| 6 ++- tests/test-util-filemonitor.c | 11 + 10 files changed, 138 insertions(+), 12 deletions(-) -- 2.18.1
[PATCH v2 3/7] iotests: Skip test 079 if it is not possible to create large files
Test 079 fails in the arm64, s390x and ppc64le LXD containers on Travis (which we will hopefully enable in our CI soon). These containers apparently do not allow large files to be created. Test 079 tries to create a 4G sparse file, which is apparently already too big for these containers, so check first whether we can really create such files before executing the test. Signed-off-by: Thomas Huth --- tests/qemu-iotests/079 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/qemu-iotests/079 b/tests/qemu-iotests/079 index 81f0c21f53..78536d3bbf 100755 --- a/tests/qemu-iotests/079 +++ b/tests/qemu-iotests/079 @@ -39,6 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow2 _supported_proto file nfs +# Some containers (e.g. non-x86 on Travis) do not allow large files +_require_large_file 4G + echo "=== Check option preallocation and cluster_size ===" echo cluster_sizes="16384 32768 65536 131072 262144 524288 1048576 2097152 4194304" -- 2.18.1
[PATCH v2 4/7] tests/hd-geo-test: Skip test when images can not be created
In certain environments like restricted containers, we can not create
huge test images. To be able to use "make check" in such container
environments, too, let's skip the hd-geo-test instead of failing when
the test images could not be created.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Alex Bennée
Signed-off-by: Thomas Huth
---
tests/hd-geo-test.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
index 7e86c5416c..a249800544 100644
--- a/tests/hd-geo-test.c
+++ b/tests/hd-geo-test.c
@@ -34,8 +34,13 @@ static char *create_test_img(int secs)
fd = mkstemp(template);
g_assert(fd >= 0);
ret = ftruncate(fd, (off_t)secs * 512);
-g_assert(ret == 0);
close(fd);
+
+if (ret) {
+free(template);
+template = NULL;
+}
+
return template;
}
@@ -934,6 +939,10 @@ int main(int argc, char **argv)
for (i = 0; i < backend_last; i++) {
if (img_secs[i] >= 0) {
img_file_name[i] = create_test_img(img_secs[i]);
+if (!img_file_name[i]) {
+g_test_message("Could not create test images.");
+goto test_add_done;
+}
} else {
img_file_name[i] = NULL;
}
@@ -965,6 +974,7 @@ int main(int argc, char **argv)
"skipping hd-geo/override/* tests");
}
+test_add_done:
ret = g_test_run();
for (i = 0; i < backend_last; i++) {
--
2.18.1
[PATCH v2 6/7] configure: allow disable of cross compilation containers
From: Alex Bennée
Our docker infrastructure isn't quite as multiarch as we would wish so
let's allow the user to disable it if they want. This will allow us to
use still run check-tcg on non-x86 CI setups.
Signed-off-by: Alex Bennée
Reviewed-by: Stefan Weil
Signed-off-by: Thomas Huth
---
configure | 8 +++-
tests/tcg/configure.sh | 6 --
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/configure b/configure
index 6099be1d84..fe6d0971f1 100755
--- a/configure
+++ b/configure
@@ -302,6 +302,7 @@ audio_win_int=""
libs_qga=""
debug_info="yes"
stack_protector=""
+use_containers="yes"
if test -e "$source_path/.git"
then
@@ -1539,6 +1540,10 @@ for opt do
;;
--disable-plugins) plugins="no"
;;
+ --enable-containers) use_containers="yes"
+ ;;
+ --disable-containers) use_containers="no"
+ ;;
*)
echo "ERROR: unknown option $opt"
echo "Try '$0 --help' for more information"
@@ -1722,6 +1727,7 @@ Advanced options (experts only):
track the maximum stack usage of stacks created by
qemu_alloc_stack
--enable-plugins
enable plugins via shared library loading
+ --disable-containers don't use containers for cross-building
Optional features, enabled with --enable-FEATURE and
disabled with --disable-FEATURE, default is enabled if available:
@@ -8039,7 +8045,7 @@ done
(for i in $cross_cc_vars; do
export $i
done
-export target_list source_path
+export target_list source_path use_containers
$source_path/tests/tcg/configure.sh)
# temporary config to build submodules
diff --git a/tests/tcg/configure.sh b/tests/tcg/configure.sh
index 6c4a471aea..210e68396f 100755
--- a/tests/tcg/configure.sh
+++ b/tests/tcg/configure.sh
@@ -36,8 +36,10 @@ TMPC="${TMPDIR1}/qemu-conf.c"
TMPE="${TMPDIR1}/qemu-conf.exe"
container="no"
-if has "docker" || has "podman"; then
- container=$($python $source_path/tests/docker/docker.py probe)
+if test $use_containers = "yes"; then
+if has "docker" || has "podman"; then
+container=$($python $source_path/tests/docker/docker.py probe)
+fi
fi
# cross compilers defaults, can be overridden with --cross-cc-ARCH
--
2.18.1
[PATCH v2 5/7] tests/test-util-filemonitor: Skip test on non-x86 Travis containers
test-util-filemonitor fails in restricted non-x86 Travis containers
since they apparently blacklisted some required system calls there.
Let's simply skip the test if we detect such an environment.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Alex Bennée
Signed-off-by: Thomas Huth
---
tests/test-util-filemonitor.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
index 301cd2db61..45009c69f4 100644
--- a/tests/test-util-filemonitor.c
+++ b/tests/test-util-filemonitor.c
@@ -406,10 +406,21 @@ test_file_monitor_events(void)
char *pathdst = NULL;
QFileMonitorTestData data;
GHashTable *ids = g_hash_table_new(g_int64_hash, g_int64_equal);
+char *travis_arch;
qemu_mutex_init(&data.lock);
data.records = NULL;
+/*
+ * This test does not work on Travis LXD containers since some
+ * syscalls are blocked in that environment.
+ */
+travis_arch = getenv("TRAVIS_ARCH");
+if (travis_arch && !g_str_equal(travis_arch, "x86_64")) {
+g_test_skip("Test does not work on non-x86 Travis containers.");
+return;
+}
+
/*
* The file monitor needs the main loop running in
* order to receive events from inotify. We must
--
2.18.1
Re: [PATCH v2 3/7] iotests: Skip test 079 if it is not possible to create large files
On 04/12/2019 16.46, Thomas Huth wrote: > Test 079 fails in the arm64, s390x and ppc64le LXD containers on Travis For the record: It's working on s390x as noticed by Cleber. It's only failing on arm64 and ppc64le. After fixing the problem with 060 which fails on all three architectures, I did not pay enough attention when I wrote this commit message here and thought that this would be the very same problem, but apparently it is working on s390x. Thus "s390x" should be removed from the commit message. Thomas
Re: [PATCH] tests: skip block layer tests if !CONFIG_TOOLS
On 11/12/2019 15.24, Paolo Bonzini wrote: > From: Marc-André Lureau > > The block tests, as well as ahci-test needs qemu-img. Do not run > them if it wasn't built. > > Signed-off-by: Marc-André Lureau > Signed-off-by: Paolo Bonzini > --- > tests/Makefile.include | 4 > 1 file changed, 4 insertions(+) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 8566f5f..f07c761 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -169,7 +169,9 @@ check-qtest-pci-$(CONFIG_IVSHMEM_DEVICE) += > tests/ivshmem-test$(EXESUF) > check-qtest-i386-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF) > check-qtest-i386-y += tests/fdc-test$(EXESUF) > check-qtest-i386-y += tests/ide-test$(EXESUF) > +ifeq ($(CONFIG_TOOLS),y) > check-qtest-i386-y += tests/ahci-test$(EXESUF) > +endif Please use "check-qtest-i386-$(CONFIG_TOOLS) += ..." instead. Thomas
Re: [PATCH] virtio-blk: deprecate SCSI passthrough
On 13/12/2019 15.46, Stefan Hajnoczi wrote: > The Linux virtio_blk.ko guest driver is removing legacy SCSI passthrough > support. Deprecate this feature in QEMU too. > > Signed-off-by: Stefan Hajnoczi > --- > qemu-deprecated.texi | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi > index 4b4b7425ac..ef94d497da 100644 > --- a/qemu-deprecated.texi > +++ b/qemu-deprecated.texi > @@ -285,6 +285,17 @@ spec you can use the ``-cpu rv64gcsu,priv_spec=v1.9.1`` > command line argument. > > @section Device options > > +@subsection Emulated device options > + > +@subsubsection -device virtio-blk,scsi=on|off (since 5.0.0) > + > +The virtio-blk SCSI passthrough feature is a legacy VIRTIO feature. VIRTIO > 1.0 > +and later do not support it because the virtio-scsi device was introduced for > +full SCSI support. Use virtio-scsi instead when SCSI passthrough is > required. > + > +Note this also applies to ``-device virtio-blk-pci,scsi=on|off'', which is an > +alias. ... and "-device virtio-blk-ccw,scsi=on|off". With that added: Reviewed-by: Thomas Huth
[PATCH v2] iotests: Add more "skip_if_unsupported" statements to the python tests
The python code already contains a possibility to skip tests if the
corresponding driver is not available in the qemu binary - use it
in more spots to avoid that the tests are failing if the driver has
been disabled.
Signed-off-by: Thomas Huth
---
v2: Thanks to Max' "iotests: Allow skipping test cases" patch (see
commit 6be012252018249d3a), this patch has been greatly simplified
by only marking the setUp functions instead of all functions from
a class.
tests/qemu-iotests/030 | 1 +
tests/qemu-iotests/040 | 2 ++
tests/qemu-iotests/041 | 1 +
tests/qemu-iotests/245 | 2 ++
4 files changed, 6 insertions(+)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index f3766f2a81..a585554c61 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -530,6 +530,7 @@ class TestQuorum(iotests.QMPTestCase):
children = []
backing = []
+@iotests.skip_if_unsupported(['quorum'])
def setUp(self):
opts = ['driver=quorum', 'vote-threshold=2']
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 762ad1ebcb..74f62c3c4a 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -106,6 +106,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0
524288', backing_img).find("verification failed"))
self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288
524288', backing_img).find("verification failed"))
+@iotests.skip_if_unsupported(['throttle'])
def test_commit_with_filter_and_quit(self):
result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
self.assert_qmp(result, 'return', {})
@@ -125,6 +126,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.has_quit = True
# Same as above, but this time we add the filter after starting the job
+@iotests.skip_if_unsupported(['throttle'])
def test_commit_plus_filter_and_quit(self):
result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
self.assert_qmp(result, 'return', {})
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 8568426311..ef95fba656 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -871,6 +871,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
image_len = 1 * 1024 * 1024 # MB
IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
+@iotests.skip_if_unsupported(['quorum'])
def setUp(self):
self.vm = iotests.VM()
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index e66a23c5f0..36d7ca6ded 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -478,6 +478,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# This test verifies that we can't change the children of a block
# device during a reopen operation in a way that would create
# cycles in the node graph
+@iotests.skip_if_unsupported(['blkverify'])
def test_graph_cycles(self):
opts = []
@@ -534,6 +535,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.assert_qmp(result, 'return', {})
# Misc reopen tests with different block drivers
+@iotests.skip_if_unsupported(['quorum'])
def test_misc_drivers(self):
## quorum ##
--
2.18.1
[PATCH] iotests: Add more "_require_drivers" checks to the shell-based tests
Test 051 should be skipped if nbd is not available, and 267 should
be skipped if copy-on-read is not enabled.
Signed-off-by: Thomas Huth
---
tests/qemu-iotests/051 | 1 +
tests/qemu-iotests/267 | 1 +
2 files changed, 2 insertions(+)
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 53bcdbc911..a13bce2fd0 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -41,6 +41,7 @@ _supported_proto file
# A compat=0.10 image is created in this test which does not support anything
# other than refcount_bits=16
_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+_require_drivers nbd
do_run_qemu()
{
diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
index 17ac640a83..c1536f45b9 100755
--- a/tests/qemu-iotests/267
+++ b/tests/qemu-iotests/267
@@ -40,6 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow2
_supported_proto file
_supported_os Linux
+_require_drivers copy-on-read
# Internal snapshots are (currently) impossible with refcount_bits=1
_unsupported_imgopts 'refcount_bits=1[^0-9]'
--
2.18.1
Priority of -accel (was: [PATCH] tests/qemu-iotests: Update tests to recent desugarized -accel option)
On 06/01/2020 14.09, Philippe Mathieu-Daudé wrote:
> Commit 6f6e1698a6 desugarized "-machine accel=" to a list
> of "-accel" options. Since now "-machine accel" and "-accel"
> became incompatible, update the iotests to the new format.
>
> Error reported here:
> https://gitlab.com/qemu-project/qemu/-/jobs/385801004#L3400
>
> Reported-by: GitLab CI
> Fixes: 6f6e1698a6 (vl: configure accelerators from -accel options)
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/qemu-iotests/235 | 2 +-
> tests/qemu-iotests/check | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> index fedd111fd4..3d7533980d 100755
> --- a/tests/qemu-iotests/235
> +++ b/tests/qemu-iotests/235
> @@ -49,7 +49,7 @@ qemu_img_create('-f', iotests.imgfmt, '-o',
> 'preallocation=metadata', disk,
> str(size))
>
> vm = QEMUMachine(iotests.qemu_prog)
> -vm.add_args('-machine', 'accel=kvm:tcg')
> +vm.add_args('-accel', 'kvm', '-accel', 'tcg')
Looking at this, I wonder whether we really want the "-accel" option to
prioritize the accelerators in the order of appearance? A lot of other
CLI tools give the highest priority to the last parameter instead, e.g.
"gcc -O3 -O1" compiles with -O1, and not with -O3.
Also I think it might be quite common that there are shell scripts which
call "qemu-system-xxx -accel xyz $*" ... and if we don't invert the
priorities of -accel, it will be impossible to override -accel in that
case...
Thomas
Re: Priority of -accel (was: [PATCH] tests/qemu-iotests: Update tests to recent desugarized -accel option)
On 07/01/2020 11.14, Paolo Bonzini wrote:
> On 07/01/20 11:03, Thomas Huth wrote:
>>>
>>> vm = QEMUMachine(iotests.qemu_prog)
>>> -vm.add_args('-machine', 'accel=kvm:tcg')
>>> +vm.add_args('-accel', 'kvm', '-accel', 'tcg')
>> Looking at this, I wonder whether we really want the "-accel" option to
>> prioritize the accelerators in the order of appearance? A lot of other
>> CLI tools give the highest priority to the last parameter instead, e.g.
>> "gcc -O3 -O1" compiles with -O1, and not with -O3.
>>
>> Also I think it might be quite common that there are shell scripts which
>> call "qemu-system-xxx -accel xyz $*" ... and if we don't invert the
>> priorities of -accel, it will be impossible to override -accel in that
>> case...
>
> Hmm, it does match "-machine accel=kvm:tcg" and in general I think it's
> more self-explanatory. However, it is indeed less friendly to scripts.
> On one hand those could be changed to place "-accel xyz" after $* (or
> better "$@"), on the other hand we could also add a priority option to
> "-accel". What do you think?
I don't think we need a separate priority parameter here. But IMHO it's
really rather common practice to prioritize the last option. So while
it might be more "self-explanatory" to a CLI newbie if the first
occurrence got the highest priority, it might be rather confusing
instead for a CLI veteran...?
What do others on the list here think about this?
Thomas
Re: [PATCH] iotests: fix usage -machine accel= together with -accel option
On 23/12/2019 09.39, Paolo Bonzini wrote:
> On 23/12/19 08:43, Vladimir Sementsov-Ogievskiy wrote:
>> diff --git a/vl.c b/vl.c
>> index 86474a55c9..9fb859969c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2779,7 +2779,7 @@ static void configure_accelerators(const char
>> *progname)
>> for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
>> /*
>> * Filter invalid accelerators here, to prevent obscenities
>> - * such as "-machine accel=tcg,,thread=single".
>> + * such as "-machine accel=tcg,thread=single".
>
> The double comma is intentional. Without the "if" below, the comma
> would be escaped and parsed as "-accel tcg,thread=single".
>
>> */
>> if (accel_find(*tmp)) {
>> qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp,
>> true);
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index 90970b0549..2890785a10 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -587,13 +587,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
>>
>> case "$QEMU_PROG" in
>> *qemu-system-arm|*qemu-system-aarch64)
>> -export QEMU_OPTIONS="-nodefaults -display none -machine
>> virt,accel=qtest"
>> +export QEMU_OPTIONS="-nodefaults -display none -machine virt -accel
>> qtest"
>> ;;
>> *qemu-system-tricore)
>> -export QEMU_OPTIONS="-nodefaults -display none -machine
>> tricore_testboard,accel=qtest"
>> +export QEMU_OPTIONS="-nodefaults -display none -machine
>> tricore_testboard -accel qtest"
>> ;;
>> *)
>> -export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
>> +export QEMU_OPTIONS="-nodefaults -display none -accel qtest"
>> ;;
>> esac
>>
>>
>
> This part is good, but what is the reproducer?
Make the iotests run with either qemu-system-arm, qemu-system-aarch64 or
qemu-system-tricore, e.g.:
QEMU_PROG=aarch64-softmmu/qemu-system-aarch64 make check-block
HTH,
Thomas
Re: Priority of -accel
On 07/01/2020 15.27, Daniel P. Berrangé wrote: > On Tue, Jan 07, 2020 at 03:20:40PM +0100, Philippe Mathieu-Daudé wrote: >> On 1/7/20 3:14 PM, Thomas Huth wrote: >>> On 07/01/2020 13.54, Daniel P. Berrangé wrote: >>>> On Tue, Jan 07, 2020 at 01:23:18PM +0100, Paolo Bonzini wrote: >>>>> On 07/01/20 13:18, Thomas Huth wrote: >>>>>> I don't think we need a separate priority parameter here. But IMHO it's >>>>>> really rather common practice to prioritize the last option. So while >>>>>> it might be more "self-explanatory" to a CLI newbie if the first >>>>>> occurrence got the highest priority, it might be rather confusing >>>>>> instead for a CLI veteran...? >>>>> >>>>> Prioritising the last certainly makes sense for a choose-one-only >>>>> option, but I'm not sure it's the same for a choose-best option. After >>>>> all it was -machine accel=kvm:tcg, not -machine accel=tcg:kvm... >>>> >>>> IIUC, the main use case for specifying multiple accelerators is >>>> so that lazy invokations can ask for a hardware virt, but then get >>>> fallback to TCG if not available. For things that should be platform >>>> portabile, there's more than just kvm to consider though, as we have >>>> many accelerators. Listing all possible accelerators is kind of >>>> crazy though no matter what the syntax is. >>>> >>>> How about taking a completely different approach, inspired by the >>>> -cpu arg and implement: >>>> >>>> -machine accel=best >>> >>> Something like that sounds like the best solution to me, but I'd maybe >>> rather not call it "best", since the definition of "best" might depend >>> on your use-case (e.g. do you want to use a CPU close to the host or >>> something different which might be better emulated by TCG?). >>> >>> What about "-accel any" or "-accel fastest" or something similar? >> >> 'any' is a russian roulette, you don't want it to return 'qtest' ;) > > We could make it return "qtest" only on April 1st ;-P ... or we finally dare to let QEMU chose the "best" accelerator by default if no "-accel" option has been specified... Thus if no "-accel" has been specified, then use kvm/haxm/whpv if possible or TCG otherwise. And if "-accel" has been specified, then only use that accelerator and don't allow a second "-accel" to be specified. Thomas
Re: Priority of -accel (was: [PATCH] tests/qemu-iotests: Update tests to recent desugarized -accel option)
On 07/01/2020 15.37, Paolo Bonzini wrote: > On 07/01/20 14:55, Christophe de Dinechin wrote: >> So what about ranking the accelerators, so that all combinaisons >> -accel=kvm:tcg, -accel=tcg:kvm, -accel kvm -accel tcg, etc would > > (I assume you mean "-machine accel=kvm:tcg" and "-machine accel=tcg:kvm" > for the first two. This is the "older" way which has now become sugar > for "-accel kvm -accel tcg"). > >> all pickup kvm if available, and tcg as a fallback? Implementation-wise, >> it would simply mean ranking the accelerators and updating the accelerator >> only if it’s available and better. > > This is an interesting idea. I like this better than "-accel best", > because "-accel best" has the problem that you can't add suboptions to > it (the suboptions for the various accelerators are disjoint). > > It would break backwards compatibility for "-machine accel=tcg:kvm", > which so far meant "use TCG if compiled in, otherwise use KVM". This is > not something I would have a problem with... except that "tcg:kvm" is > the default if no -accel option is provided! Note that we need "-M accel=tcg:kvm" (or "-accel tcg -accel kvm" now) in tests/boot-serial-test.c for example, since some machines can't use KVM on certain hosts (e.g. with KVM-HV on POWER8/9). Thomas
Re: Priority of -accel (was: [PATCH] tests/qemu-iotests: Update tests to recent desugarized -accel option)
On 07/01/2020 13.54, Daniel P. Berrangé wrote: > On Tue, Jan 07, 2020 at 01:23:18PM +0100, Paolo Bonzini wrote: >> On 07/01/20 13:18, Thomas Huth wrote: >>> I don't think we need a separate priority parameter here. But IMHO it's >>> really rather common practice to prioritize the last option. So while >>> it might be more "self-explanatory" to a CLI newbie if the first >>> occurrence got the highest priority, it might be rather confusing >>> instead for a CLI veteran...? >> >> Prioritising the last certainly makes sense for a choose-one-only >> option, but I'm not sure it's the same for a choose-best option. After >> all it was -machine accel=kvm:tcg, not -machine accel=tcg:kvm... > > IIUC, the main use case for specifying multiple accelerators is > so that lazy invokations can ask for a hardware virt, but then get > fallback to TCG if not available. For things that should be platform > portabile, there's more than just kvm to consider though, as we have > many accelerators. Listing all possible accelerators is kind of > crazy though no matter what the syntax is. > > How about taking a completely different approach, inspired by the > -cpu arg and implement: > > -machine accel=best Something like that sounds like the best solution to me, but I'd maybe rather not call it "best", since the definition of "best" might depend on your use-case (e.g. do you want to use a CPU close to the host or something different which might be better emulated by TCG?). What about "-accel any" or "-accel fastest" or something similar? Thomas
Re: Priority of -accel (was: [PATCH] tests/qemu-iotests: Update tests to recent desugarized -accel option)
On 08/01/2020 11.39, Alex Bennée wrote: > > Thomas Huth writes: > >> On 07/01/2020 13.54, Daniel P. Berrangé wrote: >>> On Tue, Jan 07, 2020 at 01:23:18PM +0100, Paolo Bonzini wrote: >>>> On 07/01/20 13:18, Thomas Huth wrote: >>>>> I don't think we need a separate priority parameter here. But IMHO it's >>>>> really rather common practice to prioritize the last option. So while >>>>> it might be more "self-explanatory" to a CLI newbie if the first >>>>> occurrence got the highest priority, it might be rather confusing >>>>> instead for a CLI veteran...? >>>> >>>> Prioritising the last certainly makes sense for a choose-one-only >>>> option, but I'm not sure it's the same for a choose-best option. After >>>> all it was -machine accel=kvm:tcg, not -machine accel=tcg:kvm... >>> >>> IIUC, the main use case for specifying multiple accelerators is >>> so that lazy invokations can ask for a hardware virt, but then get >>> fallback to TCG if not available. For things that should be platform >>> portabile, there's more than just kvm to consider though, as we have >>> many accelerators. Listing all possible accelerators is kind of >>> crazy though no matter what the syntax is. >>> >>> How about taking a completely different approach, inspired by the >>> -cpu arg and implement: >>> >>> -machine accel=best >> >> Something like that sounds like the best solution to me, but I'd maybe >> rather not call it "best", since the definition of "best" might depend >> on your use-case (e.g. do you want to use a CPU close to the host or >> something different which might be better emulated by TCG?). > > Indeed - you may well want to do TCG on Aarch64 if you want to test new > instructions. > >> >> What about "-accel any" or "-accel fastest" or something similar? > > "any" is just ambiguous, "fastest" is just begging for me to find a > micro-benchmark that TCG outperforms on ;-) > > "-accel default" could be considered to have vibes of Do The Right > Thing (tm) and could in time actually become so! "-accel default" sounds like the default behavior that you'd also get if you don't use this option at all ... what about "-accel auto" to say that QEMU should pick an accelerator automatically? Thomas
Re: Priority of -accel
On 08/01/2020 14.24, Paolo Bonzini wrote: > On 08/01/20 14:10, Daniel P. Berrangé wrote: >> On Wed, Jan 08, 2020 at 01:41:59PM +0100, Paolo Bonzini wrote: >>> On 08/01/20 11:58, Thomas Huth wrote: >>>>> "-accel default" could be considered to have vibes of Do The Right >>>>> Thing (tm) and could in time actually become so! >>>> >>>> "-accel default" sounds like the default behavior that you'd also get if >>>> you don't use this option at all ... what about "-accel auto" to say >>>> that QEMU should pick an accelerator automatically? >>> >>> Questions to answer before thinking about the name: how would it >>> co-operate with other "-accel" options? how would you pass sub-options >>> to the accelerators? >> >> If people don't have a preference for a specific accelerator, just need >> "a working accelerator", then I think it is reasonable to assume they >> won't want/need to pass options to the accelerators either. >> >> "-accel default" is targetting the simple "do the right thing" use >> case, so IMHO doesn't need to support per-accelerator options. > > So basically the idea is to add an option that means "ignore every other > -accel option and act as if we had "-accel kvm -accel tcg"? That seems > like a hack to me, especially since you can achieve the same effect with > a binary named qemu-kvm and no -accel options at all. But we could disallow multiple "-accel" options in that case (or just always use the last in the list), so we don't have to deal with priorities of the options here at all... Well, not sure whether that's really really better than what we currently have, so maybe we should just keep it in the current shape... Thomas
Re: Pre-Christmas meeting notes
On 09/01/2020 16.03, Vladimir Sementsov-Ogievskiy wrote: > 06.01.2020 20:15, Max Reitz wrote: >> Misc >> >> >> The Wiki’s TODO list is horribly outdated. What should we do about >> it? Maybe archive it and start a new one? (Most of the things on the >> current list are either done or we don’t want to do anymore.) > > > May be, create block/TODO.txt instead? It would be simpler to manage file > in git than wiki page. Is it? We removed a bunch of stale TODO files in the past already: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=d494d79eabfdac0 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9e564a1dde5abc7 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ba43da36983a0bf And there are some more TODO files in the target/ subfolders which look also quite stale... so I somewhat doubt that a TODO list in git is much better than a TODO list in the wiki. Maybe we just need a proper bug/feature tracker instead (since Launchpad is IMHO quite a bad choice for bug tracking, too)... Thomas
Re: [PATCH v2] iotests: Add more "skip_if_unsupported" statements to the python tests
On 19/12/2019 13.19, Kevin Wolf wrote: > Am 18.12.2019 um 15:43 hat Thomas Huth geschrieben: >> The python code already contains a possibility to skip tests if the >> corresponding driver is not available in the qemu binary - use it >> in more spots to avoid that the tests are failing if the driver has >> been disabled. >> >> Signed-off-by: Thomas Huth >> --- >> v2: Thanks to Max' "iotests: Allow skipping test cases" patch (see >> commit 6be012252018249d3a), this patch has been greatly simplified >> by only marking the setUp functions instead of all functions from >> a class. > > Ah, nice. I didn't know this worked on setup() functions. > >> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 >> index f3766f2a81..a585554c61 100755 >> --- a/tests/qemu-iotests/030 >> +++ b/tests/qemu-iotests/030 >> @@ -530,6 +530,7 @@ class TestQuorum(iotests.QMPTestCase): >> children = [] >> backing = [] >> >> +@iotests.skip_if_unsupported(['quorum']) >> def setUp(self): >> opts = ['driver=quorum', 'vote-threshold=2'] > > test_stream_quorum(), which is the only test case in this class, already > contains a check: > > if not iotests.supports_quorum(): > return > > We should probably remove this check because it's dead code now. Interesting - but apparently, the check did not work right, since the test aborts if I run it with a QEMU binary that does not have quorum enabled. Anyway, I'll respin the patch with the old check removed. Thanks, Thomas
[PATCH v3] iotests: Add more "skip_if_unsupported" statements to the python tests
The python code already contains a possibility to skip tests if the
corresponding driver is not available in the qemu binary - use it
in more spots to avoid that the tests are failing if the driver has
been disabled.
While we're at it, we can now also remove some of the old checks that
were using iotests.supports_quorum() - and which were apparently not
working as expected since the tests aborted instead of being skipped
when "quorum" was missing in the QEMU binary.
Signed-off-by: Thomas Huth
---
v3:
- Remove the old iotests.supports_quorum()-based tests
- Check for "throttle" in 245, too
tests/qemu-iotests/030 | 4 +---
tests/qemu-iotests/040 | 2 ++
tests/qemu-iotests/041 | 39 +++
tests/qemu-iotests/245 | 2 ++
4 files changed, 8 insertions(+), 39 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index be35bde06f..0990681c1e 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -530,6 +530,7 @@ class TestQuorum(iotests.QMPTestCase):
children = []
backing = []
+@iotests.skip_if_unsupported(['quorum'])
def setUp(self):
opts = ['driver=quorum', 'vote-threshold=2']
@@ -560,9 +561,6 @@ class TestQuorum(iotests.QMPTestCase):
os.remove(img)
def test_stream_quorum(self):
-if not iotests.supports_quorum():
-return
-
self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map',
self.children[0]),
qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map',
self.backing[0]),
'image file map matches backing file before
streaming')
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 762ad1ebcb..74f62c3c4a 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -106,6 +106,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0
524288', backing_img).find("verification failed"))
self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288
524288', backing_img).find("verification failed"))
+@iotests.skip_if_unsupported(['throttle'])
def test_commit_with_filter_and_quit(self):
result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
self.assert_qmp(result, 'return', {})
@@ -125,6 +126,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.has_quit = True
# Same as above, but this time we add the filter after starting the job
+@iotests.skip_if_unsupported(['throttle'])
def test_commit_plus_filter_and_quit(self):
result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
self.assert_qmp(result, 'return', {})
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index d7be30b62b..c07437fda1 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -871,6 +871,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
image_len = 1 * 1024 * 1024 # MB
IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
+@iotests.skip_if_unsupported(['quorum'])
def setUp(self):
self.vm = iotests.VM()
@@ -891,9 +892,8 @@ class TestRepairQuorum(iotests.QMPTestCase):
#assemble the quorum block device from the individual files
args = { "driver": "quorum", "node-name": "quorum0",
"vote-threshold": 2, "children": [ "img0", "img1", "img2" ] }
-if iotests.supports_quorum():
-result = self.vm.qmp("blockdev-add", **args)
-self.assert_qmp(result, 'return', {})
+result = self.vm.qmp("blockdev-add", **args)
+self.assert_qmp(result, 'return', {})
def tearDown(self):
@@ -906,9 +906,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
pass
def test_complete(self):
-if not iotests.supports_quorum():
-return
-
self.assert_no_active_block_jobs()
result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
@@ -925,9 +922,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
'target image does not match source after mirroring')
def test_cancel(self):
-if not iotests.supports_quorum():
-return
-
self.assert_no_active_block_jobs()
result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
@@ -9
Re: [PATCH v4 3/6] iotests: Test 183 does not work on macOS and OpenBSD
On 20/01/2020 15.36, Max Reitz wrote: > On 02.12.19 11:10, Thomas Huth wrote: >> In the long term, we might want to add test 183 to the "auto" group >> (but it still fails occasionally, so we cannot do that yet). However, >> when running 183 in Cirrus-CI on macOS, or with our vm-build-openbsd >> target, it currently always fails with an "Timeout waiting for return >> on handle 0" error. >> >> Let's mark it as supported only on systems where the test is working >> fine (i.e. Linux, FreeBSD and NetBSD). >> >> Signed-off-by: Thomas Huth >> --- >> tests/qemu-iotests/183 | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183 >> index bced83fae0..0bbae13647 100755 >> --- a/tests/qemu-iotests/183 >> +++ b/tests/qemu-iotests/183 >> @@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 >> . ./common.filter >> . ./common.qemu >> >> +_supported_os Linux FreeBSD NetBSD > > I don’t suppose you have data on OpenBSD? I've checked it with our "make vm-build-openbsd" target (see patch description) and it was failing there in the same way as on macOS. > Reviewed-by: Max Reitz Thanks! Thomas
Re: [PATCH v4 5/6] iotests: Skip Python-based tests if QEMU does not support virtio-blk
On 20/01/2020 15.50, Max Reitz wrote: > On 02.12.19 11:10, Thomas Huth wrote: >> We are going to enable some of the python-based tests in the "auto" group, >> and these tests require virtio-blk to work properly. Running iotests >> without virtio-blk likely does not make too much sense anyway, so instead >> of adding a check for the availability of virtio-blk to each and every >> test (which does not sound very appealing), let's rather add a check for >> this a central spot in the "check" script instead (so that it is still >> possible to run "make check" for qemu-system-tricore for example). >> >> Signed-off-by: Thomas Huth >> --- >> tests/qemu-iotests/check | 8 ++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check >> index 90970b0549..bce3035d5a 100755 >> --- a/tests/qemu-iotests/check >> +++ b/tests/qemu-iotests/check >> @@ -642,7 +642,11 @@ fi >> python_usable=false >> if $PYTHON -c 'import sys; sys.exit(0 if sys.version_info >= (3,6) else 1)' >> then >> -python_usable=true >> +# Our python framework also requires virtio-blk >> +if "$QEMU_PROG" -M none -device help | grep -q virtio-blk >/dev/null >> 2>&1 >> +then >> +python_usable=true >> +fi >> fi >> >> default_machine=$($QEMU_PROG -machine help | sed -n '/(default)/ s/ .*//p') >> @@ -830,7 +834,7 @@ do >> run_command="$PYTHON $seq" >> else >> run_command="false" >> -echo "Unsupported Python version" > $seq.notrun >> +echo "Unsupported Python version or missing virtio-blk" > >> $seq.notrun > > A $python_unusable_because might be helpful (set in to-be-added else > branches for the ifs that set python_usable to true), but either way: Sounds like a good idea, I'll give it a try and send a v5. Thanks, Thomas
[PATCH v5 0/6] Enable more iotests during "make check-block"
As discussed here: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg00697.html and here: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01388.html it would be good to have some more valuable iotests enabled in the "auto" group to get better iotest coverage during "make check". Since these Python-based tests require a QEMU that features a 'virtio-blk' device, we can only run the Python tests if this device is available. With binaries like qemu-system-tricore, the Python-based tests will be skipped. v5: - Added $python_unusable_because text in the fifth patch - Rebased to master and checked that everything still works fine on Travis-CI, Cirrus-CI, Gitlab-CI, OpenBSD and NetBSD. v4: - The check for 'virtio-blk' is now done in the tests/qemu-iotests/check script instead of tests/check-block.sh (to avoid to duplicate the code that searches for the right QEMU binary - and we can also still run the shell-based tests this way). - Added the new patch to check for the availability of virtio devices in the iotests 127 and 267. - The patch that drops test 130 from the "auto" group has already been merged and thus been dropped from this series. v3: - Test 183 fails on Patchew, so I removed it from the "auto" group again v2: - Checked the iotests with NetBSD, too (now that Eduardo has re-activated Gerd's patches for creating NetBSD VM images) - Use 'openbsd' instead of 'openbsd6' - Use 'grep -q' instead of 'grep' for grep'ing silently - Added the patch to disable 130 from the "auto" group John Snow (1): iotests: remove 'linux' from default supported platforms Thomas Huth (5): iotests: Test 041 only works on certain systems iotests: Test 183 does not work on macOS and OpenBSD iotests: Check for the availability of the required devices in 267 and 127 iotests: Skip Python-based tests if QEMU does not support virtio-blk iotests: Enable more tests in the 'auto' group to improve test coverage tests/qemu-iotests/041| 3 ++- tests/qemu-iotests/127| 2 ++ tests/qemu-iotests/183| 1 + tests/qemu-iotests/267| 2 ++ tests/qemu-iotests/check | 12 ++-- tests/qemu-iotests/common.rc | 14 ++ tests/qemu-iotests/group | 14 +++--- tests/qemu-iotests/iotests.py | 16 +++- 8 files changed, 49 insertions(+), 15 deletions(-) -- 2.18.1
[PATCH v5 1/6] iotests: remove 'linux' from default supported platforms
From: John Snow
verify_platform will check an explicit whitelist and blacklist instead.
The default will now be assumed to be allowed to run anywhere.
For tests that do not specify their platforms explicitly, this has the effect of
enabling these tests on non-linux platforms. For tests that always specified
linux explicitly, there is no change.
For Python tests on FreeBSD at least; only seven python tests fail:
045 147 149 169 194 199 211
045 and 149 appear to be misconfigurations,
147 and 194 are the AF_UNIX path too long error,
169 and 199 are bitmap migration bugs, and
211 is a bug that shows up on Linux platforms, too.
This is at least good evidence that these tests are not Linux-only. If
they aren't suitable for other platforms, they should be disabled on a
per-platform basis as appropriate.
Therefore, let's switch these on and deal with the failures.
Reviewed-by: Max Reitz
Signed-off-by: John Snow
---
tests/qemu-iotests/iotests.py | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 13fd8b5cd2..1f6d918d6d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -925,9 +925,14 @@ def verify_protocol(supported=[], unsupported=[]):
if not_sup or (imgproto in unsupported):
notrun('not suitable for this protocol: %s' % imgproto)
-def verify_platform(supported_oses=['linux']):
-if True not in [sys.platform.startswith(x) for x in supported_oses]:
-notrun('not suitable for this OS: %s' % sys.platform)
+def verify_platform(supported=None, unsupported=None):
+if unsupported is not None:
+if any((sys.platform.startswith(x) for x in unsupported)):
+notrun('not suitable for this OS: %s' % sys.platform)
+
+if supported is not None:
+if not any((sys.platform.startswith(x) for x in supported)):
+notrun('not suitable for this OS: %s' % sys.platform)
def verify_cache_mode(supported_cache_modes=[]):
if supported_cache_modes and (cachemode not in supported_cache_modes):
@@ -1018,7 +1023,8 @@ def execute_unittest(output, verbosity, debug):
sys.stderr.write(out)
def execute_test(test_function=None,
- supported_fmts=[], supported_oses=['linux'],
+ supported_fmts=[],
+ supported_platforms=None,
supported_cache_modes=[], unsupported_fmts=[],
supported_protocols=[], unsupported_protocols=[]):
"""Run either unittest or script-style tests."""
@@ -1035,7 +1041,7 @@ def execute_test(test_function=None,
verbosity = 1
verify_image_format(supported_fmts, unsupported_fmts)
verify_protocol(supported_protocols, unsupported_protocols)
-verify_platform(supported_oses)
+verify_platform(supported=supported_platforms)
verify_cache_mode(supported_cache_modes)
if debug:
--
2.18.1
[PATCH v5 3/6] iotests: Test 183 does not work on macOS and OpenBSD
In the long run, we might want to add test 183 to the "auto" group (but it still fails occasionally, so we cannot do that yet). However, when running 183 in Cirrus-CI on macOS, or with our vm-build-openbsd target, it currently always fails with an "Timeout waiting for return on handle 0" error. Let's mark it as supported only on systems where the test is working most of the time (i.e. Linux, FreeBSD and NetBSD). Reviewed-by: Max Reitz Signed-off-by: Thomas Huth --- tests/qemu-iotests/183 | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183 index 3f74b9f62d..a5e0d7e8b7 100755 --- a/tests/qemu-iotests/183 +++ b/tests/qemu-iotests/183 @@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.filter . ./common.qemu +_supported_os Linux FreeBSD NetBSD _supported_fmt qcow2 raw qed quorum _supported_proto file -- 2.18.1
[PATCH v5 2/6] iotests: Test 041 only works on certain systems
041 works fine on Linux, FreeBSD, NetBSD and OpenBSD, but fails on macOS. Let's mark it as only supported on the systems where we know that it is working fine. Reviewed-by: Max Reitz Signed-off-by: Thomas Huth --- tests/qemu-iotests/041 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index c07437fda1..0181f7a9b6 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -1134,4 +1134,5 @@ class TestOrphanedSource(iotests.QMPTestCase): if __name__ == '__main__': iotests.main(supported_fmts=['qcow2', 'qed'], - supported_protocols=['file']) + supported_protocols=['file'], + supported_platforms=['linux', 'freebsd', 'netbsd', 'openbsd']) -- 2.18.1
[PATCH v5 4/6] iotests: Check for the availability of the required devices in 267 and 127
We are going to enable 127 in the "auto" group, but it only works if
virtio-scsi and scsi-hd are available - which is not the case with
QEMU binaries like qemu-system-tricore for example, so we need a
proper check for the availability of these devices here.
A very similar problem exists in iotest 267 - it has been added to
the "auto" group already, but requires virtio-blk and thus currently
fails with qemu-system-tricore for example. Let's also add aproper
check there.
Reviewed-by: Max Reitz
Signed-off-by: Thomas Huth
---
tests/qemu-iotests/127 | 2 ++
tests/qemu-iotests/267 | 2 ++
tests/qemu-iotests/common.rc | 14 ++
3 files changed, 18 insertions(+)
diff --git a/tests/qemu-iotests/127 b/tests/qemu-iotests/127
index b64926ab31..a4fc866038 100755
--- a/tests/qemu-iotests/127
+++ b/tests/qemu-iotests/127
@@ -43,6 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow2
_supported_proto file
+_require_devices virtio-scsi scsi-hd
+
IMG_SIZE=64K
_make_test_img $IMG_SIZE
diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
index c296877168..3146273eef 100755
--- a/tests/qemu-iotests/267
+++ b/tests/qemu-iotests/267
@@ -46,6 +46,8 @@ _require_drivers copy-on-read
# and generally impossible with external data files
_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
+_require_devices virtio-blk
+
do_run_qemu()
{
echo Testing: "$@"
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index d088392ab6..4dfe92e6e3 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -699,5 +699,19 @@ _require_large_file()
rm "$TEST_IMG"
}
+# Check that a set of devices is available in the QEMU binary
+#
+_require_devices()
+{
+available=$($QEMU -M none -device help | \
+grep ^name | sed -e 's/^name "//' -e 's/".*$//')
+for device
+do
+if ! echo "$available" | grep -q "$device" ; then
+_notrun "$device not available"
+fi
+done
+}
+
# make sure this script returns success
true
--
2.18.1
[PATCH v5 6/6] iotests: Enable more tests in the 'auto' group to improve test coverage
According to Kevin, tests 030, 040 and 041 are among the most valuable tests that we have, so we should always run them if possible, even if they take a little bit longer. According to Max, it would be good to have a test for iothreads and migration. 127 and 256 seem to be good candidates for iothreads. For migration, let's enable 181 and 203 (which also tests iothreads). (091 would be a good candidate for migration, too, but Alex Bennée reported that this test fails on ZFS file systems, so it can't be included yet) Reviewed-by: Max Reitz Signed-off-by: Thomas Huth --- tests/qemu-iotests/group | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index cb2b789e44..15a005b467 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -51,7 +51,7 @@ 027 rw auto quick 028 rw backing quick 029 rw auto quick -030 rw backing +030 rw auto backing 031 rw auto quick 032 rw auto quick 033 rw auto quick @@ -61,8 +61,8 @@ 037 rw auto backing quick 038 rw auto backing quick 039 rw auto quick -040 rw -041 rw backing +040 rw auto +041 rw auto backing 042 rw auto quick 043 rw auto backing 044 rw @@ -148,7 +148,7 @@ 124 rw backing 125 rw 126 rw auto backing -127 rw backing quick +127 rw auto backing quick 128 rw quick 129 rw quick 130 rw quick @@ -197,7 +197,7 @@ 177 rw auto quick 178 img 179 rw auto quick -181 rw migration +181 rw auto migration 182 rw quick 183 rw migration 184 rw auto quick @@ -218,7 +218,7 @@ 200 rw 201 rw migration 202 rw quick -203 rw migration +203 rw auto migration 204 rw quick 205 rw quick 206 rw @@ -270,7 +270,7 @@ 253 rw quick 254 rw backing quick 255 rw quick -256 rw quick +256 rw auto quick 257 rw 258 rw quick 260 rw quick -- 2.18.1
[PATCH v5 5/6] iotests: Skip Python-based tests if QEMU does not support virtio-blk
We are going to enable some of the python-based tests in the "auto" group, and these tests require virtio-blk to work properly. Running iotests without virtio-blk likely does not make too much sense anyway, so instead of adding a check for the availability of virtio-blk to each and every test (which does not sound very appealing), let's rather add a check for this a central spot in the "check" script instead (so that it is still possible to run "make check" for qemu-system-tricore for example). Signed-off-by: Thomas Huth --- tests/qemu-iotests/check | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index 2890785a10..1629b6c914 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -642,7 +642,15 @@ fi python_usable=false if $PYTHON -c 'import sys; sys.exit(0 if sys.version_info >= (3,6) else 1)' then -python_usable=true +# Our python framework also requires virtio-blk +if "$QEMU_PROG" -M none -device help | grep -q virtio-blk >/dev/null 2>&1 +then +python_usable=true +else +python_unusable_because="Missing virtio-blk in QEMU binary" +fi +else +python_unusable_because="Unsupported Python version" fi default_machine=$($QEMU_PROG -machine help | sed -n '/(default)/ s/ .*//p') @@ -830,7 +838,7 @@ do run_command="$PYTHON $seq" else run_command="false" -echo "Unsupported Python version" > $seq.notrun +echo "$python_unusable_because" > $seq.notrun fi else run_command="./$seq" -- 2.18.1
[PATCH] gitlab-ci: Refresh the list of iotests
iotest 147 and 205 have recently been marked as "NBD-only", so they are currently simply skipped and thus can be removed. iotest 129 occasionally fails in the gitlab-CI, and according to Max, there are some known issues with this test (see for example this URL: https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg00499.html ), so for the time being, let's disable it until the problems are fixed. The iotests 040, 127, 203 and 256 are scheduled to become part of "make check-block", so we also do not have to test them seperately here anymore. On the other side, new iotests have been added to the QEMU repository in the past months, so we can now add some new test > 256 instead. Signed-off-by: Thomas Huth --- .gitlab-ci.yml | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d9a90f795d..52d73c1c72 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -51,12 +51,12 @@ build-tcg-disabled: - make check-qapi-schema - cd tests/qemu-iotests/ - ./check -raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048 -052 063 077 086 101 104 106 113 147 148 150 151 152 157 159 160 -163 170 171 183 184 192 194 197 205 208 215 221 222 226 227 236 - - ./check -qcow2 028 040 051 056 057 058 065 067 068 082 085 091 095 096 102 -122 124 127 129 132 139 142 144 145 147 151 152 155 157 165 194 -196 197 200 202 203 205 208 209 215 216 218 222 227 234 246 247 -248 250 254 255 256 +052 063 077 086 101 104 106 113 148 150 151 152 157 159 160 163 +170 171 183 184 192 194 197 208 215 221 222 226 227 236 253 277 + - ./check -qcow2 028 051 056 057 058 065 067 068 082 085 091 095 096 102 122 +124 132 139 142 144 145 151 152 155 157 165 194 196 197 200 202 +208 209 215 216 218 222 227 234 246 247 248 250 254 255 257 258 +260 261 262 263 264 270 272 273 277 279 build-user: script: -- 2.18.1
[PATCH] docs/devel: Fix qtest paths and info about check-block in testing.rst
The qtests have recently been moved to a separate subdirectory, so the paths that are mentioned in the documentation have to be adjusted accordingly. And some of the iotests are now always run as part of "make check", so this information has to be adjusted here, too. Signed-off-by: Thomas Huth --- docs/devel/testing.rst | 23 --- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index ab5be0c729..770a987ea4 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -16,8 +16,8 @@ The usual way to run these tests is: make check -which includes QAPI schema tests, unit tests, and QTests. Different sub-types -of "make check" tests will be explained below. +which includes QAPI schema tests, unit tests, QTests and some iotests. +Different sub-types of "make check" tests will be explained below. Before running tests, it is best to build QEMU programs first. Some tests expect the executables to exist and will fail with obscure messages if they @@ -79,8 +79,8 @@ QTest cases can be executed with make check-qtest -The QTest library is implemented by ``tests/libqtest.c`` and the API is defined -in ``tests/libqtest.h``. +The QTest library is implemented by ``tests/qtest/libqtest.c`` and the API is +defined in ``tests/qtest/libqtest.h``. Consider adding a new QTest case when you are introducing a new virtual hardware, or extending one if you are adding functionalities to an existing @@ -94,20 +94,20 @@ libqos instead of directly calling into libqtest. Steps to add a new QTest case are: 1. Create a new source file for the test. (More than one file can be added as - necessary.) For example, ``tests/test-foo-device.c``. + necessary.) For example, ``tests/qtest/foo-test.c``. 2. Write the test code with the glib and libqtest/libqos API. See also existing tests and the library headers for reference. -3. Register the new test in ``tests/Makefile.include``. Add the test executable - name to an appropriate ``check-qtest-*-y`` variable. For example: +3. Register the new test in ``tests/qtest/Makefile.include``. Add the test + executable name to an appropriate ``check-qtest-*-y`` variable. For example: - ``check-qtest-generic-y = tests/test-foo-device$(EXESUF)`` + ``check-qtest-generic-y = tests/qtest/foo-test$(EXESUF)`` 4. Add object dependencies of the executable in the Makefile, including the test source file(s) and other interesting objects. For example: - ``tests/test-foo-device$(EXESUF): tests/test-foo-device.o $(libqos-obj-y)`` + ``tests/qtest/foo-test$(EXESUF): tests/qtest/foo-test.o $(libqos-obj-y)`` Debugging a QTest failure is slightly harder than the unit test because the tests look up QEMU program names in the environment variables, such as @@ -152,8 +152,9 @@ parser (either fixing a bug or extending/modifying the syntax). To do this: check-block --- -``make check-block`` is a legacy command to invoke block layer iotests and is -rarely used. See "QEMU iotests" section below for more information. +``make check-block`` runs a subset of the block layer iotests (the tests that +are in the "auto" group in ``tests/qemu-iotests/group``). +See the "QEMU iotests" section below for more information. GCC gcov support -- 2.18.1
Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver
On 25/09/2020 17.11, Vladimir Sementsov-Ogievskiy wrote: > 25.09.2020 12:11, Max Reitz wrote: >> On 25.09.20 10:49, Vladimir Sementsov-Ogievskiy wrote: >>> 25.09.2020 11:26, Max Reitz wrote: On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > tests/qemu-iotests/298 | 186 > + > tests/qemu-iotests/298.out | 5 + > tests/qemu-iotests/group | 1 + > 3 files changed, 192 insertions(+) > create mode 100644 tests/qemu-iotests/298 > create mode 100644 tests/qemu-iotests/298.out >> >> [...] >> > +class TestTruncate(iotests.QMPTestCase): The same decorator could be placed here, although this class doesn’t start a VM, and so is unaffected by the allowlist. Still may be relevant in case of block modules, I don’t know. >>> >>> Or just global test skip at file top >> >> Hm. Like verify_quorum()? Is there a generic function for that already? >> >> [...] >> > + # Probably we'll want preallocate filter to keep align to > cluster when > + # shrink preallocation, so, ignore small differece > + self.assertLess(abs(stat.st_size - refstat.st_size), 64 * > 1024) > + > + # Preallocate filter may leak some internal clusters (for > example, if > + # guest write far over EOF, skipping some clusters - they > will remain > + # fallocated, preallocate filter don't care about such > leaks, it drops > + # only trailing preallocation. True, but that isn’t what’s happening here. (We only write 10M at 0, so there are no gaps.) Why do we need this 1M margin? >>> >>> We write 10M, but qcow2 also writes metadata as it wants >> >> Ah, yes, sure. Shouldn’t result in 1M, but why not. >> > + self.assertLess(abs(stat.st_blocks - refstat.st_blocks) * > 512, > + 1024 * 1024) [...] > diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group > index ff59cfd2d4..15d5f9619b 100644 > --- a/tests/qemu-iotests/group > +++ b/tests/qemu-iotests/group > @@ -307,6 +307,7 @@ > 295 rw > 296 rw > 297 meta > +298 auto quick I wouldn’t mark it as quick, there is at least one preallocate=full of 140M, and one of 40M, plus multiple 10M data writes and falloc preallocations. Also, since you mark it as “auto”, have you run this test on all CI-relevant hosts? (Among other things I can’t predict) I wonder how preallocation behaves on macOS. Just because that one was always a bit weird about not-really-data areas. >>> >>> Ofcourse, I didn't run on all hosts. I'm a bit out of sync about this.. >> >> Well, someone has to do it. The background story is that tests are >> added to auto all the time (because “why not”), and then they fail on >> BSD or macOS. We have BSD docker test build targets at least, so they >> can be easily tested. (Well, it takes like half an hour, but you know.) >> >> (We don’t have macOS builds, as far as I can tell, but I personally >> don’t even know why we run the iotests on macOS at all. (Well, I also >> wonder about the BSDs, but given the test build targets, I shouldn’t >> complain, I suppose.)) >> >> (Though macOS isn’t part of the gating CI, is it? I seem to remember >> macOS errors are generally only reported to me half a week after the >> pull request is merged, which is even worse.) >> >> Anyway. I just ran the test for OpenBSD >> (EXTRA_CONFIGURE_OPTS='--target-list=x86_64-softmmu' \ >> make vm-build-openbsd) > > Oh, I didn't know that it's so simple. Running the tests on macOS is also quite simple if you have a github account. You simply add the "Cirrus-CI" from the marketplace to your forked qemu repository there, and then push your work to a branch in that repo. Cirrus-CI then automatically tests your stuff on macOS (and also FreeBSD), e.g.: https://cirrus-ci.com/build/4961684689256448 Thomas
[PATCH] tests/docker: Add genisoimage to the docker file
genisoimage is needed for running the tests/qtest/cdrom-test qtest. Signed-off-by: Thomas Huth --- tests/docker/dockerfiles/centos8.docker | 1 + tests/docker/dockerfiles/debian-amd64.docker | 1 + tests/docker/dockerfiles/fedora.docker | 1 + tests/docker/dockerfiles/ubuntu2004.docker | 1 + 4 files changed, 4 insertions(+) diff --git a/tests/docker/dockerfiles/centos8.docker b/tests/docker/dockerfiles/centos8.docker index f435616d6a..0fc2697491 100644 --- a/tests/docker/dockerfiles/centos8.docker +++ b/tests/docker/dockerfiles/centos8.docker @@ -8,6 +8,7 @@ ENV PACKAGES \ dbus-daemon \ gcc \ gcc-c++ \ +genisoimage \ gettext \ git \ glib2-devel \ diff --git a/tests/docker/dockerfiles/debian-amd64.docker b/tests/docker/dockerfiles/debian-amd64.docker index d2500dcff1..314c6bae83 100644 --- a/tests/docker/dockerfiles/debian-amd64.docker +++ b/tests/docker/dockerfiles/debian-amd64.docker @@ -14,6 +14,7 @@ RUN apt update && \ RUN apt update && \ DEBIAN_FRONTEND=noninteractive eatmydata \ apt install -y --no-install-recommends \ +genisoimage \ libbz2-dev \ liblzo2-dev \ libgcrypt20-dev \ diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker index ec783418c8..85c975543d 100644 --- a/tests/docker/dockerfiles/fedora.docker +++ b/tests/docker/dockerfiles/fedora.docker @@ -15,6 +15,7 @@ ENV PACKAGES \ findutils \ gcc \ gcc-c++ \ +genisoimage \ gettext \ git \ glib2-devel \ diff --git a/tests/docker/dockerfiles/ubuntu2004.docker b/tests/docker/dockerfiles/ubuntu2004.docker index cafe8443fb..f4b9556b9e 100644 --- a/tests/docker/dockerfiles/ubuntu2004.docker +++ b/tests/docker/dockerfiles/ubuntu2004.docker @@ -3,6 +3,7 @@ ENV PACKAGES flex bison \ ccache \ clang-10\ gcc \ +genisoimage \ gettext \ git \ glusterfs-common \ -- 2.18.2
Re: [PATCH v7 01/13] qtest: rename qtest_qmp_receive to qtest_qmp_receive_dict
On 06/10/2020 14.38, Maxim Levitsky wrote: > In the next patch a new version of qtest_qmp_receive will be > reintroduced that will buffer received qmp events for later > consumption in qtest_qmp_eventwait_ref > > No functional change intended. > > Suggested-by: Paolo Bonzini > Signed-off-by: Maxim Levitsky > --- > tests/qtest/ahci-test.c| 4 ++-- > tests/qtest/device-plug-test.c | 2 +- > tests/qtest/drive_del-test.c | 2 +- > tests/qtest/libqos/libqtest.h | 4 ++-- > tests/qtest/libqtest.c | 16 > tests/qtest/pvpanic-test.c | 2 +- > tests/qtest/qmp-test.c | 18 +- > 7 files changed, 24 insertions(+), 24 deletions(-) Acked-by: Thomas Huth
Re: [PATCH v7 02/13] qtest: Reintroduce qtest_qmp_receive
On 06/10/2020 14.38, Maxim Levitsky wrote: > The new qtest_qmp_receive buffers all the received qmp events, allowing > qtest_qmp_eventwait_ref to return them. > > This is intended to solve the race in regard to ordering of qmp events > vs qmp responses, as soon as the callers start using the new interface. > > In addition to that, define qtest_qmp_event_ref a function which only scans > the buffer that qtest_qmp_receive stores the events to. > > This is intended for callers that are only interested in events that were > received during the last call to the qtest_qmp_receive. > > Suggested-by: Paolo Bonzini > Signed-off-by: Maxim Levitsky > --- > tests/qtest/libqos/libqtest.h | 23 > tests/qtest/libqtest.c| 49 ++- > 2 files changed, 71 insertions(+), 1 deletion(-) > > diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h > index a41135fc92..19429a536d 100644 > --- a/tests/qtest/libqos/libqtest.h > +++ b/tests/qtest/libqos/libqtest.h > @@ -198,6 +198,16 @@ void qtest_qmp_vsend(QTestState *s, const char *fmt, > va_list ap) > */ > QDict *qtest_qmp_receive_dict(QTestState *s); > > +/** > + * qtest_qmp_receive: > + * @s: #QTestState instance to operate on. > + * > + * Reads a QMP message from QEMU and returns the response. > + * Buffers all the events received meanwhile, until a > + * call to qtest_qmp_eventwait > + */ > +QDict *qtest_qmp_receive(QTestState *s); Re-introducing qtest_qmp_receive() with different behavior than before will likely make backports of other later patches a pain, and might also break other patches that use this function but are not merged yet. Could you please use a different name for this function instead? Maye qtest_qmp_receive_buffered() or something like that? Thomas
Re: [PATCH v7 02/13] qtest: Reintroduce qtest_qmp_receive
On 12/10/2020 15.47, Paolo Bonzini wrote: > On 12/10/20 13:14, Thomas Huth wrote: >>> +/** >>> + * qtest_qmp_receive: >>> + * @s: #QTestState instance to operate on. >>> + * >>> + * Reads a QMP message from QEMU and returns the response. >>> + * Buffers all the events received meanwhile, until a >>> + * call to qtest_qmp_eventwait >>> + */ >>> +QDict *qtest_qmp_receive(QTestState *s); >> Re-introducing qtest_qmp_receive() with different behavior than before will >> likely make backports of other later patches a pain, and might also break >> other patches that use this function but are not merged yet. Could you >> please use a different name for this function instead? Maye >> qtest_qmp_receive_buffered() or something like that? > > We chose to use the same name because the new version generally is the > one you want and, except for the handling of events, is exactly the same > as before. In other words, I'm treating the new semantics more as a > bugfix than a feature. > > The only trap that backports of later patches could fall into is if they > want to look at events, but it would be caught easily because the test > would fail. Ok, thanks for the explanation! ... but I think it might be good to have this information in the patch description, though. Thomas
Re: [PATCH v2 9/9] block: check availablity for preadv/pwritev on mac
On 19/10/2020 03.39, Joelle van Dyne wrote: > From: osy That "From:" line looks wrong ... could you please fix the "Author" of your patches / your git config? > macOS 11/iOS 14 added preadv/pwritev APIs. Due to weak linking, configure > will succeed with CONFIG_PREADV even when targeting a lower OS version. We > therefore need to check at run time if we can actually use these APIs. That sounds like the wrong approach to me ... could you please try to fix the check in "configure" instead? E.g. by running compile_prog with "-Werror", so that the test fails if there is no valid prototype available? Thomas
Re: [PATCH 1/4] qdev: Fix two typos
On 19/10/2020 18.36, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky
> ---
> include/hw/qdev-core.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 868973319e..3761186804 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -163,8 +163,8 @@ struct NamedClockList {
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> - *When accessed outsize big qemu lock, must be accessed with
> - *atomic_load_acquire()
> + *When accessed outside big qemu lock, must be accessed with
> + *qatomic_load_acquire()
> * @reset: ResettableState for the device; handled by Resettable interface.
> *
> * This structure should not be accessed directly. We declare it here
>
Reviewed-by: Thomas Huth
Re: [PATCH 3/4] libqtest: fix memory leak in the qtest_qmp_event_ref
On 19/10/2020 18.37, Maxim Levitsky wrote:
> The g_list_remove_link doesn't free the link element,
> opposed to what I thought.
> Switch to g_list_delete_link that does free it.
>
> Also refactor the code a bit.
> Thanks for Max Reitz for helping me with this.
>
> Signed-off-by: Maxim Levitsky
> ---
> tests/qtest/libqtest.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index bd96cb6fdd..9ae052d566 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -795,15 +795,12 @@ void qtest_qmp_send_raw(QTestState *s, const char *fmt,
> ...)
>
> QDict *qtest_qmp_event_ref(QTestState *s, const char *event)
> {
> -GList *next = NULL;
> -QDict *response;
> -
> -for (GList *it = s->pending_events; it != NULL; it = next) {
> +while (s->pending_events) {
>
> -next = it->next;
> -response = (QDict *)it->data;
> +GList *first = s->pending_events;
> +QDict *response = (QDict *)first->data;
>
> -s->pending_events = g_list_remove_link(s->pending_events, it);
> +s->pending_events = g_list_delete_link(s->pending_events, first);
>
> if (!strcmp(qdict_get_str(response, "event"), event)) {
> return response;
>
Thanks, queued (together with patch 2) to qtest-next:
https://gitlab.com/huth/qemu/-/commits/qtest-next/
Thomas
Re: [PATCH v6 11/11] qapi: Use QAPI_LIST_ADD() where possible
On 27/10/2020 06.05, Eric Blake wrote:
> Anywhere we create a list of just one item or by prepending items
> (typically because order doesn't matter), we can use the now-public
> macro. But places where we must keep the list in order by appending
> remain open-coded.
>
> Signed-off-by: Eric Blake
> ---
> docs/devel/writing-qmp-commands.txt | 13 +++--
> hw/net/rocker/rocker_fp.h | 2 +-
> block/gluster.c | 19 +
> chardev/char.c | 21 +++
> hw/core/machine.c | 6 +
> hw/net/rocker/rocker.c | 8 +++---
> hw/net/rocker/rocker_fp.c | 14 +-
> hw/net/virtio-net.c | 21 +--
> migration/migration.c | 7 ++---
> migration/postcopy-ram.c| 7 ++---
> monitor/hmp-cmds.c | 11
> qemu-img.c | 5 ++--
> qga/commands-posix.c| 13 +++--
> qga/commands-win32.c| 17 +++-
> qga/commands.c | 6 +
> qom/qom-qmp-cmds.c | 29 ++--
> target/arm/helper.c | 6 +
> target/arm/monitor.c| 13 ++---
> target/i386/cpu.c | 6 +
> target/mips/helper.c| 6 +
> target/s390x/cpu_models.c | 12 ++---
> tests/test-clone-visitor.c | 7 ++---
> tests/test-qobject-output-visitor.c | 42 ++---
> tests/test-visitor-serialization.c | 5 +---
> trace/qmp.c | 22 +++
> ui/vnc.c| 21 +--
> util/qemu-config.c | 14 +++---
> target/ppc/translate_init.c.inc | 12 ++---
> 28 files changed, 119 insertions(+), 246 deletions(-)
>
> diff --git a/docs/devel/writing-qmp-commands.txt
> b/docs/devel/writing-qmp-commands.txt
> index 46a6c48683f5..3e11eeaa1893 100644
> --- a/docs/devel/writing-qmp-commands.txt
> +++ b/docs/devel/writing-qmp-commands.txt
> @@ -531,15 +531,10 @@ TimerAlarmMethodList *qmp_query_alarm_methods(Error
> **errp)
> bool current = true;
>
> for (p = alarm_timers; p->name; p++) {
> -TimerAlarmMethodList *info = g_malloc0(sizeof(*info));
> -info->value = g_malloc0(sizeof(*info->value));
> -info->value->method_name = g_strdup(p->name);
> -info->value->current = current;
> -
> -current = false;
> -
> -info->next = method_list;
> -method_list = info;
> + TimerAlarmMethod *value = g_new0(TimerAlarmMethod, 1);
White space damage - please replace the TAB with spaces.
> +value->method_name = g_strdup(p->name);
> +value->current = current;
> +QAPI_LIST_ADD(method_list, value);
> }
Thomas
Re: --enable-xen on gitlab CI? (was Re: [PATCH 09/36] qdev: Make qdev_get_prop_ptr() get Object* arg)
On 30/10/2020 18.13, Paolo Bonzini wrote: > On 30/10/20 12:35, Eduardo Habkost wrote: >> >> What is necessary to make sure we have a CONFIG_XEN=y job in >> gitlab CI? Maybe just including xen-devel in some of the >> container images is enough? > > Fedora already has it, but build-system-fedora does not include > x86_64-softmmu. Eduardo, could you try to add xen-devel to the centos8 container? If that does not work, we can still move the x86_64-softmmu target to the fedora pipeline instead. Thomas
Re: [PATCH] Prefer 'on' | 'off' over 'yes' | 'no' for bool options
On 04/11/2020 15.05, Daniel P. Berrangé wrote:
> Update some docs and test cases to use 'on' | 'off' as the preferred
> value for bool options.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> docs/system/vnc-security.rst | 6 +++---
> include/authz/listfile.h | 2 +-
> qemu-options.hx | 4 ++--
> tests/qemu-iotests/233 | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/docs/system/vnc-security.rst b/docs/system/vnc-security.rst
> index b237b07330..e97b42dfdc 100644
> --- a/docs/system/vnc-security.rst
> +++ b/docs/system/vnc-security.rst
> @@ -89,7 +89,7 @@ but with ``verify-peer`` set to ``yes`` instead.
> .. parsed-literal::
>
> |qemu_system| [...OPTIONS...] \
> - -object
> tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
> + -object
> tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=on \
> -vnc :1,tls-creds=tls0 -monitor stdio
>
> .. _vnc_005fsec_005fcertificate_005fpw:
> @@ -103,7 +103,7 @@ authentication to provide two layers of authentication
> for clients.
> .. parsed-literal::
>
> |qemu_system| [...OPTIONS...] \
> - -object
> tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
> + -object
> tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=on \
> -vnc :1,tls-creds=tls0,password -monitor stdio
> (qemu) change vnc password
> Password:
> @@ -145,7 +145,7 @@ x509 options:
> .. parsed-literal::
>
> |qemu_system| [...OPTIONS...] \
> - -object
> tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
> + -object
> tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=on \
> -vnc :1,tls-creds=tls0,sasl -monitor stdio
>
> .. _vnc_005fsetup_005fsasl:
> diff --git a/include/authz/listfile.h b/include/authz/listfile.h
> index 0a1e5bddd3..0b7fe72198 100644
> --- a/include/authz/listfile.h
> +++ b/include/authz/listfile.h
> @@ -73,7 +73,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(QAuthZListFile,
> * The object can be created on the command line using
> *
> * -object authz-list-file,id=authz0,\
> - * filename=/etc/qemu/myvm-vnc.acl,refresh=yes
> + * filename=/etc/qemu/myvm-vnc.acl,refresh=on
> *
> */
> struct QAuthZListFile {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 2c83390504..0bdc07bc47 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -5002,7 +5002,7 @@ SRST
> Note the use of quotes due to the x509 distinguished name
> containing whitespace, and escaping of ','.
>
> -``-object authz-listfile,id=id,filename=path,refresh=yes|no``
> +``-object authz-listfile,id=id,filename=path,refresh=on|off``
> Create an authorization object that will control access to
> network services.
>
> @@ -5047,7 +5047,7 @@ SRST
>
> # |qemu_system| \\
> ... \\
> - -object
> authz-simple,id=auth0,filename=/etc/qemu/vnc-sasl.acl,refresh=yes \\
> + -object
> authz-simple,id=auth0,filename=/etc/qemu/vnc-sasl.acl,refresh=on \\
> ...
>
> ``-object authz-pam,id=id,service=string``
> diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
> index a5c17c3963..0b99530f7f 100755
> --- a/tests/qemu-iotests/233
> +++ b/tests/qemu-iotests/233
> @@ -83,7 +83,7 @@ echo
> echo "== check plain client to TLS server fails =="
>
> nbd_server_start_tcp_socket \
> ---object
> tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes
> \
> +--object
> tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=on \
> --tls-creds tls0 \
> -f $IMGFMT "$TEST_IMG" 2>> "$TEST_DIR/server.log"
>
> @@ -128,7 +128,7 @@ echo "== check TLS with authorization =="
> nbd_server_stop
>
> nbd_server_start_tcp_socket \
> ---object
> tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes
> \
> +--object
> tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=on \
> --object "authz-simple,id=authz0,identity=CN=localhost,, \
>O=Cthulu Dark Lord Enterprises client1,,L=R'lyeh,,C=South Pacific" \
> --tls-authz authz0 \
>
Reviewed-by: Thomas Huth
Re: [PATCH-for-6.0 3/3] gitlab-ci: Test the --disable-virtio-legacy option
On 05/11/2020 13.43, Philippe Mathieu-Daudé wrote: > Add the recently introduced '--disable-virtio-legacy' configuration > to the 'build-disabled' job. > > Signed-off-by: Philippe Mathieu-Daudé > --- > .gitlab-ci.yml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 3b15ae5c302..21fa1a459fd 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -218,6 +218,7 @@ build-disabled: >--disable-strip --disable-tpm --disable-usb-redir --disable-vdi >--disable-vhost-crypto --disable-vhost-net --disable-vhost-scsi >--disable-vhost-user --disable-vhost-vdpa --disable-vhost-vsock > + --disable-virtio-legacy >--disable-virglrenderer --disable-vnc --disable-vte --disable-vvfat >--disable-xen --disable-zstd > TARGETS: arm-softmmu i386-softmmu ppc64-softmmu mips64-softmmu > Acked-by: Thomas Huth
Re: [PATCH 3/3] configure: mark vhost-user Linux-only
On 10/11/2020 18.11, Stefan Hajnoczi wrote: > The vhost-user protocol uses the Linux eventfd feature and is typically > connected to Linux kvm.ko ioeventfd and irqfd file descriptors. The > protocol specification in docs/interop/vhost-user.rst does not describe > how platforms without eventfd support work. > > The QEMU vhost-user devices compile on other POSIX host operating > systems because eventfd usage is abstracted in QEMU. The libvhost-user > programs in contrib/ do not compile but we failed to notice since they > are not built by default. > > Make it clear that vhost-user is only supported on Linux for the time > being. If someone wishes to support it on other platforms then the > details can be added to vhost-user.rst and CI jobs can test the feature > to prevent bitrot. > > Signed-off-by: Stefan Hajnoczi > --- > configure | 9 - > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/configure b/configure > index 5ae73fa32c..ef431f86c0 100755 > --- a/configure > +++ b/configure > @@ -328,7 +328,7 @@ vhost_net="" > vhost_crypto="" > vhost_scsi="" > vhost_vsock="" > -vhost_user="" > +vhost_user="no" > vhost_user_blk_server="auto" > vhost_user_fs="" > kvm="auto" > @@ -718,7 +718,6 @@ fi > case $targetos in > MINGW32*) >mingw32="yes" > - vhost_user="no" >audio_possible_drivers="dsound sdl" >if check_include dsound.h; then > audio_drv_list="dsound" > @@ -797,6 +796,7 @@ Linux) >audio_possible_drivers="oss alsa sdl pa" >linux="yes" >linux_user="yes" > + vhost_user="yes" > ;; > esac > > @@ -2339,9 +2339,8 @@ fi > # vhost interdependencies and host support > > # vhost backends > -test "$vhost_user" = "" && vhost_user=yes > -if test "$vhost_user" = "yes" && test "$mingw32" = "yes"; then > - error_exit "vhost-user isn't available on win32" > +if test "$vhost_user" = "yes" && test "$linux" != "yes"; then > + error_exit "vhost-user is only available on Linux" > fi > test "$vhost_vdpa" = "" && vhost_vdpa=$linux > if test "$vhost_vdpa" = "yes" && test "$linux" != "yes"; then > Reviewed-by: Thomas Huth
Re: [PATCH v2 3/4] nand: put it into the 'storage' category
On 11/11/2020 17.47, Gan Qixin wrote:
> The category of the nand device is not set, put it into the 'storage'
> category.
>
> Signed-off-by: Gan Qixin
> ---
> hw/block/nand.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index bcceb64ebb..1d7a48a2ec 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -449,6 +449,7 @@ static void nand_class_init(ObjectClass *klass, void
> *data)
> dc->reset = nand_reset;
> dc->vmsd = &vmstate_nand;
> device_class_set_props(dc, nand_properties);
> +set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> }
>
> static const TypeInfo nand_info = {
>
Reviewed-by: Thomas Huth
Re: [PATCH 1/3] tests/qtest/fuzz-test: Quit test_lp1878642 once done
On 01/12/2020 16.13, Philippe Mathieu-Daudé wrote:
> Missed in fd250172842 ("qtest: add a reproducer for LP#1878642").
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/qtest/fuzz-test.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> index 9cb4c42bdea..87b72307a5b 100644
> --- a/tests/qtest/fuzz-test.c
> +++ b/tests/qtest/fuzz-test.c
> @@ -45,6 +45,7 @@ static void
> test_lp1878642_pci_bus_get_irq_level_assert(void)
> qtest_outl(s, 0xcf8, 0x8400f841);
> qtest_outl(s, 0xcfc, 0xebed205d);
> qtest_outl(s, 0x5d02, 0xebed205d);
> +qtest_quit(s);
> }
>
> int main(int argc, char **argv)
>
Reviewed-by: Thomas Huth
Re: [PATCH 2/3] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
On 01/12/2020 16.13, Philippe Mathieu-Daudé wrote:
> cdb_len can not be zero... (or less than 6) here, else we have a
> out-of-bound read first in scsi_cdb_length():
>
> 71 int scsi_cdb_length(uint8_t *buf)
> 72 {
> 73 int cdb_len;
> 74
> 75 switch (buf[0] >> 5) {
> 76 case 0:
> 77 cdb_len = 6;
> 78 break;
>
> Then another out-of-bound read when the size returned by
> scsi_cdb_length() is used.
>
> Add a reproducer which triggers:
>
> $ make check-qtest-x86_64
> Running test qtest-x86_64/fuzz-test
> qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion
> `cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len' failed.
> tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6
> (Aborted) (core dumped)
> ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
>
> Inspired-by: Alexander Bulekov
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/scsi/megasas.c | 1 +
> tests/qtest/fuzz-test.c | 196
For the final version, I think you should add the fuzz-test after the fix
(patch 3) ... otherwise this breaks bisection later...
Thomas
Re: [PATCH v2 3/4] tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer
On 01/12/2020 20.10, Philippe Mathieu-Daudé wrote:
> Add a reproducer which triggers (without the previous patch):
>
> $ make check-qtest-x86_64
> Running test qtest-x86_64/fuzz-test
> qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion
> `cdb_len > 0 && scsi_cdb_length(cdb) <= cdb_len' failed.
> tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6
> (Aborted) (core dumped)
> ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
>
> Signed-off-by: Alexander Bulekov
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/qtest/fuzz-test.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> index 87b72307a5b..31f90cfb4fc 100644
> --- a/tests/qtest/fuzz-test.c
> +++ b/tests/qtest/fuzz-test.c
> @@ -48,6 +48,23 @@ static void
> test_lp1878642_pci_bus_get_irq_level_assert(void)
> qtest_quit(s);
> }
>
> +static void test_megasas_cdb_len_zero(void)
> +{
> +QTestState *s;
> +
> +s = qtest_init("-M pc -nodefaults "
> + "-device megasas-gen2 -device scsi-cd,drive=null0 "
> + "-blockdev
> driver=null-co,read-zeroes=on,node-name=null0");
> +
> +qtest_outl(s, 0xcf8, 0x80001011);
> +qtest_outb(s, 0xcfc, 0xbb);
> +qtest_outl(s, 0xcf8, 0x80001002);
> +qtest_outl(s, 0xcfc, 0xf3ff2966);
> +qtest_writeb(s, 0x4600, 0x03);
> +qtest_outw(s, 0xbb40, 0x460b);
> +qtest_quit(s);
> +}
> +
> int main(int argc, char **argv)
> {
> const char *arch = qtest_get_arch();
> @@ -59,6 +76,8 @@ int main(int argc, char **argv)
> test_lp1878263_megasas_zero_iov_cnt);
> qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
> test_lp1878642_pci_bus_get_irq_level_assert);
> +qtest_add_func("fuzz/test_megasas_cdb_len_zero",
> + test_megasas_cdb_len_zero);
> }
>
> return g_test_run();
>
Acked-by: Thomas Huth
Re: [PATCH v2 02/12] libqtest: add qtest_socket_server()
On 07/12/2020 18.20, Stefan Hajnoczi wrote: > Add an API that returns a new UNIX domain socket in the listen state. > The code for this was already there but only used internally in > init_socket(). > > This new API will be used by vhost-user-blk-test. > > Signed-off-by: Stefan Hajnoczi > --- > tests/qtest/libqos/libqtest.h | 8 +++ > tests/qtest/libqtest.c| 40 --- > 2 files changed, 31 insertions(+), 17 deletions(-) > > diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h > index 724f65aa94..e5f1ec590c 100644 > --- a/tests/qtest/libqos/libqtest.h > +++ b/tests/qtest/libqos/libqtest.h > @@ -132,6 +132,14 @@ void qtest_qmp_send(QTestState *s, const char *fmt, ...) > void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...) > GCC_FMT_ATTR(2, 3); Reviewed-by: Thomas Huth
[Qemu-block] Use after free problem somewhere in ahci.c or ich.c code
Hi! Looks like there is a use-after-free problem somewhere in the ahci.c or ich.c code when trying to add the ich9-ahci on a old PC machine. Using valgrind, I get: $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S ==6604== Memcheck, a memory error detector ==6604== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==6604== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info ==6604== Command: x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S ==6604== QEMU 2.9.93 monitor - type 'help' for more information (qemu) device_add ich9-ahci,id=ich9-ahci ==6604== Invalid read of size 8 ==6604==at 0x609AB0: object_unparent (object.c:445) ==6604==by 0x4C4478: device_unparent (qdev.c:1095) ==6604==by 0x60A364: object_finalize_child_property (object.c:1396) ==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427) ==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634) ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807) ==6604==by 0x46B689: hmp_device_add (hmp.c:1925) ==6604==by 0x364083: handle_hmp_command (monitor.c:3119) ==6604==by 0x365439: monitor_command_cb (monitor.c:3922) ==6604==by 0x6E5D27: readline_handle_byte (readline.c:393) ==6604==by 0x364311: monitor_read (monitor.c:3905) ==6604==by 0x67C573: mux_chr_read (char-mux.c:216) ==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd ==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530) ==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3) ==6604==by 0x50100E: pci_ich9_uninit (ich.c:161) ==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083) ==6604==by 0x4C5EE9: device_set_realized (qdev.c:988) ==6604==by 0x608DCD: property_set_bool (object.c:1886) ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27) ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162) ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630) ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807) ==6604==by 0x46B689: hmp_device_add (hmp.c:1925) ==6604==by 0x364083: handle_hmp_command (monitor.c:3119) ==6604== Block was alloc'd at ==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711) ==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3) ==6604==by 0x50094F: ahci_realize (ahci.c:1468) ==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115) ==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002) ==6604==by 0x4C5E69: device_set_realized (qdev.c:914) ==6604==by 0x608DCD: property_set_bool (object.c:1886) ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27) ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162) ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630) ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807) ==6604==by 0x46B689: hmp_device_add (hmp.c:1925) ==6604== ==6604== Invalid read of size 8 ==6604==at 0x60A350: object_finalize_child_property (object.c:1395) ==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427) ==6604==by 0x4C4478: device_unparent (qdev.c:1095) ==6604==by 0x60A364: object_finalize_child_property (object.c:1396) ==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427) ==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634) ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807) ==6604==by 0x46B689: hmp_device_add (hmp.c:1925) ==6604==by 0x364083: handle_hmp_command (monitor.c:3119) ==6604==by 0x365439: monitor_command_cb (monitor.c:3922) ==6604==by 0x6E5D27: readline_handle_byte (readline.c:393) ==6604==by 0x364311: monitor_read (monitor.c:3905) ==6604== Address 0x15fc5428 is 30,296 bytes inside a block of size 36,288 free'd ==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530) ==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3) ==6604==by 0x50100E: pci_ich9_uninit (ich.c:161) ==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083) ==6604==by 0x4C5EE9: device_set_realized (qdev.c:988) ==6604==by 0x608DCD: property_set_bool (object.c:1886) ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27) ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162) ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630) ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807) ==6604==by 0x46B689: hmp_device_add (hmp.c:1925) ==6604==by 0x364083: handle_hmp_command (monitor.c:3119) ==6604== Block was alloc'd at ==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711) ==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3) ==6604==by 0x50094F: ahci_realize (ahci.c:1468) ==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115) ==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002) ==6604==by 0x4C5E69: device_set_realized (qdev.c:914) ==6604==by 0x608DCD: property_set_bool
Re: [Qemu-block] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote: > On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote: >> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote: >>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new >>> property >>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines. >>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" >>> added the >>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug' >>> property. >>> >>> Check for use_acpi_pci_hotplug before calling >>> acpi_pcihp_device_[un]plug_cb(). [...] >>> Reported-by: Thomas Huth >>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé >> >> Looks like this is a very old bug, isn't it? >> Objections to merging this after the release? > > Yes, I'm also inclined to delay it so we can release 2.10, I tagged > "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll > let him decide if it is worth crying wolf :) It's very likely no-one but > him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot > plugging AHCI devices :D I'm fine if this gets included in 2.11 - it's quite unlikely that a user tries hot-plug ahci on such an old machine type, I think. But we maybe should include this in the 2.10.1 stable release, so I'm putting qemu-stable on CC now. Anyway, your patch seems to fix the issue for me, thanks! Tested-by: Thomas Huth
Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
On 23.08.2017 07:40, Thomas Huth wrote: > On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote: >> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote: >>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote: >>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new >>>> property >>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines. >>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" >>>> added the >>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug' >>>> property. >>>> >>>> Check for use_acpi_pci_hotplug before calling >>>> acpi_pcihp_device_[un]plug_cb(). > [...] >>>> Reported-by: Thomas Huth >>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com> >>>> Signed-off-by: Philippe Mathieu-Daudé >>> >>> Looks like this is a very old bug, isn't it? >>> Objections to merging this after the release? >> >> Yes, I'm also inclined to delay it so we can release 2.10, I tagged >> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll >> let him decide if it is worth crying wolf :) It's very likely no-one but >> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot >> plugging AHCI devices :D > > I'm fine if this gets included in 2.11 - it's quite unlikely that a user > tries hot-plug ahci on such an old machine type, I think. But we maybe > should include this in the 2.10.1 stable release, so I'm putting > qemu-stable on CC now. > > Anyway, your patch seems to fix the issue for me, thanks! > > Tested-by: Thomas Huth ... No, I was too fast here. I'm afraid, it still crashes with mips64el: $ valgrind mips64el-softmmu/qemu-system-mips64el -S -nographic -M malta,accel=qtest ==17935== Memcheck, a memory error detector ==17935== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==17935== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info ==17935== Command: mips64el-softmmu/qemu-system-mips64el -S -nographic -M malta,accel=qtest ==17935== QEMU 2.9.93 monitor - type 'help' for more information (qemu) device_add ich9-ahci ==17935== Invalid read of size 8 ==17935==at 0x5F6F10: object_unparent (object.c:445) ==17935==by 0x4BB2C8: device_unparent (qdev.c:1095) ==17935==by 0x5F77C4: object_finalize_child_property (object.c:1396) ==17935==by 0x5F6706: object_property_del_child.isra.7 (object.c:427) ==17935==by 0x448BC8: qdev_device_add (qdev-monitor.c:634) ==17935==by 0x449122: qmp_device_add (qdev-monitor.c:807) ==17935==by 0x462B29: hmp_device_add (hmp.c:1925) ==17935==by 0x370F83: handle_hmp_command (monitor.c:3119) ==17935==by 0x371E59: monitor_command_cb (monitor.c:3922) ==17935==by 0x6D3187: readline_handle_byte (readline.c:393) ==17935==by 0x371211: monitor_read (monitor.c:3905) ==17935==by 0x6699D3: mux_chr_read (char-mux.c:216) ==17935== Address 0x21c549d8 is 30,328 bytes inside a block of size 36,288 free'd ==17935==at 0x4C2ACDD: free (vg_replace_malloc.c:530) ==17935==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3) ==17935==by 0x4FB94E: pci_ich9_uninit (ich.c:161) ==17935==by 0x5350FB: pci_qdev_unrealize (pci.c:1083) ==17935==by 0x4BCD39: device_set_realized (qdev.c:988) ==17935==by 0x5F622D: property_set_bool (object.c:1886) ==17935==by 0x5FA31E: object_property_set_qobject (qom-qobject.c:27) ==17935==by 0x5F7FCF: object_property_set_bool (object.c:1162) ==17935==by 0x448B93: qdev_device_add (qdev-monitor.c:630) ==17935==by 0x449122: qmp_device_add (qdev-monitor.c:807) ==17935==by 0x462B29: hmp_device_add (hmp.c:1925) ==17935==by 0x370F83: handle_hmp_command (monitor.c:3119) ==17935== Block was alloc'd at ==17935==at 0x4C2B975: calloc (vg_replace_malloc.c:711) ==17935==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3) ==17935==by 0x4FB28F: ahci_realize (ahci.c:1468) ==17935==by 0x4FB9D8: pci_ich9_ahci_realize (ich.c:115) ==17935==by 0x5366BD: pci_qdev_realize (pci.c:2002) ==17935==by 0x4BCCB9: device_set_realized (qdev.c:914) ==17935==by 0x5F622D: property_set_bool (object.c:1886) ==17935==by 0x5FA31E: object_property_set_qobject (qom-qobject.c:27) ==17935==by 0x5F7FCF: object_property_set_bool (object.c:1162) ==17935==by 0x448B93: qdev_device_add (qdev-monitor.c:630) ==17935==by 0x449122: qmp_device_add (qdev-monitor.c:807) ==17935==by 0x462B29: hmp_device_add (hmp.c:1925) Do you've got an idea how to fix that, too? Thomas
[Qemu-block] [PATCH for-2.11] hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false
QEMU currently aborts with an assertion message when the user is trying
to remove a dscm1 again:
$ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
QEMU 2.9.93 monitor - type 'help' for more information
(qemu) device_add dscm1,id=xyz
(qemu) device_del xyz
**
ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
Aborted (core dumped)
Looks like this device has to be wired up in code and is not meant
to be hot-pluggable, so let's mark it with user_creatable = false.
Signed-off-by: Thomas Huth
---
hw/ide/microdrive.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index e3fd30e..17917c0 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -575,12 +575,15 @@ PCMCIACardState *dscm1_init(DriveInfo *dinfo)
static void dscm1_class_init(ObjectClass *oc, void *data)
{
PCMCIACardClass *pcc = PCMCIA_CARD_CLASS(oc);
+DeviceClass *dc = DEVICE_CLASS(oc);
pcc->cis = dscm1_cis;
pcc->cis_len = sizeof(dscm1_cis);
pcc->attach = dscm1_attach;
pcc->detach = dscm1_detach;
+/* Reason: Needs to be wired-up in code, see dscm1_init() */
+dc->user_creatable = false;
}
static const TypeInfo dscm1_type_info = {
--
1.8.3.1
Re: [Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
On 28.08.2017 18:34, Igor Mammedov wrote:
> Fixes read after freeing error reported
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
>
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
>
>qdev_device_add()
> object_property_set_bool('realized', true)
>device_set_realized()
> ...
> pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
>...
>s->dev = g_new0(AHCIDevice, ports);
>...
> AHCIDevice *ad = &s->dev[i];
> ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
> ^^^ creates bus in memory allocated by above gnew()
> and adds it as child propety to ahci device
> ...
> hotplug_handler_plug(); -> goto post_realize_fail;
> pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
> ...
>g_free(s->dev);
>^^^ free memory that holds children busses
>
> return with error from device_set_realized()
>
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
>
> Reported-by: Thomas Huth
> Signed-off-by: Igor Mammedov
> ---
> hw/ide/ahci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>
> ide_exit(s);
> }
> +object_unparent(OBJECT(&ad->port));
> }
>
> g_free(s->dev);
>
Thanks, this fixes the problem for me with both, x86_64 and mips64el!
Tested-by: Thomas Huth
Re: [Qemu-block] [Qemu-devel] [PATCH] isa-fdc: assert replaced by proper error exit
On 01.09.2017 13:07, Eduardo Otubo wrote:
> When not available, isa-fdc falls into assert instead of proper error
> exit. This patch fixes this behavior.
>
> Signed-off-by: Eduardo Otubo
> ---
> hw/block/fdc.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 401129073b..0b6def4e1d 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2699,11 +2699,15 @@ static void isabus_fdc_realize(DeviceState *dev,
> Error **errp)
> fdctrl->dma_chann = isa->dma;
> if (fdctrl->dma_chann != -1) {
> fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
> -assert(fdctrl->dma);
> +if (!fdctrl->dma) {
> +error_setg(errp, "isa-fdc not supported");
> +goto error;
> +}
> }
>
> qdev_set_legacy_instance_id(dev, isa->iobase, 2);
> fdctrl_realize_common(dev, fdctrl, &err);
> +error:
> if (err != NULL) {
> error_propagate(errp, err);
> return;
Maybe add the reproducer to the commit message:
qemu-system-ppc64 -S -machine powernv -device isa-fdc
Reviewed-by: Thomas Huth
Re: [Qemu-block] [Qemu-devel] [PATCH v6 12/29] libqos: Track QTestState with QPCIBus
On 01.09.2017 20:03, Eric Blake wrote:
> When initializing a QPCIBus, track which QTestState the bus is
> associated with (so that a later patch can then explicitly use
> that test state for all communication on the bus, rather than
> blindly relying on global_qtest). Update the initialization
> functions to take another parameter, and update all callers to
> pass in state (for now, most callers get away with passing the
> current global_qtest as the current state, although this required
> fixing the order of initialization to ensure qtest_start() is
> called before qpci_init*() in rtl8139-test, and provided an
> opportunity to pass in the allocator in e1000e-test).
>
> Signed-off-by: Eric Blake
> ---
[...]
> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
> index 6226546c28..c95428e1cb 100644
> --- a/tests/libqos/libqos.c
> +++ b/tests/libqos/libqos.c
> @@ -26,8 +26,8 @@ QOSState *qtest_vboot(QOSOps *ops, const char *cmdline_fmt,
> va_list ap)
> if (ops->init_allocator) {
> qs->alloc = ops->init_allocator(ALLOC_NO_FLAGS);
> }
> -if (ops->qpci_init && qs->alloc) {
> -qs->pcibus = ops->qpci_init(qs->alloc);
> +if (ops->qpci_init) {
Why did you remove the check for qs->alloc?
> +qs->pcibus = ops->qpci_init(qs->qts, qs->alloc);
> }
> }
>
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 02ce49927a..85b34c6d13 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -115,11 +115,14 @@ static void qpci_pc_config_writel(QPCIBus *bus, int
> devfn, uint8_t offset, uint3
> outl(0xcfc, value);
> }
>
> -QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> {
> QPCIBusPC *ret;
>
> +assert(qts);
> +
> ret = g_malloc(sizeof(*ret));
> +ret->bus.qts = qts;
>
> ret->bus.pio_readb = qpci_pc_pio_readb;
> ret->bus.pio_readw = qpci_pc_pio_readw;
> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> index 2043f1e123..cd9b8f52d2 100644
> --- a/tests/libqos/pci-spapr.c
> +++ b/tests/libqos/pci-spapr.c
> @@ -154,11 +154,14 @@ static void qpci_spapr_config_writel(QPCIBus *bus, int
> devfn, uint8_t offset,
> #define SPAPR_PCI_MMIO32_WIN_SIZE0x8000 /* 2 GiB */
> #define SPAPR_PCI_IO_WIN_SIZE0x1
>
> -QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
> +QPCIBus *qpci_init_spapr(QTestState *qts, QGuestAllocator *alloc)
> {
> QPCIBusSPAPR *ret;
>
> +assert(qts);
> +
> ret = g_malloc(sizeof(*ret));
+1 for using g_malloc0 here instead.
> +ret->bus.qts = qts;
>
> ret->alloc = alloc;
>
> @@ -201,6 +204,7 @@ QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
> ret->bus.mmio_alloc_ptr = ret->mmio32.pci_base;
> ret->bus.mmio_limit = ret->mmio32.pci_base + ret->mmio32.size;
>
> +
Superfluous white space change.
> return &ret->bus;
> }
Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations
On 01.09.2017 20:03, Eric Blake wrote: > Drop one more client of global_qtest by teaching all fw_cfg test > functionality (invoked through alloc-pc) to pass in an explicit > QTestState, adjusting all callers. In particular, fw_cfg-test > had to reorder things to create the test state prior to creating > the fw_cfg. > > Signed-off-by: Eric Blake > --- > tests/libqos/fw_cfg.h | 10 ++ > tests/libqos/libqos.h | 2 +- > tests/libqos/malloc-pc.h| 4 ++-- > tests/libqos/malloc-spapr.h | 2 +- > tests/libqos/malloc.h | 1 + > tests/boot-order-test.c | 6 +++--- > tests/e1000e-test.c | 2 +- > tests/fw_cfg-test.c | 14 ++ > tests/ide-test.c| 2 +- > tests/libqos/fw_cfg.c | 14 -- > tests/libqos/libqos.c | 2 +- > tests/libqos/malloc-pc.c| 8 > tests/libqos/malloc-spapr.c | 4 ++-- > tests/vhost-user-test.c | 2 +- > 14 files changed, 38 insertions(+), 35 deletions(-) > > diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h > index e8371b2317..396dd4ee1e 100644 > --- a/tests/libqos/fw_cfg.h > +++ b/tests/libqos/fw_cfg.h > @@ -15,10 +15,12 @@ > > > typedef struct QFWCFG QFWCFG; > +typedef struct QTestState QTestState; Not sure, but I slightly remember that typedeffing a struct like this in multiple places can cause compiler warnings or errors with certain versions of GCC or clang? So a file that includes both, fw_cfg.h and libqtest.h will then fail to compile? I think it would be better to change the include order in the .c files instead, so that libqtest.h is always included before fw_cfg.h. Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/29] libqos: Use explicit QTestState for virtio operations
On 01.09.2017 20:03, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all virtio test
> functionality to pass in an explicit QTestState in constructors,
> where it is then reused for later access. Adjust all callers.
> This gets us one step closer to eliminating implicit use of
> global_qtest.
>
> Signed-off-by: Eric Blake
> ---
> tests/libqos/virtio-mmio.h | 3 +-
> tests/libqos/virtio.h | 5 ++-
> tests/libqos/virtio-mmio.c | 54 +---
> tests/libqos/virtio-pci.c | 3 ++
> tests/libqos/virtio.c | 77
> ++
> tests/virtio-blk-test.c| 3 +-
> 6 files changed, 84 insertions(+), 61 deletions(-)
>
> diff --git a/tests/libqos/virtio-mmio.h b/tests/libqos/virtio-mmio.h
> index e3e52b9ce1..bd01386054 100644
> --- a/tests/libqos/virtio-mmio.h
> +++ b/tests/libqos/virtio-mmio.h
> @@ -41,6 +41,7 @@ typedef struct QVirtioMMIODevice {
>
> extern const QVirtioBus qvirtio_mmio;
>
> -QVirtioMMIODevice *qvirtio_mmio_init_device(uint64_t addr, uint32_t
> page_size);
> +QVirtioMMIODevice *qvirtio_mmio_init_device(QTestState *qts, uint64_t addr,
> +uint32_t page_size);
>
> #endif
> diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
> index 8fbcd1869c..d180d54fc4 100644
> --- a/tests/libqos/virtio.h
> +++ b/tests/libqos/virtio.h
> @@ -19,6 +19,7 @@ typedef struct QVirtioBus QVirtioBus;
>
> typedef struct QVirtioDevice {
> const QVirtioBus *bus;
> +QTestState *qts;
> /* Device type */
> uint16_t device_type;
> } QVirtioDevice;
> @@ -35,12 +36,14 @@ typedef struct QVirtQueue {
> uint16_t last_used_idx;
> bool indirect;
> bool event;
> +QTestState *qts;
> } QVirtQueue;
>
> typedef struct QVRingIndirectDesc {
> uint64_t desc; /* This points to an array fo struct vring_desc */
> uint16_t index;
> uint16_t elem;
> +QTestState *qts;
> } QVRingIndirectDesc;
Adding QTestState to QVRingIndirectDesc and QVirtQueue sounds somewhat
ugly to me. I think they should either rather have a pointer to the
associated QVirtioDevice, or the functions where this is needed
(qvring_init() for example) should get a "QTestState *" parameter instead.
Just my 0.02 €.
Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v6 17/29] ahci-test: Drop dependence on global_qtest
On 01.09.2017 20:03, Eric Blake wrote: > Managing parallel connections to two different monitors via > the implicit global_qtest makes it hard to copy-and-paste code > to tests that are not aware of the implicit state; the > management of global_qtest is even harder to follow because > it was masked behind set_context(). > > Instead, explicitly pass QTestState* around (generally, by > reusing the member already present in ahci->parent QOSState), > and call explicit qtest_* functions on all places that > interact with a monitor. > > We can assert that the conversion is correct by checking that > global_qtest remains NULL throughout the test (a later patch > that changes global_qtest to not be a public global variable > will drop the assertions). > > Bonus: there were several spots that were constructing a JSON > string, then passing that through qmp() as the format, rather > than directly using qmp() to construct the JSON. Fixing that > gets us one step closer to enabling -Wformat checking on > constructed JSON. > > Signed-off-by: Eric Blake > --- > tests/libqos/libqos.h| 1 - > tests/ahci-test.c| 83 > +++- > tests/libqos/ahci.c | 45 +- > tests/libqos/libqos-pc.c | 2 +- > tests/libqos/libqos.c| 37 ++--- > 5 files changed, 73 insertions(+), 95 deletions(-) Might be easier to review if you'd split the changes to libqos.c into a separate patch. But anyway: Reviewed-by: Thomas Huth
Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations
On 05.09.2017 12:12, Thomas Huth wrote: > On 01.09.2017 20:03, Eric Blake wrote: >> Drop one more client of global_qtest by teaching all fw_cfg test >> functionality (invoked through alloc-pc) to pass in an explicit >> QTestState, adjusting all callers. In particular, fw_cfg-test >> had to reorder things to create the test state prior to creating >> the fw_cfg. >> >> Signed-off-by: Eric Blake >> --- >> tests/libqos/fw_cfg.h | 10 ++ >> tests/libqos/libqos.h | 2 +- >> tests/libqos/malloc-pc.h| 4 ++-- >> tests/libqos/malloc-spapr.h | 2 +- >> tests/libqos/malloc.h | 1 + >> tests/boot-order-test.c | 6 +++--- >> tests/e1000e-test.c | 2 +- >> tests/fw_cfg-test.c | 14 ++ >> tests/ide-test.c| 2 +- >> tests/libqos/fw_cfg.c | 14 -- >> tests/libqos/libqos.c | 2 +- >> tests/libqos/malloc-pc.c| 8 >> tests/libqos/malloc-spapr.c | 4 ++-- >> tests/vhost-user-test.c | 2 +- >> 14 files changed, 38 insertions(+), 35 deletions(-) >> >> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h >> index e8371b2317..396dd4ee1e 100644 >> --- a/tests/libqos/fw_cfg.h >> +++ b/tests/libqos/fw_cfg.h >> @@ -15,10 +15,12 @@ >> >> >> typedef struct QFWCFG QFWCFG; >> +typedef struct QTestState QTestState; > > Not sure, but I slightly remember that typedeffing a struct like this in > multiple places can cause compiler warnings or errors with certain > versions of GCC or clang? So a file that includes both, fw_cfg.h and > libqtest.h will then fail to compile? > > I think it would be better to change the include order in the .c files > instead, so that libqtest.h is always included before fw_cfg.h. Ah, well, I just saw that you also sent a fixup patch for this. Anyway, I'm not a fan of including header files from other header files, so changing the include order in the .c files sounds like the better solution to me. Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v6 28/29] libqtest: Remove qtest_start() and qtest_end() shortcuts
On 01.09.2017 20:03, Eric Blake wrote:
> Remove the trivial wrappers qtest_start() and qtest_end(), to make
> it obvious in the rest of the testsuite where we are still relying on
> global_qtest. Doing this makes it easier to see what remaining
> cleanups will be needed if we don't want an implicit dependency
> on global state. Many tests can also take advantage of qtest_init()
> doing formatting of args, avoiding a temporary local variable.
>
> Signed-off-by: Eric Blake
> ---
> tests/libqtest.h | 26 --
> tests/libqtest.c | 4 +--
> tests/ac97-test.c | 4 +--
> tests/boot-order-test.c| 11 +++-
> tests/boot-serial-test.c | 12 +++--
> tests/device-introspect-test.c | 24 -
> tests/display-vga-test.c | 18 +
> tests/drive_del-test.c | 17 ++--
> tests/ds1338-test.c| 2 +-
> tests/e1000-test.c | 10 ++-
> tests/e1000e-test.c| 14 +++---
> tests/eepro100-test.c | 11 ++--
> tests/endianness-test.c| 33 +--
> tests/es1370-test.c| 4 +--
> tests/fdc-test.c | 4 +--
> tests/hd-geo-test.c| 16 +--
> tests/i440fx-test.c| 16 +--
> tests/i82801b11-test.c | 5 ++--
> tests/ide-test.c | 4 +--
> tests/intel-hda-test.c | 11
> tests/ioh3420-test.c | 7 ++---
> tests/ipmi-bt-test.c | 11 +++-
> tests/ipmi-kcs-test.c | 5 +---
> tests/ipoctal232-test.c| 5 ++--
> tests/ivshmem-test.c | 4 +--
> tests/libqos/libqos.c | 2 +-
> tests/m25p80-test.c| 9 +++
> tests/m48t59-test.c| 2 +-
> tests/ne2000-test.c| 4 +--
> tests/numa-test.c | 28 ++--
> tests/nvme-test.c | 7 ++---
> tests/pc-cpu-test.c| 26 +++---
> tests/pcnet-test.c | 4 +--
> tests/pnv-xscom-test.c | 14 ++
> tests/prom-env-test.c | 13 -
> tests/pvpanic-test.c | 4 +--
> tests/pxe-test.c | 14 --
> tests/q35-test.c | 8 +++---
> tests/qom-test.c | 7 ++---
> tests/rtc-test.c | 2 +-
> tests/rtl8139-test.c | 4 +--
> tests/spapr-phb-test.c | 5 ++--
> tests/tco-test.c | 12 -
> tests/test-arm-mptimer.c | 4 +--
> tests/test-filter-mirror.c | 16 +--
> tests/test-filter-redirector.c | 60
> --
> tests/test-hmp.c | 7 ++---
> tests/test-netfilter.c | 9 +++
> tests/test-x86-cpuid-compat.c | 13 -
> tests/tmp105-test.c| 2 +-
> tests/tpci200-test.c | 4 +--
> tests/usb-hcd-ehci-test.c | 25 +-
> tests/usb-hcd-ohci-test.c | 4 +--
> tests/usb-hcd-xhci-test.c | 4 +--
> tests/virtio-balloon-test.c| 4 +--
> tests/virtio-blk-test.c| 13 -
> tests/virtio-console-test.c| 8 +++---
> tests/virtio-net-test.c| 4 +--
> tests/virtio-rng-test.c| 4 +--
> tests/virtio-serial-test.c | 4 +--
> tests/vmgenid-test.c | 29 ++--
> tests/vmxnet3-test.c | 4 +--
> 62 files changed, 267 insertions(+), 394 deletions(-)
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index d338de3e22..0459526187 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -509,32 +509,6 @@ void qtest_add_data_func_full(const char *str, void
> *data,
> void qtest_add_abrt_handler(GHookFunc fn, const void *data);
>
> /**
> - * qtest_start:
> - * @args: other arguments to pass to QEMU
> - *
> - * Start QEMU and assign the resulting #QTestState to a global variable.
> - * The global variable is used by "shortcut" functions documented below.
> - *
> - * Returns: #QTestState instance.
> - */
> -static inline QTestState *qtest_start(const char *args)
> -{
> -global_qtest = qtest_init("%s", args);
> -return global_qtest;
> -}
> -
> -/**
> - * qtest_end:
> - *
> - * Shut down the QEMU process started by qtest_start().
> - */
> -static inline void qtest_end(void)
> -{
> -qtest_quit(global_qtest);
> -global_qtest = NULL;
> -}
> -
> -/**
> * qmp:
> * @fmt...: QMP message to send to qemu
> *
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 18facbf130..fa4e47c137 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -970,7 +970,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char
> *machine))
> QString *qstr;
> const char *mname;
>
> -qtest_start("-machine none");
> +global_qtest = qtest_init("-machine none");
> response = qmp("{ 'execute': 'query-machines' }");
> g_assert(response);
> list = qdict_get_qlist(response, "return
Re: [Qemu-block] [Qemu-devel] [PATCH v6 29/29] libqtest: Rename qtest_init() to qtest_start()
On 01.09.2017 20:03, Eric Blake wrote: > We already have another qtest_init() in the tree, for the > top-level qtest.c device; having two functions with different > signatures is confusing. Rename the libqtest version to > qtest_start() to eliminate the duplication. This is too much code churn for my taste, and I also do not like the idea of naming the function qtest_start() - since this was a function with different semantics before your patch 28/29, so this will cause confusion for all the people who are used to the old qtest_start() function or who want to backport patches that have done after this change to a code level before this change. If you are really bugged by the qtest_init() name clash, I think it's way easier if you rename the qtest_init() in the qtest.c file instead. Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v6 12/29] libqos: Track QTestState with QPCIBus
On 06.09.2017 23:00, Eric Blake wrote:
> On 09/05/2017 04:36 AM, Thomas Huth wrote:
>> On 01.09.2017 20:03, Eric Blake wrote:
>>> When initializing a QPCIBus, track which QTestState the bus is
>>> associated with (so that a later patch can then explicitly use
>>> that test state for all communication on the bus, rather than
>>> blindly relying on global_qtest). Update the initialization
>>> functions to take another parameter, and update all callers to
>>> pass in state (for now, most callers get away with passing the
>>> current global_qtest as the current state, although this required
>>> fixing the order of initialization to ensure qtest_start() is
>>> called before qpci_init*() in rtl8139-test, and provided an
>>> opportunity to pass in the allocator in e1000e-test).
>>>
>>> Signed-off-by: Eric Blake
>>> ---
>> [...]
>>> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
>>> index 6226546c28..c95428e1cb 100644
>>> --- a/tests/libqos/libqos.c
>>> +++ b/tests/libqos/libqos.c
>>> @@ -26,8 +26,8 @@ QOSState *qtest_vboot(QOSOps *ops, const char
>>> *cmdline_fmt, va_list ap)
>>> if (ops->init_allocator) {
>>> qs->alloc = ops->init_allocator(ALLOC_NO_FLAGS);
>>> }
>>> -if (ops->qpci_init && qs->alloc) {
>>> -qs->pcibus = ops->qpci_init(qs->alloc);
>>> +if (ops->qpci_init) {
>>
>> Why did you remove the check for qs->alloc?
>>
>>> +qs->pcibus = ops->qpci_init(qs->qts, qs->alloc);
>
> Because we want to ensure qpci_init() is called to set qs->qts
> (presumably, whether or not qs->alloc is set). Furthermore, only two
> files declare a 'static QOSOps' structure in the first place
> (libqos-pc.c and libqos-spapr.c); where both files set both the
> .init_allocator and .qpci_init callbacks; a little bit of auditing shows
> that the .init_allocator() never returns NULL (although that requires
> browsing yet more files for malloc-{pc,spapr}.c).
OK, thanks for the explanation! ... but maybe we should
g_assert(gs->alloc) somewhere instead? ... just an idea, I'm also fine
if you leave it away.
Thomas
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] iotests: use -ccw on s390x for 051
On 08.09.2017 13:54, Kevin Wolf wrote: > Am 08.09.2017 um 13:24 hat Cornelia Huck geschrieben: >> On Fri, 8 Sep 2017 13:04:25 +0200 >> Kevin Wolf wrote: >> >>> Am 05.09.2017 um 17:16 hat Cornelia Huck geschrieben: The default cpu model on s390x does not provide zPCI, which is not yet wired up on tcg. Moreover, virtio-ccw is the standard on s390x, so use the -ccw instead of the -pci versions of virtio devices on s390x. Provide an output file for s390x. Signed-off-by: Cornelia Huck --- tests/qemu-iotests/051 | 9 +- tests/qemu-iotests/051.s390-ccw-virtio.out | 434 + 2 files changed, 442 insertions(+), 1 deletion(-) create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out >>> >>> It's already a pain to have two separate output files for 051, let's try >>> to avoid adding a third one. Even more so since I think that the split >>> between 051.out and 051.pc.out was already made for s390, so I'm not >>> sure if anyone would actually still make use of the plain 051.out >>> output if s390 got it's own one. >> >> Are there no non-pc and non-s390 machines for which this is run? > > Who knows? But I'm not aware of anyone who is interested in something > else and has contributed to the test cases until now. FWIW, as far as I know, Lukáš is running this test also on ppc64 in our weekly regression run. So it would be good to keep that working, please :-) >> Another approach would be to drop the -pci postfix, but I don't want to >> introduce more usage of aliases. > > Maybe that would indeed be the easiest way. As long as we don't intend > to remove the alias from qemu, there's no reason not to use it in tests. Maybe we should even use it in a couple of places on purpose - so we get some test coverage for them? Thomas
Re: [Qemu-block] [PATCH v7 08/38] libqos: Track QTestState with QPCIBus
On 11.09.2017 19:19, Eric Blake wrote: > When initializing a QPCIBus, track which QTestState the bus is > associated with (so that a later patch can then explicitly use > that test state for all communication on the bus, rather than > blindly relying on global_qtest). Update the initialization > functions to take another parameter, and update all callers to > pass in state (for now, most callers get away with passing the > current global_qtest as the current state, although this required > fixing the order of initialization to ensure qtest_start() is > called before qpci_init*() in rtl8139-test, and provided an > opportunity to pass in the allocator in e1000e-test). > > Touch up some allocations to use g_new0() rather than g_malloc() > while in the area, and simplify some code (all implementations > of QOSOps provide a .init_allocator() that never fails). > > Signed-off-by: Eric Blake Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 09/38] libqos: Track QTestState with QVirtioBus
On 11.09.2017 19:19, Eric Blake wrote: > When initializing a QVirtioDevice (which always has an associated > QVirtioBus), we want to track which QTestState to use for all > I/O processed through that bus and device. Copy the paradigm > used for QPCIBus, and track the test state at the bus level; this > in turn requires a separate bus object per device (and associated > cleanup) rather than just sharing a const version of the dispatch > table. I fail to see why we need a separate bus object here for each device. The bus is only available one time, not multiple times, isn't it? So there should also only be one bus object floating around, not multiple ones... or do I miss something? Thomas
Re: [Qemu-block] [PATCH v7 10/38] libqos: Move/rename qpci_unplug_acpi_device_test() to pci.c
On 11.09.2017 19:19, Eric Blake wrote:
> Commit 2f8b2767 originally added qpci_plug_device_test() and
> qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c.
> Later, commit cf716b31 moved one half of the pair to pci.c
> when adding PPC64 support. Keep the implementations of the
> two functions together, and shorten the name to
> qpci_unplug_device_test(), since all callers use the two
> functions in tandem.
>
> Signed-off-by: Eric Blake
> ---
> tests/libqos/pci.h | 2 +-
> tests/e1000e-test.c | 2 +-
> tests/ivshmem-test.c| 2 +-
> tests/libqos/pci-pc.c | 23 ---
> tests/libqos/pci.c | 23 +++
> tests/virtio-blk-test.c | 2 +-
> tests/virtio-net-test.c | 2 +-
> tests/virtio-rng-test.c | 2 +-
> 8 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> index 429c382282..fdda7eca6e 100644
> --- a/tests/libqos/pci.h
> +++ b/tests/libqos/pci.h
> @@ -111,5 +111,5 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
>
> void qpci_plug_device_test(const char *driver, const char *id,
> uint8_t slot, const char *opts);
> -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
> +void qpci_unplug_device_test(const char *id, uint8_t slot);
> #endif
> diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
> index d8085d944e..4c663a3019 100644
> --- a/tests/e1000e-test.c
> +++ b/tests/e1000e-test.c
> @@ -461,7 +461,7 @@ static void test_e1000e_hotplug(gconstpointer data)
> qtest_start("-device e1000e");
>
> qpci_plug_device_test("e1000e", "e1000e_net", slot, NULL);
> -qpci_unplug_acpi_device_test("e1000e_net", slot);
> +qpci_unplug_device_test("e1000e_net", slot);
>
> qtest_end();
> }
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index 37763425ee..8c9ed6a568 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -427,7 +427,7 @@ static void test_ivshmem_hotplug(void)
>
> qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
> if (strcmp(arch, "ppc64") != 0) {
> -qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
> +qpci_unplug_device_test("iv1", PCI_SLOT_HP);
> }
>
> qtest_end();
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index e267fd1a44..6305d142a5 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -19,9 +19,6 @@
> #include "qemu-common.h"
>
>
> -#define ACPI_PCIHP_ADDR 0xae00
> -#define PCI_EJ_BASE 0x0008
> -
> typedef struct QPCIBusPC
> {
> QPCIBus bus;
> @@ -156,23 +153,3 @@ void qpci_free_pc(QPCIBus *bus)
>
> g_free(s);
> }
> -
> -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
> -{
> -QDict *response;
> -char *cmd;
> -
> -cmd = g_strdup_printf("{'execute': 'device_del',"
> - " 'arguments': {"
> - " 'id': '%s'"
> - "}}", id);
> -response = qmp(cmd);
> -g_free(cmd);
> -g_assert(response);
> -g_assert(!qdict_haskey(response, "error"));
> -QDECREF(response);
> -
> -outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
> -
> -qmp_eventwait("DEVICE_DELETED");
> -}
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 2dcdeade2a..9f36ec73ef 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -16,6 +16,9 @@
> #include "hw/pci/pci_regs.h"
> #include "qemu/host-utils.h"
>
> +#define ACPI_PCIHP_ADDR 0xae00
> +#define PCI_EJ_BASE 0x0008
> +
> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> void (*func)(QPCIDevice *dev, int devfn, void
> *data),
> void *data)
> @@ -412,3 +415,23 @@ void qpci_plug_device_test(const char *driver, const
> char *id,
> g_assert(!qdict_haskey(response, "error"));
> QDECREF(response);
> }
> +
> +void qpci_unplug_device_test(const char *id, uint8_t slot)
> +{
> +QDict *response;
> +char *cmd;
> +
> +cmd = g_strdup_printf("{'execute': 'device_del',"
> + " 'arguments': {"
> + " 'id': '%s'"
> + "}}", id);
> +response = qmp(cmd);
> +g_free(cmd);
> +g_assert(response);
> +g_assert(!qdict_haskey(response, "error"));
> +QDECREF(response);
> +
> +outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
> +
> +qmp_eventwait("DEVICE_DELETED");
> +}
No, that's a bad idea. ACPI and that outb() is clearly something
specific to x86, so this should not reside in pci.c but in pci-pc.c
We might be able to unify this - I've had a similar patch here:
https://patchwork.kernel.org/patch/9905031/
... but I think this needs some more careful thinking and discussion, so
I'd suggest that you remove this from your already huge patch series for
now and we fix it later instead.
Thomas
Re: [Qemu-block] [PATCH v7 12/38] libqos: Use explicit QTestState for virtio operations
On 11.09.2017 19:19, Eric Blake wrote: > Now that QVirtioDevice and QVirtQueue point back to QVirtioBus, > we can reuse the explicit QTestState stored there rather than > relying on implicit global_qtest. We also have to pass QTestState > through a few functions that can't trace back through > QVirtioDevice, and update those callers. > > Drop some useless casts while touching things. > > Signed-off-by: Eric Blake > --- > tests/libqos/virtio.h | 6 ++-- > tests/libqos/virtio-mmio.c | 57 ++- > tests/libqos/virtio-pci.c | 8 ++--- > tests/libqos/virtio.c | 84 > ++ > tests/virtio-blk-test.c| 11 +++--- > 5 files changed, 94 insertions(+), 72 deletions(-) Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 13/38] libqos: Use explicit QTestState for fw_cfg operations
On 11.09.2017 19:19, Eric Blake wrote: > Drop one more client of global_qtest by teaching all fw_cfg test > functionality (invoked through alloc-pc) to pass in an explicit > QTestState, adjusting all callers. In particular, fw_cfg-test > had to reorder things to create the test state prior to creating > the fw_cfg (and drop a pointless strdup in the meantime), but that > test now no longer depends on global_qtest. > > Signed-off-by: Eric Blake Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 16/38] libqos: Use explicit QTestState for ahci operations
On 11.09.2017 19:20, Eric Blake wrote: > Drop one more client of global_qtest by teaching all ahci test > functionality to pass in an explicit QTestState. The state was > already available, so no callers had to be adjusted. > > Signed-off-by: Eric Blake Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 26/38] libqtest: Merge qtest_end() into qtest_quit()
On 11.09.2017 19:20, Eric Blake wrote: > Rather than have two similar shutdown functions, where one requires > the use of global_qtest in the header, it is better to have a single > shutdown function that still takes care of cleaning up global_qtest > if it is set. All callers are updated. > > Signed-off-by: Eric Blake Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()
On 11.09.2017 19:20, Eric Blake wrote:
> We have several callers that were formatting the argument strings
> themselves; consolidate this effort by adding new convenience
> functions directly in libqtest, and update all call-sites that
> can benefit from it.
[...]
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index e8c2e11817..b535d7768f 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -245,6 +245,27 @@ QTestState *qtest_start(const char *extra_args)
> return global_qtest = s;
> }
>
> +QTestState *qtest_vstartf(const char *fmt, va_list ap)
> +{
> +char *args = g_strdup_vprintf(fmt, ap);
> +QTestState *s;
> +
> +s = qtest_start(args);
> +global_qtest = NULL;
Don't you need a g_free(args) here?
> +return s;
> +}
> +
> +QTestState *qtest_startf(const char *fmt, ...)
> +{
> +va_list ap;
> +QTestState *s;
> +
> +va_start(ap, fmt);
> +s = qtest_vstartf(fmt, ap);
> +va_end(ap);
> +return s;
> +}
[...]
> diff --git a/tests/e1000-test.c b/tests/e1000-test.c
> index 0c5fcdcc44..12bc526ad6 100644
> --- a/tests/e1000-test.c
> +++ b/tests/e1000-test.c
> @@ -14,16 +14,8 @@
> static void test_device(gconstpointer data)
> {
> const char *model = data;
> -QTestState *s;
> -char *args;
>
> -args = g_strdup_printf("-device %s", model);
> -s = qtest_start(args);
> -
> -if (s) {
> -qtest_quit(s);
> -}
> -g_free(args);
> +qtest_quit(qtest_startf("-device %s", model));
Just my personal taste, but I think I'd be nicer to keep this on
separate lines:
QTestState *s;
s = qtest_startf("-device %s", model);
qtest_quit(s);
> }
[...]
> diff --git a/tests/eepro100-test.c b/tests/eepro100-test.c
> index bdc8a67d57..fc9ea84d66 100644
> --- a/tests/eepro100-test.c
> +++ b/tests/eepro100-test.c
> @@ -13,18 +13,9 @@
> static void test_device(gconstpointer data)
> {
> const char *model = data;
> -QTestState *s;
> -char *args;
> -
> -args = g_strdup_printf("-device %s", model);
> -s = qtest_start(args);
>
> /* Tests only initialization so far. TODO: Implement functional tests */
> -
> -if (s) {
> -qtest_quit(s);
> -}
> -g_free(args);
> +qtest_quit(qtest_startf("-device %s", model));
> }
dito
[...]
> diff --git a/tests/numa-test.c b/tests/numa-test.c
> index fa21d26935..e2f6c68be8 100644
> --- a/tests/numa-test.c
> +++ b/tests/numa-test.c
> @@ -12,20 +12,14 @@
> #include "qemu/osdep.h"
> #include "libqtest.h"
>
> -static char *make_cli(const char *generic_cli, const char *test_cli)
> -{
> -return g_strdup_printf("%s %s", generic_cli ? generic_cli : "",
> test_cli);
> -}
> -
> static void test_mon_explicit(const void *data)
> {
> char *s;
> -char *cli;
> +const char *args = data;
>
> -cli = make_cli(data, "-smp 8 "
> - "-numa node,nodeid=0,cpus=0-3 "
> - "-numa node,nodeid=1,cpus=4-7 ");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 "
> +"-numa node,nodeid=0,cpus=0-3 "
> +"-numa node,nodeid=1,cpus=4-7 ", args);
>
> s = hmp("info numa");
> g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
> @@ -33,16 +27,14 @@ static void test_mon_explicit(const void *data)
> g_free(s);
>
> qtest_quit(global_qtest);
> -g_free(cli);
> }
>
> static void test_mon_default(const void *data)
> {
> char *s;
> -char *cli;
> +const char *args = data;
>
> -cli = make_cli(data, "-smp 8 -numa node -numa node");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 -numa node -numa node", args);
>
> s = hmp("info numa");
> g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
> @@ -50,18 +42,16 @@ static void test_mon_default(const void *data)
> g_free(s);
>
> qtest_quit(global_qtest);
> -g_free(cli);
> }
>
> static void test_mon_partial(const void *data)
> {
> char *s;
> -char *cli;
> +const char *args = data;
>
> -cli = make_cli(data, "-smp 8 "
> - "-numa node,nodeid=0,cpus=0-1 "
> - "-numa node,nodeid=1,cpus=4-5 ");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 "
> +"-numa node,nodeid=0,cpus=0-1 "
> +"-numa node,nodeid=1,cpus=4-5 ", args);
Does GCC emit a warning if you'd used data here directly? Otherwise I
think you could simply do this without the local args variable...
Thomas
Re: [Qemu-block] [PATCH v7 29/38] libqtest: Merge qtest_init() into qtest_start()
On 11.09.2017 19:20, Eric Blake wrote:
> Remove the trivial wrapper qtest_init(), and change qtest_start()
> to no longer implicitly set global_qtest, to make it obvious in the
> rest of the testsuite where we are still relying on global_qtest.
> Everything now uses qtest_start() (and friends) and qtest_quit(),
> and explicitly tracks the QTestState between the two (although in
> many cases, this tracking is still done through global_qtest).
> Doing this makes it easier to see what remaining cleanups will be
> needed if we don't want an implicit dependency on global state.
[...]
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 817e3a5580..2a21bf4605 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -27,7 +27,7 @@ extern QTestState *global_qtest;
> * qtest_start_without_qmp_handshake:
> * @extra_args: other arguments to pass to QEMU.
> *
> - * Returns: #QTestState instance. Does not affect #global_qtest.
> + * Returns: #QTestState instance, handshaking not yet completed.
I'd rather add a description a la:
* Start QEMU, but do not execute the QMP handshake yet.
*
* Returns: #QTestState instance
> */
> QTestState *qtest_start_without_qmp_handshake(const char *extra_args);
>
> @@ -35,10 +35,7 @@ QTestState *qtest_start_without_qmp_handshake(const char
> *extra_args);
> * qtest_start:
> * @args: other arguments to pass to QEMU
> *
> - * Start QEMU and assign the resulting #QTestState to #global_qtest.
> - * The global variable is used by "shortcut" functions documented below.
> *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.
I'd rather change the description instead of removing it:
* Start QEMU and execute the initial QMP handshake
*
* Returns: #QTestState instance.
> */
> QTestState *qtest_start(const char *args);
>
> @@ -47,10 +44,7 @@ QTestState *qtest_start(const char *args);
> * @fmt...: Format for creating other arguments to pass to QEMU, formatted
> * like sprintf().
> *
> - * Start QEMU and return the resulting #QTestState (but unlike qtest_start(),
> - * #global_qtest is left at NULL).
> - *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.
dito
> */
> QTestState *qtest_startf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>
> @@ -60,30 +54,11 @@ QTestState *qtest_startf(const char *fmt, ...)
> GCC_FMT_ATTR(1, 2);
> * like vsprintf().
> * @ap: Format arguments.
> *
> - * Start QEMU and return the resulting #QTestState (but unlike qtest_start(),
> - * #global_qtest is left at NULL).
> - *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.
dito
> */
> QTestState *qtest_vstartf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
>
> /**
> - * qtest_init:
> - * @extra_args: other arguments to pass to QEMU.
> - *
> - * Returns: #QTestState instance. Does not affect #global_qtest.
> - */
> -static inline QTestState *qtest_init(const char *extra_args)
> -{
> -QTestState *s;
> -
> -assert(!global_qtest);
> -s = qtest_start(extra_args);
> -global_qtest = NULL;
> -return s;
> -}
[...]
> diff --git a/tests/display-vga-test.c b/tests/display-vga-test.c
> index 8667330e3c..d638f15ec3 100644
> --- a/tests/display-vga-test.c
> +++ b/tests/display-vga-test.c
> @@ -12,39 +12,33 @@
>
> static void pci_cirrus(void)
> {
> -qtest_start("-vga none -device cirrus-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device cirrus-vga"));
I'd prefer to keep this on separate lines ... but that's again just my
personal taste. (also for the othe changes below)
> }
>
> static void pci_stdvga(void)
> {
> -qtest_start("-vga none -device VGA");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device VGA"));
> }
>
> static void pci_secondary(void)
> {
> -qtest_start("-vga none -device secondary-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device secondary-vga"));
> }
>
> static void pci_multihead(void)
> {
> -qtest_start("-vga none -device VGA -device secondary-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device VGA -device secondary-vga"));
> }
>
> static void pci_virtio_gpu(void)
> {
> -qtest_start("-vga none -device virtio-gpu-pci");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device virtio-gpu-pci"));
> }
>
> #ifdef CONFIG_VIRTIO_VGA
> static void pci_virtio_vga(void)
> {
> -qtest_start("-vga none -device virtio-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device virtio-vga"));
> }
> #endif
[...]
> diff --git a/tests/ne2000-test.c b/tests/ne2000-test.c
> index cae83c5c4c..8e6f7b07c6 100644
> --- a/tests/ne2000-test.c
> +++ b/tests/ne2000-test.c
> @@ -22,7 +22,7 @@ int main(int argc, char **argv)
> g_test_init(&argc, &argv, NULL);
> qt
Re: [Qemu-block] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()
On 11.09.2017 19:20, Eric Blake wrote: > Maintaining two layers of libqtest APIs, one that takes an explicit > QTestState object, and the other that uses the implicit global_qtest, > is annoying. In the interest of getting rid of global implicit > state and having less code to maintain, merge: > qtest_clock_set() > qtest_clock_step() > qtest_clock_step_next() > with their short counterparts. All callers that previously > used the short form now make it explicit that they are relying on > global_qtest, and later patches can then clean things up to remove > the global variable. > > Signed-off-by: Eric Blake > --- > tests/libqtest.h | 50 > tests/libqtest.c | 6 ++-- > tests/e1000e-test.c | 2 +- > tests/fdc-test.c | 4 +-- > tests/ide-test.c | 2 +- > tests/libqos/virtio.c| 8 +++--- > tests/rtc-test.c | 74 > > tests/rtl8139-test.c | 10 +++ > tests/tco-test.c | 22 +++--- > tests/test-arm-mptimer.c | 25 +--- > tests/wdt_ib700-test.c | 12 > 11 files changed, 90 insertions(+), 125 deletions(-) > > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 5651b77d2f..26d5f37bc9 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -417,17 +417,17 @@ void qtest_bufwrite(QTestState *s, uint64_t addr, > void qtest_memset(QTestState *s, uint64_t addr, uint8_t patt, size_t size); > > /** > - * qtest_clock_step_next: > + * clock_step_next: > * @s: #QTestState instance to operate on. > * > * Advance the QEMU_CLOCK_VIRTUAL to the next deadline. > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_step_next(QTestState *s); > +int64_t clock_step_next(QTestState *s); > > /** > - * qtest_clock_step: > + * clock_step: > * @s: QTestState instance to operate on. > * @step: Number of nanoseconds to advance the clock by. > * > @@ -435,10 +435,10 @@ int64_t qtest_clock_step_next(QTestState *s); > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_step(QTestState *s, int64_t step); > +int64_t clock_step(QTestState *s, int64_t step); > > /** > - * qtest_clock_set: > + * clock_set: > * @s: QTestState instance to operate on. > * @val: Nanoseconds value to advance the clock to. > * > @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step); > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_set(QTestState *s, int64_t val); > +int64_t clock_set(QTestState *s, int64_t val); Could we please keep the "qtest" prefix here and rather get rid of the other ones? Even if it's more to type, I prefer to have a proper prefix here so that it is clear at the first sight that the functions belong to the qtest framework. Thomas
Re: [Qemu-block] [PATCH v7 32/38] libqtest: Merge qtest_irq*() with irq*()
On 11.09.2017 19:20, Eric Blake wrote: > Maintaining two layers of libqtest APIs, one that takes an explicit > QTestState object, and the other that uses the implicit global_qtest, > is annoying. In the interest of getting rid of global implicit > state and having less code to maintain, merge: > qtest_get_irq() > qtest_irq_intercept_in() > qtest_irq_intercept_out() > with their short counterparts. All callers that previously > used the short form now make it explicit that they are relying on > global_qtest, and later patches can then clean things up to remove > the global variable. > > Signed-off-by: Eric Blake > --- > tests/libqtest.h | 47 ++- > tests/libqtest.c | 6 +++--- > tests/fdc-test.c | 48 > > tests/ide-test.c | 17 + > tests/ipmi-bt-test.c | 6 +++--- > tests/ipmi-kcs-test.c| 8 > tests/libqos/libqos-pc.c | 2 +- > tests/rtc-test.c | 10 +- > tests/tco-test.c | 2 +- > tests/wdt_ib700-test.c | 8 > 10 files changed, 60 insertions(+), 94 deletions(-) > > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 26d5f37bc9..8398c0fd07 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -176,33 +176,33 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...) > GCC_FMT_ATTR(2, 3); > char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap); > > /** > - * qtest_get_irq: > + * get_irq: > * @s: #QTestState instance to operate on. > * @num: Interrupt to observe. > * > * Returns: The level of the @num interrupt. > */ > -bool qtest_get_irq(QTestState *s, int num); > +bool get_irq(QTestState *s, int num); > > /** > - * qtest_irq_intercept_in: > + * irq_intercept_in: > * @s: #QTestState instance to operate on. > * @string: QOM path of a device. > * > * Associate qtest irqs with the GPIO-in pins of the device > * whose path is specified by @string. > */ > -void qtest_irq_intercept_in(QTestState *s, const char *string); > +void irq_intercept_in(QTestState *s, const char *string); > > /** > - * qtest_irq_intercept_out: > + * irq_intercept_out: > * @s: #QTestState instance to operate on. > * @string: QOM path of a device. > * > * Associate qtest irqs with the GPIO-out pins of the device > * whose path is specified by @string. > */ > -void qtest_irq_intercept_out(QTestState *s, const char *string); > +void irq_intercept_out(QTestState *s, const char *string); Could we please also keep the qtest prefix here? Thomas
Re: [Qemu-block] [PATCH v7 33/38] libqtest: Merge qtest_{in, out}[bwl]() with {in, out}[bwl]()
On 11.09.2017 19:20, Eric Blake wrote: > Maintaining two layers of libqtest APIs, one that takes an explicit > QTestState object, and the other that uses the implicit global_qtest, > is annoying. In the interest of getting rid of global implicit > state and having less code to maintain, merge: > qtest_outb() > qtest_outw() > qtest_outl() > qtest_inb() > qtest_inw() > qtest_inl() > with their short counterparts. All callers that previously > used the short form now make it explicit that they are relying on > global_qtest, and later patches can then clean things up to remove > the global variable. > > Signed-off-by: Eric Blake > --- > tests/libqtest.h| 99 > ++--- > tests/multiboot/libc.h | 2 +- > tests/libqtest.c| 14 +++ > tests/boot-order-test.c | 4 +- > tests/endianness-test.c | 12 +++--- > tests/fdc-test.c| 77 -- > tests/hd-geo-test.c | 4 +- > tests/ipmi-bt-test.c| 12 +++--- > tests/ipmi-kcs-test.c | 8 ++-- > tests/libqos/fw_cfg.c | 4 +- > tests/libqos/pci-pc.c | 44 +++--- > tests/libqos/pci.c | 2 +- > tests/m48t59-test.c | 8 ++-- > tests/multiboot/libc.c | 2 +- > tests/pvpanic-test.c| 4 +- > tests/rtc-test.c| 8 ++-- > tests/wdt_ib700-test.c | 8 ++-- > 17 files changed, 120 insertions(+), 192 deletions(-) > > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 8398c0fd07..520f745e7b 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -205,61 +205,61 @@ void irq_intercept_in(QTestState *s, const char > *string); > void irq_intercept_out(QTestState *s, const char *string); > > /** > - * qtest_outb: > + * outb: > * @s: #QTestState instance to operate on. > * @addr: I/O port to write to. > * @value: Value being written. > * > * Write an 8-bit value to an I/O port. > */ > -void qtest_outb(QTestState *s, uint16_t addr, uint8_t value); > +void outb(QTestState *s, uint16_t addr, uint8_t value); Could we please also keep the qtest prefix here? ... same applies for all your other following "Merge ..." patches in this series... Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/10] cleanup qemu-iotests
On 12.09.2017 16:44, Paolo Bonzini wrote: > The purpose of this series is to separate the "check" sources from > the tests. After these patches, common.config is reduced to simple > shell initialization, and common.rc is only included by the tests. > > Along the way, a lot of dead code is removed too. > > In v2, the following patches: > > qemu-iotests: do not do useless search for QEMU_*_PROG > qemu-iotests: do not search for binaries in the current directory > qemu-iotests: include common.env and common.config early > > have been replaced by "qemu-iotests: cleanup and fix search for programs", > which also preserves the behavior of searching for programs as symlinks > in the current directory. > > Paolo > > Paolo Bonzini (10): > qemu-iotests: remove dead code > qemu-iotests: get rid of AWK_PROG > qemu-iotests: move "check" code out of common.rc > qemu-iotests: cleanup and fix search for programs > qemu-iotests: limit non-_PROG-suffixed variables to common.rc > qemu-iotests: do not include common.rc in "check" > qemu-iotests: disintegrate more parts of common.config > qemu-iotests: fix uninitialized variable > qemu-iotests: get rid of $iam > qemu-iotests: merge "check" and "common" > > tests/qemu-iotests/039.out | 10 +- > tests/qemu-iotests/061.out | 4 +- > tests/qemu-iotests/137.out | 2 +- > tests/qemu-iotests/check | 575 > ++- > tests/qemu-iotests/common| 459 --- > tests/qemu-iotests/common.config | 206 +- > tests/qemu-iotests/common.qemu | 1 + > tests/qemu-iotests/common.rc | 205 +++--- Meta comment: Could we maybe also rename "tests/qemu-iotests" to "tests/iotests" ? The "qemu" prefix sounds always very superfluous to me here... Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v7 09/38] libqos: Track QTestState with QVirtioBus
On 12.09.2017 15:28, Eric Blake wrote: > On 09/12/2017 02:21 AM, Thomas Huth wrote: [...] >> I fail to see why we need a separate bus object here for each device. >> The bus is only available one time, not multiple times, isn't it? So >> there should also only be one bus object floating around, not multiple >> ones... or do I miss something? > > You are right that there is only one bus for the machine - the problem > is that the object representing the bus is dynamically created on the > fly at the point where the device is first grabbed, rather than up front > as part of creating the machine (in other words, the device search is > not performed on a pre-existing bus object). > > Contrast qpci_device_find() (lookup based on a pre-existing bus) with > qvirtio_mmio_init_device() (lookup that did not use a pre-existing bus); > also note that qvirtio_pci_device_find() takes a pre-existing QPCIBus > (since on that architecture, the QVirtioBus is itself a device on the > QPCIBus). > > I suppose that we could indeed refactor the code to require callers to > create the QVirtioBus object up front, and then make the device lookup > (both qvirtio_mmio_init_device() and qvirtio_pci_device_find()) take a > QVirtioBus parameter instead of QTestState or QPCIBus. That's slightly > more work, but I'm happy to attempt it if you think it will be better. At least to me, that sounds like the right way to go. I guess we might run into other problems later if we have multiple instances of the bus object floating around ... so sorry if this is extra work, but I'd say let's better do it properly now instead of having to rework this again later. Thomas signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v7 10/38] libqos: Move/rename qpci_unplug_acpi_device_test() to pci.c
On 12.09.2017 15:28, Eric Blake wrote: > On 09/12/2017 02:29 AM, Thomas Huth wrote: >> On 11.09.2017 19:19, Eric Blake wrote: >>> Commit 2f8b2767 originally added qpci_plug_device_test() and >>> qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c. >>> Later, commit cf716b31 moved one half of the pair to pci.c >>> when adding PPC64 support. Keep the implementations of the >>> two functions together, and shorten the name to >>> qpci_unplug_device_test(), since all callers use the two >>> functions in tandem. >>> > >> >> No, that's a bad idea. ACPI and that outb() is clearly something >> specific to x86, so this should not reside in pci.c but in pci-pc.c >> >> We might be able to unify this - I've had a similar patch here: >> >> https://patchwork.kernel.org/patch/9905031/ >> >> ... but I think this needs some more careful thinking and discussion, so >> I'd suggest that you remove this from your already huge patch series for >> now and we fix it later instead. > > Okay, I'm fine dropping this patch, and can base my respin on top of > your cleanup instead. Note that my patches are currently on halt since I'm waiting for your qemu_startf() reworks to get included first :-) ... so feel free to pick up ideas or patches from my series into your series if that helps. Thomas signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()
On 12.09.2017 15:32, Eric Blake wrote:
> On 09/12/2017 05:14 AM, Thomas Huth wrote:
>> On 11.09.2017 19:20, Eric Blake wrote:
>>> We have several callers that were formatting the argument strings
>>> themselves; consolidate this effort by adding new convenience
>>> functions directly in libqtest, and update all call-sites that
>>> can benefit from it.
[...]
>>> static void test_mon_partial(const void *data)
>>> {
>>> char *s;
>>> -char *cli;
>>> +const char *args = data;
>>>
>>> -cli = make_cli(data, "-smp 8 "
>>> - "-numa node,nodeid=0,cpus=0-1 "
>>> - "-numa node,nodeid=1,cpus=4-5 ");
>>> -qtest_start(cli);
>>> +global_qtest = qtest_startf("%s -smp 8 "
>>> +"-numa node,nodeid=0,cpus=0-1 "
>>> +"-numa node,nodeid=1,cpus=4-5 ", args);
>>
>> Does GCC emit a warning if you'd used data here directly? Otherwise I
>> think you could simply do this without the local args variable...
>
> Passing void* through varargs, with the intent of the receiver parsing
> it as char*, is technically undefined in C. I don't know if gcc warns,
> but I'm also worried that clang might warn. I prefer to err on the side
> of defined behavior in this case, even though it annoyingly requires a
> temporary variable.
OK, sounds reasonable, so let's keep it!
Thomas
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v2 3/3] iotests: use virtio aliases for 067
On 13.09.2017 11:10, Cornelia Huck wrote:
> The default cpu model on s390x does not provide zPCI, which is
> not yet wired up on tcg. Moreover, virtio-ccw is the standard
> on s390x.
>
> Using virtio-scsi will implicitly pick the right device, so just
> switch to that for simplicity.
>
> Signed-off-by: Cornelia Huck
> ---
> tests/qemu-iotests/067 | 3 ++-
> tests/qemu-iotests/067.out | 2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
> index 5d4ca4bc61..cbb3da286a 100755
> --- a/tests/qemu-iotests/067
> +++ b/tests/qemu-iotests/067
> @@ -141,7 +141,7 @@ echo
> echo === Empty drive with -device and device_del ===
> echo
>
> -run_qemu -device virtio-scsi-pci -device scsi-cd,id=cd0 < +run_qemu -device virtio-scsi -device scsi-cd,id=cd0 < { "execute": "qmp_capabilities" }
> { "execute": "query-block" }
> { "execute": "device_del", "arguments": { "id": "cd0" } }
> @@ -150,6 +150,7 @@ run_qemu -device virtio-scsi-pci -device scsi-cd,id=cd0
> < { "execute": "quit" }
> EOF
>
> +
Superfluous white space change?
With that nit removed:
Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v2 2/3] iotests: use -ccw on s390x for 051
On 13.09.2017 11:10, Cornelia Huck wrote: > The default cpu model on s390x does not provide zPCI, which is > not yet wired up on tcg. Moreover, virtio-ccw is the standard > on s390x, so use the -ccw instead of the -pci versions of virtio > devices on s390x. > > Signed-off-by: Cornelia Huck > --- > tests/qemu-iotests/051| 12 +++- > tests/qemu-iotests/051.out| 2 +- > tests/qemu-iotests/051.pc.out | 2 +- > 3 files changed, 13 insertions(+), 3 deletions(-) Reviewed-by: Thomas Huth
[Qemu-block] [PATCH] block: Clean up some bad code in the vvfat driver
Remove the unnecessary home-grown redefinition of the assert() macro here,
and remove the unusable debug code at the end of the checkpoint() function.
The code there uses assert() with side-effects (assignment to the "mapping"
variable), which should be avoided. Looking more closely, it seems as it is
apparently also only usable for one certain directory layout (with a file
named USB.H in it) and thus is of no use for the rest of the world.
Signed-off-by: Thomas Huth
---
block/vvfat.c | 26 ++
1 file changed, 2 insertions(+), 24 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index c54fa94..777a8cd 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -56,15 +56,6 @@
static void checkpoint(void);
-#ifdef __MINGW32__
-void nonono(const char* file, int line, const char* msg) {
-fprintf(stderr, "Nonono! %s:%d %s\n", file, line, msg);
-exit(-5);
-}
-#undef assert
-#define assert(a) do {if (!(a)) nonono(__FILE__, __LINE__, #a);}while(0)
-#endif
-
#else
#define DLOG(a)
@@ -3269,24 +3260,11 @@ static void bdrv_vvfat_init(void)
block_init(bdrv_vvfat_init);
#ifdef DEBUG
-static void checkpoint(void) {
+static void checkpoint(void)
+{
assert(((mapping_t*)array_get(&(vvv->mapping), 0))->end == 2);
check1(vvv);
check2(vvv);
assert(!vvv->current_mapping || vvv->current_fd ||
(vvv->current_mapping->mode & MODE_DIRECTORY));
-#if 0
-if (((direntry_t*)vvv->directory.pointer)[1].attributes != 0xf)
-fprintf(stderr, "Nonono!\n");
-mapping_t* mapping;
-direntry_t* direntry;
-assert(vvv->mapping.size >= vvv->mapping.item_size * vvv->mapping.next);
-assert(vvv->directory.size >= vvv->directory.item_size *
vvv->directory.next);
-if (vvv->mapping.next<47)
-return;
-assert((mapping = array_get(&(vvv->mapping), 47)));
-assert(mapping->dir_index < vvv->directory.next);
-direntry = array_get(&(vvv->directory), mapping->dir_index);
-assert(!memcmp(direntry->name, "USB H ", 11) || direntry->name[0]==0);
-#endif
}
#endif
--
1.8.3.1
Re: [Qemu-block] [Qemu-devel] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()
On 12.09.2017 15:35, Eric Blake wrote: > On 09/12/2017 05:45 AM, Thomas Huth wrote: >> On 11.09.2017 19:20, Eric Blake wrote: >>> Maintaining two layers of libqtest APIs, one that takes an explicit >>> QTestState object, and the other that uses the implicit global_qtest, >>> is annoying. In the interest of getting rid of global implicit >>> state and having less code to maintain, merge: >>> qtest_clock_set() >>> qtest_clock_step() >>> qtest_clock_step_next() >>> with their short counterparts. All callers that previously >>> used the short form now make it explicit that they are relying on >>> global_qtest, and later patches can then clean things up to remove >>> the global variable. >>> > >>> @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step); >>> * >>> * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. >>> */ >>> -int64_t qtest_clock_set(QTestState *s, int64_t val); >>> +int64_t clock_set(QTestState *s, int64_t val); >> Could we please keep the "qtest" prefix here and rather get rid of the >> other ones? Even if it's more to type, I prefer to have a proper prefix >> here so that it is clear at the first sight that the functions belong to >> the qtest framework. > > I suppose we can, although it makes more lines that are likely to bump > up against 80 columns, and thus slightly more churn to reformat things > to keep checkpatch happy. I like the shorter name, because less typing > is easier to remember. I'd prefer a second opinion on naming before > doing anything about it though - Markus or Paolo, do you have any > preference? IMHO you should at least keep the qtest prefix in patch 33/38 to avoid confusion with the system definitions that have the same names (see "man 2 outb" for example). Thomas signature.asc Description: OpenPGP digital signature
[Qemu-block] Dead code in cpu-models.h (was: block: Clean up some bad code in the vvfat driver)
On 19.09.2017 10:06, Paolo Bonzini wrote: > On 13/09/2017 21:08, John Snow wrote: [...] >> Farewell, bitrot code. >> >> Reviewed-by: John Snow >> >> Out of curiosity, I wonder ... >> >> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l >> 320 > > $ git grep -c '#if 0' | sort -k2 --field-separator=: -n > ... > hw/net/eepro100.c:21 > target/ppc/cpu-models.h:76 > > whoa :) Igor recently already removed the dead definitions from cpu-models.c : https://git.qemu.org/?p=qemu.git;a=commitdiff;h=aef779605779579afbafff I guess we could now remove the corresponding dead definitions from the header, too... Any volunteers? Thomas
Re: [Qemu-block] [Qemu-devel] Dead code in cpu-models.h
On 19.09.2017 20:54, John Snow wrote: > > > On 09/19/2017 04:14 AM, Thomas Huth wrote: >> On 19.09.2017 10:06, Paolo Bonzini wrote: >>> On 13/09/2017 21:08, John Snow wrote: >> [...] >>>> Farewell, bitrot code. >>>> >>>> Reviewed-by: John Snow >>>> >>>> Out of curiosity, I wonder ... >>>> >>>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l >>>> 320 >>> >>> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n >>> ... >>> hw/net/eepro100.c:21 >>> target/ppc/cpu-models.h:76 >>> >>> whoa :) >> >> Igor recently already removed the dead definitions from cpu-models.c : >> >> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=aef779605779579afbafff >> >> I guess we could now remove the corresponding dead definitions from the >> header, too... >> >> Any volunteers? >> >> Thomas > > Well, I can just take a chainsaw to it blindly if you want to critique it. > I think it should at least be aligned with the changes that have been done to cpu-models.c (e.g. the definition for CPU_POWERPC_401B3 has been removed from the .c file, so the CPU_POWERPC_401B3 could be removed from the .h file, too. Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH] block: Clean up some bad code in the vvfat driver
On 19.09.2017 21:01, John Snow wrote: > > > On 09/19/2017 04:06 AM, Paolo Bonzini wrote: >> On 13/09/2017 21:08, John Snow wrote: >>> >>> >>> On 09/13/2017 06:21 AM, Thomas Huth wrote: >>>> Remove the unnecessary home-grown redefinition of the assert() macro here, >>>> and remove the unusable debug code at the end of the checkpoint() function. >>>> The code there uses assert() with side-effects (assignment to the "mapping" >>>> variable), which should be avoided. Looking more closely, it seems as it is >>>> apparently also only usable for one certain directory layout (with a file >>>> named USB.H in it) and thus is of no use for the rest of the world. >>>> >>>> Signed-off-by: Thomas Huth >>> >>> Farewell, bitrot code. >>> >>> Reviewed-by: John Snow >>> >>> Out of curiosity, I wonder ... >>> >>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l >>> 320 >> >> >> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n >> ... >> hw/net/eepro100.c:21 >> target/ppc/cpu-models.h:76 >> >> whoa :) >> > > Wonder if '#if 0' should be against the style guide / in checkpatch. checkpatch already complains if you have a "#if 0" in your patch, so I think we should be pretty fine here already - but of course you can still add a paragraph to the CODING_STYLE if you like. Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v1 3/8] hw: Replace fprintf(stderr, "*\n" with error_report()
On 26.09.2017 02:08, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr,
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> Signed-off-by: Alistair Francis
> Cc: Andrzej Zaborowski
> Cc: Jan Kiszka
> Cc: Stefan Hajnoczi
> Cc: Paolo Bonzini
> Cc: Thomas Huth
> Cc: Gerd Hoffmann
> Cc: "Michael S. Tsirkin"
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: John Snow
> Cc: Christian Borntraeger
> Cc: Cornelia Huck
> Cc: Alexander Graf
> Cc: Michael Walle
> Cc: Paul Burton
> Cc: Aurelien Jarno
> Cc: Yongbok Kim
> Cc: "Hervé Poussineau"
> Cc: Anthony Green
> Cc: Jason Wang
> Cc: Chris Wulff
> Cc: Marek Vasut
> Cc: Jia Liu
> Cc: Stafford Horne
> Cc: Marcel Apfelbaum
> Cc: Magnus Damm
> Cc: Fabien Chouteau
> Cc: Mark Cave-Ayland
> Cc: Artyom Tarasenko
> Cc: qemu-...@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-...@nongnu.org
> ---
>
> hw/arm/armv7m.c | 2 +-
> hw/arm/boot.c | 34 +--
> hw/arm/gumstix.c| 13
> hw/arm/mainstone.c | 7 ++--
> hw/arm/musicpal.c | 2 +-
> hw/arm/omap1.c | 5 +--
> hw/arm/omap2.c | 21 ++--
> hw/arm/omap_sx1.c | 6 ++--
> hw/arm/palm.c | 10 +++---
> hw/arm/pxa2xx.c | 7 ++--
> hw/arm/stellaris.c | 3 +-
> hw/arm/tosa.c | 17 +-
> hw/arm/versatilepb.c| 2 +-
> hw/arm/vexpress.c | 8 ++---
> hw/arm/z2.c | 6 ++--
> hw/block/dataplane/virtio-blk.c | 6 ++--
> hw/block/onenand.c | 8 ++---
> hw/block/tc58128.c | 44 -
> hw/bt/core.c| 15 +
> hw/bt/hci-csr.c | 17 +-
> hw/bt/hci.c | 30 -
> hw/bt/hid.c | 2 +-
> hw/bt/l2cap.c | 47 ++-
> hw/bt/sdp.c | 7 ++--
> hw/char/exynos4210_uart.c | 6 ++--
> hw/char/mcf_uart.c | 5 +--
> hw/char/sh_serial.c | 9 +++---
> hw/core/loader.c| 31 +-
> hw/core/ptimer.c| 7 ++--
> hw/cris/axis_dev88.c| 3 +-
> hw/cris/boot.c | 5 +--
> hw/display/blizzard.c | 20 ++--
> hw/display/omap_dss.c | 14
> hw/display/pl110.c | 2 +-
[Qemu-block] [PATCH] hw/block/onenand: Remove dead code block
The condition of the for-loop makes sure that b is always smaller
than s->blocks, so the "if (b >= s->blocks)" statement is completely
superfluous here.
Buglink: https://bugs.launchpad.net/qemu/+bug/1715007
Signed-off-by: Thomas Huth
---
hw/block/onenand.c | 4
1 file changed, 4 deletions(-)
diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 30e40f3..de65c9e 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -520,10 +520,6 @@ static void onenand_command(OneNANDState *s)
s->intstatus |= ONEN_INT;
for (b = 0; b < s->blocks; b ++) {
-if (b >= s->blocks) {
-s->status |= ONEN_ERR_CMD;
-break;
-}
if (s->blockwp[b] == ONEN_LOCK_LOCKTIGHTEN)
break;
--
1.8.3.1
Re: [Qemu-block] [PATCH 61/88] tests: use g_new() family of functions
On 07.10.2017 01:49, Philippe Mathieu-Daudé wrote: > From: Marc-André Lureau > > Signed-off-by: Marc-André Lureau > Signed-off-by: Philippe Mathieu-Daudé > [PMD: split of some files in other commits of the same series, add libqtest.c] > --- > tests/ahci-test.c | 4 ++-- > tests/fw_cfg-test.c | 4 ++-- > tests/libqos/ahci.c | 2 +- > tests/libqos/libqos.c | 2 +- > tests/libqos/malloc.c | 6 +++--- > tests/libqtest.c | 2 +- > tests/pc-cpu-test.c | 2 +- > 7 files changed, 11 insertions(+), 11 deletions(-) Reviewed-by: Thomas Huth
Re: [Qemu-block] [Qemu-devel] [PATCH v3 42/46] util: Replace fprintf(stderr, "*\n" with error_report()
On 19.10.2017 18:18, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
[...]
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 5946ac09f0..29fff51fcf 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -15,6 +15,7 @@
>
> #include "qemu/osdep.h"
> #include "qemu-common.h"
> +#include "qemu/error-report.h"
> #include "block/block.h"
> #include "qemu/rcu_queue.h"
> #include "qemu/sockets.h"
> @@ -703,8 +704,8 @@ void aio_context_setup(AioContext *ctx)
> {
> /* TODO remove this in final patch submission */
> if (getenv("QEMU_AIO_POLL_MAX_NS")) {
> -fprintf(stderr, "The QEMU_AIO_POLL_MAX_NS environment variable has "
> -"been replaced with -object iothread,poll-max-ns=NUM\n");
> +error_report("The QEMU_AIO_POLL_MAX_NS environment variable has "
> +"been replaced with -object iothread,poll-max-ns=NUM");
> exit(1);
> }
The comment in front of this code block indicates that this should
rather be removed completely. Stefan, do you agree?
> diff --git a/util/error.c b/util/error.c
> index 3efdd69162..e423368ca0 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -32,7 +32,7 @@ Error *error_fatal;
> static void error_handle_fatal(Error **errp, Error *err)
> {
> if (errp == &error_abort) {
> -fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> +error_report("Unexpected error in %s() at %s:%d:",
> err->func, err->src, err->line);
Indentation is still wrong if the statement spans more than one line :-(
Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v3 42/46] util: Replace fprintf(stderr, "*\n" with error_report()
On 19.10.2017 21:47, Stefan Weil wrote:
> Am 19.10.2017 um 19:53 schrieb Thomas Huth:
>> On 19.10.2017 18:18, Alistair Francis wrote:
>>> Replace a large number of the fprintf(stderr, "*\n" calls with
>>> error_report(). The functions were renamed with these commands and then
>>> compiler issues where manually fixed.
>> [...]
>>> diff --git a/util/aio-posix.c b/util/aio-posix.c
>>> index 5946ac09f0..29fff51fcf 100644
>>> --- a/util/aio-posix.c
>>> +++ b/util/aio-posix.c
>>> @@ -15,6 +15,7 @@
>>>
>>> #include "qemu/osdep.h"
>>> #include "qemu-common.h"
>>> +#include "qemu/error-report.h"
>>> #include "block/block.h"
>>> #include "qemu/rcu_queue.h"
>>> #include "qemu/sockets.h"
>>> @@ -703,8 +704,8 @@ void aio_context_setup(AioContext *ctx)
>>> {
>>> /* TODO remove this in final patch submission */
>>> if (getenv("QEMU_AIO_POLL_MAX_NS")) {
>>> -fprintf(stderr, "The QEMU_AIO_POLL_MAX_NS environment variable has
>>> "
>>> -"been replaced with -object iothread,poll-max-ns=NUM\n");
>>> +error_report("The QEMU_AIO_POLL_MAX_NS environment variable has "
>>> +"been replaced with -object iothread,poll-max-ns=NUM");
>>> exit(1);
>>> }
>>
>> The comment in front of this code block indicates that this should
>> rather be removed completely. Stefan, do you agree?
>
> I assume you asked the other Stefan, but I think he'll agree as I do,
Right, the code has been introduced by this commit:
commit 4a1cba3802554a3b077d436002519ff1fb0c18bf
Author: Stefan Hajnoczi
Date: Thu Dec 1 19:26:42 2016 +
aio: add polling mode to AioContext
and apparently the environment variable has never been used in the
committed code base, so removing this code block sounds like the right
way to go.
Thomas
[Qemu-block] [PATCH] hw/ide/ahci: Move allwinner code into a separate file
The allwinner code is only needed for the allwinner board (for which
we also have a separate CONFIG_ALLWINNER_A10 config switch), so it
does not make sense that we compile this for all the other boards
that need AHCI, too. Let's move it to a separate file that is only
compiled when CONFIG_ALLWINNER_A10 is set.
Signed-off-by: Thomas Huth
---
hw/ide/Makefile.objs| 1 +
hw/ide/ahci-allwinner.c | 127
hw/ide/ahci.c | 95
3 files changed, 128 insertions(+), 95 deletions(-)
create mode 100644 hw/ide/ahci-allwinner.c
diff --git a/hw/ide/Makefile.objs b/hw/ide/Makefile.objs
index 729e9bd..f0edca3 100644
--- a/hw/ide/Makefile.objs
+++ b/hw/ide/Makefile.objs
@@ -10,3 +10,4 @@ common-obj-$(CONFIG_IDE_VIA) += via.o
common-obj-$(CONFIG_MICRODRIVE) += microdrive.o
common-obj-$(CONFIG_AHCI) += ahci.o
common-obj-$(CONFIG_AHCI) += ich.o
+common-obj-$(CONFIG_ALLWINNER_A10) += ahci-allwinner.o
diff --git a/hw/ide/ahci-allwinner.c b/hw/ide/ahci-allwinner.c
new file mode 100644
index 000..c3f1604
--- /dev/null
+++ b/hw/ide/ahci-allwinner.c
@@ -0,0 +1,127 @@
+/*
+ * QEMU Allwinner AHCI Emulation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/hw.h"
+#include "qemu/error-report.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/dma.h"
+#include "hw/ide/internal.h"
+#include "hw/ide/ahci_internal.h"
+
+#include "trace.h"
+
+#define ALLWINNER_AHCI_BISTAFR((0xa0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_BISTCR ((0xa4 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_BISTFCTR ((0xa8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_BISTSR ((0xac - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_BISTDECR ((0xb0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_DIAGNR0((0xb4 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_DIAGNR1((0xb8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_OOBR ((0xbc - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_PHYCS0R((0xc0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_PHYCS1R((0xc4 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_PHYCS2R((0xc8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_TIMER1MS ((0xe0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_GPARAM1R ((0xe8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_GPARAM2R ((0xec - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_PPARAMR((0xf0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_TESTR ((0xf4 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_VERSIONR ((0xf8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_IDR((0xfc - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_RWCR ((0xfc - ALLWINNER_AHCI_MMIO_OFF) / 4)
+
+static uint64_t allwinner_ahci_mem_read(void *opaque, hwaddr addr,
+unsigned size)
+{
+AllwinnerAHCIState *a = opaque;
+AHCIState *s = &(SYSBUS_AHCI(a)->ahci);
+uint64_t val = a->regs[addr / 4];
+
+switch (addr / 4) {
+case ALLWINNER_AHCI_PHYCS0R:
+val |= 0x2 << 28;
+break;
+case ALLWINNER_AHCI_PHYCS2R:
+val &= ~(0x1 << 24);
+break;
+}
+trace_allwinner_ahci_mem_read(s, a, addr, val, size);
+return val;
+}
+
+static void allwinner_ahci_mem_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+AllwinnerAHCIState *a = opaque;
+AHCIState *s = &(SYSBUS_AHCI(a)->ahci);
+
+trace_allwinner_ahci_mem_write(s, a, addr, val, size);
+a->regs[addr / 4] = val;
+}
+
+static const MemoryRegionOps allwinner_ahci_mem_ops = {
+.read = allwinner_ahci_mem_read,
+.write = allwinner_ahci_mem_write,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void allwinner_ahci_init(Object *obj)
+{
+SysbusAHCIState *s = SYSBUS_AHCI(obj);
+AllwinnerAHCIState *a = ALLWINNER_AHCI(obj);
+
+memory_region_init_io(&a->mmio, OBJECT(obj), &allwinner_ahci_mem_ops, a,
+ "allwinne
Re: [Qemu-block] [Qemu-devel] qemu-img crash when resize a qcow2 file created with preallocation=full/falloc .
On 24.10.2017 05:28, Changlimin wrote: > Hi, > I am glad to see that qcow2 file created with preallocation=full/falloc can > be resized. But when I test it, qemu-img crashs. > qemu-img: block/qcow2-refcount.c:530: qcow2_refcount_area: Assertion > `!(start_offset % s->cluster_size)' failed. > > These are commands: > qemu-img create -f qcow2 -o preallocation=full full.img 1G > qemu-img resize --preallocation=full full.img +1G Which version of qemu-img are you using? v2.10 or the latest git master branch? Thomas
Re: [Qemu-block] [Qemu-devel] [Bug 1728615] [NEW] qemu-io crashes with SIGABRT and Assertion `c->entries[i].offset != 0' failed
On 30.10.2017 15:43, R.Nageswara Sastry wrote: > Public bug reported: > > git is at HEAD a93ece47fd9edbd4558db24300056c9a57d3bcd4 > This is on ppc64le architecture. > > Re-production steps: > > 1. Copy the attached files named backing_img.file and test.img to a directory > 2. And customize the following command to point to the above directory and > run the same. > /usr/bin/qemu-io /test.img -c "write 1352192 1707520" > > 3.Output of the above command. > qemu-io: block/qcow2-cache.c:411: qcow2_cache_entry_mark_dirty: Assertion > `c->entries[i].offset != 0' failed. > Aborted (core dumped) > > from gdb: > (gdb) bt > #0 0x3fff833eeff0 in raise () from /lib64/libc.so.6 > #1 0x3fff833f136c in abort () from /lib64/libc.so.6 > #2 0x3fff833e4c44 in __assert_fail_base () from /lib64/libc.so.6 > #3 0x3fff833e4d34 in __assert_fail () from /lib64/libc.so.6 > #4 0x1006a594 in qcow2_cache_entry_mark_dirty (bs=0x2e886f60, > c=0x2e879700, table=0x3fff8120) at block/qcow2-cache.c:411 > #5 0x10056154 in alloc_refcount_block (bs=0x2e886f60, > cluster_index=2, refcount_block=0x3fff80cff808) at block/qcow2-refcount.c:417 > #6 0x10057520 in update_refcount (bs=0x2e886f60, offset=1048576, > length=524288, addend=1, decrease=false, type=QCOW2_DISCARD_NEVER) > at block/qcow2-refcount.c:834 > #7 0x10057dc8 in qcow2_alloc_clusters_at (bs=0x2e886f60, > offset=1048576, nb_clusters=1) at block/qcow2-refcount.c:1032 > #8 0x100636d8 in do_alloc_cluster_offset (bs=0x2e886f60, > guest_offset=2097152, host_offset=0x3fff80cff9e0, nb_clusters=0x3fff80cff9d8) > at block/qcow2-cluster.c:1221 > #9 0x10063afc in handle_alloc (bs=0x2e886f60, guest_offset=2097152, > host_offset=0x3fff80cffab0, bytes=0x3fff80cffab8, m=0x3fff80cffb60) > at block/qcow2-cluster.c:1324 > #10 0x10064178 in qcow2_alloc_cluster_offset (bs=0x2e886f60, > offset=1572864, bytes=0x3fff80cffb4c, host_offset=0x3fff80cffb58, > m=0x3fff80cffb60) > at block/qcow2-cluster.c:1511 > #11 0x1004d3f4 in qcow2_co_pwritev (bs=0x2e886f60, offset=1572864, > bytes=1486848, qiov=0x3fffc85f0bf0, flags=0) at block/qcow2.c:1919 > #12 0x100a9648 in bdrv_driver_pwritev (bs=0x2e886f60, offset=1352192, > bytes=1707520, qiov=0x3fffc85f0bf0, flags=16) at block/io.c:898 > #13 0x100ab630 in bdrv_aligned_pwritev (child=0x2e8927f0, > req=0x3fff80cffdd8, offset=1352192, bytes=1707520, align=1, > qiov=0x3fffc85f0bf0, flags=16) > at block/io.c:1440 > #14 0x100ac4ac in bdrv_co_pwritev (child=0x2e8927f0, offset=1352192, > bytes=1707520, qiov=0x3fffc85f0bf0, flags=BDRV_REQ_FUA) at block/io.c:1691 > #15 0x1008da0c in blk_co_pwritev (blk=0x2e879410, offset=1352192, > bytes=1707520, qiov=0x3fffc85f0bf0, flags=BDRV_REQ_FUA) at > block/block-backend.c:1085 > #16 0x1008db68 in blk_write_entry (opaque=0x3fffc85f0c08) at > block/block-backend.c:1110 > #17 0x101aa444 in coroutine_trampoline (i0=780753552, i1=0) at > util/coroutine-ucontext.c:79 > #18 0x3fff83402b9c in makecontext () from /lib64/libc.so.6 > #19 0x in ?? () > (gdb) bt full > #0 0x3fff833eeff0 in raise () from /lib64/libc.so.6 > No symbol table info available. > #1 0x3fff833f136c in abort () from /lib64/libc.so.6 > No symbol table info available. > #2 0x3fff833e4c44 in __assert_fail_base () from /lib64/libc.so.6 > No symbol table info available. > #3 0x3fff833e4d34 in __assert_fail () from /lib64/libc.so.6 > No symbol table info available. > #4 0x1006a594 in qcow2_cache_entry_mark_dirty (bs=0x2e886f60, > c=0x2e879700, table=0x3fff8120) at block/qcow2-cache.c:411 > i = 0 > __PRETTY_FUNCTION__ = "qcow2_cache_entry_mark_dirty" > #5 0x10056154 in alloc_refcount_block (bs=0x2e886f60, > cluster_index=2, refcount_block=0x3fff80cff808) at block/qcow2-refcount.c:417 > s = 0x2e893210 > refcount_table_index = 0 > ret = 0 > new_block = 0 > blocks_used = 72057594818669408 > meta_offset = 1572863 > #6 0x10057520 in update_refcount (bs=0x2e886f60, offset=1048576, > length=524288, addend=1, decrease=false, type=QCOW2_DISCARD_NEVER) > at block/qcow2-refcount.c:834 > block_index = 268870408 > refcount = 780741808 > cluster_index = 2 > table_index = 0 > s = 0x2e893210 > start = 1048576 > last = 1048576 > cluster_offset = 1048576 > refcount_block = 0x3fff8120 > old_table_index = -1 > ret = 16383 > #7 0x10057dc8 in qcow2_alloc_clusters_at (bs=0x2e886f60, > offset=1048576, nb_clusters=1) at block/qcow2-refcount.c:1032 > s = 0x2e893210 > cluster_index = 3 > refcount = 0 > i = 1 > ret = 0 > __PRETTY_FUNCTION__ = "qcow2_alloc_clusters_at" > #8 0x100636d8 in do_alloc_cluster_offset (bs=0x2e886f60, > guest_
Re: [Qemu-block] [Qemu-devel] [PATCH] scripts/make-release: No need to delete pixman/.git anymore
On 13.11.2017 10:43, no-re...@patchew.org wrote: > Hi, > > This series failed automatic build test. Please find the testing commands and > their output below. If you have docker installed, you can probably reproduce > it > locally. > > Subject: [Qemu-devel] [PATCH] scripts/make-release: No need to delete > pixman/.git anymore > Type: series > Message-id: 1510564905-6185-1-git-send-email-th...@redhat.com [...] > QEMU -- "/tmp/qemu-test/build/x86_64-softmmu/qemu-system-x86_64" > -nodefaults -machine accel=qtest > QEMU_IMG -- "/tmp/qemu-test/build/qemu-img" > QEMU_IO -- "/tmp/qemu-test/build/qemu-io" --cache writeback -f raw > QEMU_NBD -- "/tmp/qemu-test/build/qemu-nbd" > IMGFMT-- raw > IMGPROTO -- file > PLATFORM -- Linux/x86_64 fe9889ba94a0 4.11.10-300.fc26.x86_64 > TEST_DIR -- /tmp/qemu-test > SOCKET_SCM_HELPER -- /tmp/qemu-test/build/tests/qemu-iotests/socket_scm_helper > > 001 > 002 > 004 > 005 > 008 > 009 > 010 > 011 > 012 > 017 [not run] not suitable for this image format: raw > 018 [not run] not suitable for this image format: raw > 019 [not run] not suitable for this image format: raw > 020 [not run] not suitable for this image format: raw > 021 > 024 [not run] not suitable for this image format: raw > 025 > 027 [not run] not suitable for this image format: raw > 028 [not run] not suitable for this image format: raw > 029 [not run] not suitable for this image format: raw > 031 [not run] not suitable for this image format: raw > 032 > 033 > 034 [not run] not suitable for this image format: raw > 035 [not run] not suitable for this image format: raw > 036 [not run] not suitable for this image format: raw > 037 [not run] not suitable for this image format: raw > 038 [not run] not suitable for this image format: raw > 039 [not run] not suitable for this image format: raw > 042 [not run] not suitable for this image format: raw > 045 > 046 [not run] not suitable for this image format: raw > 047 [not run] not suitable for this image format: raw > 048 > 050 [not run] not suitable for this image format: raw > 052 > 053 [not run] not suitable for this image format: raw > 054 [not run] not suitable for this image format: raw > 058 [not run] not suitable for this image format: raw > 059 [not run] not suitable for this image format: raw > 060 [not run] not suitable for this image format: raw > 062 [not run] not suitable for this image format: raw > 063 > 064 [not run] not suitable for this image format: raw > 065 [not run] not suitable for this image format: raw > 066 [not run] not suitable for this image format: raw > 067 [not run] not suitable for this image format: raw > 068 [not run] not suitable for this image format: raw > 069 [not run] not suitable for this image format: raw > 070 [not run] not suitable for this image format: raw > 071 [not run] not suitable for this image format: raw > 072 [not run] not suitable for this image format: raw > 073 [not run] not suitable for this image format: raw > 074 [not run] not suitable for this image format: raw > 075 [not run] not suitable for this image format: raw > 077 - output mismatch (see 077.out.bad) > --- /tmp/qemu-test/src/tests/qemu-iotests/077.out 2017-11-13 > 09:38:59.0 + > +++ /tmp/qemu-test/build/tests/qemu-iotests/077.out.bad 2017-11-13 > 09:41:20.601278693 + > @@ -56,9 +56,9 @@ > wrote XXX/XXX bytes at offset XXX > XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > blkdebug: Resuming request 'B' > +blkdebug: Resuming request 'A' > wrote XXX/XXX bytes at offset XXX > XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -blkdebug: Resuming request 'A' > wrote XXX/XXX bytes at offset XXX > XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > wrote XXX/XXX bytes at offset XXX > 078 [not run] not suitable for this image format: raw > 081 > 082 [not run] not suitable for this image format: raw > 084 [not run] not suitable for this image format: raw > 086 > 087 [not run] not suitable for this image format: raw > 088 [not run] not suitable for this image format: raw > 089 [not run] not suitable for this image format: raw > 090 [not run] not suitable for this image format: raw > 092 [not run] not suitable for this image format: raw > 094 [not run] not suitable for this image protocol: file > 095 [not run] not suitable for this image format: raw > 096 [not run] not suitable for this image format: raw > 098
Re: [Qemu-block] [Qemu-devel] using "qemu-img convert -O qcow2" to convert qcow v1 to v2 creates a qcow v3 file?
On 14.11.2017 14:32, Max Reitz wrote: [...] > Well, do you want to document it? I'd rather deprecate it altogether. Maybe a first step could be to change qemu-img so that it refuses to create new qcow1 images (but still can convert them into other formats). So basically make qcow1 read-only? Thomas signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 4/6] iotests: 194: test also migration of dirty bitmap
On 22/05/2020 00.06, Vladimir Sementsov-Ogievskiy wrote: > Test that dirty bitmap migration works when we deal with mirror. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Eric Blake > --- > tests/qemu-iotests/194 | 14 ++ > tests/qemu-iotests/194.out | 6 ++ > 2 files changed, 16 insertions(+), 4 deletions(-) Hi! This test broke the iotest in the gitlab CI: https://gitlab.com/huth/qemu/-/jobs/578520599#L3780 it works again when I revert this commit. Could the test be reworked so that it works in CI pipelines, too? Otherwise, I think it's best if we disable it in the .gitlab-ci.yml file... Thomas
