Re: [PULL v2] Block layer patches
On 10/09/2020 00.09, Eric Blake wrote: > On 9/9/20 4:55 PM, Peter Maydell wrote: > >> >> This fails 'make check' on NetBSD and OpenBSD: >> >> ./check: line 47: realpath: command not found >> ./check: line 60: /common.env: No such file or directory >> check: failed to source common.env (make sure the qemu-iotests are run >> from tests/qemu-iotests in the build tree) >> gmake: *** [/home/qemu/qemu-test.vcb7nz/src/tests/Makefile.include:144: >> check-block] Error 1 > > BSD has 'readlink -f' (and so does coreutils on Linux), which does the > same thing as the Linux-only realpath. Seems like readlink -f does not work on macOS: https://cirrus-ci.com/task/5735398972325888?command=main#L7038 Any ideas what to use instead? Peter, why did this slip through your merge tests, do you still skip the iotests there? Thomas
Re: [PULL v2] Block layer patches
On 12/09/2020 20.38, Peter Maydell wrote: > On Sat, 12 Sep 2020 at 13:27, Thomas Huth wrote: >> Peter, why did this slip through your merge tests, do you still skip the >> iotests there? > > I forget what the reason for them being skipped is, maybe > it's because they demand a gnu sed ? The tests/check-block.sh script should tell you when you run "make check-block" ... but yes, they need gnu sed - which should be available via homebrew. Thomas
Re: [PATCH v8 00/27] W32, W64 msys2/mingw patches
On 13/09/2020 00.44, Yonggang Luo wrote: > It first introduce msys2 CI on cirrus by fixes nfs, capstone, curses and > disable partial test-char tests. > And then fixes all unit tests failure on msys2/mingw > This fixes the reviews suggested in the mailling list > All cirrus CI are passed Hi, since you're very often sending new versions of your patch series, could you please add a history to the cover letter to say what you changed in each version? Otherwise, your work is very hard to follow. I'd also suggest to really slow down the sending a little bit. Let your patches mature in your tests first, then send out a new series only if you feel that they are really ready. Nobody has the bandwith to review a patch series with 27 patches each day... Thanks, Thomas
Re: [PATCH v8 03/27] ci: fixes msys2 build by upgrading capstone to 4.0.2
On 13/09/2020 00.44, Yonggang Luo wrote:
> The currently random version capstone have the following compiling issue:
> CC /c/work/xemu/qemu/build/slirp/src/arp_table.o
> make[1]: *** No rule to make target
> “/c/work/xemu/qemu/build/capstone/capstone.lib”。 Stop.
>
> Subproject commit 1d230532840a37ac032c6ab80128238fc930c6c1 are the tagged
> version 4.0.2
> when upgrading to this version, the folder structure of include are changed to
> qemu\capstone\include
> │ platform.h
> │
> ├─capstone
> │ arm.h
> │ arm64.h
> │ capstone.h
> │ evm.h
> │ m680x.h
> │ m68k.h
> │ mips.h
> │ platform.h
> │ ppc.h
> │ sparc.h
> │ systemz.h
> │ tms320c64x.h
> │ x86.h
> │ xcore.h
> │
> └─windowsce
> intrin.h
> stdint.h
>
> in capstone. so we need add extra include path
> -I${source_path}/capstone/include/capstone
> for directly #include , and the exist include path should
> preserve, because
> in capstone code there something like #include "capstone/capstone.h"
>
> If only using
> capstone_cflags="-I${source_path}/capstone/include/capstone"
> Then will cause the following compiling error:
>
> CC cs.o
> cs.c:17:10: fatal error: 'capstone/capstone.h' file not found
> #include
> ^
> 1 error generated.
>
> Signed-off-by: Yonggang Luo
> ---
> capstone | 2 +-
> configure | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/capstone b/capstone
> index 22ead3e0bf..1d23053284 16
> --- a/capstone
> +++ b/capstone
> @@ -1 +1 @@
> -Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
> +Subproject commit 1d230532840a37ac032c6ab80128238fc930c6c1
Richard has a patch series on the list now to update and improve the
capstone submodule (see "capstone + disassembler patches") ... I think
this patch here will then not be required anymore.
Thomas
Re: [PATCH v8 13/27] tests: Enable crypto tests under msys2/mingw
On 13/09/2020 00.44, Yonggang Luo wrote:
> Fixes following tests on msys2/mingw
> 'test-crypto-tlscredsx509': ['crypto-tls-x509-helpers.c',
> 'pkix_asn1_tab.c',
>tasn1, crypto],
> 'test-crypto-tlssession': ['crypto-tls-x509-helpers.c',
> 'pkix_asn1_tab.c', 'crypto-tls-psk-helpers.c',
> tasn1, crypto],
> 'test-io-channel-tls': ['io-channel-helpers.c',
> 'crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c',
> tasn1, io, crypto]}
> These tests are failure with:
> ERROR test-crypto-tlscredsx509 - missing test plan
> ERROR test-crypto-tlssession - missing test plan
> ERROR test-io-channel-tls - missing test plan
>
> Because on win32 those test case are all disabled in the header
>
> Add qemu_socket_pair for cross platform support, convert file system
> handling functions to glib
> Add qemu_link function instead posix only link function.
> Use send ad recv from qemu that convert Windows Socks error
> to errno properly.
>
> Signed-off-by: Yonggang Luo
> ---
[...]
> +static int __stream_socketpair(struct addrinfo* addr_info, int sock[2]){
> +SOCKET listener, client, server;
> +int opt = 1;
> +
> +listener = server = client = INVALID_SOCKET;
> +listener = socket(addr_info->ai_family, addr_info->ai_socktype,
> addr_info->ai_protocol);
> +if (INVALID_SOCKET == listener)
> +goto fail;
> +
> +setsockopt(listener, SOL_SOCKET, SO_REUSEADDR,(const char*)&opt,
> sizeof(opt));
> +
> +if(SOCKET_ERROR == bind(listener, addr_info->ai_addr,
> addr_info->ai_addrlen))
> +goto fail;
> +
> +if (SOCKET_ERROR == getsockname(listener, addr_info->ai_addr,
> (int*)&addr_info->ai_addrlen))
> +goto fail;
> +
> +if(SOCKET_ERROR == listen(listener, 5))
> +goto fail;
> +
> +client = socket(addr_info->ai_family, addr_info->ai_socktype,
> addr_info->ai_protocol);
> +
> +if (INVALID_SOCKET == client)
> +goto fail;
> +
> +if (SOCKET_ERROR ==
> connect(client,addr_info->ai_addr,addr_info->ai_addrlen))
> +goto fail;
> +
> +server = accept(listener, 0, 0);
> +
> +if (INVALID_SOCKET == server)
> +goto fail;
> +
> +closesocket(listener);
> +
> +sock[0] = client;
> +sock[1] = server;
> +
> +return 0;
> +fail:
> +if(INVALID_SOCKET!=listener)
> +closesocket(listener);
> +if (INVALID_SOCKET!=client)
> +closesocket(client);
> +return -1;
> +}
> +
> +static int __dgram_socketpair(struct addrinfo* addr_info, int sock[2])
> +{
> +SOCKET client, server;
> +struct addrinfo addr, *result = NULL;
> +const char* address;
> +int opt = 1;
> +
> +server = client = INVALID_SOCKET;
> +
> +server = socket(addr_info->ai_family, addr_info->ai_socktype,
> addr_info->ai_protocol);
> +if (INVALID_SOCKET == server)
> +goto fail;
> +
> +setsockopt(server, SOL_SOCKET,SO_REUSEADDR, (const char*)&opt,
> sizeof(opt));
> +
> +if(SOCKET_ERROR == bind(server, addr_info->ai_addr,
> addr_info->ai_addrlen))
> +goto fail;
> +
> +if (SOCKET_ERROR == getsockname(server, addr_info->ai_addr,
> (int*)&addr_info->ai_addrlen))
> +goto fail;
> +
> +client = socket(addr_info->ai_family, addr_info->ai_socktype,
> addr_info->ai_protocol);
> +if (INVALID_SOCKET == client)
> +goto fail;
> +
> +memset(&addr,0,sizeof(addr));
> +addr.ai_family = addr_info->ai_family;
> +addr.ai_socktype = addr_info->ai_socktype;
> +addr.ai_protocol = addr_info->ai_protocol;
> +
> +if (AF_INET6==addr.ai_family)
> +address = "0:0:0:0:0:0:0:1";
> +else
> +address = "127.0.0.1";
> +
> +if (getaddrinfo(address, "0", &addr, &result))
> +goto fail;
> +
> +setsockopt(client,SOL_SOCKET,SO_REUSEADDR,(const char*)&opt,
> sizeof(opt));
> +if(SOCKET_ERROR == bind(client, result->ai_addr, result->ai_addrlen))
> +goto fail;
> +
> +if (SOCKET_ERROR == getsockname(client, result->ai_addr,
> (int*)&result->ai_addrlen))
> +goto fail;
> +
> +if (SOCKET_ERROR == connect(server, result->ai_addr, result->ai_addrlen))
> +goto fail;
> +
> +if (SOCKET_ERROR == connect(client, addr_info->ai_addr,
> addr_info->ai_addrlen))
> +goto fail;
> +
> +freeaddrinfo(result);
> +sock[0] = client;
> +sock[1] = server;
> +return 0;
> +
> +fail:
> +if (INVALID_SOCKET!=client)
> +closesocket(client);
> +if (INVALID_SOCKET!=server)
> +closesocket(server);
> +if (result)
> +freeaddrinfo(result);
> +return -1;
> +}
> +
> +int qemu_socketpair(int family, int type, int protocol,int recv[2]){
> +const char* address;
> +struct addrinfo addr_info,*p_addrinfo;
> +int result = -1;
> +
> +if (family == AF_UNIX)
> +{
> +family = AF_INET;
> +}
> +
> +memset(&addr_info, 0, sizeof(addr_info));
> +addr_info.ai_family
Re: [PATCH v8 16/27] cirrus: Building freebsd in a single short
On 13/09/2020 00.44, Yonggang Luo wrote:
> This reverts commit 45f7b7b9f38f5c4d1529a37c93dedfc26a231bba
> ("cirrus.yml: Split FreeBSD job into two parts").
>
> freebsd 1 hour limit not hit anymore
>
> I think we going to a wrong direction, I think there is some tests a stall
> the test runner,
> please look at
> https://cirrus-ci.com/task/5110577531977728
> When its running properly, the consumed time are little, but when tests
> running too long,
> look at the cpu usage, the cpu usage are nearly zero. doesn't consuming time.
>
> And look at
> https://cirrus-ci.com/task/6119341601062912
>
> If the tests running properly, the time consuming are little
> We should not hide the error by split them
>
> Signed-off-by: Yonggang Luo
> Reviewed-by: Daniel P. Berrangé
> Reviewed-by: Ed Maste
> ---
> .cirrus.yml | 35 ---
> 1 file changed, 8 insertions(+), 27 deletions(-)
I tried this a couple of times now, and currently the problem seems to
be gone, indeed. I'd still prefer to understand first why we have seen
the slowdown a couple of weeks ago, but if people agree that we should
revert the patch now, that's ok for me now, too.
Tested-by: Thomas Huth
Re: [PATCH v8 24/27] ci: Enable msys2 ci in cirrus
On 13/09/2020 00.44, Yonggang Luo wrote:
> Install msys2 in a proper way refer to
> https://github.com/cirruslabs/cirrus-ci-docs/issues/699
> The https://wiki.qemu.org/Hosts/W32#Native_builds_with_MSYS2 need to be
> updated.
I don't think that a request to update the wiki should be part of the
commit message here.
Stefan, could you please have a look at the wiki to see whether it needs
an update?
> There is no need of --cross-prefix, open mingw64.exe instead of msys2.exe
> then we don't
> need the --cross-prefix, besides we using environment variable settings:
> MSYS: winsymlinks:nativestrict
> MSYSTEM: MINGW64
> CHERE_INVOKING: 1
> to opening mingw64 native shell.
> We now running tests with make -i check to skip tests errors.
>
> Signed-off-by: Yonggang Luo
> Reviewed-by: Daniel P. Berrangé
> ---
> .cirrus.yml | 60 +
> 1 file changed, 60 insertions(+)
>
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 690c6882e8..1ff9f0a72f 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -44,3 +44,63 @@ macos_xcode_task:
> --enable-werror --cc=clang || { cat config.log; exit 1; }
> - gmake -j$(sysctl -n hw.ncpu)
> - gmake check
> +
> +windows_msys2_task:
> + windows_container:
> +image: cirrusci/windowsservercore:cmake
> +os_version: 2019
> +cpu: 8
> +memory: 8G
> + env:
> +MSYS: winsymlinks:nativestrict
> +MSYSTEM: MINGW64
> +CHERE_INVOKING: 1
> + printenv_script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc 'printenv'
> + install_script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
> http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz";
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
> http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz.sig";
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && pacman -U
> --noconfirm msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz"
The lines here are very long ... could you please put the stuff after
the "&&" on a new line?
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman -Sy --noconfirm"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --needed --noconfirm -S
> bash pacman pacman-mirrors msys2-runtime"
> +- taskkill /F /IM gpg-agent.exe
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -Su"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -S --needed
> +base-devel
> +git
> +mingw-w64-x86_64-python
> +mingw-w64-x86_64-python-setuptools
> +mingw-w64-x86_64-toolchain
> +mingw-w64-x86_64-SDL2
> +mingw-w64-x86_64-SDL2_image
> +mingw-w64-x86_64-gtk3
> +mingw-w64-x86_64-glib2
> +mingw-w64-x86_64-ninja
> +mingw-w64-x86_64-make
> +mingw-w64-x86_64-jemalloc
Installing jemalloc only makes sense if you also use --enable-jemalloc
later. So I'd suggest to drop this package here.
> +mingw-w64-x86_64-lzo2
> +mingw-w64-x86_64-zstd
> +mingw-w64-x86_64-libjpeg-turbo
> +mingw-w64-x86_64-pixman
> +mingw-w64-x86_64-libgcrypt
> +mingw-w64-x86_64-capstone
Hmm, so in an earlier patch, you've added an update to the capstone
submodule, but here you install the system-wide capstone as well? ...
that does not make too much sense. Which one do you intend to use?
> +mingw-w64-x86_64-libpng
> +mingw-w64-x86_64-libssh
> +mingw-w64-x86_64-libxml2
> +mingw-w64-x86_64-snappy
> +mingw-w64-x86_64-libusb
> +mingw-w64-x86_64-usbredir
> +mingw-w64-x86_64-libtasn1
> +mingw-w64-x86_64-libnfs
I think your NFS patch needs a review/ack from the block layer folks
first, so for the time being, I'd suggest drop libnfs here first and add
it later, once the nfs patch has been merged via the block layer queue.
> +mingw-w64-x86_64-nettle
> +mingw-w64-x86_64-cyrus-sasl
> +mingw-w64-x86_64-curl
> +mingw-w64-x86_64-gnutls
> +mingw-w64-x86_64-zstd"
> + script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc "mkdir build"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && ../configure
> --python=python3 --ninja=ninja"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make
> -j$NUMBER_OF_PROCESSORS"
> + test_script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make V=1 check"
> +
>
Thomas
Re: [PATCH v8 16/27] cirrus: Building freebsd in a single short
On 14/09/2020 10.50, Philippe Mathieu-Daudé wrote:
> On 9/14/20 9:27 AM, Thomas Huth wrote:
>> On 13/09/2020 00.44, Yonggang Luo wrote:
>>> This reverts commit 45f7b7b9f38f5c4d1529a37c93dedfc26a231bba
>>> ("cirrus.yml: Split FreeBSD job into two parts").
>>>
>>> freebsd 1 hour limit not hit anymore
>>>
>>> I think we going to a wrong direction, I think there is some tests a stall
>>> the test runner,
>>> please look at
>>> https://cirrus-ci.com/task/5110577531977728
>>> When its running properly, the consumed time are little, but when tests
>>> running too long,
>>> look at the cpu usage, the cpu usage are nearly zero. doesn't consuming
>>> time.
>>>
>>> And look at
>>> https://cirrus-ci.com/task/6119341601062912
>>>
>>> If the tests running properly, the time consuming are little
>>> We should not hide the error by split them
>>>
>>> Signed-off-by: Yonggang Luo
>>> Reviewed-by: Daniel P. Berrangé
>>> Reviewed-by: Ed Maste
>>> ---
>>> .cirrus.yml | 35 ---
>>> 1 file changed, 8 insertions(+), 27 deletions(-)
>>
>> I tried this a couple of times now, and currently the problem seems to
>> be gone, indeed. I'd still prefer to understand first why we have seen
>> the slowdown a couple of weeks ago, but if people agree that we should
>> revert the patch now, that's ok for me now, too.
>
> Not sure if related (probably not as failure and not timeout),
> I hit this during the night:
>
> TESTiotest-qcow2: 030 [fail]
No, that's a different (known) issue. See my patch on the list:
[PATCH v2] iotests: Skip test_stream_parallel in test 030 when doing
"make check"
Thomas
Re: [PATCH] tests/check-block: Do not run the iotests with old versions of bash
On 14/09/2020 11.36, 罗勇刚(Yonggang Luo) wrote: > > > On Sat, Sep 12, 2020 at 8:16 PM Thomas Huth <mailto:th...@redhat.com>> wrote: >> >> macOS is shipped with a very old version of the bash (3.2), which >> is currently not suitable for running the iotests anymore. Add >> a check to skip the iotests in this case - if someone still wants >> to run the iotests on macOS, they can install a newer version from >> homebrew, for example. >> >> Signed-off-by: Thomas Huth mailto:th...@redhat.com>> >> --- >> tests/check-block.sh | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/tests/check-block.sh b/tests/check-block.sh >> index 8e29c868e5..bfe1630c1e 100755 >> --- a/tests/check-block.sh >> +++ b/tests/check-block.sh >> @@ -46,6 +46,11 @@ if ! command -v bash >/dev/null 2>&1 ; then >> exit 0 >> fi >> >> +if bash --version | grep 'GNU bash, version [123]' > /dev/null 2>&1 ; > then >> + echo "bash version too old ==> Not running the qemu-iotests." >> + exit 0 >> +fi >> + >> if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then >> if ! command -v gsed >/dev/null 2>&1; then >> echo "GNU sed not available ==> Not running the qemu-iotests." >> -- >> 2.18.2 >> >> > Is that worth to convert the check-block.sh script to python script? so > it can even running under msys2/mingw？ No, you need bash for the various iotest anyway. Thomas
Re: [PATCH] tests/check-block: Do not run the iotests with old versions of bash
On 14/09/2020 11.19, Max Reitz wrote: > On 12.09.20 14:14, Thomas Huth wrote: >> macOS is shipped with a very old version of the bash (3.2), which >> is currently not suitable for running the iotests anymore. Add >> a check to skip the iotests in this case - if someone still wants >> to run the iotests on macOS, they can install a newer version from >> homebrew, for example. >> >> Signed-off-by: Thomas Huth >> --- >> tests/check-block.sh | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/tests/check-block.sh b/tests/check-block.sh >> index 8e29c868e5..bfe1630c1e 100755 >> --- a/tests/check-block.sh >> +++ b/tests/check-block.sh >> @@ -46,6 +46,11 @@ if ! command -v bash >/dev/null 2>&1 ; then >> exit 0 >> fi >> >> +if bash --version | grep 'GNU bash, version [123]' > /dev/null 2>&1 ; then > > grep -q instead of the redirections, perhaps? > > But more importantly, I think this needs a LANG=C prefix. (If I expand > the rejected major versions to [12345], it doesn’t skip without a > prefix, because the string reads “GNU bash, Version 5...” here in > LANG=de_DE.UTF-8.) Ouch, ok. But actually, I'm not quite sure anymore whether the patch is really required. I ran into the "readlink -f" problem and thought that it occurred due to the ancient version of bash on macOS, but as a I now know, readlink is a separate program and not a bash built-in, so it's a different issue... thus let's skip this patch here for now until we hit a real issue with bash again. Thomas
Re: [qemu-web PATCH] Add QEMU storage overview blog post
On 14/09/2020 12.35, Stefan Hajnoczi wrote: > On Mon, Sep 07, 2020 at 04:04:21PM +0100, Stefan Hajnoczi wrote: >> I want to kick of a series of posts about storage. The first post covers >> high-level concepts, features, and utilities in QEMU. Later posts will >> discuss configuration details, architecture, and performance. >> >> Signed-off-by: Stefan Hajnoczi >> --- >> _posts/2020-09-07-qemu-storage-overview.md | 122 +++ >> screenshots/2020-09-07-block-device-io.svg | 366 + >> screenshots/2020-09-07-lifecycle.svg | 328 ++ >> 3 files changed, 816 insertions(+) >> create mode 100644 _posts/2020-09-07-qemu-storage-overview.md >> create mode 100644 screenshots/2020-09-07-block-device-io.svg >> create mode 100644 screenshots/2020-09-07-lifecycle.svg > > Ping? Thomas, you would you be able to merge this? Sorry, your mail successfully hid in my overcrowded inbox :-( ... thanks to Paolo for pushing it! Thomas
Re: [PATCH v8 13/27] tests: Enable crypto tests under msys2/mingw
On 14/09/2020 10.19, 罗勇刚(Yonggang Luo) wrote: > > > On Mon, Sep 14, 2020 at 3:23 PM Thomas Huth <mailto:th...@redhat.com>> wrote: >> >> On 13/09/2020 00.44, Yonggang Luo wrote: >> > Fixes following tests on msys2/mingw >> > 'test-crypto-tlscredsx509': ['crypto-tls-x509-helpers.c', > 'pkix_asn1_tab.c', >> > tasn1, crypto], >> > 'test-crypto-tlssession': ['crypto-tls-x509-helpers.c', > 'pkix_asn1_tab.c', 'crypto-tls-psk-helpers.c', >> > tasn1, crypto], >> > 'test-io-channel-tls': ['io-channel-helpers.c', > 'crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c', >> > tasn1, io, crypto]} >> > These tests are failure with: >> > ERROR test-crypto-tlscredsx509 - missing test plan >> > ERROR test-crypto-tlssession - missing test plan >> > ERROR test-io-channel-tls - missing test plan >> > >> > Because on win32 those test case are all disabled in the header >> > >> > Add qemu_socket_pair for cross platform support, convert file system >> > handling functions to glib >> > Add qemu_link function instead posix only link function. >> > Use send ad recv from qemu that convert Windows Socks error >> > to errno properly. >> > >> > Signed-off-by: Yonggang Luo <mailto:luoyongg...@gmail.com>> >> > --- [...] >> Where do you've got this code from? It seems like this has been taken >> from a 3rd party source? E.g.: >> >> https://blog.csdn.net/wufuhuai/article/details/79761889 >> >> What's the license of this new code? ... please clarify such details in >> the commit description. > > The original code have no license information, neither copyleft nor > copyright, what's your suggestion > or rewrite it? > You can not simply copy code without license information and submit this as if it was your own! Please never do that again! With your Signed-off-by line, you basically acknowledge that you've read and followed the Developer Certificate of Origin: https://developercertificate.org/ If you haven't done that yet, please do it now! And for this patch here, I don't think that it is acceptable without proper license information. Thomas
Re: [PATCH] tests/check-block: Do not run the iotests with old versions of bash
On 14/09/2020 13.13, Max Reitz wrote: > On 14.09.20 12:50, Thomas Huth wrote: >> On 14/09/2020 11.19, Max Reitz wrote: >>> On 12.09.20 14:14, Thomas Huth wrote: >>>> macOS is shipped with a very old version of the bash (3.2), which >>>> is currently not suitable for running the iotests anymore. Add >>>> a check to skip the iotests in this case - if someone still wants >>>> to run the iotests on macOS, they can install a newer version from >>>> homebrew, for example. >>>> >>>> Signed-off-by: Thomas Huth >>>> --- >>>> tests/check-block.sh | 5 + >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/tests/check-block.sh b/tests/check-block.sh >>>> index 8e29c868e5..bfe1630c1e 100755 >>>> --- a/tests/check-block.sh >>>> +++ b/tests/check-block.sh >>>> @@ -46,6 +46,11 @@ if ! command -v bash >/dev/null 2>&1 ; then >>>> exit 0 >>>> fi >>>> >>>> +if bash --version | grep 'GNU bash, version [123]' > /dev/null 2>&1 ; then >>> >>> grep -q instead of the redirections, perhaps? >>> >>> But more importantly, I think this needs a LANG=C prefix. (If I expand >>> the rejected major versions to [12345], it doesn’t skip without a >>> prefix, because the string reads “GNU bash, Version 5...” here in >>> LANG=de_DE.UTF-8.) >> >> Ouch, ok. But actually, I'm not quite sure anymore whether the patch is >> really required. I ran into the "readlink -f" problem and thought that >> it occurred due to the ancient version of bash on macOS, but as a I now >> know, readlink is a separate program and not a bash built-in, so it's a >> different issue... thus let's skip this patch here for now until we hit >> a real issue with bash again. > > Yes, I had hoped this patch would fix that issue. Or perhaps at least > hide it, because if you have a newer bash, chances are your readlink has > -f, too. > > So should we just effectively revert b1cbc33a397 if readlink -f didn’t > work, i.e. check "$?" and on failure use $PWD as it was before b1cbc33a397? Sounds like the best option that I currently can see, indeed. Want me to send a patch, or will you provide one? Thomas
Re: [PATCH] iotests: Work around failing readlink -f
On 14/09/2020 13.38, Max Reitz wrote:
> On macOS, (out of the box) readlink does not have -f. If the recent
> "readlink -f" call introduced by b1cbc33a397 fails, just fall back to
> the old behavior (which means you can run the iotests only from the
> build tree, but that worked fine for six years, so it should be fine
> still).
>
> Keep any potential error message on stderr. If users want to run the
> iotests from outside the build tree, this may point them to what's wrong
> (with their system).
>
> Fixes: b1cbc33a3971b6bb005d5ac3569feae35a71de0f
>("iotests: Allow running from different directory")
> Reported-by: Claudio Fontana
> Reported-by: Thomas Huth
> Signed-off-by: Max Reitz
> ---
> Hi Thomas,
>
> I thought this would be quicker than writing a witty response on whether
> you or me should write this patch. O:)
> ---
> tests/qemu-iotests/check | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index e14a1f354d..75675e1a18 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -45,6 +45,10 @@ then
> fi
> source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to
> enter source tree"
> build_iotests=$(readlink -f $(dirname "$0"))
> +if [ "$?" -ne 0 ]; then
> +# Perhaps -f is unsupported, revert to pre-b1cbc33a397 behavior
> +build_iotests=$PWD
> +fi
> else
> # called from the source tree
> source_iotests=$PWD
Thanks, the macOS build seems to work again:
https://cirrus-ci.com/task/5431828267925504?command=main#L7033
Tested-by: Thomas Huth
Re: [PATCH v3] iotests: Drop readlink -f
On 14/09/2020 16.56, Max Reitz wrote:
> On macOS, (out of the box) readlink does not have -f. We do not really
> need readlink here, though, it was just a replacement for realpath
> (which is not available on our BSD test systems), which we needed to
> make the $(dirname) into an absolute path.
>
> Instead of using either, just use "cd; pwd" like is done for
> $source_iotests.
>
> Fixes: b1cbc33a3971b6bb005d5ac3569feae35a71de0f
>("iotests: Allow running from different directory")
> Suggested-by: Peter Maydell
> Reported-by: Claudio Fontana
> Reported-by: Thomas Huth
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/check | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index e14a1f354d..678b6e4910 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -44,7 +44,7 @@ then
> _init_error "failed to obtain source tree name from check symlink"
> fi
> source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to
> enter source tree"
> -build_iotests=$(readlink -f $(dirname "$0"))
> +build_iotests=$(cd "$(dirname "$0")"; pwd)
I assume the nested quotes are ok here? ... shell scripts always confuse
me in that regard...
Thomas
Re: [PATCH] tests/check-block: Do not run the iotests with old versions of bash
On 14/09/2020 17.03, Eric Blake wrote: > On 9/12/20 7:14 AM, Thomas Huth wrote: >> macOS is shipped with a very old version of the bash (3.2), which >> is currently not suitable for running the iotests anymore. Add >> a check to skip the iotests in this case - if someone still wants >> to run the iotests on macOS, they can install a newer version from >> homebrew, for example. >> >> Signed-off-by: Thomas Huth >> --- >> tests/check-block.sh | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/tests/check-block.sh b/tests/check-block.sh >> index 8e29c868e5..bfe1630c1e 100755 >> --- a/tests/check-block.sh >> +++ b/tests/check-block.sh >> @@ -46,6 +46,11 @@ if ! command -v bash >/dev/null 2>&1 ; then >> exit 0 >> fi >> +if bash --version | grep 'GNU bash, version [123]' > /dev/null 2>&1 >> ; then > > We're already running bash - why do we need to spawn another bash and a > grep, when we can just check $BASH_VERSION? tests/check-block.sh uses "#!/bin/sh", so it could be running with a different kind of shell, I think. Thomas
Re: [PATCH v8 27/27] Revert "configure: add --ninja option"
On 13/09/2020 16.08, Paolo Bonzini wrote: > On 13/09/20 00:44, Yonggang Luo wrote: >> This reverts commit 48328880fddf0145bdccc499160fb24dfabfbd41. >> >> The --ninja option doesn't need anymore because of upgrade meson to 0.55.2 >> At that version we can use ninjatool > > We might actually get rid of ninjatool before QEMU 5.2 goes out, if we > decide to make Ninja a mandatory build dependency. So we can hold on > patches 26 and 27. Thanks for testing though! > > I'm also not sure about patch 16, since that's not my area, but Daniel > and Ed both reviewed it so that's okay. > > Finally, instead of checking !_WIN32 it's better to check CONFIG_POSIX > or CONFIG_WIN32. That can be changed on commit though. > > Everything else seems okay. I'll wait a couple days and queue the whole > bunch up to patch 25. Patch 13 definitely needs a replacement, and I think patch 2 should likely go through the block tree instead... But I recently started to experiment with these patches, too, and I think I got a reasonable subset now which should be OK for getting merged (e.g. disabling NFS and the crypto test in the CI for now). I'll take those through my testing tree. The remaining work can then be done on top later. Thomas
Re: [PATCH v10 00/26] W32, W64 msys2/mingw patches
On 15/09/2020 19.12, Yonggang Luo wrote: [...] > It first introduce msys2 CI on cirrus by fixes nfs, capstone, curses and > disable partial test-char tests. > And then fixes all unit tests failure on msys2/mingw > This fixes the reviews suggested in the mailling list > All cirrus CI are passed Thanks a lot for your work, I've added most of your patches to my latest "testing" pull request now, so that we should get basic test coverage on msys2 now in the Cirrus-CI if it gets merged. I skipped the NFS, capstone, test-char and crypto patches for now (and replaced them with older versions of your patches where you've disabled them) - I think these patches still need some more review / work and then should go through the trees of the corresponding maintainers later. Cheers, Thomas
Re: [PATCH 3/5] qom: Remove module_obj_name parameter from OBJECT_DECLARE* macros
On 16/09/2020 20.25, Eduardo Habkost wrote: > One of the goals of having less boilerplate on QOM declarations > is to avoid human error. Requiring an extra argument that is > never used is an opportunity for mistakes. > > Remove the unused argument from OBJECT_DECLARE_TYPE and > OBJECT_DECLARE_SIMPLE_TYPE. > > Coccinelle patch used to convert all users of the macros: > > @@ > declarer name OBJECT_DECLARE_TYPE; > identifier InstanceType, ClassType, lowercase, UPPERCASE; > @@ >OBJECT_DECLARE_TYPE(InstanceType, ClassType, > -lowercase, >UPPERCASE); > > @@ > declarer name OBJECT_DECLARE_SIMPLE_TYPE; > identifier InstanceType, lowercase, UPPERCASE; > @@ >OBJECT_DECLARE_SIMPLE_TYPE(InstanceType, > -lowercase, > UPPERCASE); > > Signed-off-by: Eduardo Habkost > --- Acked-by: Thomas Huth
[PATCH v2] tests/check-block: Do not run the iotests with old versions of bash
macOS is shipped with a very old version of the bash (3.2), which is currently not suitable for running the iotests anymore (e.g. it is missing support for "readarray" which is used in the file tests/qemu-iotests/common.filter). Add a check to skip the iotests in this case - if someone still wants to run the iotests on macOS, they can install a newer version from homebrew, for example. Signed-off-by: Thomas Huth --- v2: Use LANG=C and "-q" tests/check-block.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/tests/check-block.sh b/tests/check-block.sh index a5a69060e1..f6b1bda7b9 100755 --- a/tests/check-block.sh +++ b/tests/check-block.sh @@ -46,6 +46,11 @@ if ! command -v bash >/dev/null 2>&1 ; then exit 0 fi +if LANG=C bash --version | grep -q 'GNU bash, version [123]' ; then +echo "bash version too old ==> Not running the qemu-iotests." +exit 0 +fi + if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then if ! command -v gsed >/dev/null 2>&1; then echo "GNU sed not available ==> Not running the qemu-iotests." -- 2.18.2
Re: [PATCH 0/1] block: future of sheepdog storage driver ?
On 22/09/2020 11.01, Daniel P. Berrangé wrote: [...] > Does someone have a compelling reason for QEMU to keep supporting > this driver, other than the fact that it already exists ? > > If not, it looks like a candidate for deprecating in QEMU with a > view to later removing it. I think you gave enough examples why this is a good candidate for deprecation. So I'd say: Simply send a patch to deprecate it. That's what's our deprecation process is good for. If someone still cares for sheepdog, they then can speak up and we can revert the patch that put it on the deprecation list. Thomas
Re: [PATCH v2 01/20] configure: Detect libfuse
On 22/09/2020 12.49, Max Reitz wrote:
> Signed-off-by: Max Reitz
> ---
> configure | 34 ++
> meson.build | 6 ++
> 2 files changed, 40 insertions(+)
>
> diff --git a/configure b/configure
> index ce27eafb0a..21c31e4694 100755
> --- a/configure
> +++ b/configure
> @@ -538,6 +538,7 @@ meson=""
> ninja=""
> skip_meson=no
> gettext=""
> +fuse=""
>
> bogus_os="no"
> malloc_trim=""
> @@ -1621,6 +1622,10 @@ for opt do
>;;
>--disable-libdaxctl) libdaxctl=no
>;;
> + --enable-fuse) fuse=yes
> + ;;
> + --disable-fuse) fuse=no
> + ;;
>*)
>echo "ERROR: unknown option $opt"
>echo "Try '$0 --help' for more information"
> @@ -1945,6 +1950,7 @@ disabled with --disable-FEATURE, default is enabled if
> available:
>xkbcommon xkbcommon support
>rng-nonedummy RNG, avoid using /dev/(u)random and getrandom()
>libdaxctl libdaxctl support
> + fusefuse block device export
>
> NOTE: The object files are built at the place where configure is launched
> EOF
> @@ -6206,6 +6212,28 @@ but not implemented on your system"
> fi
> fi
>
> +##
> +# FUSE support
> +
> +if test "$fuse" != "no"; then
> + cat > $TMPC < +#define FUSE_USE_VERSION 31
> +#include
> +#include
> +int main(void) { return 0; }
> +EOF
> + fuse_cflags=$(pkg-config --cflags fuse3)
> + fuse_libs=$(pkg-config --libs fuse3)
> + if compile_prog "$fuse_cflags" "$fuse_libs"; then
> +fuse=yes
> + else
> +if test "$fuse" = "yes"; then
> + feature_not_found "fuse"
> +fi
> +fuse=no
> + fi
> +fi
Could you turn this immediately into a meson test instead? See e.g.
https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg07112.html as
an example for how to do this.
Thomas
Re: [PATCH v2 1/2] block: drop moderated sheepdog mailing list from MAINTAINERS file
On 22/09/2020 18.16, Daniel P. Berrangé wrote: > The sheepdog mailing list is setup to stop and queue messages from > non-subscribers, pending moderator approval. Unfortunately it seems > that the moderation queue is not actively dealt with. Even when messages > are approved, the sender is never added to the whitelist, so every > future mail from the same sender continues to get stopped for moderation. > > MAINTAINERS entries should be responsive and not unneccessarily block > mails from QEMU contributors, so drop the sheepdog mailing list. > > Reviewed-by: Philippe Mathieu-Daudé > Signed-off-by: Daniel P. Berrangé > --- > MAINTAINERS | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3d17cad19a..8e8a4fb0a8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2852,7 +2852,6 @@ F: block/rbd.c > Sheepdog > M: Liu Yuan > L: qemu-block@nongnu.org > -L: sheep...@lists.wpkg.org > S: Odd Fixes > F: block/sheepdog.c Reviewed-by: Thomas Huth
Re: [PATCH v2 2/2] block: deprecate the sheepdog block driver
On 22/09/2020 18.16, Daniel P. Berrangé wrote: > This thread from a little over a year ago: > > http://lists.wpkg.org/pipermail/sheepdog/2019-March/thread.html > > states that sheepdog is no longer actively developed. The only mentioned > users are some companies who are said to have it for legacy reasons with > plans to replace it by Ceph. There is talk about cutting out existing > features to turn it into a simple demo of how to write a distributed > block service. There is no evidence of anyone working on that idea: > > https://github.com/sheepdog/sheepdog/commits/master > > No real commits to git since Jan 2018, and before then just some minor > technical debt cleanup.. > > There is essentially no activity on the mailing list aside from > patches to QEMU that get CC'd due to our MAINTAINERS entry. > > Fedora packages for sheepdog failed to build from upstream source > because of the more strict linker that no longer merges duplicate > global symbols. Fedora patches it to add the missing "extern" > annotations and presumably other distros do to, but upstream source > remains broken. > > There is only basic compile testing, no functional testing of the > driver. > > Since there are no build pre-requisites the sheepdog driver is currently > enabled unconditionally. This would result in configure issuing a > deprecation warning by default for all users. Thus the configure default > is changed to disable it, requiring users to pass --enable-sheepdog to > build the driver. > > Signed-off-by: Daniel P. Berrangé > --- > block/sheepdog.c | 15 +++ > configure | 5 +++-- > docs/system/deprecated.rst | 9 + > 3 files changed, 27 insertions(+), 2 deletions(-) Reviewed-by: Thomas Huth
[PATCH v2 7/7] configure: Bump the minimum required Python version to 3.6
All our supported build platforms have Python 3.6 or newer nowadays, and
there are some useful features in Python 3.6 which are not available in
3.5 yet (e.g. the type hint annotations which will allow us to statically
type the QAPI parser), so let's bump the minimum Python version to 3.6 now.
Signed-off-by: Thomas Huth
---
v2:
- Bump the version in docs/conf.py, too
- Remove the now unnecessary check in tests/qemu-iotests/iotests.py
configure | 4 ++--
docs/conf.py | 4 ++--
tests/qemu-iotests/iotests.py | 2 --
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/configure b/configure
index 7564479008..a3a643168e 100755
--- a/configure
+++ b/configure
@@ -1965,8 +1965,8 @@ fi
# Note that if the Python conditional here evaluates True we will exit
# with status 1 which is a shell 'false' value.
-if ! $python -c 'import sys; sys.exit(sys.version_info < (3,5))'; then
- error_exit "Cannot use '$python', Python >= 3.5 is required." \
+if ! $python -c 'import sys; sys.exit(sys.version_info < (3,6))'; then
+ error_exit "Cannot use '$python', Python >= 3.6 is required." \
"Use --python=/path/to/python to specify a supported Python."
fi
diff --git a/docs/conf.py b/docs/conf.py
index 0dbd90dc11..8aeac40124 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -36,9 +36,9 @@ from sphinx.errors import ConfigError
# In newer versions of Sphinx this will display nicely; in older versions
# Sphinx will also produce a Python backtrace but at least the information
# gets printed...
-if sys.version_info < (3,5):
+if sys.version_info < (3,6):
raise ConfigError(
-"QEMU requires a Sphinx that uses Python 3.5 or better\n")
+"QEMU requires a Sphinx that uses Python 3.6 or better\n")
# The per-manual conf.py will set qemu_docdir for a single-manual build;
# otherwise set it here if this is an entire-manual-set build.
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 91e4a57126..f48460480a 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -40,8 +40,6 @@ sys.path.append(os.path.join(os.path.dirname(__file__), '..',
'..', 'python'))
from qemu import qtest
from qemu.qmp import QMPMessage
-assert sys.version_info >= (3, 6)
-
# Use this logger for logging messages directly from the iotests module
logger = logging.getLogger('qemu.iotests')
logger.addHandler(logging.NullHandler())
--
2.18.2
Re: [PATCH v2 7/7] configure: Bump the minimum required Python version to 3.6
On 23/09/2020 18.34, 罗勇刚(Yonggang Luo) wrote: > Should we also warning it in meson.build, cause configure finally shoud > be removed. Sounds like a good idea for a separate patch (let's do one issue at a time...). Thomas
Re: [PATCH] iotests: Remove 030 from the auto group
On 23/09/2020 20.18, Alberto Garcia wrote: > On Fri 04 Sep 2020 10:25:13 AM CEST, Kevin Wolf wrote: >>> Test 030 is still occasionally failing in the CI ... so for the >>> time being, let's disable it in the "auto" group. We can add it >>> back once it got more stable. >>> >>> Signed-off-by: Thomas Huth >> >> I would rather just disable this one test function as 030 is a pretty >> important one that tends to catch bugs. >> >>> I just saw the problem here: >>> https://cirrus-ci.com/task/5449330930745344?command=main#L6482 >>> and Peter hit it a couple of weeks ago: >>> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00136.html >> >> I wonder how this can still happen. The test should have more than >> enough time to complete now. Except if the throttling doesn't work as >> expected. >> >> I can't seem to reproduce this even if I add rather long delays. After >> 40 seconds, all jobs have moved either by 512k (which is STREAM_CHUNK) >> or not at all. > > I also don't understand how this can fail... I assume the test is not > running for that long in the cases when it fails, right? Hard to say ... the problem only occurs occasionally, and I've never seen it happen "live", only in the CI logs after the job has failed. I guess you'd have to print timestamps in the code and then submit a lot of jobs to the CI systems that are sensitive to this problem (e.g. Cirrus and Travis) to find out... Thomas
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
[PATCH 3/4] hw/virtio/virtio-balloon: Remove the "class" property
This property was only required for compatibility reasons in the
pc-1.0 machine type and earlier. Now that these machine types have
been removed, the property is not useful anymore.
Signed-off-by: Thomas Huth
---
hw/virtio/virtio-balloon-pci.c | 11 +--
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/hw/virtio/virtio-balloon-pci.c b/hw/virtio/virtio-balloon-pci.c
index a2c5cc7207..79a3ba979a 100644
--- a/hw/virtio/virtio-balloon-pci.c
+++ b/hw/virtio/virtio-balloon-pci.c
@@ -34,21 +34,13 @@ struct VirtIOBalloonPCI {
VirtIOPCIProxy parent_obj;
VirtIOBalloon vdev;
};
-static Property virtio_balloon_pci_properties[] = {
-DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
-DEFINE_PROP_END_OF_LIST(),
-};
static void virtio_balloon_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
{
VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
-if (vpci_dev->class_code != PCI_CLASS_OTHERS &&
-vpci_dev->class_code != PCI_CLASS_MEMORY_RAM) { /* qemu < 1.1 */
-vpci_dev->class_code = PCI_CLASS_OTHERS;
-}
-
+vpci_dev->class_code = PCI_CLASS_OTHERS;
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
@@ -59,7 +51,6 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass,
void *data)
PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
k->realize = virtio_balloon_pci_realize;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
-device_class_set_props(dc, virtio_balloon_pci_properties);
pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_BALLOON;
pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
--
2.27.0
[PATCH 4/4] hw/usb/bus: Remove the "full-path" property
This property was only required for the pc-1.0 and earlier machine
types. Since these have been removed now, we can delete the property
as well.
Signed-off-by: Thomas Huth
---
hw/usb/bus.c | 7 +--
include/hw/usb.h | 2 +-
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 064f94e9c3..df7411fea8 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -19,8 +19,6 @@ static void usb_qdev_unrealize(DeviceState *qdev);
static Property usb_props[] = {
DEFINE_PROP_STRING("port", USBDevice, port_path),
DEFINE_PROP_STRING("serial", USBDevice, serial),
-DEFINE_PROP_BIT("full-path", USBDevice, flags,
-USB_DEV_FLAG_FULL_PATH, true),
DEFINE_PROP_BIT("msos-desc", USBDevice, flags,
USB_DEV_FLAG_MSOS_DESC_ENABLE, true),
DEFINE_PROP_STRING("pcap", USBDevice, pcap_filename),
@@ -596,11 +594,8 @@ static char *usb_get_dev_path(DeviceState *qdev)
{
USBDevice *dev = USB_DEVICE(qdev);
DeviceState *hcd = qdev->parent_bus->parent;
-char *id = NULL;
+char *id = qdev_get_dev_path(hcd);
-if (dev->flags & (1 << USB_DEV_FLAG_FULL_PATH)) {
-id = qdev_get_dev_path(hcd);
-}
if (id) {
char *ret = g_strdup_printf("%s/%s", id, dev->port->path);
g_free(id);
diff --git a/include/hw/usb.h b/include/hw/usb.h
index abfbfc5284..c44b77dae0 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -216,7 +216,7 @@ struct USBEndpoint {
};
enum USBDeviceFlags {
-USB_DEV_FLAG_FULL_PATH,
+USB_DEV_FLAG_FULL_PATH, /* unused since QEMU v6.0 */
USB_DEV_FLAG_IS_HOST,
USB_DEV_FLAG_MSOS_DESC_ENABLE,
USB_DEV_FLAG_MSOS_DESC_IN_USE,
--
2.27.0
[PATCH 0/4] Remove the deprecated pc-1.x machine types and related stuff
The pc-1.x machine types have been deprecated since QEMU v5.0 already, and nobody complained, so they can now be removed. While we're at it, also remove some compatibility switches and related code that are now not necessary anymore after these machine types have been removed. (We could maybe even remove more stuff like the various host_features switches in the virtio devices, but they still might be useful in certain cases, so I decided not to remove them yet) Thomas Huth (4): hw/i386: Remove the deprecated pc-1.x machine types hw/block/fdc: Remove the check_media_rate property hw/virtio/virtio-balloon: Remove the "class" property hw/usb/bus: Remove the "full-path" property docs/system/deprecated.rst | 6 -- docs/system/removed-features.rst | 6 ++ hw/block/fdc.c | 17 +- hw/i386/pc_piix.c| 94 hw/usb/bus.c | 7 +-- hw/virtio/virtio-balloon-pci.c | 11 +--- include/hw/usb.h | 2 +- tests/qemu-iotests/172.out | 35 8 files changed, 11 insertions(+), 167 deletions(-) -- 2.27.0
[PATCH 1/4] hw/i386: Remove the deprecated pc-1.x machine types
They have been deprecated since QEMU v5.0, time to remove them now.
Signed-off-by: Thomas Huth
---
docs/system/deprecated.rst | 6 --
docs/system/removed-features.rst | 6 ++
hw/i386/pc_piix.c| 94
3 files changed, 6 insertions(+), 100 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 6ac757ed9f..2fcac7861e 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -322,12 +322,6 @@ The 'scsi-disk' device is deprecated. Users should use
'scsi-hd' or
System emulator machines
-``pc-1.0``, ``pc-1.1``, ``pc-1.2`` and ``pc-1.3`` (since 5.0)
-'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-These machine types are very old and likely can not be used for live migration
-from old QEMU versions anymore. A newer machine type should be used instead.
-
Raspberry Pi ``raspi2`` and ``raspi3`` machines (since 5.2)
'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 88b81a6156..c8481cafbd 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -136,6 +136,12 @@ mips ``fulong2e`` machine alias (removed in 6.0)
This machine has been renamed ``fuloong2e``.
+``pc-1.0``, ``pc-1.1``, ``pc-1.2`` and ``pc-1.3`` (removed in 6.0)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+These machine types were very old and likely could not be used for live
+migration from old QEMU versions anymore. Use a newer machine type instead.
+
Related binaries
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6188c3e97e..2904b40163 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -359,18 +359,6 @@ static void pc_compat_1_4_fn(MachineState *machine)
pc_compat_1_5_fn(machine);
}
-static void pc_compat_1_3(MachineState *machine)
-{
-pc_compat_1_4_fn(machine);
-}
-
-/* PC compat function for pc-1.0 to pc-1.2 */
-static void pc_compat_1_2(MachineState *machine)
-{
-pc_compat_1_3(machine);
-x86_cpu_change_kvm_default("kvm-pv-eoi", NULL);
-}
-
static void pc_init_isa(MachineState *machine)
{
pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, TYPE_I440FX_PCI_DEVICE);
@@ -772,88 +760,6 @@ static void pc_i440fx_1_4_machine_options(MachineClass *m)
DEFINE_I440FX_MACHINE(v1_4, "pc-i440fx-1.4", pc_compat_1_4_fn,
pc_i440fx_1_4_machine_options);
-static void pc_i440fx_1_3_machine_options(MachineClass *m)
-{
-X86MachineClass *x86mc = X86_MACHINE_CLASS(m);
-static GlobalProperty compat[] = {
-PC_CPU_MODEL_IDS("1.3.0")
-{ "usb-tablet", "usb_version", "1" },
-{ "virtio-net-pci", "ctrl_mac_addr", "off" },
-{ "virtio-net-pci", "mq", "off" },
-{ "e1000", "autonegotiation", "off" },
-};
-
-pc_i440fx_1_4_machine_options(m);
-m->hw_version = "1.3.0";
-m->deprecation_reason = "use a newer machine type instead";
-x86mc->compat_apic_id_mode = true;
-compat_props_add(m->compat_props, compat, G_N_ELEMENTS(compat));
-}
-
-DEFINE_I440FX_MACHINE(v1_3, "pc-1.3", pc_compat_1_3,
- pc_i440fx_1_3_machine_options);
-
-
-static void pc_i440fx_1_2_machine_options(MachineClass *m)
-{
-static GlobalProperty compat[] = {
-PC_CPU_MODEL_IDS("1.2.0")
-{ "nec-usb-xhci", "msi", "off" },
-{ "nec-usb-xhci", "msix", "off" },
-{ "qxl", "revision", "3" },
-{ "qxl-vga", "revision", "3" },
-{ "VGA", "mmio", "off" },
-};
-
-pc_i440fx_1_3_machine_options
[PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
This was only required for the pc-1.0 and earlier machine types.
Now that these have been removed, we can also drop the corresponding
code from the FDC device.
Signed-off-by: Thomas Huth
---
hw/block/fdc.c | 17 ++---
tests/qemu-iotests/172.out | 35 ---
2 files changed, 2 insertions(+), 50 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 292ea87805..198940e737 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -874,7 +874,6 @@ struct FDCtrl {
FloppyDriveType type;
} qdev_for_drives[MAX_FD];
int reset_sensei;
-uint32_t check_media_rate;
FloppyDriveType fallback; /* type=auto failure fallback */
/* Timers state */
uint8_t timer0;
@@ -1021,18 +1020,10 @@ static const VMStateDescription
vmstate_fdrive_media_changed = {
}
};
-static bool fdrive_media_rate_needed(void *opaque)
-{
-FDrive *drive = opaque;
-
-return drive->fdctrl->check_media_rate;
-}
-
static const VMStateDescription vmstate_fdrive_media_rate = {
.name = "fdrive/media_rate",
.version_id = 1,
.minimum_version_id = 1,
-.needed = fdrive_media_rate_needed,
.fields = (VMStateField[]) {
VMSTATE_UINT8(media_rate, FDrive),
VMSTATE_END_OF_LIST()
@@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int
direction)
/* Check the data rate. If the programmed data rate does not match
* the currently inserted medium, the operation has to fail. */
-if (fdctrl->check_media_rate &&
-(fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
@@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
}
/* READ_ID can't automatically succeed! */
-if (fdctrl->check_media_rate &&
-(fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
@@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.qdev_for_drives[0].blk),
DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.qdev_for_drives[1].blk),
-DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
-0, true),
DEFINE_PROP_SIGNED("fdtypeA", FDCtrlISABus, state.qdev_for_drives[0].type,
FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
FloppyDriveType),
diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index 2cd4a8fd83..349ae51d6c 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -14,7 +14,6 @@ Testing:
dma = 2 (0x2)
driveA = ""
driveB = ""
-check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -44,7 +43,6 @@ Testing: -fda TEST_DIR/t.qcow2
dma = 2 (0x2)
driveA = ""
driveB = ""
-check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -84,7 +82,6 @@ Testing: -fdb TEST_DIR/t.qcow2
dma = 2 (0x2)
driveA = ""
driveB = ""
-check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -139,7 +136,6 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2
dma = 2 (0x2)
driveA = ""
driveB = ""
-check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -195,7 +191,6 @@ Testing: -fdb
dma = 2 (0x2)
driveA = ""
driveB = ""
-check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -236,7 +231,6 @@ Testing: -drive if=floppy,file=TEST_DI
Re: [PATCH 4/4] hw/usb/bus: Remove the "full-path" property
On 04/02/2021 09.36, Gerd Hoffmann wrote:
Hi,
enum USBDeviceFlags {
-USB_DEV_FLAG_FULL_PATH,
+USB_DEV_FLAG_FULL_PATH, /* unused since QEMU v6.0 */
Why not just drop it? Any remaining users?
I didn't want to change the values of the other members of the enum ... but
if you prefer, I can also a "= 1" after the next member of the enum and
remove the USB_DEV_FLAG_FULL_PATH instead.
Thomas
Re: [PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
On 05/02/2021 01.40, John Snow wrote:
On 2/3/21 12:18 PM, Thomas Huth wrote:
This was only required for the pc-1.0 and earlier machine types.
Now that these have been removed, we can also drop the corresponding
code from the FDC device.
Signed-off-by: Thomas Huth
---
hw/block/fdc.c | 17 ++---
tests/qemu-iotests/172.out | 35 ---
2 files changed, 2 insertions(+), 50 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 292ea87805..198940e737 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -874,7 +874,6 @@ struct FDCtrl {
FloppyDriveType type;
} qdev_for_drives[MAX_FD];
int reset_sensei;
- uint32_t check_media_rate;
I am a bit of a dunce when it comes to the compatibility properties... does
this mess with the migration format?
I guess it doesn't, since it's not in the VMSTATE declaration.
H, alright.
I think that should be fine, yes.
FloppyDriveType fallback; /* type=auto failure fallback */
/* Timers state */
uint8_t timer0;
@@ -1021,18 +1020,10 @@ static const VMStateDescription
vmstate_fdrive_media_changed = {
}
};
-static bool fdrive_media_rate_needed(void *opaque)
-{
- FDrive *drive = opaque;
-
- return drive->fdctrl->check_media_rate;
-}
-
static const VMStateDescription vmstate_fdrive_media_rate = {
.name = "fdrive/media_rate",
.version_id = 1,
.minimum_version_id = 1,
- .needed = fdrive_media_rate_needed,
.fields = (VMStateField[]) {
VMSTATE_UINT8(media_rate, FDrive),
VMSTATE_END_OF_LIST()
@@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl,
int direction)
/* Check the data rate. If the programmed data rate does not match
* the currently inserted medium, the operation has to fail. */
- if (fdctrl->check_media_rate &&
- (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+ if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
fdctrl->dsr & FD_DSR_DRATEMASK,
cur_drv->media_rate);
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
@@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
}
/* READ_ID can't automatically succeed! */
- if (fdctrl->check_media_rate &&
- (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+ if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
fdctrl->dsr & FD_DSR_DRATEMASK,
cur_drv->media_rate);
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
@@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
DEFINE_PROP_DRIVE("driveA", FDCtrlISABus,
state.qdev_for_drives[0].blk),
DEFINE_PROP_DRIVE("driveB", FDCtrlISABus,
state.qdev_for_drives[1].blk),
- DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus,
state.check_media_rate,
- 0, true),
Could you theoretically set this via QOM commands in QMP, and claim that
this is a break in behavior?
Though, it's ENTIRELY undocumented, so ... it's probably fine, I think.
Probably. (Please soothe my troubled mind.)
A user actually could mess with this property even on the command line, e.g.
by using:
qemu-system-x86_64 -global isa-fdc.check_media_rate=false
... but, as you said, it's completely undocumented, the property is really
just there for the internal use of machine type compatibility. We've done
such clean-ups in the past already, see e.g. c6026998eef382d7ad76 or
2a4dbaf1c0db2453ab78f, so I think this should be fine. But if you disagree,
I could replace this by a patch that adds this property to the list of
deprecated features instead, so we could at least remove it after it has
been deprecated for two releases?
Thomas
Re: [PULL v2 00/27] Block patches
On 05/02/2021 17.23, Peter Maydell wrote: On Fri, 5 Feb 2021 at 16:21, Stefan Hajnoczi wrote: Thanks, I update the patch in question. It looks like the GitLab CI doesn't include a clang version that produces this error because the pipeline passed for me: https://gitlab.com/stefanha/qemu/-/pipelines/251524779 Is there something clang-specific you want to check in the CI? Maybe clang 3.4, the oldest version supported according to ./configure? Would probably be nice I guess. My ad-hoc builds use clang 6, which is what tripped up here. We should maybe discuss first whether we can bump the minimum version of Clang that we would like to support. I once picked Clang 3.4 since that was available in EPEL for RHEL7, but I think there were newer versions of Clang available in RHEL7 via other repos later, so 3.4 is likely really just way too old now... According to https://developers.redhat.com/HW/ClangLLVM-RHEL-7 there was at least Clang 7.0 available on RHEL7. Debian stable seems to have at least 7.0, too, according to repology.org. Ubuntu 18.04 seems to have version 6, but later ones are available via updates? Anyway, I think we could at least bump the minimum version to 6.0 nowadays... Thomas
Re: [PULL v3 11/27] multi-process: setup PCI host bridge for remote device
On 06/02/2021 18.57, Philippe Mathieu-Daudé wrote:
On 2/5/21 5:44 PM, Stefan Hajnoczi wrote:
From: Jagannathan Raman
PCI host bridge is setup for the remote device process. It is
implemented using remote-pcihost object. It is an extension of the PCI
host bridge setup by QEMU.
Remote-pcihost configures a PCI bus which could be used by the remote
PCI device to latch on to.
Signed-off-by: Jagannathan Raman
Signed-off-by: John G Johnson
Signed-off-by: Elena Ufimtseva
Reviewed-by: Stefan Hajnoczi
Message-id:
0871ba857abb2eafacde07e7fe66a3f12415bfb2.1611938319.git.jag.ra...@oracle.com
Signed-off-by: Stefan Hajnoczi
---
MAINTAINERS | 2 +
include/hw/pci-host/remote.h | 29 ++
hw/pci-host/remote.c | 75
hw/pci-host/Kconfig | 3 ++
hw/pci-host/meson.build | 1 +
hw/remote/Kconfig| 1 +
6 files changed, 111 insertions(+)
create mode 100644 include/hw/pci-host/remote.h
create mode 100644 hw/pci-host/remote.c
...
+static const TypeInfo remote_pcihost_info = {
+.name = TYPE_REMOTE_PCIHOST,
+.parent = TYPE_PCIE_HOST_BRIDGE,
^
+.instance_size = sizeof(RemotePCIHost),
+.class_init = remote_pcihost_class_init,
+};
+
+static void remote_pcihost_register(void)
+{
+type_register_static(&remote_pcihost_info);
+}
+
+type_init(remote_pcihost_register)
diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
index eb03f0489d..8b8c763c28 100644
--- a/hw/pci-host/Kconfig
+++ b/hw/pci-host/Kconfig
@@ -65,3 +65,6 @@ config PCI_POWERNV
select PCI_EXPRESS
select MSI_NONBROKEN
select PCIE_PORT
+
+config REMOTE_PCIHOST
+bool
select CONFIG_PCI_EXPRESS ?
(Reported by Peter Maydell on s390x)
Side question, does it make sense to enable this feature by
default on all architectures?
Certainly not.
Thomas
Re: [PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
On 05/02/2021 21.15, John Snow wrote:
On 2/5/21 1:37 AM, Thomas Huth wrote:
On 05/02/2021 01.40, John Snow wrote:
On 2/3/21 12:18 PM, Thomas Huth wrote:
This was only required for the pc-1.0 and earlier machine types.
Now that these have been removed, we can also drop the corresponding
code from the FDC device.
Signed-off-by: Thomas Huth
---
hw/block/fdc.c | 17 ++---
tests/qemu-iotests/172.out | 35 ---
2 files changed, 2 insertions(+), 50 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 292ea87805..198940e737 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -874,7 +874,6 @@ struct FDCtrl {
FloppyDriveType type;
} qdev_for_drives[MAX_FD];
int reset_sensei;
- uint32_t check_media_rate;
I am a bit of a dunce when it comes to the compatibility properties...
does this mess with the migration format?
I guess it doesn't, since it's not in the VMSTATE declaration.
H, alright.
I think that should be fine, yes.
FloppyDriveType fallback; /* type=auto failure fallback */
/* Timers state */
uint8_t timer0;
@@ -1021,18 +1020,10 @@ static const VMStateDescription
vmstate_fdrive_media_changed = {
}
};
-static bool fdrive_media_rate_needed(void *opaque)
-{
- FDrive *drive = opaque;
-
- return drive->fdctrl->check_media_rate;
-}
-
static const VMStateDescription vmstate_fdrive_media_rate = {
.name = "fdrive/media_rate",
.version_id = 1,
.minimum_version_id = 1,
- .needed = fdrive_media_rate_needed,
.fields = (VMStateField[]) {
VMSTATE_UINT8(media_rate, FDrive),
VMSTATE_END_OF_LIST()
@@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl,
int direction)
/* Check the data rate. If the programmed data rate does not match
* the currently inserted medium, the operation has to fail. */
- if (fdctrl->check_media_rate &&
- (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+ if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
fdctrl->dsr & FD_DSR_DRATEMASK,
cur_drv->media_rate);
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
@@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
}
/* READ_ID can't automatically succeed! */
- if (fdctrl->check_media_rate &&
- (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+ if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
fdctrl->dsr & FD_DSR_DRATEMASK,
cur_drv->media_rate);
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
@@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
DEFINE_PROP_DRIVE("driveA", FDCtrlISABus,
state.qdev_for_drives[0].blk),
DEFINE_PROP_DRIVE("driveB", FDCtrlISABus,
state.qdev_for_drives[1].blk),
- DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus,
state.check_media_rate,
- 0, true),
Could you theoretically set this via QOM commands in QMP, and claim that
this is a break in behavior?
Though, it's ENTIRELY undocumented, so ... it's probably fine, I think.
Probably. (Please soothe my troubled mind.)
A user actually could mess with this property even on the command line,
e.g. by using:
qemu-system-x86_64 -global isa-fdc.check_media_rate=false
... but, as you said, it's completely undocumented, the property is really
just there for the internal use of machine type compatibility. We've done
such clean-ups in the past already, see e.g. c6026998eef382d7ad76 or
2a4dbaf1c0db2453ab78f, so I think this should be fine. But if you
disagree, I could replace this by a patch that adds this property to the
list of deprecated features instead, so we could at least remove it after
it has been deprecated for two releases?
I don't think it's necessary, personally -- just wanted to make sure I knew
the exact stakes here.
Reviewed-by: John Snow
Acked-by: John Snow
Thanks! ... since the first patch has already been merged through Michael's
tree, could you then please take this patch here through your floppy / block
branch, John? Or maybe it could also go via qemu-trivial?
Thomas
Re: [PULL v3 00/27] Block patches
On 08/02/2021 21.21, Stefan Hajnoczi wrote:
On Mon, Feb 08, 2021 at 11:02:57AM +0100, Philippe Mathieu-Daudé wrote:
On 2/8/21 10:27 AM, Stefan Hajnoczi wrote:
On Sat, Feb 06, 2021 at 05:03:20PM +, Peter Maydell wrote:
On Fri, 5 Feb 2021 at 22:53, Peter Maydell wrote:
On Fri, 5 Feb 2021 at 16:45, Stefan Hajnoczi wrote:
The following changes since commit e2c5093c993ef646e4e28f7aa78429853bcc06ac:
iotests: 30: drop from auto group (and effectively from make check)
(2021-02-05 15:16:13 +)
are available in the Git repository at:
https://gitlab.com/stefanha/qemu.git tags/block-pull-request
for you to fetch changes up to b07011f375bda3319cf72eee7cb18d310078387b:
docs: fix Parallels Image "dirty bitmap" section (2021-02-05 16:36:36 +)
Pull request
v3:
* Replace {0} array initialization with {} to make clang happy [Peter]
Fails 'make check' on s390x host:
I gave this a rerun to check it was reproducible (it is) and realised
I missed what looks like an important line in the log. As usual,
trying to disentangle which lines of a parallel make check correspond
to the failure is pretty tricky, but the lines
Type 'remote-pcihost' is missing its parent 'pcie-host-bridge'
are probably the proximate causes of the assertion failures.
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/ubuntu/qemu/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_BINARY=./qemu-system-rx tests/qtest/qos-test --tap -k
PASS 45 qtest-rx/qmp-cmd-test /rx/qmp/query-memory-size-summary
SKIP
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/ubuntu/qemu/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_BINARY=./qemu-system-s390x tests/qtest/pxe-test --tap -k
PASS 46 qtest-rx/qmp-cmd-test /rx/qmp/query-memory-devices
Type 'remote-pcihost' is missing its parent 'pcie-host-bridge'
PASS 47 qtest-rx/qmp-cmd-test /rx/qmp/query-replay
PASS 48 qtest-rx/qmp-cmd-test /rx/qmp/query-yank
PASS 49 qtest-rx/qmp-cmd-test /rx/qmp/query-name
PASS 50 qtest-rx/qmp-cmd-test /rx/qmp/query-iothreads
PASS 51 qtest-rx/qmp-cmd-test /rx/qmp/query-fdsets
PASS 52 qtest-rx/qmp-cmd-test /rx/qmp/query-command-line-options
PASS 53 qtest-rx/qmp-cmd-test /rx/qmp/query-acpi-ospm-status
PASS 54 qtest-rx/qmp-cmd-test /rx/qmp/object-add-failure-modes
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/ubuntu/qemu/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_BINARY=./qemu-system-s390x tests/qtest/test-netfilter --tap
-k
Type 'remote-pcihost' is missing its parent 'pcie-host-bridge'
socket_accept failed: Resource temporarily unavailable
socket_accept failed: Resource temporarily unavailable
**
ERROR:../../tests/qtest/libqtest.c:308:qtest_init_without_qmp_handshake:
assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
**
ERROR:../../tests/qtest/libqtest.c:308:qtest_init_without_qmp_handshake:
assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
../../tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from
signal 6 (Aborted) (core dumped)
../../tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from
signal 6 (Aborted) (core dumped)
ERROR qtest-s390x/pxe-test - Bail out!
ERROR:../../tests/qtest/libqtest.c:308:qtest_init_without_qmp_handshake:
assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
ERROR qtest-s390x/test-netfilter - Bail out!
ERROR:../../tests/qtest/libqtest.c:308:qtest_init_without_qmp_handshake:
assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
Makefile.mtest:3113: recipe for target 'run-test-387' failed
make: *** [run-test-387] Error 1
make: *** Waiting for unfinished jobs
Makefile.mtest:3121: recipe for target 'run-test-388' failed
Hi Elena and Jag,
Please take a look at this QOM failure. I guess remote-pcihost is being
built but pcie-host-bridge is missing from the s390x-softmmu target.
Fix suggested here:
https://www.mail-archive.com/qemu-block@nongnu.org/msg80536.html
But beside the fix what would be better is to restrict this feature
where it makes sense (we are having hard time building/testing all
features, better enable new ones where they are used).
Would it be enough to enable this feature on X86 hosts/targets for
mainstream CI?
Trying to check if I understand correctly:
Instead of writing configure/meson rules that enable the feature
whenever the dependencies are satisfied (KVM and PCI), explicitly enable
it on x86 targets only. The rationale is that it's not being used and
hasn't been tested on non-x86 targets, so it's not really supported
there yet.
I haven't looked very close at the patches, but if I got that right, the
problem is that this features depends on the availability of a certain PCI-e
device. So the easiest solution is maybe to add a "depends on PCI_EXPRESS"
in the "config MULTIPROCESS",
Re: [PATCH 2/2] travis: remove travis configuration and all references to Travis CI
On 09/02/2021 14.50, Daniel P. Berrangé wrote: The Travis CI system QEMU has been using has removed the unlimited free usage model, replacing it with a one-time only grant of CI minutes that is not renewed. The QEMU CI jobs quickly exhaust maintainer's free CI credits, leaving them unable to test with Travis. This is not a sustainable situation, so we have no choice by to discontinue use of Travis. GitLab CI is now the primary target, with Cirrus CI filling in some platform gaps where needed. I've currently got a series in flight that moves some of the remaining jobs to gitlab-CI: https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01924.html Could you please hold this patch 'til my series got merged first? Also I think we could still wait some more weeks with the final removal of the travis-CI either 'til travis-ci.org got shut down completely (and thus we cannot use it for QEMU at all anymore), or until we finally got the s390x and aarch64 runners up and running in the gitlab-CI. Thomas
Re: [PATCH v2] hw/block: nvme: Fix a build error in nvme_get_feature()
On 10/02/2021 12.15, Bin Meng wrote: Hi Philippe, On Wed, Feb 10, 2021 at 7:12 PM Philippe Mathieu-Daudé wrote: Hi Bin, On 2/10/21 11:23 AM, Bin Meng wrote: From: Bin Meng Current QEMU HEAD nvme.c does not compile: hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized] trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled"); ^ hw/block/nvme.c:3150:14: note: ‘result’ was declared here uint32_t result; ^ Why isn't this catched by our CI? What is your host OS? Fedora 33? I am using GCC 5.4 on Ubuntu 16.04. Please see some initial analysis from Peter about why newer version GCC does not report it. Please note that Ubuntu 16.04 is not a supported version by QEMU anymore, see https://qemu.readthedocs.io/en/latest/system/build-platforms.html and https://wiki.qemu.org/Supported_Build_Platforms for details. Thomas
[PATCH] tests/qemu-iotests: Remove test 259 from the "auto" group
Tests in the "auto" group should support qcow2 so that they can be run during "make check-block". Test 259 only supports "raw", so it currently always gets skipped when running "make check-block". Let's skip this unnecessary step and remove it from the auto group. Signed-off-by: Thomas Huth --- tests/qemu-iotests/259 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/259 b/tests/qemu-iotests/259 index 76cde429c4..1b15e8fb48 100755 --- a/tests/qemu-iotests/259 +++ b/tests/qemu-iotests/259 @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# group: rw auto quick +# group: rw quick # # Test generic image creation fallback (by using NBD) # -- 2.27.0
Re: [PATCH v9 0/6] Rework iotests/check
On 16/02/2021 02.49, John Snow wrote: On 1/26/21 10:15 AM, Vladimir Sementsov-Ogievskiy wrote: OK, thanks for handling it! When will we move to python 3.7? [...] As for RHEL/CentOS, I think it's in the same shape right now. It's 3.6-based, but I don't know if there's an optional 3.7+ package or not. There is a python 3.8 package available for RHEL8. And according to the QEMU support policy, we will soon (IIRC in May 2021) stop supporting RHEL7. Thomas
Re: [PATCH 01/14] ui, monitor: remove deprecated VNC ACL option and HMP commands
On 24/02/2021 14.11, Daniel P. Berrangé wrote: The VNC ACL concept has been replaced by the pluggable "authz" framework which does not use monitor commands. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Daniel P. Berrangé --- docs/system/deprecated.rst | 16 --- docs/system/removed-features.rst | 13 +++ hmp-commands.hx | 76 - monitor/misc.c | 187 --- ui/vnc.c | 38 --- 5 files changed, 13 insertions(+), 317 deletions(-) If I run: grep -r vnc.*acl * I also see some lines in tests/check-block-qdict.c ... are they related and should be removed, too? Apart from that, patch looks fine to me: Reviewed-by: Thomas Huth
Re: [PATCH 03/14] monitor: remove 'query-events' QMP command
On 24/02/2021 14.11, Daniel P. Berrangé wrote: The code comment suggests removing QAPIEvent_(str|lookup) symbols too, however, these are both auto-generated as standard for any enum in QAPI. As such it they'll exist whether we use them or not. Signed-off-by: Daniel P. Berrangé --- docs/system/deprecated.rst | 6 - docs/system/removed-features.rst | 6 + monitor/qmp-cmds-control.c | 24 - qapi/control.json| 45 4 files changed, 6 insertions(+), 75 deletions(-) Reviewed-by: Thomas Huth
Re: [PATCH 04/14] softmmu: remove '-usbdevice' command line option
On 24/02/2021 14.11, Daniel P. Berrangé wrote: This was replaced by the '-device usb-DEV' option. Signed-off-by: Daniel P. Berrangé --- docs/system/deprecated.rst | 9 --- docs/system/removed-features.rst | 9 +++ softmmu/vl.c | 42 3 files changed, 9 insertions(+), 51 deletions(-) Last time I tried to remove -usbdevice, there was some concerns that -usbdevice braille might still be useful for some people, see the thread that started here: https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg00651.html (and Gerd's summary here: https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg01520.html ) So we might need a new "sugared" option like "-braille" instead before we can fully remove -usbdevice? ... or we just keep -usbdevice as a bittersweet remainder? Thomas
Re: [PATCH 09/14] hw/ide: remove 'ide-drive' device
On 24/02/2021 14.11, Daniel P. Berrangé wrote:
The 'ide-hd' and 'ide-cd' devices provide suitable alternatives.
Signed-off-by: Daniel P. Berrangé
---
docs/qdev-device-use.txt | 2 +-
docs/system/deprecated.rst | 6 -
docs/system/removed-features.rst | 9
hw/i386/pc.c | 1 -
hw/ide/qdev.c| 38
hw/ppc/mac_newworld.c| 13 ---
hw/ppc/mac_oldworld.c| 13 ---
hw/sparc64/sun4u.c | 14
scripts/device-crash-test| 1 -
softmmu/vl.c | 1 -
tests/qemu-iotests/051 | 2 --
tests/qemu-iotests/051.pc.out| 10 -
12 files changed, 10 insertions(+), 100 deletions(-)
diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index 245cdf29c7..2408889334 100644
--- a/docs/qdev-device-use.txt
+++ b/docs/qdev-device-use.txt
@@ -388,7 +388,7 @@ type.
some DEVNAMEs:
default device suppressing DEVNAMEs
-CD-ROM ide-cd, ide-drive, ide-hd, scsi-cd, scsi-hd
+CD-ROM ide-cd, ide-hd, scsi-cd, scsi-hd
floppy floppy, isa-fdc
parallelisa-parallel
serial isa-serial
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index c69887dca8..f5c82a46dc 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -242,12 +242,6 @@ this CPU is also deprecated.
System emulator devices
---
-``ide-drive`` (since 4.2)
-'''''''''''''''''''''''''
-
-The 'ide-drive' device is deprecated. Users should use 'ide-hd' or
-'ide-cd' as appropriate to get an IDE hard disk or CD-ROM as needed.
-
``scsi-disk`` (since 4.2)
'''''''''''''''''''''''''
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 870a222062..8fd3fafb32 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -213,6 +213,15 @@ This machine has been renamed ``fuloong2e``.
These machine types were very old and likely could not be used for live
migration from old QEMU versions anymore. Use a newer machine type instead.
+System emulator devices
+---
+
+``ide-drive`` (removed in 6.0)
+''''''''''''''''''''''''''''''
+
+The 'ide-drive' device has been removed. Users should use 'ide-hd' or
+'ide-cd' as appropriate to get an IDE hard disk or CD-ROM as needed.
+
Related binaries
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8aa85dec54..828122e21e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -342,7 +342,6 @@ GlobalProperty pc_compat_1_4[] = {
{ "scsi-disk", "discard_granularity", "0" },
{ "ide-hd", "discard_granularity", "0" },
{ "ide-cd", "discard_granularity", "0" },
-{ "ide-drive", "discard_granularity", "0" },
{ "virtio-blk-pci", "discard_granularity", "0" },
/* DEV_NVECTORS_UNSPECIFIED as a uint32_t string: */
{ "virtio-serial-pci", "vectors", "0x" },
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 8cd19fa5e9..e70ebc83a0 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -283,20 +283,6 @@ static void ide_cd_realize(IDEDevice *dev, Error **errp)
ide_dev_initfn(dev, IDE_CD, errp);
}
-static void ide_drive_realize(IDEDevice *dev, Error **errp)
-{
-DriveInfo *dinfo = NULL;
-
-warn_report("'ide-drive' is deprecated, "
-"please use 'ide-hd' or 'ide-cd' instead");
-
-if (dev->conf.blk) {
-dinfo = blk_legacy_dinfo(dev->conf.blk);
-}
-
-ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD, errp);
-}
I wonder whether we now could also make the "media" parameter of "-drive" as
deprecated?
Anyway, for this patch:
Reviewed-by: Thomas Huth
Re: [PATCH 10/14] hw/scsi: remove 'scsi-disk' device
On 24/02/2021 14.11, Daniel P. Berrangé wrote:
The 'scsi-hd' and 'scsi-cd' devices provide suitable alternatives.
Signed-off-by: Daniel P. Berrangé
---
docs/system/deprecated.rst | 9 -
docs/system/removed-features.rst | 6
hw/i386/pc.c | 1 -
hw/scsi/scsi-disk.c | 62
hw/sparc64/sun4u.c | 1 -
scripts/device-crash-test| 1 -
tests/qemu-iotests/051 | 2 --
tests/qemu-iotests/051.pc.out| 10 --
8 files changed, 6 insertions(+), 86 deletions(-)
I see some occurrances of "scsi-disk" in the config files in the
docs/config/ directory, too ... I guess they should also be removed?
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d7c27144ba..cda7df36e3 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -749,7 +749,6 @@ static char *sun4u_fw_dev_path(FWPathProvider *p, BusState
*bus,
DeviceState *dev)
{
PCIDevice *pci;
-int bus_id;
if (!strcmp(object_get_typename(OBJECT(dev)), "pbm-bridge")) {
pci = PCI_DEVICE(dev);
This lonely hunk should be squashed into the previous (ide-disk) patch instead.
Thomas
Re: [PATCH 11/14] block: remove 'encryption_key_missing' flag from QAPI
On 24/02/2021 14.11, Daniel P. Berrangé wrote:
This has been hardcoded to "false" since 2.10.0, since secrets required
to unlock block devices are now always provided upfront instead of using
interactive prompts.
Signed-off-by: Daniel P. Berrangé
---
block/qapi.c | 1 -
docs/system/deprecated.rst | 10 ---
docs/system/removed-features.rst | 10 +++
qapi/block-core.json | 8 --
tests/qemu-iotests/184.out | 6 ++--
tests/qemu-iotests/191.out | 48 +++-
tests/qemu-iotests/273.out | 15 --
7 files changed, 33 insertions(+), 65 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 84a0aadc09..3acc118c44 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -62,7 +62,6 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
info->ro = bs->read_only;
info->drv= g_strdup(bs->drv->format_name);
info->encrypted = bs->encrypted;
-info->encryption_key_missing = false;
info->cache = g_new(BlockdevCacheInfo, 1);
*info->cache = (BlockdevCacheInfo) {
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index cb88fea94f..e746a63edf 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -147,16 +147,6 @@ Use argument ``id`` instead.
Use argument ``id`` instead.
-``query-named-block-nodes`` result ``encryption_key_missing`` (since 2.10.0)
-''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-Always false.
-
-``query-block`` result ``inserted.encryption_key_missing`` (since 2.10.0)
-'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-Always false.
-
``blockdev-add`` empty string argument ``backing`` (since 2.10.0)
'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index bb6bc8dfc8..583f14f02e 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -112,6 +112,16 @@ chardev client socket with ``wait`` option (removed in 6.0)
Character devices creating sockets in client mode should not specify
the 'wait' field, which is only applicable to sockets in server mode
+``query-named-block-nodes`` result ``encryption_key_missing`` (removed in 6.0)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Always false.
Should that be "Removed with no replacement", too ? (just like the one below)
+``query-block`` result ``inserted.encryption_key_missing`` (removed in 6.0)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Removed with no replacement
+
Apart from that nit:
Reviewed-by: Thomas Huth
Re: [PATCH 04/14] softmmu: remove '-usbdevice' command line option
On 24/02/2021 15.10, Daniel P. Berrangé wrote: On Wed, Feb 24, 2021 at 02:58:19PM +0100, Thomas Huth wrote: On 24/02/2021 14.11, Daniel P. Berrangé wrote: This was replaced by the '-device usb-DEV' option. Signed-off-by: Daniel P. Berrangé --- docs/system/deprecated.rst | 9 --- docs/system/removed-features.rst | 9 +++ softmmu/vl.c | 42 3 files changed, 9 insertions(+), 51 deletions(-) Last time I tried to remove -usbdevice, there was some concerns that -usbdevice braille might still be useful for some people, see the thread that started here: https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg00651.html (and Gerd's summary here: https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg01520.html ) Urgh, so the current deprecation docs are a bit misleading by saying -usbdevice is directly mapped to -device. So we might need a new "sugared" option like "-braille" instead before we can fully remove -usbdevice? ... or we just keep -usbdevice as a bittersweet remainder? I'm not going to implement new CLI options, and if that's needed, we ought to re-start the clock on the deprecation at that point. So this points towards just removing the deprecation warning that exists today. Or alternatively drop support for -usbdevice, except for the braille type. After that discussion in 2018, I've removed all of the "annoying" -usbdevice parameters already (see commit 99761176eeaf8525). I then more or less waited for someone to step up and implement "-braille", but it never happened and I forgot about the removal of the remaining -usbdevice parameters. Thinking about this again, replacing "-usbdevice braille" with a "-braille usb" does indeed not buy us much, so I think the best is maybe to keep the simple devices and braille around, update our documentation and remove the deprecation warning instead. Thomas
Re: [PATCH v2 1/8] ui: Replace the word 'whitelist'
On 05/02/2021 18.18, Philippe Mathieu-Daudé wrote:
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the words "whitelist"
appropriately.
[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
Reviewed-by: Gerd Hoffmann
Signed-off-by: Philippe Mathieu-Daudé
---
v2: Do not use acronyms (danpb)
---
ui/console.c | 2 +-
ui/vnc-auth-sasl.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/ui/console.c b/ui/console.c
index c5d11bc7017..5a8311ced20 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1708,7 +1708,7 @@ bool dpy_gfx_check_format(QemuConsole *con,
return false;
}
} else {
-/* default is to whitelist native 32 bpp only */
+/* default is to allow native 32 bpp only */
if (format != qemu_default_pixman_format(32, true)) {
return false;
}
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index f67111a3662..df7dc08e9fc 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -288,7 +288,7 @@ static int protocol_client_auth_sasl_step(VncState *vs,
uint8_t *data, size_t le
goto authreject;
}
-/* Check username whitelist ACL */
+/* Check the username access control list */
if (vnc_auth_sasl_check_access(vs) < 0) {
goto authreject;
}
@@ -409,7 +409,7 @@ static int protocol_client_auth_sasl_start(VncState *vs,
uint8_t *data, size_t l
goto authreject;
}
-/* Check username whitelist ACL */
+/* Check the username access control list */
if (vnc_auth_sasl_check_access(vs) < 0) {
goto authreject;
}
Reviewed-by: Thomas Huth
Re: [PATCH v2 3/8] scripts/tracetool: Replace the word 'whitelist'
On 05/02/2021 18.18, Philippe Mathieu-Daudé wrote:
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the words "whitelist"
appropriately.
[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Philippe Mathieu-Daudé
---
scripts/tracetool/__init__.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 96b1cd69a52..5bc94d95cfc 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -100,7 +100,7 @@ def validate_type(name):
if bit == "const":
continue
if bit not in ALLOWED_TYPES:
-raise ValueError("Argument type '%s' is not in whitelist. "
+raise ValueError("Argument type '%s' is not allowed. "
"Only standard C types and fixed size integer "
"types should be used. struct, union, and "
"other complex pointer types should be "
Reviewed-by: Thomas Huth
Re: [PATCH v2 5/8] seccomp: Replace the word 'blacklist'
On 05/02/2021 18.18, Philippe Mathieu-Daudé wrote:
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.
[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
Reviewed-by: Daniel P. Berrangé
Acked-by: Eduardo Otubo
Signed-off-by: Philippe Mathieu-Daudé
---
softmmu/qemu-seccomp.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/softmmu/qemu-seccomp.c b/softmmu/qemu-seccomp.c
index 377ef6937ca..4c684bc9e71 100644
--- a/softmmu/qemu-seccomp.c
+++ b/softmmu/qemu-seccomp.c
@@ -45,8 +45,8 @@ const struct scmp_arg_cmp sched_setscheduler_arg[] = {
{ .arg = 1, .op = SCMP_CMP_NE, .datum_a = SCHED_IDLE }
};
-static const struct QemuSeccompSyscall blacklist[] = {
-/* default set of syscalls to blacklist */
+static const struct QemuSeccompSyscall denylist[] = {
+/* default set of syscalls to denylist */
Since it's used as a verb in the comment, I'd rather say something like this
here:
/* default set of syscalls that should get blocked */
... "denylist" still does not sound like a verb to me.
Thomas
Re: [PATCH v3 3/5] seccomp: Replace the word 'blacklist'
On 03/03/2021 19.46, Philippe Mathieu-Daudé wrote:
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.
[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
Reviewed-by: Daniel P. Berrangé
Acked-by: Eduardo Otubo
Reviewed-by: Alex Bennée
Signed-off-by: Philippe Mathieu-Daudé
---
v3: Reworded comment (thuth)
---
softmmu/qemu-seccomp.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/softmmu/qemu-seccomp.c b/softmmu/qemu-seccomp.c
index 377ef6937ca..9c29d9cf007 100644
--- a/softmmu/qemu-seccomp.c
+++ b/softmmu/qemu-seccomp.c
@@ -45,8 +45,8 @@ const struct scmp_arg_cmp sched_setscheduler_arg[] = {
{ .arg = 1, .op = SCMP_CMP_NE, .datum_a = SCHED_IDLE }
};
-static const struct QemuSeccompSyscall blacklist[] = {
-/* default set of syscalls to blacklist */
+static const struct QemuSeccompSyscall denylist[] = {
+/* default set of syscalls that should get blocked */
{ SCMP_SYS(reboot), QEMU_SECCOMP_SET_DEFAULT },
{ SCMP_SYS(swapon), QEMU_SECCOMP_SET_DEFAULT },
{ SCMP_SYS(swapoff),QEMU_SECCOMP_SET_DEFAULT },
@@ -175,18 +175,18 @@ static int seccomp_start(uint32_t seccomp_opts, Error
**errp)
goto seccomp_return;
}
-for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
+for (i = 0; i < ARRAY_SIZE(denylist); i++) {
uint32_t action;
-if (!(seccomp_opts & blacklist[i].set)) {
+if (!(seccomp_opts & denylist[i].set)) {
continue;
}
-action = qemu_seccomp_get_action(blacklist[i].set);
-rc = seccomp_rule_add_array(ctx, action, blacklist[i].num,
-blacklist[i].narg, blacklist[i].arg_cmp);
+action = qemu_seccomp_get_action(denylist[i].set);
+rc = seccomp_rule_add_array(ctx, action, denylist[i].num,
+denylist[i].narg, denylist[i].arg_cmp);
if (rc < 0) {
error_setg_errno(errp, -rc,
- "failed to add seccomp blacklist rules");
+ "failed to add seccomp denylist rules");
goto seccomp_return;
}
}
Reviewed-by: Thomas Huth
Re: [PATCH v3 4/5] qemu-options: Replace the word 'blacklist'
On 03/03/2021 19.46, Philippe Mathieu-Daudé wrote:
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.
[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Philippe Mathieu-Daudé
---
qemu-options.hx | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index 252db9357ca..8462dc5f158 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4299,12 +4299,12 @@ DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \
"use 'obsolete' to allow obsolete system calls that are
provided\n" \
"by the kernel, but typically no longer used by
modern\n" \
"C library implementations.\n" \
-"use 'elevateprivileges' to allow or deny QEMU process to
elevate\n" \
-"its privileges by blacklisting all set*uid|gid system
calls.\n" \
+"use 'elevateprivileges' to allow or deny the QEMU process
ability\n" \
+"to elevate privileges using set*uid|gid system
calls.\n" \
"The value 'children' will deny set*uid|gid system calls
for\n" \
"main QEMU process but will allow forks and execves to run
unprivileged\n" \
"use 'spawn' to avoid QEMU to spawn new threads or processes
by\n" \
-" blacklisting *fork and execve\n" \
+" blocking *fork and execve\n" \
"use 'resourcecontrol' to disable process affinity and
schedular priority\n",
QEMU_ARCH_ALL)
SRST
Reviewed-by: Thomas Huth
Re: [PATCH v3 5/5] tests/fp/fp-test: Replace the word 'blacklist'
On 03/03/2021 19.46, Philippe Mathieu-Daudé wrote:
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.
[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
Acked-by: Alex Bennée
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Philippe Mathieu-Daudé
---
tests/fp/fp-test.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/fp/fp-test.c b/tests/fp/fp-test.c
index 06ffebd6db1..5a4cad8c8b2 100644
--- a/tests/fp/fp-test.c
+++ b/tests/fp/fp-test.c
@@ -123,7 +123,7 @@ static void not_implemented(void)
fprintf(stderr, "Not implemented.\n");
}
-static bool blacklisted(unsigned op, int rmode)
+static bool is_allowed(unsigned op, int rmode)
{
/* odd has not been implemented for any 80-bit ops */
if (rmode == softfloat_round_odd) {
@@ -161,10 +161,10 @@ static bool blacklisted(unsigned op, int rmode)
case F32_TO_EXTF80:
case F64_TO_EXTF80:
case F128_TO_EXTF80:
-return true;
+return false;
}
}
-return false;
+return true;
}
static void do_testfloat(int op, int rmode, bool exact)
@@ -194,7 +194,7 @@ static void do_testfloat(int op, int rmode, bool exact)
verCases_writeFunctionName(stderr);
fputs("\n", stderr);
-if (blacklisted(op, rmode)) {
+if (!is_allowed(op, rmode)) {
not_implemented();
return;
}
Reviewed-by: Thomas Huth
Re: [PATCH v3 03/12] libqtest: add qtest_kill_qemu()
On 23/02/2021 15.46, Stefan Hajnoczi wrote: Tests that manage multiple processes may wish to kill QEMU before destroying the QTestState. Expose a function to do that. The vhost-user-blk-test testcase will need this. Signed-off-by: Stefan Hajnoczi Reviewed-by: Wainer dos Santos Moschetta --- tests/qtest/libqos/libqtest.h | 11 +++ tests/qtest/libqtest.c| 7 --- 2 files changed, 15 insertions(+), 3 deletions(-) Reviewed-by: Thomas Huth
Re: [PATCH v3 04/12] libqtest: add qtest_remove_abrt_handler()
On 23/02/2021 15.46, Stefan Hajnoczi wrote: Add a function to remove previously-added abrt handler functions. Now that a symmetric pair of add/remove functions exists we can also balance the SIGABRT handler installation. The signal handler was installed each time qtest_add_abrt_handler() was called. Now it is installed when the abrt handler list becomes non-empty and removed again when the list becomes empty. The qtest_remove_abrt_handler() function will be used by vhost-user-blk-test. Signed-off-by: Stefan Hajnoczi Reviewed-by: Wainer dos Santos Moschetta --- tests/qtest/libqos/libqtest.h | 18 ++ tests/qtest/libqtest.c| 35 +-- 2 files changed, 47 insertions(+), 6 deletions(-) Reviewed-by: Thomas Huth
Re: [PATCH v2] MAINTAINERS: Fix the location of tools manuals
On 09/03/2021 18.41, Wainer dos Santos Moschetta wrote:
Hi,
Any issue that prevent this of being queued?
Maybe it's just not clear who should take the patch ... CC:-ing qemu-trivial
and qemu-block now, since I think it could go through the trivial or block tree.
On 2/4/21 10:59 AM, Philippe Mathieu-Daudé wrote:
On 2/4/21 2:54 PM, Wainer dos Santos Moschetta wrote:
The qemu-img.rst, qemu-nbd.rst, virtfs-proxy-helper.rst,
qemu-trace-stap.rst,
and virtiofsd.rst manuals were moved to docs/tools, so this update
MAINTAINERS
accordingly.
Fixes: a08b4a9fe6c ("docs: Move tools documentation to tools manual")
Signed-off-by: Wainer dos Santos Moschetta
---
v1: was "MAINTAINERS: Fix the location of virtiofsd.rst"
v2: Fixed the location of all files [philmd]
MAINTAINERS | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 00626941f1..174425a941 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1829,7 +1829,7 @@ S: Odd Fixes
F: hw/9pfs/
X: hw/9pfs/xen-9p*
F: fsdev/
-F: docs/interop/virtfs-proxy-helper.rst
+F: docs/tools/virtfs-proxy-helper.rst
FWIW:
Reviewed-by: Thomas Huth
Re: [PATCH] fdc: fix floppy boot for Red Hat Linux 5.2
On 12/03/2021 07.32, John Snow wrote:
The image size indicates it's an 81 track floppy disk image, which we
don't have a listing for in the geometry table. When you force the drive
type to 1.44MB, it guesses the reasonably close 18/80. When the drive
type is allowed to auto-detect or set to 2.88, it guesses a very
incorrect geometry.
auto, 144 and 288 drive types get the right geometry with the new entry
in the table.
Reported-by: Michael Tokarev
Signed-off-by: John Snow
---
hw/block/fdc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 198940e737..b2f26ba587 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -122,6 +122,7 @@ static const FDFormat fd_formats[] = {
/* First entry is default format */
/* 1.44 MB 3"1/2 floppy disks */
{ FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 2880 */
+{ FLOPPY_DRIVE_TYPE_144, 18, 81, 1, FDRIVE_RATE_500K, },
{ FLOPPY_DRIVE_TYPE_144, 20, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 3200 */
{ FLOPPY_DRIVE_TYPE_144, 21, 80, 1, FDRIVE_RATE_500K, },
{ FLOPPY_DRIVE_TYPE_144, 21, 82, 1, FDRIVE_RATE_500K, },
That whole table-based approach seems quite unreliable to me - I've seen
floppy disks with 80, 81, 82 or sometimes even 83 tracks in the past, so I
think we would do better with a more flexible way of guessing ... but for
the time being, this is certainly a quick and easy fix that also should not
have any negative impact, thus:
Reviewed-by: Thomas Huth
Re: [PATCH 07/14] machine: remove 'arch' field from 'query-cpus-fast' QMP command
On 24/02/2021 14.11, Daniel P. Berrangé wrote: Signed-off-by: Daniel P. Berrangé --- docs/system/deprecated.rst | 6 - docs/system/removed-features.rst | 6 + hw/core/machine-qmp-cmds.c | 41 qapi/machine.json| 22 - 4 files changed, 6 insertions(+), 69 deletions(-) Patch looks reasonable to me. Reviewed-by: Thomas Huth
Re: [PATCH 10/14] hw/scsi: remove 'scsi-disk' device
On 24/02/2021 14.11, Daniel P. Berrangé wrote:
The 'scsi-hd' and 'scsi-cd' devices provide suitable alternatives.
Signed-off-by: Daniel P. Berrangé
---
docs/system/deprecated.rst | 9 -
docs/system/removed-features.rst | 6
hw/i386/pc.c | 1 -
hw/scsi/scsi-disk.c | 62
hw/sparc64/sun4u.c | 1 -
scripts/device-crash-test| 1 -
tests/qemu-iotests/051 | 2 --
tests/qemu-iotests/051.pc.out| 10 --
8 files changed, 6 insertions(+), 86 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index f5c82a46dc..cb88fea94f 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -239,15 +239,6 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which
is deprecated
(the ISA has never been upstreamed to a compiler toolchain). Therefore
this CPU is also deprecated.
-System emulator devices
-
-``scsi-disk`` (since 4.2)
-'''''''''''''''''''''''''
-
-The 'scsi-disk' device is deprecated. Users should use 'scsi-hd' or
-'scsi-cd' as appropriate to get a SCSI hard disk or CD-ROM as needed.
-
System emulator machines
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 8fd3fafb32..bb6bc8dfc8 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -222,6 +222,12 @@ System emulator devices
The 'ide-drive' device has been removed. Users should use 'ide-hd' or
'ide-cd' as appropriate to get an IDE hard disk or CD-ROM as needed.
+``scsi-disk`` (removed in 6.0)
+''''''''''''''''''''''''''''''
+
+The 'scsi-disk' device is deprecated. Users should use 'scsi-hd' or
+'scsi-cd' as appropriate to get a SCSI hard disk or CD-ROM as needed.
s/is deprecated/has been removed/
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d7c27144ba..cda7df36e3 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -749,7 +749,6 @@ static char *sun4u_fw_dev_path(FWPathProvider *p, BusState
*bus,
DeviceState *dev)
{
PCIDevice *pci;
-int bus_id;
if (!strcmp(object_get_typename(OBJECT(dev)), "pbm-bridge")) {
pci = PCI_DEVICE(dev);
Please squash this hunk into the 'ide-drive' patch instead.
With the two nits fixed:
Reviewed-by: Thomas Huth
Re: [PULL 5/5] m68k: add Virtual M68k Machine
On 18/03/2021 18.28, Max Reitz wrote: [...] From that it follows that I don’t see much use in testing specific devices either. Say there’s a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine for the iotests. I see little value in testing virtio-mmio as well. (Perhaps I’m short-sighted, though.) That's a fair point. But still, if someone compiled QEMU only with a target that only provided virtio-mmio, the iotests should not fail when running "make check". To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only run with x86, aarch64, s390x and ppc64 ? Thomas
Re: [PATCH 1/4] m68k: add the virtio devices aliases
On 18/03/2021 23.39, Laurent Vivier wrote:
Similarly to 5f629d943cb0 ("s390x: fix s390 virtio aliases"),
define the virtio aliases.
This allows to start machines with virtio devices without
knowledge of the implementation type.
For instance, we can use "-device virtio-scsi" on
m68k, s390x or PC, and the device will be
"virtio-scsi-device", "virtio-scsi-ccw" or "virtio-scsi-pci".
This already exists for s390x and -ccw interfaces, adds them
for m68k and MMIO (-device) interfaces.
Signed-off-by: Laurent Vivier
---
softmmu/qdev-monitor.c | 46 +++---
1 file changed, 30 insertions(+), 16 deletions(-)
With the typo mentioned by Philippe fixed:
Reviewed-by: Thomas Huth
Re: [PULL 5/5] m68k: add Virtual M68k Machine
On 19/03/2021 11.50, Laurent Vivier wrote: Le 19/03/2021 à 10:20, Max Reitz a écrit : On 19.03.21 07:32, Thomas Huth wrote: On 18/03/2021 18.28, Max Reitz wrote: [...] From that it follows that I don’t see much use in testing specific devices either. Say there’s a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine for the iotests. I see little value in testing virtio-mmio as well. (Perhaps I’m short-sighted, though.) That's a fair point. But still, if someone compiled QEMU only with a target that only provided virtio-mmio, the iotests should not fail when running "make check". To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only run with x86, aarch64, s390x and ppc64 ? Right, that would certainly be the simplest solution. The problem with that is we can't run the tests if target-list doesn't contain one of these targets. The iotests are skipped in quite a bunch of cases already anyway, e.g. when GNU sed or bash are not available in the right version. So I think it would be also ok to skip them in builds without one of the major architectures. Anyway, since your patches are already ready, I think we also could simply go with those this time, and reconsider tweaking tests/check-block.sh the next time we run into this issue. Thomas
Re: [PATCH v3 6/6] iotests: iothreads need ioeventfd
On 19/03/2021 21.23, Laurent Vivier wrote: And ioeventfd are only available with virtio-scsi-pci or virtio-scsi-ccw, use the alias but add a rule to require virtio-scsi-pci or virtio-scsi-ccw for the tests that use iothreads. Signed-off-by: Laurent Vivier --- tests/qemu-iotests/127| 3 ++- tests/qemu-iotests/256| 6 -- tests/qemu-iotests/common.rc | 13 + tests/qemu-iotests/iotests.py | 5 + 4 files changed, 24 insertions(+), 3 deletions(-) Reviewed-by: Thomas Huth
[RFC PATCH] block/vpc: Support probing of fixed-size VHD images
Fixed-size VHD images don't have a header, only a footer. To be able
to still detect them right, support probing via the file name, too.
Without this change, images get detected as raw:
$ qemu-img create -f vpc -o subformat=fixed test.vhd 2G
Formatting 'test.vhd', fmt=vpc size=2147483648 subformat=fixed
$ qemu-img info test.vhd
image: test.vhd
file format: raw
virtual size: 2 GiB (2147992064 bytes)
disk size: 8 KiB
With this change:
$ qemu-img info test.vhd
image: test.vhd
file format: vpc
virtual size: 2 GiB (2147991552 bytes)
disk size: 8 KiB
Resolves: https://bugs.launchpad.net/qemu/+bug/1819182
Signed-off-by: Thomas Huth
---
I've marked the subject with RFC since I'm not quite sure whether this
is really a good idea... please let me know what you think about it...
block/vpc.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/block/vpc.c b/block/vpc.c
index 17a705b482..be561e4b39 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -191,8 +191,18 @@ static uint32_t vpc_checksum(void *p, size_t size)
static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
{
-if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
+if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8)) {
return 100;
+}
+
+/* It could be a fixed-size image without header -> check extension, too */
+if (filename) {
+int len = strlen(filename);
+if (len > 4 && !strcasecmp(&filename[len - 4], ".vhd")) {
+return 10;
+}
+}
+
return 0;
}
--
2.27.0
Re: [PATCH for-6.0 v2 6/8] hw/block/nvme: update dmsrl limit on namespace detachment
On 06/04/2021 09.24, Klaus Jensen wrote:
On Apr 6 09:10, Philippe Mathieu-Daudé wrote:
On 4/5/21 7:54 PM, Klaus Jensen wrote:
From: Klaus Jensen
The Non-MDTS DMSRL limit must be recomputed when namespaces are
detached.
Fixes: 645ce1a70cb6 ("hw/block/nvme: support namespace attachment command")
Signed-off-by: Klaus Jensen
Reviewed-by: Gollu Appalanaidu
---
hw/block/nvme.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index de0e726dfdd8..3dc51f407671 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4876,6 +4876,21 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
return NVME_NO_COMPLETE;
}
+static void __nvme_update_dmrsl(NvmeCtrl *n)
+{
+int nsid;
+
+for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
+NvmeNamespace *ns = nvme_ns(n, nsid);
+if (!ns) {
+continue;
+}
+
+n->dmrsl = MIN_NON_ZERO(n->dmrsl,
+BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
+}
+}
+
static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
{
@@ -4925,6 +4940,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n,
NvmeRequest *req)
}
nvme_ns_detach(ctrl, ns);
+
+__nvme_update_dmrsl(ctrl);
}
Why the '__' prefix? It doesn't seem clearer (I'm not sure there is
a convention, it makes me think of a internal macro expansion use
for preprocessor).
There are very few uses of this prefix:
hw/9pfs/cofs.c:21:static ssize_t __readlink(V9fsState *s, V9fsPath
*path, V9fsString *buf)
hw/block/nvme.c:1683:static uint16_t __nvme_zrm_open(NvmeNamespace *ns,
NvmeZone *zone,
hw/block/nvme.c:1742:static void __nvme_advance_zone_wp(NvmeNamespace
*ns, NvmeZone *zone,
hw/block/nvme.c:5213:static void __nvme_select_ns_iocs(NvmeCtrl *n,
NvmeNamespace *ns)
hw/i386/amd_iommu.c:1160:static int __amdvi_int_remap_msi(AMDVIState *iommu,
hw/intc/s390_flic_kvm.c:255:static int __get_all_irqs(KVMS390FLICState
*flic,
hw/net/rocker/rocker_desc.c:199:static bool
__desc_ring_post_desc(DescRing *ring, int err)
hw/net/sungem.c:766:static uint16_t __sungem_mii_read(SunGEMState *s,
uint8_t phy_addr,
hw/ppc/ppc.c:867:static void __cpu_ppc_store_decr(PowerPCCPU *cpu,
uint64_t *nextp,
hw/s390x/pv.c:25:static int __s390_pv_cmd(uint32_t cmd, const char
*cmdname, void *data)
pc-bios/s390-ccw/cio.c:315:static int __do_cio(SubChannelId schid,
uint32_t ccw_addr, int fmt, Irb *irb)
target/ppc/mmu-hash64.c:170:static void __helper_slbie(CPUPPCState *env,
target_ulong addr,
Thomas, Eric, is it worth cleaning these and updating the
'CODESTYLE.rst'?
Yeah ok, I think you are right that there is no clear convention on when
to use this or not. I typically just use it for functions that are
normally not supposed to be called directly.
But I don't even think its consistent in the nvme device. For my sake,
we can clean it up, I'll drop it in this case since there is no good
reason for it other than my own idea of "style".
IIRC all identifiers that start with two underscores are reserved by the C
standard:
https://busybox.net/~landley/c99-draft.html#7.1.3
Thus you should not use two underscores at the beginning here at all.
HTH,
Thomas
[PATCH] block/file-posix: Fix problem with fallocate(PUNCH_HOLE) on GPFS
A customer reported that running
qemu-img convert -t none -O qcow2 -f qcow2 input.qcow2 output.qcow2
fails for them with the following error message when the images are
stored on a GPFS file system:
qemu-img: error while writing sector 0: Invalid argument
After analyzing the strace output, it seems like the problem is in
handle_aiocb_write_zeroes(): The call to fallocate(FALLOC_FL_PUNCH_HOLE)
returns EINVAL, which can apparently happen if the file system has
a different idea of the granularity of the operation. It's arguably
a bug in GPFS, since the PUNCH_HOLE mode should not result in EINVAL
according to the man-page of fallocate(), but the file system is out
there in production and so we have to deal with it. In commit 294682cc3a
("block: workaround for unaligned byte range in fallocate()") we also
already applied the a work-around for the same problem to the earlier
fallocate(FALLOC_FL_ZERO_RANGE) call, so do it now similar with the
PUNCH_HOLE call.
Signed-off-by: Thomas Huth
---
block/file-posix.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 20e14f8e96..7a40428d52 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1675,6 +1675,13 @@ static int handle_aiocb_write_zeroes(void *opaque)
}
s->has_fallocate = false;
} else if (ret != -ENOTSUP) {
+if (ret == -EINVAL) {
+/*
+ * File systems like GPFS do not like unaligned byte ranges,
+ * treat it like unsupported (so caller falls back to pwrite)
+ */
+return -ENOTSUP;
+}
return ret;
} else {
s->has_discard = false;
--
2.27.0
[PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
QEMU currently crashes when the user tries to do something like:
qemu-system-x86_64 -M x-remote -device piix3-ide
This happens because the "isabus" variable is not initialized with
the x-remote machine yet. Add a proper check for this condition
and propagate the error to the caller, so we can fail there gracefully.
Signed-off-by: Thomas Huth
---
hw/ide/ioport.c | 16 ++--
hw/ide/piix.c | 22 +-
hw/isa/isa-bus.c | 14 ++
include/hw/ide/internal.h | 2 +-
include/hw/isa/isa.h | 13 -
5 files changed, 46 insertions(+), 21 deletions(-)
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index b613ff3bba..e6caa537fa 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,15 +50,19 @@ static const MemoryRegionPortio ide_portio2_list[] = {
PORTIO_END_OF_LIST(),
};
-void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
{
+int ret;
+
/* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
bridge has been setup properly to always register with ISA. */
-isa_register_portio_list(dev, &bus->portio_list,
- iobase, ide_portio_list, bus, "ide");
+ret = isa_register_portio_list(dev, &bus->portio_list,
+ iobase, ide_portio_list, bus, "ide");
-if (iobase2) {
-isa_register_portio_list(dev, &bus->portio2_list,
- iobase2, ide_portio2_list, bus, "ide");
+if (ret == 0 && iobase2) {
+ret = isa_register_portio_list(dev, &bus->portio2_list,
+ iobase2, ide_portio2_list, bus, "ide");
}
+
+return ret;
}
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b9860e35a5..d3e738320b 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
#include "migration/vmstate.h"
+#include "qapi/error.h"
#include "qemu/module.h"
#include "sysemu/block-backend.h"
#include "sysemu/blockdev.h"
@@ -123,7 +124,8 @@ static void piix_ide_reset(DeviceState *dev)
pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
}
-static void pci_piix_init_ports(PCIIDEState *d) {
+static int pci_piix_init_ports(PCIIDEState *d)
+{
static const struct {
int iobase;
int iobase2;
@@ -132,24 +134,30 @@ static void pci_piix_init_ports(PCIIDEState *d) {
{0x1f0, 0x3f6, 14},
{0x170, 0x376, 15},
};
-int i;
+int i, ret;
for (i = 0; i < 2; i++) {
ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-port_info[i].iobase2);
+ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+ port_info[i].iobase2);
+if (ret) {
+return ret;
+}
ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
d->bmdma[i].bus = &d->bus[i];
ide_register_restart_cb(&d->bus[i]);
}
+
+return 0;
}
static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
{
PCIIDEState *d = PCI_IDE(dev);
uint8_t *pci_conf = dev->config;
+int rc;
pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
@@ -158,7 +166,11 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
-pci_piix_init_ports(d);
+rc = pci_piix_init_ports(d);
+if (rc) {
+error_setg_errno(errp, -rc, "Failed to realize %s",
+ object_get_typename(OBJECT(dev)));
+}
}
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 7820068e6e..cffaa35e9c 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -131,13 +131,17 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion
*io, uint16_t start)
isa_init_ioport(dev, start);
}
-void isa_register_portio_list(ISADevice *dev,
- PortioList *piolist, uint16_t start,
- const MemoryRegionPortio *pio_start,
- void *opaque, const char *name)
+int isa_register_portio_list(ISADevice *dev,
+ PortioList *piolist, uint16_t start,
+ const MemoryRegionPortio *pio_start,
+ void *opaque, const char *name)
{
assert(piolist && !piolist->owner);
+if (!isabus) {
+return -ENODEV;
+}
+
/* START is how
Re: [PATCH] block/file-posix: Fix problem with fallocate(PUNCH_HOLE) on GPFS
On 16/04/2021 22.34, Nir Soffer wrote:
On Fri, Apr 16, 2021 at 8:23 AM Thomas Huth wrote:
A customer reported that running
qemu-img convert -t none -O qcow2 -f qcow2 input.qcow2 output.qcow2
fails for them with the following error message when the images are
stored on a GPFS file system:
qemu-img: error while writing sector 0: Invalid argument
After analyzing the strace output, it seems like the problem is in
handle_aiocb_write_zeroes(): The call to fallocate(FALLOC_FL_PUNCH_HOLE)
returns EINVAL, which can apparently happen if the file system has
a different idea of the granularity of the operation. It's arguably
a bug in GPFS, since the PUNCH_HOLE mode should not result in EINVAL
according to the man-page of fallocate(), but the file system is out
there in production and so we have to deal with it. In commit 294682cc3a
("block: workaround for unaligned byte range in fallocate()") we also
already applied the a work-around for the same problem to the earlier
fallocate(FALLOC_FL_ZERO_RANGE) call, so do it now similar with the
PUNCH_HOLE call.
Signed-off-by: Thomas Huth
---
block/file-posix.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 20e14f8e96..7a40428d52 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1675,6 +1675,13 @@ static int handle_aiocb_write_zeroes(void *opaque)
}
s->has_fallocate = false;
} else if (ret != -ENOTSUP) {
+if (ret == -EINVAL) {
+/*
+ * File systems like GPFS do not like unaligned byte ranges,
+ * treat it like unsupported (so caller falls back to pwrite)
+ */
+return -ENOTSUP;
This skips the next fallback, using plain fallocate(0) if we write
after the end of the file. Is this intended?
We can treat the buggy EINVAL return value as "filesystem is buggy,
let's not try other options", or "let's try the next option". Since falling
back to actually writing zeroes is so much slower, I think it is better to
try the next option.
I just did the same work-around as in commit 294682cc3a7 ... so if we agree
to try the other options, too, we should change that spot, too...
However, what is not clear to me, how would you handle s->has_write_zeroes
and s->has_discard in such a case? Set them to "false"? ... but it could
still work for some blocks with different alignment ... but if we keep them
set to "true", the code tries again and again to call these ioctls, maybe
wasting other precious cycles for this?
Maybe we should do a different approach instead: In case we hit a EINVAL
here, print an error a la:
error_report_once("You are running on a buggy file system, please complain
to the file system vendor");
and return -ENOTSUP ... then it's hopefully clear to the users why they are
getting a bad performance, and that they should complain to the file system
vendor instead to get their problem fixed.
This issue affects also libnbd (nbdcopy file backend).
Do we have a bug for GFS?
The GPFS-related bug is:
https://bugzilla.redhat.com/show_bug.cgi?id=1944861
Thomas
Re: [PATCH 01/14] hw/block/nvme: rename __nvme_zrm_open
On 19/04/2021 21.27, Klaus Jensen wrote: From: Klaus Jensen Get rid of the (reserved) double underscore use. Cc: Philippe Mathieu-Daudé Cc: Thomas Huth Signed-off-by: Klaus Jensen --- hw/block/nvme.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) I think it would be good to mention the change with NVME_ZRM_AUTO in the patch description, too. Apart from that: Reviewed-by: Thomas Huth
Re: [PATCH 02/14] hw/block/nvme: rename __nvme_advance_zone_wp
On 19/04/2021 21.27, Klaus Jensen wrote:
From: Klaus Jensen
Get rid of the (reserved) double underscore use.
Cc: Philippe Mathieu-Daudé
Cc: Thomas Huth
Signed-off-by: Klaus Jensen
---
hw/block/nvme.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 002c0672b397..d1b94e36c6fb 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1745,8 +1745,8 @@ static inline uint16_t nvme_zrm_open(NvmeNamespace *ns,
NvmeZone *zone)
return nvme_zrm_open_flags(ns, zone, 0);
}
-static void __nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
- uint32_t nlb)
+static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
+ uint32_t nlb)
{
zone->d.wp += nlb;
@@ -1766,7 +1766,7 @@ static void nvme_finalize_zoned_write(NvmeNamespace *ns, NvmeRequest *req)
nlb = le16_to_cpu(rw->nlb) + 1;
zone = nvme_get_zone_by_slba(ns, slba);
-__nvme_advance_zone_wp(ns, zone, nlb);
+nvme_advance_zone_wp(ns, zone, nlb);
}
static inline bool nvme_is_write(NvmeRequest *req)
@@ -2155,7 +2155,7 @@ out:
uint64_t sdlba = le64_to_cpu(copy->sdlba);
NvmeZone *zone = nvme_get_zone_by_slba(ns, sdlba);
-__nvme_advance_zone_wp(ns, zone, ctx->nlb);
+nvme_advance_zone_wp(ns, zone, ctx->nlb);
}
g_free(ctx->bounce);
Reviewed-by: Thomas Huth
Re: [PATCH 03/14] hw/block/nvme: rename __nvme_select_ns_iocs
On 19/04/2021 21.27, Klaus Jensen wrote: From: Klaus Jensen Get rid of the (reserved) double underscore use. Cc: Philippe Mathieu-Daudé Cc: Thomas Huth Signed-off-by: Klaus Jensen --- hw/block/nvme.c | 47 +++ 1 file changed, 23 insertions(+), 24 deletions(-) Reviewed-by: Thomas Huth
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
On 28/04/2021 11.24, Stefan Hajnoczi wrote:
On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
@@ -158,7 +166,11 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
-pci_piix_init_ports(d);
+rc = pci_piix_init_ports(d);
+if (rc) {
+error_setg_errno(errp, -rc, "Failed to realize %s",
+ object_get_typename(OBJECT(dev)));
+}
Is there an error message explaining the reason for the failure
somewhere. I can't see one in the patch and imagine users will be
puzzled/frustrated by a generic "Failed to realize" error message. Can
you make it more meaningful?
Do you have a suggestion for a better message?
Thomas
Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)
On 23/01/2021 11.03, P J P wrote:
From: Prasad J Pandit
While processing ioport command in 'fdctrl_write_dor', device
controller may select a drive which is not initialised with a
block device. This may result in a NULL pointer dereference.
Add checks to avoid it.
Fixes: CVE-2021-20196
Reported-by: Gaoning Pan
Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
Signed-off-by: Prasad J Pandit
---
hw/block/fdc.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 3636874432..13a9470d19 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1429,7 +1429,9 @@ static void fdctrl_write_dor(FDCtrl *fdctrl, uint32_t
value)
}
}
/* Selected drive */
-fdctrl->cur_drv = value & FD_DOR_SELMASK;
+if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
+fdctrl->cur_drv = value & FD_DOR_SELMASK;
+}
fdctrl->dor = value;
}
@@ -1894,6 +1896,10 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
uint32_t pos;
cur_drv = get_cur_drv(fdctrl);
+if (!cur_drv->blk) {
+FLOPPY_DPRINTF("No drive connected\n");
+return 0;
+}
fdctrl->dsr &= ~FD_DSR_PWRDOWN;
if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
FLOPPY_DPRINTF("error: controller not ready for reading\n");
@@ -2420,7 +2426,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t
value)
if (pos == FD_SECTOR_LEN - 1 ||
fdctrl->data_pos == fdctrl->data_len) {
cur_drv = get_cur_drv(fdctrl);
-if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
+if (cur_drv->blk == NULL
+|| blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
BDRV_SECTOR_SIZE, 0) < 0) {
FLOPPY_DPRINTF("error writing sector %d\n",
fd_sector(cur_drv));
Ping again!
Could anybody review / pick this up?
Thomas
Re: [PATCH] block/file-posix: Fix problem with fallocate(PUNCH_HOLE) on GPFS
On 19/04/2021 07.06, Thomas Huth wrote:
On 16/04/2021 22.34, Nir Soffer wrote:
On Fri, Apr 16, 2021 at 8:23 AM Thomas Huth wrote:
A customer reported that running
qemu-img convert -t none -O qcow2 -f qcow2 input.qcow2 output.qcow2
fails for them with the following error message when the images are
stored on a GPFS file system:
qemu-img: error while writing sector 0: Invalid argument
After analyzing the strace output, it seems like the problem is in
handle_aiocb_write_zeroes(): The call to fallocate(FALLOC_FL_PUNCH_HOLE)
returns EINVAL, which can apparently happen if the file system has
a different idea of the granularity of the operation. It's arguably
a bug in GPFS, since the PUNCH_HOLE mode should not result in EINVAL
according to the man-page of fallocate(), but the file system is out
there in production and so we have to deal with it. In commit 294682cc3a
("block: workaround for unaligned byte range in fallocate()") we also
already applied the a work-around for the same problem to the earlier
fallocate(FALLOC_FL_ZERO_RANGE) call, so do it now similar with the
PUNCH_HOLE call.
Signed-off-by: Thomas Huth
---
block/file-posix.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 20e14f8e96..7a40428d52 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1675,6 +1675,13 @@ static int handle_aiocb_write_zeroes(void *opaque)
}
s->has_fallocate = false;
} else if (ret != -ENOTSUP) {
+ if (ret == -EINVAL) {
+ /*
+ * File systems like GPFS do not like unaligned byte
ranges,
+ * treat it like unsupported (so caller falls back to
pwrite)
+ */
+ return -ENOTSUP;
This skips the next fallback, using plain fallocate(0) if we write
after the end of the file. Is this intended?
We can treat the buggy EINVAL return value as "filesystem is buggy,
let's not try other options", or "let's try the next option". Since falling
back to actually writing zeroes is so much slower, I think it is better to
try the next option.
I just did the same work-around as in commit 294682cc3a7 ... so if we agree
to try the other options, too, we should change that spot, too...
However, what is not clear to me, how would you handle s->has_write_zeroes
and s->has_discard in such a case? Set them to "false"? ... but it could
still work for some blocks with different alignment ... but if we keep them
set to "true", the code tries again and again to call these ioctls, maybe
wasting other precious cycles for this?
Maybe we should do a different approach instead: In case we hit a EINVAL
here, print an error a la:
error_report_once("You are running on a buggy file system, please complain
to the file system vendor");
and return -ENOTSUP ... then it's hopefully clear to the users why they are
getting a bad performance, and that they should complain to the file system
vendor instead to get their problem fixed.
Ping!
Any recommendations how to proceed here?
Thomas
Re: [RFC PATCH] block/vpc: Support probing of fixed-size VHD images
On 29/03/2021 09.25, Thomas Huth wrote:
Fixed-size VHD images don't have a header, only a footer. To be able
to still detect them right, support probing via the file name, too.
Without this change, images get detected as raw:
$ qemu-img create -f vpc -o subformat=fixed test.vhd 2G
Formatting 'test.vhd', fmt=vpc size=2147483648 subformat=fixed
$ qemu-img info test.vhd
image: test.vhd
file format: raw
virtual size: 2 GiB (2147992064 bytes)
disk size: 8 KiB
With this change:
$ qemu-img info test.vhd
image: test.vhd
file format: vpc
virtual size: 2 GiB (2147991552 bytes)
disk size: 8 KiB
Resolves: https://bugs.launchpad.net/qemu/+bug/1819182
Signed-off-by: Thomas Huth
---
I've marked the subject with RFC since I'm not quite sure whether this
is really a good idea... please let me know what you think about it...
block/vpc.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/block/vpc.c b/block/vpc.c
index 17a705b482..be561e4b39 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -191,8 +191,18 @@ static uint32_t vpc_checksum(void *p, size_t size)
static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
{
-if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
+if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8)) {
return 100;
+}
+
+/* It could be a fixed-size image without header -> check extension, too */
+if (filename) {
+int len = strlen(filename);
+if (len > 4 && !strcasecmp(&filename[len - 4], ".vhd")) {
+return 10;
+}
+}
+
return 0;
}
Ping!
Anybody any comments on this one?
Thomas
Re: [PATCH] block/file-posix: Fix problem with fallocate(PUNCH_HOLE) on GPFS
On 19/05/2021 12.21, Thomas Huth wrote:
On 19/04/2021 07.06, Thomas Huth wrote:
On 16/04/2021 22.34, Nir Soffer wrote:
On Fri, Apr 16, 2021 at 8:23 AM Thomas Huth wrote:
A customer reported that running
qemu-img convert -t none -O qcow2 -f qcow2 input.qcow2 output.qcow2
fails for them with the following error message when the images are
stored on a GPFS file system:
qemu-img: error while writing sector 0: Invalid argument
After analyzing the strace output, it seems like the problem is in
handle_aiocb_write_zeroes(): The call to fallocate(FALLOC_FL_PUNCH_HOLE)
returns EINVAL, which can apparently happen if the file system has
a different idea of the granularity of the operation. It's arguably
a bug in GPFS, since the PUNCH_HOLE mode should not result in EINVAL
according to the man-page of fallocate(), but the file system is out
there in production and so we have to deal with it. In commit 294682cc3a
("block: workaround for unaligned byte range in fallocate()") we also
already applied the a work-around for the same problem to the earlier
fallocate(FALLOC_FL_ZERO_RANGE) call, so do it now similar with the
PUNCH_HOLE call.
Signed-off-by: Thomas Huth
---
block/file-posix.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 20e14f8e96..7a40428d52 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1675,6 +1675,13 @@ static int handle_aiocb_write_zeroes(void *opaque)
}
s->has_fallocate = false;
} else if (ret != -ENOTSUP) {
+ if (ret == -EINVAL) {
+ /*
+ * File systems like GPFS do not like unaligned byte
ranges,
+ * treat it like unsupported (so caller falls back to
pwrite)
+ */
+ return -ENOTSUP;
This skips the next fallback, using plain fallocate(0) if we write
after the end of the file. Is this intended?
We can treat the buggy EINVAL return value as "filesystem is buggy,
let's not try other options", or "let's try the next option". Since falling
back to actually writing zeroes is so much slower, I think it is better to
try the next option.
I just did the same work-around as in commit 294682cc3a7 ... so if we
agree to try the other options, too, we should change that spot, too...
However, what is not clear to me, how would you handle s->has_write_zeroes
and s->has_discard in such a case? Set them to "false"? ... but it could
still work for some blocks with different alignment ... but if we keep
them set to "true", the code tries again and again to call these ioctls,
maybe wasting other precious cycles for this?
Maybe we should do a different approach instead: In case we hit a EINVAL
here, print an error a la:
error_report_once("You are running on a buggy file system, please complain
to the file system vendor");
and return -ENOTSUP ... then it's hopefully clear to the users why they
are getting a bad performance, and that they should complain to the file
system vendor instead to get their problem fixed.
Ping!
Any recommendations how to proceed here?
Never mind, Kevin just told me that he already replied - but apparently that
mail did not make it into my "qemu-devel" folder, so I did not see it :-(
... Anyway, I'll try to rework my patch accordingly:
https://lists.gnu.org/archive/html/qemu-block/2021-04/msg00488.html
Thomas
[PATCH] block/ssh: Bump minimum libssh version to 0.8.7
It has been over two years since RHEL-8 was released, and thus per the
platform build policy, we no longer need to support RHEL-7 as a build
target. So from the RHEL-7 perspective, we do not have to support
libssh v0.7 anymore now.
Let's look at the versions from other distributions and operating
systems - according to repology.org, current shipping versions are:
RHEL-8: 0.9.4
Debian Buster: 0.8.7
openSUSE Leap 15.2: 0.8.7
Ubuntu LTS 18.04: 0.8.0 *
Ubuntu LTS 20.04: 0.9.3
FreeBSD: 0.9.5
Fedora 33: 0.9.5
Fedora 34: 0.9.5
OpenBSD: 0.9.5
macOS HomeBrew: 0.9.5
HaikuPorts: 0.9.5
* The version of libssh in Ubuntu 18.04 claims to be 0.8.0 from the
name of the package, but in reality it is a 0.7 patched up as a
Frankenstein monster with patches from the 0.8 development branch.
This gave us some headaches in the past already and so it never worked
with QEMU. All attempts to get it supported have failed in the past,
patches for QEMU have never been merged and a request to Ubuntu to
fix it in their 18.04 distro has been ignored:
https://bugs.launchpad.net/ubuntu/+source/libssh/+bug/1847514
Thus we really should ignore the libssh in Ubuntu 18.04 in QEMU, too.
Fix it by bumping the minimum libssh version to something that is
greater than 0.8.0 now. Debian Buster and openSUSE Leap have the
oldest version and so 0.8.7 is the new minimum.
Signed-off-by: Thomas Huth
---
block/ssh.c | 59 -
configure | 19 +
2 files changed, 1 insertion(+), 77 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index ebe3d8b631..b51a031620 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -277,7 +277,6 @@ static void ssh_parse_filename(const char *filename, QDict
*options,
static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
{
int ret;
-#ifdef HAVE_LIBSSH_0_8
enum ssh_known_hosts_e state;
int r;
ssh_key pubkey;
@@ -343,46 +342,6 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
Error **errp)
error_setg(errp, "error while checking for known server (%d)", state);
goto out;
}
-#else /* !HAVE_LIBSSH_0_8 */
-int state;
-
-state = ssh_is_server_known(s->session);
-trace_ssh_server_status(state);
-
-switch (state) {
-case SSH_SERVER_KNOWN_OK:
-/* OK */
-trace_ssh_check_host_key_knownhosts();
-break;
-case SSH_SERVER_KNOWN_CHANGED:
-ret = -EINVAL;
-error_setg(errp,
- "host key does not match the one in known_hosts; this "
- "may be a possible attack");
-goto out;
-case SSH_SERVER_FOUND_OTHER:
-ret = -EINVAL;
-error_setg(errp,
- "host key for this server not found, another type exists");
-goto out;
-case SSH_SERVER_FILE_NOT_FOUND:
-ret = -ENOENT;
-error_setg(errp, "known_hosts file not found");
-goto out;
-case SSH_SERVER_NOT_KNOWN:
-ret = -EINVAL;
-error_setg(errp, "no host key was found in known_hosts");
-goto out;
-case SSH_SERVER_ERROR:
-ret = -EINVAL;
-error_setg(errp, "server error");
-goto out;
-default:
-ret = -EINVAL;
-error_setg(errp, "error while checking for known server (%d)", state);
-goto out;
-}
-#endif /* !HAVE_LIBSSH_0_8 */
/* known_hosts checking successful. */
ret = 0;
@@ -438,11 +397,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
unsigned char *server_hash;
size_t server_hash_len;
-#ifdef HAVE_LIBSSH_0_8
r = ssh_get_server_publickey(s->session, &pubkey);
-#else
-r = ssh_get_publickey(s->session, &pubkey);
-#endif
if (r != SSH_OK) {
session_error_setg(errp, s, "failed to read remote host key");
return -EINVAL;
@@ -1233,8 +1188,6 @@ static void unsafe_flush_warning(BDRVSSHState *s, const
char *what)
}
}
-#ifdef HAVE_LIBSSH_0_8
-
static coroutine_fn int ssh_flush(BDRVSSHState *s, BlockDriverState *bs)
{
int r;
@@ -1271,18 +1224,6 @@ static coroutine_fn int ssh_co_flush(BlockDriverState
*bs)
return ret;
}
-#else /* !HAVE_LIBSSH_0_8 */
-
-static coroutine_fn int ssh_co_flush(BlockDriverState *bs)
-{
-BDRVSSHState *s = bs->opaque;
-
-unsafe_flush_warning(s, "libssh >= 0.8.0");
-return 0;
-}
-
-#endif /* !HAVE_LIBSSH_0_8 */
-
static int64_t ssh_getlength(BlockDriverState *bs)
{
BDRVSSHState *s = bs->opaque;
diff --git a/configure b/configure
index 879a8e8f17..bf1c740494 100755
--- a/configure
+++ b/configure
@@ -3512,7 +3512,7 @@ fi
##
# libssh probe
if test "$libssh" != "no" ; then
- if $pkg_config --exists libssh; then
+ if $pkg_
[PATCH 0/2] Improve the fallocate() EINVAL in handle_aiocb_write_zeroes()
On buggy file systems, fallocate() can return EINVAL for unaligned accesses. Improve the situation by ignoring this for PUNCH_HOLE, too (but we also print out an error message in this case now, since PUNCH_HOLE should really never return EINVAL according to the man page). The second patch reworks the handling for ZERO_RANGE a little bit so that we now also try the other fallbacks in this case now. Thomas Huth (2): block/file-posix: Fix problem with fallocate(PUNCH_HOLE) on GPFS block/file-posix: Try other fallbacks after invalid FALLOC_FL_ZERO_RANGE block/file-posix.c | 28 +++- 1 file changed, 19 insertions(+), 9 deletions(-) -- 2.27.0
[PATCH 2/2] block/file-posix: Try other fallbacks after invalid FALLOC_FL_ZERO_RANGE
If fallocate(... FALLOC_FL_ZERO_RANGE ...) returns EINVAL, it's likely
an indication that the file system is buggy and does not implement
unaligned accesses right. We still might be lucky with the other
fallback fallocate() calls later in this function, though, so we should
not return immediately and try the others first.
Since FALLOC_FL_ZERO_RANGE could also return EINVAL if the file descriptor
is not a regular file, we ignore this filesystem bug silently, without
printing an error message for the user.
Signed-off-by: Thomas Huth
---
block/file-posix.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 134ff01d82..a96b6fa8fb 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1625,17 +1625,17 @@ static int handle_aiocb_write_zeroes(void *opaque)
if (s->has_write_zeroes) {
int ret = do_fallocate(s->fd, FALLOC_FL_ZERO_RANGE,
aiocb->aio_offset, aiocb->aio_nbytes);
-if (ret == -EINVAL) {
-/*
- * Allow falling back to pwrite for file systems that
- * do not support fallocate() for an unaligned byte range.
- */
-return -ENOTSUP;
-}
-if (ret == 0 || ret != -ENOTSUP) {
+if (ret == -ENOTSUP) {
+s->has_write_zeroes = false;
+} else if (ret == 0 || ret != -EINVAL) {
return ret;
}
-s->has_write_zeroes = false;
+/*
+ * Note: Some file systems do not like unaligned byte ranges, and
+ * return EINVAL in such a case, though they should not do it according
+ * to the man-page of fallocate(). Thus we simply ignore this return
+ * value and try the other fallbacks instead.
+ */
}
#endif
--
2.27.0
[PATCH 1/2] block/file-posix: Fix problem with fallocate(PUNCH_HOLE) on GPFS
A customer reported that running
qemu-img convert -t none -O qcow2 -f qcow2 input.qcow2 output.qcow2
fails for them with the following error message when the images are
stored on a GPFS file system :
qemu-img: error while writing sector 0: Invalid argument
After analyzing the strace output, it seems like the problem is in
handle_aiocb_write_zeroes(): The call to fallocate(FALLOC_FL_PUNCH_HOLE)
returns EINVAL, which can apparently happen if the file system has
a different idea of the granularity of the operation. It's arguably
a bug in GPFS, since the PUNCH_HOLE mode should not result in EINVAL
according to the man-page of fallocate(), but the file system is out
there in production and so we have to deal with it. In commit 294682cc3a
("block: workaround for unaligned byte range in fallocate()") we also
already applied the a work-around for the same problem to the earlier
fallocate(FALLOC_FL_ZERO_RANGE) call, so do it now similar with the
PUNCH_HOLE call. But instead of silently catching and returning
-ENOTSUP (which causes the caller to fall back to writing zeroes),
let's rather inform the user once about the buggy file system and
try the other fallback instead.
Signed-off-by: Thomas Huth
---
block/file-posix.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 10b71d9a13..134ff01d82 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1650,6 +1650,16 @@ static int handle_aiocb_write_zeroes(void *opaque)
return ret;
}
s->has_fallocate = false;
+} else if (ret == -EINVAL) {
+/*
+ * Some file systems like older versions of GPFS do not like un-
+ * aligned byte ranges, and return EINVAL in such a case, though
+ * they should not do it according to the man-page of fallocate().
+ * Warn about the bad filesystem and try the final fallback
instead.
+ */
+warn_report_once("Your file system is misbehaving: "
+ "fallocate(FALLOC_FL_PUNCH_HOLE) returned EINVAL.
"
+ "Please report this bug to your file sytem
vendor.");
} else if (ret != -ENOTSUP) {
return ret;
} else {
--
2.27.0
Re: [PATCH v2 1/2] vhost-user-blk-test: fix Coverity open(2) false positives
void start_vhost_user_blk(GString *cmd_line, int
vus_instances,
storage_daemon_command->str);
pid_t pid = fork();
if (pid == 0) {
+int fd;
+
/*
* Close standard file descriptors so tap-driver.pl pipe detects when
* our parent terminates.
*/
close(0);
+fd = open("/dev/null", O_RDONLY);
+g_assert_cmpint(fd, ==, 0);
close(1);
-open("/dev/null", O_RDONLY);
-open("/dev/null", O_WRONLY);
+fd = open("/dev/null", O_WRONLY);
+g_assert_cmpint(fd, ==, 1);
execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL);
exit(1);
Reviewed-by: Thomas Huth
Re: [PATCH v2 2/2] ui/vdagent: fix clipboard info memory leak in error path
On 01/06/2021 17.57, Stefan Hajnoczi wrote:
If the size of a VD_AGENT_CLIPBOARD_GRAB message is invalid we leak info
when returning early.
Thanks to Coverity for spotting this:
*** CID 1453266: Resource leaks (RESOURCE_LEAK)
/qemu/ui/vdagent.c: 465 in vdagent_chr_recv_clipboard()
459 info = qemu_clipboard_info_new(&vd->cbpeer, s);
460 if (size > sizeof(uint32_t) * 10) {
461 /*
462 * spice has 6 types as of 2021. Limiting to 10 entries
463 * so we we have some wiggle room.
464 */
CID 1453266: Resource leaks (RESOURCE_LEAK)
Variable "info" going out of scope leaks the storage it points to.
465 return;
466 }
467 while (size >= sizeof(uint32_t)) {
468 trace_vdagent_cb_grab_type(GET_NAME(type_name, *(uint32_t
*)data));
469 switch (*(uint32_t *)data) {
470 case VD_AGENT_CLIPBOARD_UTF8_TEXT:
Fixes: f0349f4d8947ad32d0fa4678cbf5dbb356fcbda1 ("ui/vdagent: add clipboard
support")
Reviewed-by: Gerd Hoffmann
Signed-off-by: Stefan Hajnoczi
---
ui/vdagent.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ui/vdagent.c b/ui/vdagent.c
index a253a8fe63..8fc54d330e 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -456,7 +456,6 @@ static void vdagent_chr_recv_clipboard(VDAgentChardev *vd,
VDAgentMessage *msg)
switch (msg->type) {
case VD_AGENT_CLIPBOARD_GRAB:
trace_vdagent_cb_grab_selection(GET_NAME(sel_name, s));
-info = qemu_clipboard_info_new(&vd->cbpeer, s);
if (size > sizeof(uint32_t) * 10) {
/*
* spice has 6 types as of 2021. Limiting to 10 entries
@@ -464,6 +463,7 @@ static void vdagent_chr_recv_clipboard(VDAgentChardev *vd,
VDAgentMessage *msg)
*/
return;
}
+info = qemu_clipboard_info_new(&vd->cbpeer, s);
while (size >= sizeof(uint32_t)) {
trace_vdagent_cb_grab_type(GET_NAME(type_name, *(uint32_t
*)data));
switch (*(uint32_t *)data) {
Reviewed-by: Thomas Huth
[PATCH] docs/interop/live-block-operations: Do not hard-code the QEMU binary name
In downstream, we want to use a different name for the QEMU binary, and some people might also use the docs for non-x86 binaries, that's why we already created the |qemu_system| placeholder in the past. Use it now in the live-block-operations doc, too. Signed-off-by: Thomas Huth --- docs/interop/live-block-operations.rst | 32 +++--- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/docs/interop/live-block-operations.rst b/docs/interop/live-block-operations.rst index 1073b930dc..477d085f54 100644 --- a/docs/interop/live-block-operations.rst +++ b/docs/interop/live-block-operations.rst @@ -127,13 +127,15 @@ Interacting with a QEMU instance To show some example invocations of command-line, we will use the following invocation of QEMU, with a QMP server running over UNIX -socket:: +socket: -$ ./qemu-system-x86_64 -display none -no-user-config \ --M q35 -nodefaults -m 512 \ --blockdev node-name=node-A,driver=qcow2,file.driver=file,file.node-name=file,file.filename=./a.qcow2 \ --device virtio-blk,drive=node-A,id=virtio0 \ --monitor stdio -qmp unix:/tmp/qmp-sock,server=on,wait=off +.. parsed-literal:: + + $ |qemu_system| -display none -no-user-config -nodefaults \\ +-m 512 -blockdev \\ + node-name=node-A,driver=qcow2,file.driver=file,file.node-name=file,file.filename=./a.qcow2 \\ +-device virtio-blk,drive=node-A,id=virtio0 \\ +-monitor stdio -qmp unix:/tmp/qmp-sock,server=on,wait=off The ``-blockdev`` command-line option, used above, is available from QEMU 2.9 onwards. In the above invocation, notice the ``node-name`` @@ -692,14 +694,16 @@ And start the destination QEMU (we already have the source QEMU running -- discussed in the section: `Interacting with a QEMU instance`_) instance, with the following invocation. (As noted earlier, for simplicity's sake, the destination QEMU is started on the same host, but -it could be located elsewhere):: - -$ ./qemu-system-x86_64 -display none -no-user-config \ --M q35 -nodefaults -m 512 \ --blockdev node-name=node-TargetDisk,driver=qcow2,file.driver=file,file.node-name=file,file.filename=./target-disk.qcow2 \ --device virtio-blk,drive=node-TargetDisk,id=virtio0 \ --S -monitor stdio -qmp unix:./qmp-sock2,server=on,wait=off \ --incoming tcp:localhost: +it could be located elsewhere): + +.. parsed-literal:: + + $ |qemu_system| -display none -no-user-config -nodefaults \\ +-m 512 -blockdev \\ + node-name=node-TargetDisk,driver=qcow2,file.driver=file,file.node-name=file,file.filename=./target-disk.qcow2 \\ +-device virtio-blk,drive=node-TargetDisk,id=virtio0 \\ +-S -monitor stdio -qmp unix:./qmp-sock2,server=on,wait=off \\ +-incoming tcp:localhost: Given the disk image chain on source QEMU:: -- 2.27.0
Re: [PATCH v16 96/99] tests/qtest: split the cdrom-test into arm/aarch64
On 08/06/2021 15.42, John Snow wrote: On 6/4/21 11:53 AM, Alex Bennée wrote: The assumption that the qemu-system-aarch64 image can run all 32 bit machines is about to be broken and besides it's not likely this is What's changing? I'm not deeply familiar with aarch64. Why is this assumption about to be broken? That's also quite a surprise to me. Do you have any pointers? improving out coverage by much. Test the "virt" machine for both arm and aarch64 as it can be used by either architecture. Sounds fine to me, but I didn't write this part of the test. Thomas, you wrote this section IIRC -- does this reduce coverage in any meaningful way? I think we built only aarch64 in a couple of our CI pipelines, assuming that it covers all normal arm machines, too ... so we might want to revisit our CI pipelines to check whether we then need more test coverage for qemu-system-arm instead. But apart from that, the patch looks ok to me. Thomas
Re: [PATCH v16 96/99] tests/qtest: split the cdrom-test into arm/aarch64
On 08/06/2021 16.27, Alex Bennée wrote: Richard Henderson writes: On 6/4/21 8:53 AM, Alex Bennée wrote: The assumption that the qemu-system-aarch64 image can run all 32 bit machines is about to be broken... Um, what? Really what we want is to probe the -M (machines) that a binary supports rather than just barfing the test because we've built a QEMU that doesn't support all the random 32 bit machines. Ok, so this is rather about being able to use the qtests with custom QEMU builds where you tweaked the default-configs? That's certainly a valid endeavor, but then I think this patch is the wrong way to go. You should rather add a qtest_has_machine() function that checks whether a machine is available in the target binary or not, and then only add the tests to the test plan that match the list of available machines. (we have similar issues in downstream RHEL where we want to limit the amount of machines, so a qtest_has_machine() function would certainly be welcome) Thomas
Re: [PATCH v16 96/99] tests/qtest: split the cdrom-test into arm/aarch64
On 08/06/2021 16.41, Alex Bennée wrote: Thomas Huth writes: On 08/06/2021 15.42, John Snow wrote: On 6/4/21 11:53 AM, Alex Bennée wrote: The assumption that the qemu-system-aarch64 image can run all 32 bit machines is about to be broken and besides it's not likely this is What's changing? I'm not deeply familiar with aarch64. Why is this assumption about to be broken? That's also quite a surprise to me. Do you have any pointers? It's at the top of the series. If you build qemu-system-aarch64 with a custom config you won't be able to instantiate those machines. Ideally it would probe and maybe fail safe if the binary doesn't support the model. Is that possible in qtest? Not yet. You'd need to do something similar like Philippe did in patch 03/99, but instead of running query-accel, you have to run query-machines instead. Thomas
Re: [PATCH v16 96/99] tests/qtest: split the cdrom-test into arm/aarch64
On 08/06/2021 16.27, Alex Bennée wrote: Richard Henderson writes: On 6/4/21 8:53 AM, Alex Bennée wrote: The assumption that the qemu-system-aarch64 image can run all 32 bit machines is about to be broken... Um, what? Really what we want is to probe the -M (machines) that a binary supports rather than just barfing the test because we've built a QEMU that doesn't support all the random 32 bit machines. r~ and besides it's not likely this is improving out coverage by much. Test the "virt" machine for both arm and aarch64 as it can be used by either architecture. I think this point still stands though, I don't think we get much from running the cdrom test with realview et all on qemu-system-aarch64. In a lot of CI pipelines, we are either building aarch64 or arm, but not both, so I think it might be good to keep the tests in here. Thomas
Re: [PATCH v16 96/99] tests/qtest: split the cdrom-test into arm/aarch64
On 08/06/2021 17.35, Alex Bennée wrote: Thomas Huth writes: On 08/06/2021 16.27, Alex Bennée wrote: Richard Henderson writes: On 6/4/21 8:53 AM, Alex Bennée wrote: The assumption that the qemu-system-aarch64 image can run all 32 bit machines is about to be broken... Um, what? Really what we want is to probe the -M (machines) that a binary supports rather than just barfing the test because we've built a QEMU that doesn't support all the random 32 bit machines. r~ and besides it's not likely this is improving out coverage by much. Test the "virt" machine for both arm and aarch64 as it can be used by either architecture. I think this point still stands though, I don't think we get much from running the cdrom test with realview et all on qemu-system-aarch64. In a lot of CI pipelines, we are either building aarch64 or arm, but not both, so I think it might be good to keep the tests in here. We do test instantiating the cdrom with -M virt, exactly how many extra lines of coverage do we get for the rest? $ grep -r block_default_type.*IF_ hw/arm/ hw/arm/tosa.c:mc->block_default_type = IF_IDE; hw/arm/cubieboard.c:mc->block_default_type = IF_IDE; hw/arm/virt.c:mc->block_default_type = IF_VIRTIO; hw/arm/spitz.c:mc->block_default_type = IF_IDE; hw/arm/orangepi.c:mc->block_default_type = IF_SD; hw/arm/raspi.c:mc->block_default_type = IF_SD; hw/arm/realview.c:mc->block_default_type = IF_SCSI; hw/arm/realview.c:mc->block_default_type = IF_SCSI; hw/arm/xlnx-zcu102.c:mc->block_default_type = IF_IDE; hw/arm/highbank.c:mc->block_default_type = IF_IDE; hw/arm/highbank.c:mc->block_default_type = IF_IDE; hw/arm/sbsa-ref.c:mc->block_default_type = IF_IDE; hw/arm/versatilepb.c:mc->block_default_type = IF_SCSI; hw/arm/versatilepb.c:mc->block_default_type = IF_SCSI; ... thus these tests check quite a bit of different ways to pass a cdrom drive to the machine. I'd rather suggest to keep them, but you're the arm guy here, so if you don't like these tests anymore, feel free to drop them. Thomas
Re: [PATCH 1/4] block.c: fix compilation error on possible unitialized variable
On 08/06/2021 16.09, Cleber Rosa wrote: GCC from CentOS Stream 8 is erroring out on a possibily unitialized varible. Full version info for the compiler used: gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-1) Signed-off-by: Cleber Rosa --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block.c b/block.c index 3f456892d0..08f29e6b65 100644 --- a/block.c +++ b/block.c @@ -4866,7 +4866,7 @@ static int bdrv_replace_node_common(BlockDriverState *from, Transaction *tran = tran_new(); g_autoptr(GHashTable) found = NULL; g_autoptr(GSList) refresh_list = NULL; -BlockDriverState *to_cow_parent; +BlockDriverState *to_cow_parent = NULL; int ret; Already reported here: https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01229.html Thomas
Re: Prevent compiler warning on block.c
On 05/05/2021 10.05, Vladimir Sementsov-Ogievskiy wrote:
05.05.2021 10:59, Miroslav Rezanina wrote:
Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced
uninitialized
variable to_cow_parent in bdrv_replace_node_common function that is used
only when
detach_subchain is true. It is used in two places. First if block properly
initialize
the variable and second block use it.
However, compiler treats this two blocks as two independent cases so it
thinks first
block can fail test and second one pass (although both use same
condition). This cause
warning that variable can be uninitialized in second block.
To prevent this warning, initialize the variable with NULL.
Signed-off-by: Miroslav Rezanina
---
diff --git a/block.c b/block.c
index 874c22c43e..3ca27bd2d9 100644
--- a/block.c
+++ b/block.c
@@ -4851,7 +4851,7 @@ static int bdrv_replace_node_common(BlockDriverState
*from,
Transaction *tran = tran_new();
g_autoptr(GHashTable) found = NULL;
g_autoptr(GSList) refresh_list = NULL;
- BlockDriverState *to_cow_parent;
+ BlockDriverState *to_cow_parent = NULL;
int ret;
if (detach_subchain) {
Reviewed-by: Vladimir Sementsov-Ogievskiy
This just popped up again here:
https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg02067.html
Kevin, Max, could you please pick it up to get this problem fixed?
Thomas
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
On 16/04/2021 14.52, Thomas Huth wrote:
QEMU currently crashes when the user tries to do something like:
qemu-system-x86_64 -M x-remote -device piix3-ide
It's now several months later already, and this crash has still not been
fixed yet. The next softfreeze is around the corner and the
"device-crash-test" still stumbles accross this problem.
If the other solutions are not mergable yet (what's the status here?), could
we please include my patch for QEMU v6.1 instead, so that we get this
silenced in the device-crash-test script?
Thanks,
Thomas
This happens because the "isabus" variable is not initialized with
the x-remote machine yet. Add a proper check for this condition
and propagate the error to the caller, so we can fail there gracefully.
Signed-off-by: Thomas Huth
---
hw/ide/ioport.c | 16 ++--
hw/ide/piix.c | 22 +-
hw/isa/isa-bus.c | 14 ++
include/hw/ide/internal.h | 2 +-
include/hw/isa/isa.h | 13 -
5 files changed, 46 insertions(+), 21 deletions(-)
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index b613ff3bba..e6caa537fa 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,15 +50,19 @@ static const MemoryRegionPortio ide_portio2_list[] = {
PORTIO_END_OF_LIST(),
};
-void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
{
+int ret;
+
/* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
bridge has been setup properly to always register with ISA. */
-isa_register_portio_list(dev, &bus->portio_list,
- iobase, ide_portio_list, bus, "ide");
+ret = isa_register_portio_list(dev, &bus->portio_list,
+ iobase, ide_portio_list, bus, "ide");
-if (iobase2) {
-isa_register_portio_list(dev, &bus->portio2_list,
- iobase2, ide_portio2_list, bus, "ide");
+if (ret == 0 && iobase2) {
+ret = isa_register_portio_list(dev, &bus->portio2_list,
+ iobase2, ide_portio2_list, bus, "ide");
}
+
+return ret;
}
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b9860e35a5..d3e738320b 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
#include "migration/vmstate.h"
+#include "qapi/error.h"
#include "qemu/module.h"
#include "sysemu/block-backend.h"
#include "sysemu/blockdev.h"
@@ -123,7 +124,8 @@ static void piix_ide_reset(DeviceState *dev)
pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
}
-static void pci_piix_init_ports(PCIIDEState *d) {
+static int pci_piix_init_ports(PCIIDEState *d)
+{
static const struct {
int iobase;
int iobase2;
@@ -132,24 +134,30 @@ static void pci_piix_init_ports(PCIIDEState *d) {
{0x1f0, 0x3f6, 14},
{0x170, 0x376, 15},
};
-int i;
+int i, ret;
for (i = 0; i < 2; i++) {
ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-port_info[i].iobase2);
+ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+ port_info[i].iobase2);
+if (ret) {
+return ret;
+}
ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
d->bmdma[i].bus = &d->bus[i];
ide_register_restart_cb(&d->bus[i]);
}
+
+return 0;
}
static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
{
PCIIDEState *d = PCI_IDE(dev);
uint8_t *pci_conf = dev->config;
+int rc;
pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
@@ -158,7 +166,11 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
-pci_piix_init_ports(d);
+rc = pci_piix_init_ports(d);
+if (rc) {
+error_setg_errno(errp, -rc, "Failed to realize %s",
+ object_get_typename(OBJECT(dev)));
+}
}
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 7820068e6e..cffaa35e9c 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -131,13 +131,17 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion
*io, uint16_t start)
isa_init_ioport(dev, start);
}
-void isa_register_portio_list(ISADevice *dev,
- Porti
Failing iotest 206
Hi,
iotest 206 fails for me with:
$ ./check -qcow2 206
QEMU -- ".../tests/qemu-iotests/../../qemu-system-x86_64"
-nodefaults -display none -accel qtest
QEMU_IMG -- ".../tests/qemu-iotests/../../qemu-img"
QEMU_IO -- ".../tests/qemu-iotests/../../qemu-io" --cache writeback
--aio threads -f qcow2
QEMU_NBD -- ".../tests/qemu-iotests/../../qemu-nbd"
IMGFMT-- qcow2
IMGPROTO -- file
PLATFORM -- Linux/x86_64 thuth.remote.csb 4.18.0-305.3.1.el8_4.x86_64
TEST_DIR -- .../tests/qemu-iotests/scratch
SOCK_DIR -- /tmp/tmpx4hiqpkd
SOCKET_SCM_HELPER -- .../tests/qemu-iotests/socket_scm_helper
206 fail [10:00:50] [10:00:54] 3.4s (last: 6.2s) output
mismatch (see 206.out.bad)
--- 206.out
+++ 206.out.bad
@@ -99,55 +99,19 @@
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
{"driver": "qcow2", "encrypt": {"cipher-alg": "twofish-128", "cipher-mode":
"ctr", "format": "luks", "hash-alg": "sha1", "iter-time": 10, "ivgen-alg":
"plain64", "ivgen-hash-alg": "md5", "key-secret": "keysec0"}, "file":
{"driver": "file", "filename": "TEST_DIR/PID-t.qcow2"}, "size": 33554432}}}
{"return": {}}
+Job failed: Unsupported cipher algorithm twofish-128 with ctr mode
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
image: TEST_IMG
file format: IMGFMT
virtual size: 32 MiB (33554432 bytes)
-encrypted: yes
cluster_size: 65536
Format specific information:
compat: 1.1
compression type: zlib
lazy refcounts: false
refcount bits: 16
-encrypt:
-ivgen alg: plain64
-hash alg: sha1
-cipher alg: twofish-128
-uuid: ----
-format: luks
-cipher mode: ctr
-slots:
-[0]:
-active: true
-iters: XXX
-key offset: 4096
-stripes: 4000
-[1]:
-active: false
-key offset: 69632
-[2]:
-active: false
-key offset: 135168
-[3]:
-active: false
-key offset: 200704
-[4]:
-active: false
-key offset: 266240
-[5]:
-active: false
-key offset: 331776
-[6]:
-active: false
-key offset: 397312
-[7]:
-active: false
-key offset: 462848
-payload offset: 528384
-master key iters: XXX
corrupt: false
extended l2: false
Looks like it is missing a check for the availability of the corresponding
crypto stuff? Does anybody got a clue how to fix this?
Thomas
