Re: [PATCH] tests/qemu_iotests: Minimize usage of used ports
Dne 06. 02. 20 v 16:03 Max Reitz napsal(a):
> On 03.02.20 08:59, Lukáš Doktor wrote:
>> Using a range of ports from 32768 to 65538 is dangerous as some
>> application might already be listening there and interfere with the
>> testing. There is no way to reserve ports, but let's decrease the chance
>> of interactions by only using ports that were free at the time of
>> importing this module.
>>
>> Without this patch CI occasionally fails with used ports. Additionally I
>> tried listening on the first port to be tried via "nc -l localhost
>> $port" and no matter how many other ports were available it always
>> hanged for infinity.
>
> I’m afraid I don’t quite understand. The new functions check whether
> the ports are available for use by creating a server on them (i.e.,
> binding a socket there). The current code just lets qemu create a
> server there, and see if that works or not.
>
> So the only difference I can see is that instead of trying out random
> ports during the test and see whether they’re free to use we do this
> check only once when the test is started.
>
> And the only problem I can imagine from your description is that there
> is some other tool on the system that tries to set up a server but
> cannot because we already have an NBD server there (by accident).
>
> But I don’t see how checking for free ports once at startup solves that
> problem reliably.
>
> If what I guessed above is right, the only reliable solution I can
> imagine would be to allow users to specify the port range through
> environment variables, and then you’d have to specify a range that you
> know is free for use.
>
> Max
>
Hello Max,
thank you and I am sorry for not digging deep enough. This week my CI failed
with:
01:24:06 DEBUG| [stdout] +ERROR: test_inet (__main__.QemuNBD)
01:24:06 DEBUG| [stdout]
+--
01:24:06 DEBUG| [stdout] +Traceback (most recent call last):
01:24:06 DEBUG| [stdout] + File "147", line 85, in setUp
01:24:06 DEBUG| [stdout] +self.vm.launch()
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/machine.py",
line 302, in launch
01:24:06 DEBUG| [stdout] +self._launch()
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/machine.py",
line 319, in _launch
01:24:06 DEBUG| [stdout] +self._pre_launch()
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/qtest.py",
line 106, in _pre_launch
01:24:06 DEBUG| [stdout] +super(QEMUQtestMachine, self)._pre_launch()
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/machine.py",
line 270, in _pre_launch
01:24:06 DEBUG| [stdout] +self._qmp =
qmp.QEMUMonitorProtocol(self._vm_monitor, server=True)
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/qmp.py",
line 60, in __init__
01:24:06 DEBUG| [stdout] +self.__sock.bind(self.__address)
01:24:06 DEBUG| [stdout] +OSError: [Errno 98] Address already in use
I made the mistake of reproducing this on my home system using the qemu
revision that I had and assuming it's caused by a used port. So I limited the
port range and used nc to occupy the port. It sort-of reproduced but instead of
Address already in use it hanged until I kill the nc process. Then it failed
with:
+Traceback (most recent call last):
+ File "147", line 124, in test_inet
+flatten_sock_addr(address))
+ File "147", line 59, in client_test
+self.assert_qmp(result, 'return', {})
+ File "/home/medic/Work/Projekty/qemu/tests/qemu-iotests/iotests.py", line
821, in assert_qmp
+result = self.dictpath(d, path)
+ File "/home/medic/Work/Projekty/qemu/tests/qemu-iotests/iotests.py", line
797, in dictpath
+self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "return" in "{'error': {'class':
'GenericError', 'desc': 'Failed to read initial magic: Unexpected end-of-file
before all bytes were read'}}"
After a brief study I thought qemu is not doing the job well enough and wanted
to add a protection. Anyway after a more thorough overview I came to a
different conclusion and that is that we are facing the same issue as with
incoming migration about a year ago. What happened is that I started "nc -l
localhost 32789" which results in:
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
nc 26758 medic3u IPv6 9579487 0t0
Re: [PATCH] tests/qemu_iotests: Minimize usage of used ports
Dne 06. 02. 20 v 17:59 Max Reitz napsal(a):
> On 06.02.20 17:48, Eric Blake wrote:
>> On 2/6/20 10:37 AM, Max Reitz wrote:
>>
thank you and I am sorry for not digging deep enough. This week my CI
failed with:
01:24:06 DEBUG| [stdout] +ERROR: test_inet (__main__.QemuNBD)
01:24:06 DEBUG| [stdout]
+--
01:24:06 DEBUG| [stdout] +Traceback (most recent call last):
01:24:06 DEBUG| [stdout] + File "147", line 85, in setUp
01:24:06 DEBUG| [stdout] + self.vm.launch()
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/machine.py",
line 302, in launch
01:24:06 DEBUG| [stdout] + self._launch()
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/machine.py",
line 319, in _launch
01:24:06 DEBUG| [stdout] + self._pre_launch()
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/qtest.py",
line 106, in _pre_launch
01:24:06 DEBUG| [stdout] + super(QEMUQtestMachine,
self)._pre_launch()
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/machine.py",
line 270, in _pre_launch
01:24:06 DEBUG| [stdout] + self._qmp =
qmp.QEMUMonitorProtocol(self._vm_monitor, server=True)
01:24:06 DEBUG| [stdout] + File
"/home/jenkins/ppc64le/qemu-master/tests/qemu-iotests/../../python/qemu/qmp.py",
line 60, in __init__
01:24:06 DEBUG| [stdout] + self.__sock.bind(self.__address)
01:24:06 DEBUG| [stdout] +OSError: [Errno 98] Address already in use
>>
>> Was this test 147? If so, see:
>> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg01469.html
>>
>> because that failure matches what I was seeing.
>>
I made the mistake of reproducing this on my home system using the
qemu revision that I had and assuming it's caused by a used port. So
I limited the port range and used nc to occupy the port. It sort-of
reproduced but instead of Address already in use it hanged until I
kill the nc process. Then it failed with:
+Traceback (most recent call last):
+ File "147", line 124, in test_inet
+ flatten_sock_addr(address))
+ File "147", line 59, in client_test
+ self.assert_qmp(result, 'return', {})
+ File
"/home/medic/Work/Projekty/qemu/tests/qemu-iotests/iotests.py", line
821, in assert_qmp
+ result = self.dictpath(d, path)
+ File
"/home/medic/Work/Projekty/qemu/tests/qemu-iotests/iotests.py", line
797, in dictpath
+ self.fail('failed path traversal for "%s" in "%s"' % (path,
str(d)))
+AssertionError: failed path traversal for "return" in "{'error':
{'class': 'GenericError', 'desc': 'Failed to read initial magic:
Unexpected end-of-file before all bytes were read'}}"
>>
>> That's a secondary failure, I assume if the initial bug is fixed we are
>> less likely to hit the secondary one; but the secondary one may still be
>> worth fixing.
>>
After a brief study I thought qemu is not doing the job well enough
and wanted to add a protection. Anyway after a more thorough overview
I came to a different conclusion and that is that we are facing the
same issue as with incoming migration about a year ago. What happened
is that I started "nc -l localhost 32789" which results in:
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
nc 26758 medic 3u IPv6 9579487 0t0 TCP localhost:32789
(LISTEN)
Then we start the server by "_try_server_up" where qemu-nbd detects
the port is occupied on IPv6 but available on IPv4, so it claims it:
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
nc 26758 medic 3u IPv6 9579487 0t0 TCP
localhost:32789 (LISTEN)
qemu-nbd 26927 medic 4u IPv4 9591857 0t0 TCP
localhost:32789 (LISTEN)
and reports success. Then we try to connect but the hotplugged VM
first attempts to connect on the IPv6 address and hangs for infinity.
Now is this an expected behavior? If so then we need the
find_free_address (but preferably directly in _try_server_up just
before starting the qemu-nbd) to leave as little time-frame for
collision as possible. Otherwise the test is alright and qemu-nbd
needs a fix to bail out in case some address is already used (IIRC
this is what incoming migration does).
>>>
>>> Ah, OK.
>>>
>>> Well, expected behavior... It’s a shame, that’s what it is.
>>
>> In libnbd, we recently improved the testsuite by switching over to
>> systemd-style fd passing: instead of asking qemu-nbd to open a random
>> port (and hoping it is available), we instead pre-open the
Performance improvement with 58a2e3f5c37be02dac3086b81bdda9414b931edf
Hello Stefan, folks, the perf-ci detected and bisected the 58a2e3f5c37 - block: compile out assert_bdrv_graph_readable() by default - as a performance improvement especially with 4K reads with multiple jobs (but minor improvements were observed in other variants) https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/140-improvement.html Based on the commit message I guess it's expected so take this just as a record of improvement. Regards, Lukáš PS: The list of perf-ci issues is available here: https://docs.google.com/spreadsheets/d/1HEXI5wDsNgAIgXl5MIhGond898Vz5A1Hrkl0lZmWEbg/edit#gid=0 OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: Performance improvement with 58a2e3f5c37be02dac3086b81bdda9414b931edf
Dne 16. 05. 23 v 15:19 Stefan Hajnoczi napsal(a): > On Tue, 16 May 2023 at 08:56, Lukáš Doktor wrote: >> >> Hello Stefan, folks, >> >> the perf-ci detected and bisected the 58a2e3f5c37 - block: compile out >> assert_bdrv_graph_readable() by default - as a performance improvement >> especially with 4K reads with multiple jobs (but minor improvements were >> observed in other variants) >> >> >> https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/140-improvement.html > > Thanks, that's good to know. How much improvement was there? I don't > understand how to read the report that you linked to. > Hello Stefan, the linked report is a bisection report that shows how the throughput evolved throughout the bisection. I only used a single test: TunedLibvirt/fio-nvme-Aj-4i/:./read-4KiB/throughput/iops_sec.mean to bisect and the commit shas are presented in 3-letter format in table header (sorted by "git log" order). There should be full-sha on hover, but this is currently broken and I haven't had time to fix that yet so one has to map these using bisect log: git bisect start # good: [271477b59e723250f17a7e20f139262057921b6a] Merge tag 'compression-code-pull-request' of https://gitlab.com/juan.quintela/qemu into staging git bisect good 271477b59e723250f17a7e20f139262057921b6a # bad: [d530697ca20e19f7a626f4c1c8b26fccd0dc4470] Merge tag 'pull-testing-updates-100523-1' of https://gitlab.com/stsquad/qemu into staging git bisect bad d530697ca20e19f7a626f4c1c8b26fccd0dc4470 # good: [69aa0d371f67b1c042ed4f3ff4a481d561b54d21] nbd: Mark nbd_co_do_establish_connection() and callers GRAPH_RDLOCK git bisect good 69aa0d371f67b1c042ed4f3ff4a481d561b54d21 # good: [3217b84f3cd813a7daffc64b26543c313f3a042a] tests/docker: bump the xtensa base to debian:11-slim git bisect good 3217b84f3cd813a7daffc64b26543c313f3a042a # good: [e19b157f3c66c44e3b89cb50a2030f0187b968e9] block: Mark bdrv_refresh_limits() and callers GRAPH_RDLOCK git bisect good e19b157f3c66c44e3b89cb50a2030f0187b968e9 # good: [2cf72cb5eb690b55974b4ace5ee42c22ace3fa8e] gitlab: enable minimal device profile for aarch64 --disable-tcg git bisect good 2cf72cb5eb690b55974b4ace5ee42c22ace3fa8e # bad: [caa9cbd566877b34e9abcc04d936116fc5e0ab28] Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging git bisect bad caa9cbd566877b34e9abcc04d936116fc5e0ab28 # bad: [58a2e3f5c37be02dac3086b81bdda9414b931edf] block: compile out assert_bdrv_graph_readable() by default git bisect bad 58a2e3f5c37be02dac3086b81bdda9414b931edf # first bad commit: [58a2e3f5c37be02dac3086b81bdda9414b931edf] block: compile out assert_bdrv_graph_readable() by default In the report you can see that commits 271..2cf scored between -0.6 - 2.1 and commits 58a..d53 scored 4.1 - 6.4. Anyway from the regular pipelines I can say that: NVMe (RHEL9): * TunedLibvirt/fio-nvme-Aj-4i/:./read-4KiB/throughput/iops_sec.mean improved about 5% * TunedLibvirt/fio-nvme-1j-1i/:./read-4KiB/throughput/iops_sec.mean improved about 3-4% and is slightly more jittery than before Rotational disk (RHEL8): * TunedLibvirt/fio-rot-Aj-8i/:./read-1024KiB/throughput/iops_sec.mean improved about 5% * TunedLibvirt/fio-rot-Aj-8i/:./write-1024KiB/throughput/iops_sec.mean improved about 5% other cases are within the usual jittery and I haven't noticed regressions in any cases. Regards, Lukáš > Stefan > >> >> Based on the commit message I guess it's expected so take this just as a >> record of improvement. >> >> Regards, >> Lukáš >> >> PS: The list of perf-ci issues is available here: >> https://docs.google.com/spreadsheets/d/1HEXI5wDsNgAIgXl5MIhGond898Vz5A1Hrkl0lZmWEbg/edit#gid=0 > OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: qemu-nbd performance regression in bd2cd4a4
I see, let me mark it as "expected" regression and hopefully I'll detect the optimization if they are ever implemented. Thank you for the explanation. Regards, Lukáš Dne 06. 04. 23 v 17:07 Eric Blake napsal(a): > On Thu, Apr 06, 2023 at 12:55:38PM +0200, Lukáš Doktor wrote: >> Hello Florian, folks, >> >> my CI caught ~5% regression (in 60s runs, when using 240s it was about 10%) >> in qemu-nbd performance bisected multiple-times up to >> bd2cd4a441ded163b62371790876f28a9b834317 in fio when using 4k blocks read. >> Note that other scenarios (reads using 1024k blocks, writes using 4 nor >> 1024k blocks) were not affected. Is this expected? > > Large operations (1024k blocks) are dominated by the transaction > itself, and not the network overhead. Small operations (4k reads) > used to benefit from TCP batching (introduces latency, but less > network overhead), but we intentionally started corking things > (decreases latency, but now the network is prone to send smaller > packets which means more network overhead). So a slight decrease in > performance for only small size traffic is not surprising. I'm not > sure if anything can be done about it in the short term, because the > benefits in the other direction (magnitude order of improvement for > TLS traffic) by being transactional instead of batching outweigh the > network overhead of small transactions, and most clients are going to > do more than just minimum-size reads. > > However, commit bd2cd4a44 does mention a potential future optimization > of not uncorking if there is an easy way to detect if another reply in > the queue will be sent shortly. Also, distinct actions for corking > and uncorking costs extra system calls; it may be possible to utilize > MSG_MORE on the existing data syscall paths instead of having to > separately cork/uncork, which in turn could still mark message > transaction boundaries with less overhead. > OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Performance improvement with 81f730d4
Hello Paolo, the perf-ci detected and bisected the 81f730d4 - block, block-backend: write some hot coroutine wrappers by hand - as a performance improvement with 4K read/writes across various profiles. Without pinning it was about 10%, with pinned qemu about 15%. https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/130-improvement.html Based on the commit message I guess it's expected, so my question is whether I should report such obvious improvements or only report improvements/regressions that are not that obvious? Regards, Lukáš OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: Performance improvement with 81f730d4
Hello Kevin Dne 19. 04. 23 v 10:16 Kevin Wolf napsal(a): > Hi Lukáš, > > Am 19.04.2023 um 08:02 hat Lukáš Doktor geschrieben: >> Hello Paolo, >> >> the perf-ci detected and bisected the 81f730d4 - block, block-backend: >> write some hot coroutine wrappers by hand - as a performance >> improvement with 4K read/writes across various profiles. Without >> pinning it was about 10%, with pinned qemu about 15%. >> >> >> https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/130-improvement.html > > I must admit that I don't really understand how to read this page, and > which build corresponds to which QEMU revision (or even which one is > older and which one is newer). > > All the build names are links, but they only result in a 404. The report is originally designed for CI, where the links point to Jenkins results. In bisection I'm reusing the same report, replacing the links with full qemu commit hashes, which for some reason did not happen in this report. I'll take a look at that. The only way to tell the builds apart now is the 3-letter sha which is used as the build name. The builds are ordered as they appear in the git history. You can see that 81f is the first one that performs differently. I'm using 2 out of 3 mode to improve reproducibility, which is why there are usually 2 builds of the same short name. Usually I attach the bisect log but this time it was so obvious I skipped it. Let me attach it now to simplify mapping 3-letter short names to full qemu shas: git bisect start # good: [6c50845a9183610cfd4cfffd48dfc704cd340882] hw/i2c/allwinner-i2c: Fix subclassing of TYPE_AW_I2C_SUN6I git bisect good 6c50845a9183610cfd4cfffd48dfc704cd340882 # bad: [7dbd6f8a27e30fe14adb3d5869097cddf24038d6] Update version for v8.0.0-rc4 release git bisect bad 7dbd6f8a27e30fe14adb3d5869097cddf24038d6 # bad: [3fe64abcde55cf6f4ea5883106301baad219a7cc] block/nfs: do not poll within a coroutine git bisect bad 3fe64abcde55cf6f4ea5883106301baad219a7cc # good: [8c6f27e7d85a794698eb1cd32c58df28cece50d1] block: remove has_variable_length from BlockDriver git bisect good 8c6f27e7d85a794698eb1cd32c58df28cece50d1 # good: [9ed98cae151368cc89c4bb77c9f325f7185e8f09] block-backend: ignore inserted state in blk_co_nb_sectors git bisect good 9ed98cae151368cc89c4bb77c9f325f7185e8f09 # bad: [abb02ce0e76a8e00026699a863ab2d11d88f56d4] Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging git bisect bad abb02ce0e76a8e00026699a863ab2d11d88f56d4 # bad: [81f730d4d0e8af9c0211c3fedf406df0046341a9] block, block-backend: write some hot coroutine wrappers by hand git bisect bad 81f730d4d0e8af9c0211c3fedf406df0046341a9 # first bad commit: [81f730d4d0e8af9c0211c3fedf406df0046341a9] block, block-backend: write some hot coroutine wrappers by hand > >> Based on the commit message I guess it's expected, so my question is >> whether I should report such obvious improvements or only report >> improvements/regressions that are not that obvious? > > I think it's useful to know that we did indeed fix the performance > problem, so as far as I am concerned, please do report them. > OK, I'll keep doing that. > When we're addressing a regression, the most interesting part would of > course be comparing to the version before the regression was introduced > (pre-d8fbf9aa85a in this case) to see if the regression was fully fixed. > Maybe the page you linked above does already explain this and I just > don't know where to look? :-) > I wasn't aware of this to be addressing d8fbf9aa85a regression. If I find enough time I'll try to compare those. Regards, Lukáš > Kevin OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Performance improvement with 6d740fb01b9f0f5ea7a82f4d5e458d91940a19ee
Hello Stefan, folks, the perf-ci detected and bisected the 6d740fb - aio-posix: do not nest poll handlers - as a performance improvement when using multiple concurrent jobs and 4k (22%) as well as 1024k (63%) blocks on aarch64 (on a slow rotational disk). https://ldoktor.github.io/tmp/RedHat-virtlab-arm09/v8.0.0/150-improvement.html Based on the commit message I guess it's expected so take this just as a record of an improvement. Bisect log: git bisect start # good: [aa222a8e4f975284b3f8f131653a4114b3d333b3] Merge tag 'for_upstream' of https://git.kernel.org/pub/scm/virt/kvm/mst/qemu into staging git bisect good aa222a8e4f975284b3f8f131653a4114b3d333b3 # bad: [ad3387396a71416cacc0b394e5e440dd6e9ba19a] Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging git bisect bad ad3387396a71416cacc0b394e5e440dd6e9ba19a # good: [71438d8dac07f28c01cf6d90fce14efe04c77824] graph-lock: Honour read locks even in the main thread git bisect good 71438d8dac07f28c01cf6d90fce14efe04c77824 # good: [4b424c757188f7a47630a4d8edcf4ad9f19255bc] scripts: make sure scripts are invoked via $(PYTHON) git bisect good 4b424c757188f7a47630a4d8edcf4ad9f19255bc # bad: [80fc5d260002432628710f8b0c7cfc7d9b97bb9d] graph-lock: Disable locking for now git bisect bad 80fc5d260002432628710f8b0c7cfc7d9b97bb9d # bad: [6d740fb01b9f0f5ea7a82f4d5e458d91940a19ee] aio-posix: do not nest poll handlers git bisect bad 6d740fb01b9f0f5ea7a82f4d5e458d91940a19ee # good: [78935fcd88ec2d26d50e45043b262f0326e6d410] iotests/245: Check if 'compress' driver is available git bisect good 78935fcd88ec2d26d50e45043b262f0326e6d410 # first bad commit: [6d740fb01b9f0f5ea7a82f4d5e458d91940a19ee] aio-posix: do not nest poll handlers Regards, Lukáš PS: The list of perf-ci issues is available here: https://docs.google.com/spreadsheets/d/1HEXI5wDsNgAIgXl5MIhGond898Vz5A1Hrkl0lZmWEbg/edit#gid=0 OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: Performance improvement with 6d740fb01b9f0f5ea7a82f4d5e458d91940a19ee
Dne 25. 05. 23 v 17:21 Stefan Hajnoczi napsal(a): > On Thu, 25 May 2023 at 06:18, Lukáš Doktor wrote: >> the perf-ci detected and bisected the 6d740fb - aio-posix: do not nest poll >> handlers - as a performance improvement when using multiple concurrent jobs >> and 4k (22%) as well as 1024k (63%) blocks on aarch64 (on a slow rotational >> disk). >> >> >> https://ldoktor.github.io/tmp/RedHat-virtlab-arm09/v8.0.0/150-improvement.html >> >> Based on the commit message I guess it's expected so take this just as a >> record of an improvement. > > The commit was not intended to change performance and I'm not sure why > it happens! > It had and today the x86_64 pipeline finished which shows similar improvement just not in read but rather in write instead and only for 4k blocks (~40%). For 1024k blocks I can see it scoring a bit better (~1.5%). Reads are too jittery to really tell anything on that machine. Anyway I have not done any thorough testing, just a bisection with the most significant setting. From around the same time I can see a NVMe regression in 4k writes, but first bisection job showed nothing. I'll increase the range and try again as each job since that day shows similar drop. Regards, Lukáš > Thanks for letting me know. > > Stefan > OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Qemu-storage-daemon crash with 8ab8140 or later
Hello folks, I found a functional regression in qemu which is making qemu-storage-daemon to crash when using /dev/ram0 as the host device. I bisected it up to: commit 8ab8140a04cf771d63e9754d6ba6c1e676bfe507 Author: Kevin Wolf Date: Fri Feb 3 16:22:02 2023 +0100 block: Mark bdrv_co_refresh_total_sectors() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_co_refresh_total_sectors() need to hold a reader lock for the graph. Signed-off-by: Kevin Wolf Message-Id: <20230203152202.49054-24-kw...@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf To reproduce it create the export by: mkdir -p /var/lib/runperf/runperf-libblkio/ modprobe brd rd_nr=1 rd_size=1048576 max_part=0 qemu-storage-daemon --blockdev driver=host_device,node-name=file,filename=/dev/ram0,cache.direct=on --object iothread,id=iothread0 --export type=vhost-user-blk,iothread=iothread0,id=export,node-name=file,addr.type=unix,addr.path=/var/lib/runperf/runperf-libblkio/vhost-user-blk.sock,writable=on Then create the following fio job in /var/lib/runperf/runperf-libblkio/libblkio.fio: [global] bs=4k ioengine=libblkio libblkio_driver=virtio-blk-vhost-user libblkio_path=/var/lib/runperf/runperf-libblkio/vhost-user-blk.sock rw=read iodepth=1 hipri=1# Can not be set by pbench-fio, use 0 or 1 direct=1 sync=0 time_based=1 clocksource=gettimeofday ramp_time=5 runtime=10 write_bw_log=fio write_iops_log=fio write_lat_log=fio log_avg_msec=1000 write_hist_log=fio log_hist_msec=1 # log_hist_coarseness=4 # 76 bins [job0] And run it using: timeout -s 9 20 fio /var/lib/runperf/runperf-libblkio/libblkio.fio Before the 8ab8140a04cf771d63e9754d6ba6c1e676bfe507 this worked well, using every newer commit results in qemu-storage-daemon coredump and fio hang (which is why I added the timeout to this example) Coredump: [root@virtlab722 ~]# coredumpctl info PID: 40972 (qemu-storage-da) UID: 0 (root) GID: 0 (root) Signal: 6 (ABRT) Timestamp: Mon 2023-03-13 07:51:09 EDT (34s ago) Command Line: qemu-storage-daemon --blockdev driver=host_device,node-name=file,filename=/dev/ram0,cache.direct=on --object iothread,id=iothread0 -> Executable: /usr/local/bin/qemu-storage-daemon Control Group: /user.slice/user-0.slice/session-5.scope Unit: session-5.scope Slice: user-0.slice Session: 5 Owner UID: 0 (root) Boot ID: 7705815f01cb4ccefe6ccfe2c0ba Machine ID: 617b70389a7d4b4280ec97bdcb203def Hostname: virtlab722.virt.lab.eng.bos.redhat.com Storage: /var/lib/systemd/coredump/core.qemu-storage-da.0.7705815f01cb4ccefe6ccfe2c0ba.40972.167870826900.zst (present) Size on Disk: 115.8K Message: Process 40972 (qemu-storage-da) of user 0 dumped core. Stack trace of thread 40974: #0 0x7f89c22a154c __pthread_kill_implementation (libc.so.6 + 0xa154c) #1 0x7f89c2254d46 raise (libc.so.6 + 0x54d46) #2 0x7f89c22287f3 abort (libc.so.6 + 0x287f3) #3 0x7f89c222871b __assert_fail_base.cold (libc.so.6 + 0x2871b) #4 0x7f89c224dce6 __assert_fail (libc.so.6 + 0x4dce6) #5 0x5577565ac060 assert_bdrv_graph_readable (qemu-storage-daemon + 0xb9060) #6 0x557756585552 bdrv_co_nb_sectors (qemu-storage-daemon + 0x92552) #7 0x557756585639 bdrv_get_geometry (qemu-storage-daemon + 0x92639) #8 0x557756570a55 virtio_blk_sect_range_ok (qemu-storage-daemon + 0x7da55) #9 0x557756570387 vu_blk_virtio_process_req (qemu-storage-daemon + 0x7d387) #10 0x55775669cbcb coroutine_trampoline (qemu-storage-daemon + 0x1a9bcb) #11 0x7f89c222a360 n/a (libc.so.6 + 0x2a360) ELF object binary architecture: AMD x86-64 Regards, Lukáš Doktor OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Fio regression caused by f9fc8932b11f3bcf2a2626f567cb6fdd36a33a94
Hello Mike, Paolo, others, in my perf pipeline I noticed a regression bisected to the f9fc8932b11f3bcf2a2626f567cb6fdd36a33a94 - "thread-posix: remove the posix semaphore support" commit and I'd like to ask you to verify it might have caused that and eventually consider fixing it. The regression is visible, reproducible and clearly bisectable to this commit with the following 2 scenarios: 1. fio write 4KiB using the nbd ioengine on localhost 2. fio read 4KiB using #cpu jobs and iodepth=8 on a rotational disk using qcow2 image and default virt-install but smaller regressions can be seen under other scenarios as well since this commit. You can find the report from bisections here: https://ldoktor.github.io/tmp/RedHat-virtlab506/v7.0.0/RedHat-virtlab506-f9fc8932b11f3bcf2a2626f567cb6fdd36a33a94-RHEL-8.4.0-20210503.1-1.html https://ldoktor.github.io/tmp/RedHat-virtlab506/v7.0.0/RedHat-virtlab506-f9fc8932b11f3bcf2a2626f567cb6fdd36a33a94-RHEL-8.4.0-20210503.1-2.html Regards, Lukáš OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: Fio regression caused by f9fc8932b11f3bcf2a2626f567cb6fdd36a33a94
Hello all,
thank you for the responses, I ran 3 runs per each commit using 5 iteration of
fio-nbd using
f9fc8932b11f3bcf2a2626f567cb6fdd36a33a94
f9fc8932b11f3bcf2a2626f567cb6fdd36a33a94 + Stefan's commit
d7482ffe9756919531307330fd1c6dbec66e8c32
using the regressed f9fc8932b11f3bcf2a2626f567cb6fdd36a33a94 as a base-line the
relative percentage results were:
f9f| 0.0 | -2.8 | 0.6
stefan | -3.1 | -1.2 | -2.2
d74| 7.2 | 9.1 | 8.2
Not sure whether the Stefan's commit was suppose to be applied on top of the
f9fc893b commit but at least for fio-nbd 4k writes it slightly worsen the
situation.
Do you want me to try the fio inside guest as well, or is this fio-nbd check
sufficient for now?
Also let me briefly share the details about the execution:
---
mkdir -p /var/lib/runperf/runperf-nbd/
truncate -s 256M /var/lib/runperf/runperf-nbd//disk.img
nohup qemu-nbd -t -k /var/lib/runperf/runperf-nbd//socket -f raw
/var/lib/runperf/runperf-nbd//disk.img &> $(mktemp
/var/lib/runperf/runperf-nbd//qemu_nbd_.log) & echo $! >>
/var/lib/runperf/runperf-nbd//kill_pids
for PID in $(cat /var/lib/runperf/runperf-nbd//kill_pids); do disown -h $PID;
done
export TERM=xterm-256color
true
mkdir -p /var/lib/runperf/runperf-nbd/
cat > /var/lib/runperf/runperf-nbd/nbd.fio << \Gr1UaS
# To use fio to test nbdkit:
#
# nbdkit -U - memory size=256M --run 'export unixsocket; fio examples/nbd.fio'
#
# To use fio to test qemu-nbd:
#
# rm -f /tmp/disk.img /tmp/socket
# truncate -s 256M /tmp/disk.img
# export target=/tmp/socket
# qemu-nbd -t -k $target -f raw /tmp/disk.img &
# fio examples/nbd.fio
# killall qemu-nbd
[global]
bs = $@
runtime = 30
ioengine = nbd
iodepth = 32
direct = 1
sync = 0
time_based = 1
clocksource = gettimeofday
ramp_time = 5
write_bw_log = fio
write_iops_log = fio
write_lat_log = fio
log_avg_msec = 1000
write_hist_log = fio
log_hist_msec = 1
# log_hist_coarseness = 4 # 76 bins
rw = $@
uri=nbd+unix:///?socket=/var/lib/runperf/runperf-nbd/socket
# Starting from nbdkit 1.14 the following will work:
#uri=${uri}
[job0]
offset=0
[job1]
offset=64m
[job2]
offset=128m
[job3]
offset=192m
Gr1UaS
benchmark_bin=/usr/local/bin/fio pbench-fio --block-sizes=4
--job-file=/var/lib/runperf/runperf-nbd/nbd.fio --numjobs=4 --runtime=60
--samples=5 --test-types=write --clients=$WORKER_IP
---
I am using pbench to run the execution, but you can simply replace the "$@"
variables in the produced "/var/lib/runperf/runperf-nbd/nbd.fio" and run it
directly using fio.
Regards,
Lukáš
Dne 05. 05. 22 v 15:27 Paolo Bonzini napsal(a):
> On 5/5/22 14:44, Daniel P. Berrangé wrote:
>>> util/thread-pool.c uses qemu_sem_*() to notify worker threads when work
>>> becomes available. It makes sense that this operation is
>>> performance-critical and that's why the benchmark regressed.
>>
>> Doh, I questioned whether the change would have a performance impact,
>> and it wasn't thought to be used in perf critical places
>
> The expectation was that there would be no contention and thus no overhead
> because of the pool->lock that exists anyway, but that was optimistic.
>
> Lukáš, can you run a benchmark with this condvar implementation that was
> suggested by Stefan:
>
> https://lore.kernel.org/qemu-devel/20220505131346.823941-1-pbonz...@redhat.com/raw
>
> ?
>
> If it still regresses, we can either revert the patch or look at a different
> implementation (even getting rid of the global queue is an option).
>
> Thanks,
>
> Paolo
>
OpenPGP_0x26B362E47FCF22C1.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 0/2] thread-pool: fix performance regression
Hello Paolo, folks, I gave it a try (on top of the f9fc8932) and it's better than the f9fc8932, better than the previous patch by Stefan, but still I'm not reaching the performance of d7482ffe97 (before the f9fc8932 commit): f9f| 0.0 | -2.8 | 0.6 stefan | -3.1 | -1.2 | -2.2 paolo | 5.3 | 5.4 | 7.1 d74| 7.2 | 9.1 | 8.2 Anyway it's definitely closer to the previous baseline (~-2%). Note I have not tried other scenarios, just the 4K nbd writes on rotational disk. I'll try running more throughout the night. Regards, Lukáš Dne 06. 05. 22 v 13:47 Paolo Bonzini napsal(a): > Together, these two patches fix the performance regression induced by > QemuSemaphore; individually they don't though. > > 6.2: >iops: min=58051, max=62260, avg=60282.57, stdev=1081.18, samples=30 > clat percentiles (usec): 1.00th=[ 490], 99.99th=[ 775] >iops: min=59401, max=61290, avg=60651.27, stdev=468.24, samples=30 > clat percentiles (usec): 1.00th=[ 490], 99.99th=[ 717] >iops: min=59583, max=60816, avg=60353.43, stdev=282.69, samples=30 > clat percentiles (usec): 1.00th=[ 490], 99.99th=[ 701] >iops: min=58099, max=60713, avg=59739.53, stdev=755.49, samples=30 > clat percentiles (usec): 1.00th=[ 494], 99.99th=[ 717] > > > patched: >iops: min=60616, max=62522, avg=61654.37, stdev=555.67, samples=30 > clat percentiles (usec): 1.00th=[ 474], 99.99th=[ 1303] >iops: min=61841, max=63600, avg=62878.47, stdev=442.40, samples=30 > clat percentiles (usec): 1.00th=[ 465], 99.99th=[ 685] >iops: min=62976, max=63910, avg=63531.60, stdev=261.05, samples=30 > clat percentiles (usec): 1.00th=[ 461], 99.99th=[ 693] >iops: min=60803, max=63623, avg=62653.37, stdev=808.76, samples=30 > clat percentiles (usec): 1.00th=[ 465], 99.99th=[ 685] > > Paolo > > Supersedes: <20220505131346.823941-1-pbonz...@redhat.com> > > Paolo Bonzini (2): > thread-pool: optimize scheduling of completion bottom half > thread-pool: replace semaphore with condition variable > > util/thread-pool.c | 34 +- > 1 file changed, 13 insertions(+), 21 deletions(-) > OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 0/2] thread-pool: fix performance regression
Dne 06. 05. 22 v 20:55 Lukáš Doktor napsal(a): > Hello Paolo, folks, I gave it a try (on top of the f9fc8932) and it's better > than the f9fc8932, better than the previous patch by Stefan, but still I'm > not reaching the performance of d7482ffe97 (before the f9fc8932 commit): > > f9f| 0.0 | -2.8 | 0.6 > stefan | -3.1 | -1.2 | -2.2 > paolo | 5.3 | 5.4 | 7.1 > d74| 7.2 | 9.1 | 8.2 > > Anyway it's definitely closer to the previous baseline (~-2%). Note I have > not tried other scenarios, just the 4K nbd writes on rotational disk. I'll > try running more throughout the night. > I tried a couple of iterations of fio-nbd 4/64/256KB read/writes on a rotational disk and overall the latest fix results in a steady 2.5% throughput regression for the 4KiB writes. The remaining tested variants performed similarly. Please let me know if you want me to test the fio execution inside the guest as well or some other variants. Regards, Lukáš OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH] virtio-blk: add missing AioContext lock
Dne 08. 02. 23 v 12:11 Emanuele Giuseppe Esposito napsal(a):
> virtio_blk_update_config() calls blk_get_geometry and blk_getlength,
> and both functions eventually end up calling bdrv_poll_co when not
> running in a coroutine:
> - blk_getlength is a co_wrapper_mixed function
> - blk_get_geometry calls bdrv_get_geometry -> bdrv_nb_sectors, a
> co_wrapper_mixed function too
>
> Since we are not running in a coroutine, we need to take s->blk
> AioContext lock, otherwise bdrv_poll_co will inevitably call
> AIO_WAIT_WHILE and therefore try to un unlock() an AioContext lock
> that was never acquired.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2167838
>
> Steps to reproduce the issue: simply boot a VM with
> -object '{"qom-type":"iothread","id":"iothread1"}' \
> -blockdev
> '{"driver":"file","filename":"$QCOW2","aio":"native","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}'
> \
> -blockdev
> '{"node-name":"libvirt-1-format","read-only":false,"cache":{"direct":true,"no-flush":false},"driver":"qcow2","file":"libvirt-1-storage"}'
> \
> -device
> virtio-blk-pci,iothread=iothread1,drive=libvirt-1-format,id=virtio-disk0,bootindex=1,write-cache=on
>
> and observe that it will fail not manage to boot with
> "qemu_mutex_unlock_impl: Operation not permitted"
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> hw/block/virtio-blk.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 1762517878..cefca93b31 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -894,6 +894,10 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
> uint8_t *config)
> uint64_t capacity;
> int64_t length;
> int blk_size = conf->logical_block_size;
> +AioContext *ctx;
> +
> +ctx = blk_get_aio_context(s->blk);
> +aio_context_acquire(ctx);
>
> blk_get_geometry(s->blk, &capacity);
> memset(&blkcfg, 0, sizeof(blkcfg));
> @@ -917,6 +921,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
> uint8_t *config)
> * per track (cylinder).
> */
> length = blk_getlength(s->blk);
> +aio_context_release(ctx);
> if (length > 0 && length / conf->heads / conf->secs % blk_size) {
> blkcfg.geometry.sectors = conf->secs & ~s->sector_mask;
> } else {
Thanks, I can't talk about the correctness of the code but function-wise it
addresses the issue.
Tested-by: Lukáš Doktor
OpenPGP_0x26B362E47FCF22C1.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: Proposal for a regular upstream performance testing
Dear qemu developers, you might remember the "replied to" email from a bit over year ago to raise a discussion about a qemu performance regression CI. On KVM forum I presented https://www.youtube.com/watch?v=Cbm3o4ACE3Y&list=PLbzoR-pLrL6q4ZzA4VRpy42Ua4-D2xHUR&index=9 some details about my testing pipeline. I think it's stable enough to become part of the official CI so people can consume, rely on it and hopefully even suggest configuration changes. The CI consists of: 1. Jenkins pipeline(s) - internal, not available to developers, running daily builds of the latest available commit 2. Publicly available anonymized results: https://ldoktor.github.io/tmp/RedHat-Perf-worker1/ 3. (optional) a manual gitlab pulling job which triggered by the Jenkins pipeline when that particular commit is checked The (1) is described here: https://run-perf.readthedocs.io/en/latest/jenkins.html and can be replicated on other premises and the individual jobs can be executed directly https://run-perf.readthedocs.io on any linux box using Fedora guests (via pip or container https://run-perf.readthedocs.io/en/latest/container.html ). As for the (3) I made a testing pipeline available here: https://gitlab.com/ldoktor/qemu/-/pipelines with one always-passing test and one allow-to-fail actual testing job. If you think such integration would be useful, I can add it as another job to the official qemu repo. Note the integration is a bit hacky as, due to resources, we can not test all commits but rather test on daily basis, which is not officially supported by gitlab. Note the aim of this project is to ensure some very basic system-level workflow performance stays the same or that the differences are described and ideally pinned to individual commits. It should not replace thorough release testing or low-level performance tests. Regards, Lukáš Dne 26. 11. 20 v 9:10 Lukáš Doktor napsal(a): > Hello guys, > > I had been around qemu on the Avocado-vt side for quite some time and a while > ago I shifted my focus on performance testing. Currently I am not aware of > any upstream CI that would continuously monitor the upstream qemu performance > and I'd like to change that. There is a lot to cover so please bear with me. > > Goal > > > The goal of this initiative is to detect system-wide performance regressions > as well as improvements early, ideally pin-point the individual commits and > notify people that they should fix things. All in upstream and ideally with > least human interaction possible. > > Unlike the recent work of Ahmed Karaman's > https://ahmedkrmn.github.io/TCG-Continuous-Benchmarking/ my aim is on the > system-wide performance inside the guest (like fio, uperf, ...) > > Tools > = > > In house we have several different tools used by various teams and I bet > there are tons of other tools out there that can do that. I can not speak for > all teams but over the time many teams at Red Hat have come to like pbench > https://distributed-system-analysis.github.io/pbench/ to run the tests and > produce machine readable results and use other tools (Ansible, scripts, ...) > to provision the systems and to generate the comparisons. > > As for myself I used python for PoC and over the last year I pushed hard to > turn it into a usable and sensible tool which I'd like to offer: > https://run-perf.readthedocs.io/en/latest/ anyway I am open to suggestions > and comparisons. As I am using it downstream to watch regressions I do plan > on keep developing the tool as well as the pipelines (unless a better tool is > found that would replace it or it's parts). > > How > === > > This is a tough question. Ideally this should be a standalone service that > would only notify the author of the patch that caused the change with a bunch > of useful data so they can either address the issue or just be aware of this > change and mark it as expected. > > Ideally the community should have a way to also issue their custom builds in > order to verify their patches so they can debug and address issues better > than just commit to qemu-master. > > The problem with those is that we can not simply use travis/gitlab/... > machines for running those tests, because we are measuring in-guest actual > performance. We can't just stop the time when the machine decides to schedule > another container/vm. I briefly checked the public bare-metal offerings like > rackspace but these are most probably not sufficient either because (unless > I'm wrong) they only give you a machine but it is not guaranteed that it will > be the same machine the next time. If we are to compare the results we don't > need just the same model, we really need the very same machine. Any change t
Re: Proposal for a regular upstream performance testing
Hello Stefan, Dne 21. 03. 22 v 10:42 Stefan Hajnoczi napsal(a): > On Mon, Mar 21, 2022 at 09:46:12AM +0100, Lukáš Doktor wrote: >> Dear qemu developers, >> >> you might remember the "replied to" email from a bit over year ago to raise >> a discussion about a qemu performance regression CI. On KVM forum I >> presented >> https://www.youtube.com/watch?v=Cbm3o4ACE3Y&list=PLbzoR-pLrL6q4ZzA4VRpy42Ua4-D2xHUR&index=9 >> some details about my testing pipeline. I think it's stable enough to >> become part of the official CI so people can consume, rely on it and >> hopefully even suggest configuration changes. >> >> The CI consists of: >> >> 1. Jenkins pipeline(s) - internal, not available to developers, running >> daily builds of the latest available commit >> 2. Publicly available anonymized results: >> https://ldoktor.github.io/tmp/RedHat-Perf-worker1/ > > This link is 404. > My mistake, it works well without the tailing slash: https://ldoktor.github.io/tmp/RedHat-Perf-worker1 >> 3. (optional) a manual gitlab pulling job which triggered by the Jenkins >> pipeline when that particular commit is checked >> >> The (1) is described here: >> https://run-perf.readthedocs.io/en/latest/jenkins.html and can be replicated >> on other premises and the individual jobs can be executed directly >> https://run-perf.readthedocs.io on any linux box using Fedora guests (via >> pip or container https://run-perf.readthedocs.io/en/latest/container.html ). >> >> As for the (3) I made a testing pipeline available here: >> https://gitlab.com/ldoktor/qemu/-/pipelines with one always-passing test and >> one allow-to-fail actual testing job. If you think such integration would be >> useful, I can add it as another job to the official qemu repo. Note the >> integration is a bit hacky as, due to resources, we can not test all commits >> but rather test on daily basis, which is not officially supported by gitlab. >> >> Note the aim of this project is to ensure some very basic system-level >> workflow performance stays the same or that the differences are described >> and ideally pinned to individual commits. It should not replace thorough >> release testing or low-level performance tests. > > If I understand correctly the GitLab CI integration you described > follows the "push" model where Jenkins (running on your own machine) > triggers a manual job in GitLab CI simply to indicate the status of the > nightly performance regression test? > > What process should QEMU follow to handle performance regressions > identified by your job? In other words, which stakeholders need to > triage, notify, debug, etc when a regression is identified? > > My guess is: > - Someone (you or the qemu.git committer) need to watch the job status and > triage failures. > - That person then notifies likely authors of suspected commits so they can > investigate. > - The authors need a way to reproduce the issue - either locally or by > pushing commits to GitLab and waiting for test results. > - Fixes will be merged as additional qemu.git commits since commit history > cannot be rewritten. > - If necessary a git-revert(1) commit can be merged to temporarily undo a > commit that caused issues. > > Who will watch the job status and triage failures? > > Stefan This is exactly the main question I'd like to resolve as part of considering-this-to-be-official-part-of-the-upstream-qemu-testing. At this point our team is offering it's service to maintain this single worker for daily jobs, monitoring the status and pinging people in case of bisectable results. From the upstream qemu community we are mainly looking for a feedback: * whether they'd want to be notified of such issues (and via what means) * whether the current approach seems to be actually performing useful tasks * whether the reports are understandable * whether the reports should be regularly pushed into publicly available place (or just on regression/improvement) * whether there are any volunteers to be interested in non-clearly-bisectable issues (probably by-topic) Note that not all issues needs to be addressed, some might only result in notes that should help us understand why qemu behaves differently after rebasing our downstream version. As for the hopefully-not-so-distant-future we already have a second machine based on el9 with NVMe disk in process of preparations and if this pipeline proves to be useful we do have plans to cover other architecture(s) as well. Aside of this other companies might replicate our setup based on the documentation with their machines, their scenarios and their distros of choice. Regards, Lukáš OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: Proposal for a regular upstream performance testing
Hello Stefan, folks, I seem to have another hit, an improvement actually and it seems to be bisected all the way to you, Stefan. Let me use this as another example of how such process could look like and we can use this to hammer-out the details like via what means to submit the request, whom to notify and how to proceed further. --- Last week I noticed an improvement in TunedLibvirt/fio-rot-Aj-8i/:./write-4KiB/throughput/iops_sec.mean (, fio, rotationary disk, raw file on host xfs partition, jobs=#cpus, iodepth=8, 4k writes) check and bisected it to: commit fc8796465c6cd4091efe6a2f8b353f07324f49c7 Author: Stefan Hajnoczi Date: Wed Feb 23 15:57:03 2022 + aio-posix: fix spurious ->poll_ready() callbacks in main loop Could you please confirm that it does make sense and that it is expected? (looks like it from the description). Note that this commit was pin pointed using 2 out of 3 commits result, there were actually some little differences between commits fc8 and cc5. The fc8 and 202 results scored similarly to both, good and bad commits with 2 being closer to the bad one. Since cc5 they seem to stabilize further scoring slightly lower than the median fc8 result. Anyway I don't have enough data to declare anything. I can bisect it further if needed. The git bisect log: git bisect start # good: [ecf1bbe3227cc1c54d7374aa737e7e0e60ee0c29] Merge tag 'pull-ppc-20220321' of https://github.com/legoater/qemu into staging git bisect good ecf1bbe3227cc1c54d7374aa737e7e0e60ee0c29 # bad: [9d36d5f7e0dc905d8cb3dd437e479eb536417d3b] Merge tag 'pull-block-2022-03-22' of https://gitlab.com/hreitz/qemu into staging git bisect bad 9d36d5f7e0dc905d8cb3dd437e479eb536417d3b # bad: [0f7d7d72aa99c8e48bbbf37262a9c66c83113f76] iotests: use qemu_img_json() when applicable git bisect bad 0f7d7d72aa99c8e48bbbf37262a9c66c83113f76 # bad: [cc5387a544325c26dcf124ac7d3999389c24e5c6] block/rbd: fix write zeroes with growing images git bisect bad cc5387a544325c26dcf124ac7d3999389c24e5c6 # good: [b21e2380376c470900fcadf47507f4d5ade75e85] Use g_new() & friends where that makes obvious sense git bisect good b21e2380376c470900fcadf47507f4d5ade75e85 # bad: [2028ab513bf0232841a909e1368309858919dbcc] Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging git bisect bad 2028ab513bf0232841a909e1368309858919dbcc # bad: [fc8796465c6cd4091efe6a2f8b353f07324f49c7] aio-posix: fix spurious ->poll_ready() callbacks in main loop git bisect bad fc8796465c6cd4091efe6a2f8b353f07324f49c7 # good: [8a947c7a586e16a048894e1a0a73d154435e90ef] aio-posix: fix build failure io_uring 2.2 git bisect good 8a947c7a586e16a048894e1a0a73d154435e90ef # first bad commit: [fc8796465c6cd4091efe6a2f8b353f07324f49c7] aio-posix: fix spurious ->poll_ready() callbacks in main loop Also please find the bisection report attached. I can attach the VM xml file or other logs if needed. Regards, Lukáš Dne 22. 03. 22 v 16:05 Stefan Hajnoczi napsal(a): > On Mon, Mar 21, 2022 at 11:29:42AM +0100, Lukáš Doktor wrote: >> Hello Stefan, >> >> Dne 21. 03. 22 v 10:42 Stefan Hajnoczi napsal(a): >>> On Mon, Mar 21, 2022 at 09:46:12AM +0100, Lukáš Doktor wrote: >>>> Dear qemu developers, >>>> >>>> you might remember the "replied to" email from a bit over year ago to >>>> raise a discussion about a qemu performance regression CI. On KVM forum I >>>> presented >>>> https://www.youtube.com/watch?v=Cbm3o4ACE3Y&list=PLbzoR-pLrL6q4ZzA4VRpy42Ua4-D2xHUR&index=9 >>>> some details about my testing pipeline. I think it's stable enough to >>>> become part of the official CI so people can consume, rely on it and >>>> hopefully even suggest configuration changes. >>>> >>>> The CI consists of: >>>> >>>> 1. Jenkins pipeline(s) - internal, not available to developers, running >>>> daily builds of the latest available commit >>>> 2. Publicly available anonymized results: >>>> https://ldoktor.github.io/tmp/RedHat-Perf-worker1/ >>> >>> This link is 404. >>> >> >> My mistake, it works well without the tailing slash: >> https://ldoktor.github.io/tmp/RedHat-Perf-worker1 >> >>>> 3. (optional) a manual gitlab pulling job which triggered by the Jenkins >>>> pipeline when that particular commit is checked >>>> >>>> The (1) is described here: >>>> https://run-perf.readthedocs.io/en/latest/jenkins.html and can be >>>> replicated on other premises and the individual jobs can be executed >>>> directly https://run-perf.readthedocs.io on any linux box using Fedora >>>> guests (via pip or container >>>>
Re: Proposal for a regular upstream performance testing
Dne 28. 03. 22 v 11:57 Stefan Hajnoczi napsal(a): > On Mon, Mar 28, 2022 at 08:18:43AM +0200, Lukáš Doktor wrote: >> Hello Stefan, folks, >> >> I seem to have another hit, an improvement actually and it seems to be >> bisected all the way to you, Stefan. Let me use this as another example of >> how such process could look like and we can use this to hammer-out the >> details like via what means to submit the request, whom to notify and how to >> proceed further. >> >> --- >> >> Last week I noticed an improvement in >> TunedLibvirt/fio-rot-Aj-8i/:./write-4KiB/throughput/iops_sec.mean >> (, fio, rotationary >> disk, raw file on host xfs partition, jobs=#cpus, iodepth=8, 4k writes) >> check and bisected it to: >> >> commit fc8796465c6cd4091efe6a2f8b353f07324f49c7 >> Author: Stefan Hajnoczi >> Date: Wed Feb 23 15:57:03 2022 + >> >> aio-posix: fix spurious ->poll_ready() callbacks in main loop >> >> Could you please confirm that it does make sense and that it is expected? >> (looks like it from the description). >> >> Note that this commit was pin pointed using 2 out of 3 commits result, there >> were actually some little differences between commits fc8 and cc5. The fc8 >> and 202 results scored similarly to both, good and bad commits with 2 being >> closer to the bad one. Since cc5 they seem to stabilize further scoring >> slightly lower than the median fc8 result. Anyway I don't have enough data >> to declare anything. I can bisect it further if needed. > > Yes, I can imagine that commit fc8796465c6c might improve non-IOThread > performance! > Hello Stefan and thank you for this confirmation as well as questions. Let me explain that a bit and perhaps you can help me to improve the report as it was designed for CI team and can be I can see where the confusion might come from. > I don't know how to read the report: > - What is the difference between "Group stats" and "Failures"? > - Why are there 3 different means in "Group stats"? The group stats combine (average) multiple individual failures with some common aspect and stricter thresholds are then used based on the number of matches. The group names contain asterisk (*) character which works the same way linux globs work and all matching tests are included. In this report it doesn't really makes sense as only single test variant was performed, but when various tests/scenarios are being used it can indicate change across all fio-write jobs, or all tests on various profiles... > - Why are there 3 "fc8" columns in "Failures"? To avoid bisect wandering away I'm usually using 3-5 samples per run. Still this wasn't reliable enough in some cases and bisect ended up on random commits so I added a 2 out of 3 feature. Every commit is checked twice and when they don't match it runs the third one. In the end all of the attempts are included in the report to better visualize how stable the results are. > > I don't feel confident searching git-log(1) output with 3-character > commit IDs. git itself uses 12 characters for short commit IDs with a > reasonably low chance of collisions. > I chose 3 letters only as usually the bisection window is quite small and there is the bisection log with all the full commits attached. Using longer hashes results in very wide table, which I wanted to avoid. Perhaps I can modify the links which currently point to the jenkins results to point to the qemu sha instead, what do you think? Regards, Lukáš > Stefan OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH] virtio-blk: fix host notifier issues during dataplane start/stop
Thank you, Stefan, I tested it with the extended set of tests and it addresses
the issue.
Regards,
Lukáš
Tested-by: Lukas Doktor
Dne 04. 07. 23 v 17:15 Stefan Hajnoczi napsal(a):
> The main loop thread can consume 100% CPU when using --device
> virtio-blk-pci,iothread=. ppoll() constantly returns but
> reading virtqueue host notifiers fails with EAGAIN. The file descriptors
> are stale and remain registered with the AioContext because of bugs in
> the virtio-blk dataplane start/stop code.
>
> The problem is that the dataplane start/stop code involves drain
> operations, which call virtio_blk_drained_begin() and
> virtio_blk_drained_end() at points where the host notifier is not
> operational:
> - In virtio_blk_data_plane_start(), blk_set_aio_context() drains after
> vblk->dataplane_started has been set to true but the host notifier has
> not been attached yet.
> - In virtio_blk_data_plane_stop(), blk_drain() and blk_set_aio_context()
> drain after the host notifier has already been detached but with
> vblk->dataplane_started still set to true.
>
> I would like to simplify ->ioeventfd_start/stop() to avoid interactions
> with drain entirely, but couldn't find a way to do that. Instead, this
> patch accepts the fragile nature of the code and reorders it so that
> vblk->dataplane_started is false during drain operations. This way the
> virtio_blk_drained_begin() and virtio_blk_drained_end() calls don't
> touch the host notifier. The result is that
> virtio_blk_data_plane_start() and virtio_blk_data_plane_stop() have
> complete control over the host notifier and stale file descriptors are
> no longer left in the AioContext.
>
> This patch fixes the 100% CPU consumption in the main loop thread and
> correctly moves host notifier processing to the IOThread.
>
> Fixes: 1665d9326fd2 ("virtio-blk: implement BlockDevOps->drained_begin()")
> Reported-by: Lukáš Doktor
> Signed-off-by: Stefan Hajnoczi
> ---
> hw/block/dataplane/virtio-blk.c | 67 +++--
> 1 file changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index c227b39408..da36fcfd0b 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -219,13 +219,6 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
>
> memory_region_transaction_commit();
>
> -/*
> - * These fields are visible to the IOThread so we rely on implicit
> barriers
> - * in aio_context_acquire() on the write side and aio_notify_accept() on
> - * the read side.
> - */
> -s->starting = false;
> -vblk->dataplane_started = true;
> trace_virtio_blk_data_plane_start(s);
>
> old_context = blk_get_aio_context(s->conf->conf.blk);
> @@ -244,6 +237,18 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> event_notifier_set(virtio_queue_get_host_notifier(vq));
> }
>
> +/*
> + * These fields must be visible to the IOThread when it processes the
> + * virtqueue, otherwise it will think dataplane has not started yet.
> + *
> + * Make sure ->dataplane_started is false when blk_set_aio_context() is
> + * called above so that draining does not cause the host notifier to be
> + * detached/attached prematurely.
> + */
> +s->starting = false;
> +vblk->dataplane_started = true;
> +smp_wmb(); /* paired with aio_notify_accept() on the read side */
> +
> /* Get this show started by hooking up our callbacks */
> if (!blk_in_drain(s->conf->conf.blk)) {
> aio_context_acquire(s->ctx);
> @@ -273,7 +278,6 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
>fail_guest_notifiers:
> vblk->dataplane_disabled = true;
> s->starting = false;
> -vblk->dataplane_started = true;
> return -ENOSYS;
> }
>
> @@ -327,6 +331,32 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
> aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
> }
>
> +/*
> + * Batch all the host notifiers in a single transaction to avoid
> + * quadratic time complexity in address_space_update_ioeventfds().
> + */
> +memory_region_transaction_begin();
> +
> +for (i = 0; i < nvqs; i++) {
> +virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
> +}
> +
> +/*
> + * The transaction expects the ioeventfds to be open when it
> + * commits. Do it now, before the cleanup loop.
> + */
> +memory_region_transaction_commit();
> +
> +for (i = 0; i < nvqs; i++) {
> +virti
Re: [PATCH] block/nvme: invoke blk_io_plug_call() outside q->lock
Thank you, Stefan, I tested this one as well and it boots now and seems to
behave correctly under the load as well.
Regards,
Lukáš
Tested-by: Lukas Doktor
Dne 12. 07. 23 v 21:16 Stefan Hajnoczi napsal(a):
> blk_io_plug_call() is invoked outside a blk_io_plug()/blk_io_unplug()
> section while opening the NVMe drive from:
>
> nvme_file_open() ->
> nvme_init() ->
> nvme_identify() ->
> nvme_admin_cmd_sync() ->
> nvme_submit_command() ->
> blk_io_plug_call()
>
> blk_io_plug_call() immediately invokes the given callback when the
> current thread is not plugged, as is the case during nvme_file_open().
>
> Unfortunately, nvme_submit_command() calls blk_io_plug_call() with
> q->lock still held:
>
> ...
> q->sq.tail = (q->sq.tail + 1) % NVME_QUEUE_SIZE;
> q->need_kick++;
> blk_io_plug_call(nvme_unplug_fn, q);
> qemu_mutex_unlock(&q->lock);
> ^^^
>
> nvme_unplug_fn() deadlocks trying to acquire q->lock because the lock is
> already acquired by the same thread. The symptom is that QEMU hangs
> during startup while opening the NVMe drive.
>
> Fix this by moving the blk_io_plug_call() outside q->lock. This is safe
> because no other thread runs code related to this queue and
> blk_io_plug_call()'s internal state is immune to thread safety issues
> since it is thread-local.
>
> Reported-by: Lukáš Doktor
> Fixes: f2e590002bd6 ("block/nvme: convert to blk_io_plug_call() API")
> Signed-off-by: Stefan Hajnoczi
> ---
> block/nvme.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 7ca85bc44a..b6e95f0b7e 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -501,8 +501,9 @@ static void nvme_submit_command(NVMeQueuePair *q,
> NVMeRequest *req,
> q->sq.tail * NVME_SQ_ENTRY_BYTES, cmd, sizeof(*cmd));
> q->sq.tail = (q->sq.tail + 1) % NVME_QUEUE_SIZE;
> q->need_kick++;
> +qemu_mutex_unlock(&q->lock);
> +
> blk_io_plug_call(nvme_unplug_fn, q);
> -qemu_mutex_unlock(&q->lock);
> }
>
> static void nvme_admin_cmd_sync_cb(void *opaque, int ret)
OpenPGP_0x26B362E47FCF22C1.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: Performance improvement with 6d740fb01b9f0f5ea7a82f4d5e458d91940a19ee
Dne 26. 05. 23 v 12:56 Stefan Hajnoczi napsal(a): > On Fri, 26 May 2023 at 04:07, Lukáš Doktor wrote: >> >> Dne 25. 05. 23 v 17:21 Stefan Hajnoczi napsal(a): >>> On Thu, 25 May 2023 at 06:18, Lukáš Doktor wrote: >>>> the perf-ci detected and bisected the 6d740fb - aio-posix: do not nest >>>> poll handlers - as a performance improvement when using multiple >>>> concurrent jobs and 4k (22%) as well as 1024k (63%) blocks on aarch64 (on >>>> a slow rotational disk). >>>> >>>> >>>> https://ldoktor.github.io/tmp/RedHat-virtlab-arm09/v8.0.0/150-improvement.html >>>> >>>> Based on the commit message I guess it's expected so take this just as a >>>> record of an improvement. >>> >>> The commit was not intended to change performance and I'm not sure why >>> it happens! >>> >> >> It had and today the x86_64 pipeline finished which shows similar >> improvement just not in read but rather in write instead and only for 4k >> blocks (~40%). For 1024k blocks I can see it scoring a bit better (~1.5%). >> Reads are too jittery to really tell anything on that machine. Anyway I have >> not done any thorough testing, just a bisection with the most significant >> setting. >> >> From around the same time I can see a NVMe regression in 4k writes, but >> first bisection job showed nothing. I'll increase the range and try again as >> each job since that day shows similar drop. > Hello Stefan, folks, the regression proved to be there and stably reproducible. With NVMe 4k writes with jobs=10 and iodepth=4 I can see a 50% regression on my machine: https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/150-regression.html The rest of the cases doesn't show any change at all. I can provide more data if someone is interested. Regards, Lukáš > Thanks! > > Stefan > OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: Performance improvement and regression with 6d740fb01b9f0f5ea7a82f4d5e458d91940a19ee
Dne 31. 05. 23 v 19:14 Stefan Hajnoczi napsal(a): > On Wed, 31 May 2023 at 12:50, Lukáš Doktor wrote: >> >> Dne 26. 05. 23 v 12:56 Stefan Hajnoczi napsal(a): >>> On Fri, 26 May 2023 at 04:07, Lukáš Doktor wrote: >>>> >>>> Dne 25. 05. 23 v 17:21 Stefan Hajnoczi napsal(a): >>>>> On Thu, 25 May 2023 at 06:18, Lukáš Doktor wrote: >>>>>> the perf-ci detected and bisected the 6d740fb - aio-posix: do not nest >>>>>> poll handlers - as a performance improvement when using multiple >>>>>> concurrent jobs and 4k (22%) as well as 1024k (63%) blocks on aarch64 >>>>>> (on a slow rotational disk). >>>>>> >>>>>> >>>>>> https://ldoktor.github.io/tmp/RedHat-virtlab-arm09/v8.0.0/150-improvement.html >>>>>> >>>>>> Based on the commit message I guess it's expected so take this just as a >>>>>> record of an improvement. >>>>> >>>>> The commit was not intended to change performance and I'm not sure why >>>>> it happens! >>>>> >>>> >>>> It had and today the x86_64 pipeline finished which shows similar >>>> improvement just not in read but rather in write instead and only for 4k >>>> blocks (~40%). For 1024k blocks I can see it scoring a bit better (~1.5%). >>>> Reads are too jittery to really tell anything on that machine. Anyway I >>>> have not done any thorough testing, just a bisection with the most >>>> significant setting. >>>> >>>> From around the same time I can see a NVMe regression in 4k writes, but >>>> first bisection job showed nothing. I'll increase the range and try again >>>> as each job since that day shows similar drop. >>> >> >> Hello Stefan, folks, >> >> the regression proved to be there and stably reproducible. With NVMe 4k >> writes with jobs=10 and iodepth=4 I can see a 50% regression on my machine: >> >> >> https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/150-regression.html >> >> The rest of the cases doesn't show any change at all. I can provide more >> data if someone is interested. > > Which commit caused the regression? Hello Stefan, the same one that caused the improvement on rotational disks: 6d740fb01b9f0f5ea7a82f4d5e458d91940a19ee Lukáš > > Stefan > OpenPGP_0x26B362E47FCF22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
[Qemu-devel] [PATCH v5 09/10] qmp.py: Avoid overriding a builtin object
The "id" is a builtin method to get object's identity and should not be
overridden. This might bring some issues in case someone was directly
calling "cmd(..., id=id)" but I haven't found such usage on brief search
for "cmd\(.*id=".
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qmp/qmp.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index f2f5a9b..ef12e8a 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
print >>sys.stderr, "QMP:<<< %s" % resp
return resp
-def cmd(self, name, args=None, id=None):
+def cmd(self, name, args=None, cmd_id=None):
"""
Build a QMP command and send it to the QMP Monitor.
@param name: command name (string)
@param args: command arguments (dict)
-@param id: command id (dict, list, string or int)
+@param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
-if id:
-qmp_cmd['id'] = id
+if cmd_id:
+qmp_cmd['id'] = cmd_id
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
--
2.9.4
[Qemu-devel] [PATCH v5 06/10] qmp.py: Couple of pylint/style fixes
No actual code changes, just initializing attributes earlier to avoid
AttributeError on early introspection, a few pylint/style fixes and
docstring clarifications.
Signed-off-by: Lukáš Doktor
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qmp/qmp.py | 37 -
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651..782d1ac 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -13,19 +13,30 @@ import errno
import socket
import sys
+
class QMPError(Exception):
pass
+
class QMPConnectError(QMPError):
pass
+
class QMPCapabilitiesError(QMPError):
pass
+
class QMPTimeoutError(QMPError):
pass
+
class QEMUMonitorProtocol:
+
+#: Socket's error class
+error = socket.error
+#: Socket's timeout
+timeout = socket.timeout
+
def __init__(self, address, server=False, debug=False):
"""
Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
self.__address = address
self._debug = debug
self.__sock = self.__get_sock()
+self.__sockfile = None
if server:
self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
self.__sock.bind(self.__address)
@@ -56,7 +68,7 @@ class QEMUMonitorProtocol:
def __negotiate_capabilities(self):
greeting = self.__json_read()
-if greeting is None or not greeting.has_key('QMP'):
+if greeting is None or "QMP" not in greeting:
raise QMPConnectError
# Greeting seems ok, negotiate capabilities
resp = self.cmd('qmp_capabilities')
@@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
continue
return resp
-error = socket.error
-
def __get_events(self, wait=False):
"""
Check for new events in the stream and cache them in __events.
@@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
"""
# Check for new events regardless and pull them into the cache:
@@ -175,7 +185,7 @@ class QEMUMonitorProtocol:
@param args: command arguments (dict)
@param id: command id (dict, list, string or int)
"""
-qmp_cmd = { 'execute': name }
+qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
if id:
@@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
+"""
+Build and send a QMP command to the monitor, report errors if any
+"""
ret = self.cmd(cmd, kwds)
if ret.has_key('error'):
raise Exception(ret['error']['desc'])
@@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
def pull_event(self, wait=False):
"""
-Get and delete the first available QMP event.
+Pulls a single event.
@param wait (bool): block until an event is available.
@param wait (float): If wait is a float, treat it as a timeout value.
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The first available QMP event, or None.
"""
@@ -217,8 +230,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The list of available QMP events.
"""
@@ -235,8 +248,6 @@ class QEMUMonitorProtocol:
self.__sock.close()
self.__sockfile.close()
-timeout = socket.timeout
-
def settimeout(self, timeout):
self.__sock.settimeout(timeout)
--
2.9.4
[Qemu-devel] [PATCH v5 10/10] qtest.py: Few pylint/style fixes
No actual code changes, just few pylint/style fixes. Signed-off-by: Lukáš Doktor Reviewed-by: John Snow --- scripts/qtest.py | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/scripts/qtest.py b/scripts/qtest.py index ab183c0..df0daf2 100644 --- a/scripts/qtest.py +++ b/scripts/qtest.py @@ -11,14 +11,11 @@ # Based on qmp.py. # -import errno import socket -import string import os -import subprocess -import qmp.qmp import qemu + class QEMUQtestProtocol(object): def __init__(self, address, server=False): """ @@ -83,8 +80,10 @@ class QEMUQtestMachine(qemu.QEMUMachine): socket_scm_helper=None): if name is None: name = "qemu-%d" % os.getpid() -super(QEMUQtestMachine, self).__init__(binary, args, name=name, test_dir=test_dir, - socket_scm_helper=socket_scm_helper) +super(QEMUQtestMachine, + self).__init__(binary, args, name=name, test_dir=test_dir, + socket_scm_helper=socket_scm_helper) +self._qtest = None self._qtest_path = os.path.join(test_dir, name + "-qtest.sock") def _base_args(self): -- 2.9.4
[Qemu-devel] [PATCH v5 08/10] qmp.py: Avoid "has_key" usage
The "has_key" is deprecated in favor of "__in__" operator.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qmp/qmp.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 95ff5cc..f2f5a9b 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -197,7 +197,7 @@ class QEMUMonitorProtocol(object):
Build and send a QMP command to the monitor, report errors if any
"""
ret = self.cmd(cmd, kwds)
-if ret.has_key('error'):
+if "error" in ret:
raise Exception(ret['error']['desc'])
return ret['return']
--
2.9.4
[Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes
Hello guys, I'm reading the available python modules to exercise qemu and while reading them I fixed some issues that caught my attention. It usually starts with a simple pylint/docstring fixes and slowly graduates to more controversial ones so I'm open to suggestion to remove some of them. Kind regards, Lukáš Changes in v2 - Squashed 2nd and 10th patches into 2nd one - Use repr() in MonitorResponseError's description - Improved commit message of the 6th patch - Two tweaks to docstrings changed in the 6th patch - Also updated qmp-shell to use new-style super calls (7th patch) - Fixed the accidental rename of qmp `cmd_id` (kept the id->cmd_id patch) - Changed the style of the style-fix in the 10th commit Changes in v3 - Don't use repr in the 5th patch in MonitorResponseError Changes in v4 - Use correct git base (remove unwanted commits) Changes in v5 - Avoid bool comparison - Change report to return in one docstring - Removed the unnecessary spaces around single-line docstring Lukáš Doktor (10): qemu.py: Pylint/style fixes qemu|qtest: Avoid dangerous arguments qemu.py: Use iteritems rather than keys() qemu.py: Simplify QMP key-conversion qemu.py: Use custom exceptions rather than Exception qmp.py: Couple of pylint/style fixes qmp.py: Use object-based class for QEMUMonitorProtocol qmp.py: Avoid "has_key" usage qmp.py: Avoid overriding a builtin object qtest.py: Few pylint/style fixes scripts/qemu.py | 98 +-- scripts/qmp/qmp-shell | 4 +-- scripts/qmp/qmp.py| 49 -- scripts/qtest.py | 13 --- 4 files changed, 109 insertions(+), 55 deletions(-) -- 2.9.4
[Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
No actual code changes, just several pylint/style fixes and docstring
clarifications.
Signed-off-by: Lukáš Doktor
---
scripts/qemu.py | 76 -
1 file changed, 53 insertions(+), 23 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8..466aaab 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,8 +23,22 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
test_dir="/var/tmp",
- monitor_address=None, socket_scm_helper=None, debug=False):
+def __init__(self, binary, args=[], wrapper=[], name=None,
+ test_dir="/var/tmp", monitor_address=None,
+ socket_scm_helper=None, debug=False):
+'''
+Create a QEMUMachine object
+
+@param binary: path to the qemu binary (str)
+@param args: initial list of extra arguments
+@param wrapper: list of arguments used as prefix to qemu binary
+@param name: name of this object (used for log/monitor/... file names)
+@param test_dir: base location to put log/monitor/... files in
+@param monitor_address: custom address for QMP monitor
+@param socket_scm_helper: path to scm_helper binary (to forward fds)
+@param debug: enable debug mode (forwarded to QMP helper and such)
+@note: Qemu process is not started until launch() is used.
+'''
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
@@ -33,12 +47,13 @@ class QEMUMachine(object):
self._qemu_log_path = os.path.join(test_dir, name + ".log")
self._popen = None
self._binary = binary
-self._args = list(args) # Force copy args in case we modify them
+self._args = list(args) # Force copy args in case we modify them
self._wrapper = wrapper
self._events = []
self._iolog = None
self._socket_scm_helper = socket_scm_helper
self._debug = debug
+self._qmp = None
# This can be used to add an unused monitor instance.
def add_monitor_telnet(self, ip, port):
@@ -64,16 +79,16 @@ class QEMUMachine(object):
if self._socket_scm_helper is None:
print >>sys.stderr, "No path to socket_scm_helper set"
return -1
-if os.path.exists(self._socket_scm_helper) == False:
+if not os.path.exists(self._socket_scm_helper):
print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
return -1
fd_param = ["%s" % self._socket_scm_helper,
"%d" % self._qmp.get_sock_fd(),
"%s" % fd_file_path]
devnull = open('/dev/null', 'rb')
-p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
- stderr=sys.stderr)
-return p.wait()
+proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
+stderr=sys.stderr)
+return proc.wait()
@staticmethod
def _remove_if_exists(path):
@@ -99,8 +114,8 @@ class QEMUMachine(object):
return self._popen.pid
def _load_io_log(self):
-with open(self._qemu_log_path, "r") as fh:
-self._iolog = fh.read()
+with open(self._qemu_log_path, "r") as iolog:
+self._iolog = iolog.read()
def _base_args(self):
if isinstance(self._monitor_address, tuple):
@@ -114,8 +129,8 @@ class QEMUMachine(object):
'-display', 'none', '-vga', 'none']
def _pre_launch(self):
-self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
server=True,
-debug=self._debug)
+self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
+server=True, debug=self._debug)
def _post_launch(self):
self._qmp.accept()
@@ -131,9 +146,11 @@ class QEMUMachine(object):
qemulog = open(self._qemu_log_path, 'wb')
try:
self._pre_launch()
-args = self._wrapper + [self._binary] + self._base_args() +
self._args
+args = (self._wrapper + [self._binary] + self._base_args() +
+self._args)
self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
- stderr=subprocess.STDOUT,
shell=False)
+ stderr=subprocess.STDOUT,
+ shell=False)
self._post_launch()
except:
if
[Qemu-devel] [PATCH v5 02/10] qemu|qtest: Avoid dangerous arguments
The list object is mutable in python and potentially might modify other
object's arguments when used as default argument. Reproducer:
>>> vm1 = QEMUMachine("qemu")
>>> vm2 = QEMUMachine("qemu")
>>> vm1._wrapper.append("foo")
>>> print vm2._wrapper
['foo']
In this case the `args` is actually copied so it would be safe to keep
it, but it's not a good practice to keep it. The same issue applies in
inherited qtest module.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
Reviewed-by: John Snow
---
scripts/qemu.py | 6 +-
scripts/qtest.py | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 466aaab..888f5b3 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,7 +23,7 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
+def __init__(self, binary, args=None, wrapper=None, name=None,
test_dir="/var/tmp", monitor_address=None,
socket_scm_helper=None, debug=False):
'''
@@ -39,6 +39,10 @@ class QEMUMachine(object):
@param debug: enable debug mode (forwarded to QMP helper and such)
@note: Qemu process is not started until launch() is used.
'''
+if args is None:
+args = []
+if wrapper is None:
+wrapper = []
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
diff --git a/scripts/qtest.py b/scripts/qtest.py
index d5aecb5..ab183c0 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -79,7 +79,7 @@ class QEMUQtestProtocol(object):
class QEMUQtestMachine(qemu.QEMUMachine):
'''A QEMU VM'''
-def __init__(self, binary, args=[], name=None, test_dir="/var/tmp",
+def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
socket_scm_helper=None):
if name is None:
name = "qemu-%d" % os.getpid()
--
2.9.4
[Qemu-devel] [PATCH v5 03/10] qemu.py: Use iteritems rather than keys()
Let's avoid creating an in-memory list of keys and query for each value and use `iteritems` which is an iterator of key-value pairs. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Reviewed-by: Philippe Mathieu-Daudé --- scripts/qemu.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 888f5b3..571f852 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -185,11 +185,11 @@ class QEMUMachine(object): def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the result dict''' qmp_args = dict() -for key in args.keys(): +for key, value in args.iteritems(): if conv_keys: -qmp_args[key.translate(self.underscore_to_dash)] = args[key] +qmp_args[key.translate(self.underscore_to_dash)] = value else: -qmp_args[key] = args[key] +qmp_args[key] = value return self._qmp.cmd(cmd, args=qmp_args) -- 2.9.4
Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a): > Lukáš Doktor writes: > >> No actual code changes, just several pylint/style fixes and docstring >> clarifications. >> >> Signed-off-by: Lukáš Doktor >> --- >> scripts/qemu.py | 76 >> - >> 1 file changed, 53 insertions(+), 23 deletions(-) >> >> diff --git a/scripts/qemu.py b/scripts/qemu.py >> index 880e3e8..466aaab 100644 >> --- a/scripts/qemu.py >> +++ b/scripts/qemu.py >> @@ -23,8 +23,22 @@ import qmp.qmp >> class QEMUMachine(object): >> '''A QEMU VM''' >> >> -def __init__(self, binary, args=[], wrapper=[], name=None, >> test_dir="/var/tmp", >> - monitor_address=None, socket_scm_helper=None, debug=False): >> +def __init__(self, binary, args=[], wrapper=[], name=None, >> + test_dir="/var/tmp", monitor_address=None, >> + socket_scm_helper=None, debug=False): >> +''' >> +Create a QEMUMachine object > > Initialize a QEMUMachine > > Rationale: it's __init__, not __create__, and "object" is redundant. > sure >> + >> +@param binary: path to the qemu binary (str) > > Drop (str), because what else could it be? it could be shlex.split of arguments to be passed to process. Anyway no strong opinion here so I dropping it... > >> +@param args: initial list of extra arguments > > If this is the initial list, then what's the final list? > It's the basic set of arguments which can be modified before the execution. Do you think it requires additional explanation, or would you like to improve it somehow? >> +@param wrapper: list of arguments used as prefix to qemu binary >> +@param name: name of this object (used for log/monitor/... file >> names) > prefix for socket and log file names (default: qemu-PID) > Sure, both make sense to me. >> +@param test_dir: base location to put log/monitor/... files in > > where to create socket and log file > > Aside: test_dir is a lousy name. Agree but changing names is tricky as people might be using kwargs to set it. Anyway using your description here, keeping the possible rename for a separate patchset (if needed). > >> +@param monitor_address: custom address for QMP monitor > > Yes, but what *is* a custom address? Its user _base_args() appears to > expect either a pair of strings (host, port) or a string filename. > If you insist I can add something like "a tuple(host, port) or string to specify path", but I find it unnecessary detailed... >> +@param socket_scm_helper: path to scm_helper binary (to forward fds) > > What is an scm_helper, and why would I want to use it? > To forward a file descriptor. It's for example used in tests/qemu-iotests/045 or tests/qemu-iotests/147 >> +@param debug: enable debug mode (forwarded to QMP helper and such) > > What is a QMP helper? To what else is debug forwarded? > Debug is set in `self._debug` and can be consumed by anyone who has access to this variable. Currently that is the QMP, but people can inherit and use that variable to adjust their behavior. >> +@note: Qemu process is not started until launch() is used. > > until launch(). > OK >> +''' > > It's an improvement. > >> if name is None: >> name = "qemu-%d" % os.getpid() >> if monitor_address is None: >> @@ -33,12 +47,13 @@ class QEMUMachine(object): >> self._qemu_log_path = os.path.join(test_dir, name + ".log") >> self._popen = None >> self._binary = binary >> -self._args = list(args) # Force copy args in case we modify them >> +self._args = list(args) # Force copy args in case we modify them >> self._wrapper = wrapper >> self._events = [] >> self._iolog = None >> self._socket_scm_helper = socket_scm_helper >> self._debug = debug >> +self._qmp = None >> >> # This can be used to add an unused monitor instance. >> def add_monitor_telnet(self, ip, port): >> @@ -64,16 +79,16 @@ class QEMUMachine(object): >> if self._socket_scm_helper is None: >> print >>sys.stderr, "No path to socket_scm_helper set" >> return -1 >> -if os.path.exists(self._socket_scm
Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
Dne 16.8.2017 v 18:58 Markus Armbruster napsal(a): > Lukáš Doktor writes: > >> Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a): >>> Lukáš Doktor writes: >>> >>>> No actual code changes, just several pylint/style fixes and docstring >>>> clarifications. >>>> >>>> Signed-off-by: Lukáš Doktor >>>> --- >>>> scripts/qemu.py | 76 >>>> - >>>> 1 file changed, 53 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/scripts/qemu.py b/scripts/qemu.py >>>> index 880e3e8..466aaab 100644 >>>> --- a/scripts/qemu.py >>>> +++ b/scripts/qemu.py >>>> @@ -23,8 +23,22 @@ import qmp.qmp >>>> class QEMUMachine(object): >>>> '''A QEMU VM''' >>>> >>>> -def __init__(self, binary, args=[], wrapper=[], name=None, >>>> test_dir="/var/tmp", >>>> - monitor_address=None, socket_scm_helper=None, >>>> debug=False): >>>> +def __init__(self, binary, args=[], wrapper=[], name=None, >>>> + test_dir="/var/tmp", monitor_address=None, >>>> + socket_scm_helper=None, debug=False): >>>> +''' >>>> +Create a QEMUMachine object >>> >>> Initialize a QEMUMachine >>> >>> Rationale: it's __init__, not __create__, and "object" is redundant. >>> >> >> sure >> >>>> + >>>> +@param binary: path to the qemu binary (str) >>> >>> Drop (str), because what else could it be? >> >> it could be shlex.split of arguments to be passed to process. Anyway no >> strong opinion here so I dropping it... >> >>> >>>> +@param args: initial list of extra arguments >>> >>> If this is the initial list, then what's the final list? >>> >> >> It's the basic set of arguments which can be modified before the execution. >> Do you think it requires additional explanation, or would you like to >> improve it somehow? > > Can this list of extra arguments really be *modified*? Adding more > arguments doesn't count for me --- I'd consider them added to the > "non-extra" arguments. > Yes, one can remove, shuffle or modify it. > Drop "initial"? I can do that but it can give false impression that the args will be present. Anyway it's probably just a corner case so I'll drop it. > >>>> +@param wrapper: list of arguments used as prefix to qemu binary >>>> +@param name: name of this object (used for log/monitor/... file >>>> names) >>> prefix for socket and log file names (default: qemu-PID) >>> >> >> Sure, both make sense to me. >> >>>> +@param test_dir: base location to put log/monitor/... files in >>> >>> where to create socket and log file >>> >>> Aside: test_dir is a lousy name. >> >> Agree but changing names is tricky as people might be using kwargs to set >> it. Anyway using your description here, keeping the possible rename for a >> separate patchset (if needed). > > I'm merely observing the lousiness of this name. I'm not asking you to > do anything about it :) > >>>> +@param monitor_address: custom address for QMP monitor >>> >>> Yes, but what *is* a custom address? Its user _base_args() appears to >>> expect either a pair of strings (host, port) or a string filename. >>> >> >> If you insist I can add something like "a tuple(host, port) or string to >> specify path", but I find it unnecessary detailed... > > I'm not the maintainer, I'm definitely not insisting on anything. > > If you're aiming for brevity, then drop "custom". > OK, removing in v6 >>>> +@param socket_scm_helper: path to scm_helper binary (to forward >>>> fds) >>> >>> What is an scm_helper, and why would I want to use it? >>> >> >> To forward a file descriptor. It's for example used in >> tests/qemu-iotests/045 or tests/qemu-iotests/147 > > What about "socket_scm_helper: helper program, required for send_fd_scm()"? > >>>> +@param debug: enable debug mode (forwarded to QMP helper and such) >>> >>> What is a QMP
Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
Dne 17.8.2017 v 07:24 Markus Armbruster napsal(a): > Lukáš Doktor writes: > >> Dne 16.8.2017 v 18:58 Markus Armbruster napsal(a): >>> Lukáš Doktor writes: >>> >>>> Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a): >>>>> Lukáš Doktor writes: >>>>> >>>>>> No actual code changes, just several pylint/style fixes and docstring >>>>>> clarifications. >>>>>> >>>>>> Signed-off-by: Lukáš Doktor >>>>>> --- >>>>>> scripts/qemu.py | 76 >>>>>> - >>>>>> 1 file changed, 53 insertions(+), 23 deletions(-) >>>>>> >>>>>> diff --git a/scripts/qemu.py b/scripts/qemu.py >>>>>> index 880e3e8..466aaab 100644 >>>>>> --- a/scripts/qemu.py >>>>>> +++ b/scripts/qemu.py >>>>>> @@ -23,8 +23,22 @@ import qmp.qmp >>>>>> class QEMUMachine(object): >>>>>> '''A QEMU VM''' >>>>>> >>>>>> -def __init__(self, binary, args=[], wrapper=[], name=None, >>>>>> test_dir="/var/tmp", >>>>>> - monitor_address=None, socket_scm_helper=None, >>>>>> debug=False): >>>>>> +def __init__(self, binary, args=[], wrapper=[], name=None, >>>>>> + test_dir="/var/tmp", monitor_address=None, >>>>>> + socket_scm_helper=None, debug=False): >>>>>> +''' >>>>>> +Create a QEMUMachine object >>>>> >>>>> Initialize a QEMUMachine >>>>> >>>>> Rationale: it's __init__, not __create__, and "object" is redundant. >>>>> >>>> >>>> sure >>>> >>>>>> + >>>>>> +@param binary: path to the qemu binary (str) >>>>> >>>>> Drop (str), because what else could it be? >>>> >>>> it could be shlex.split of arguments to be passed to process. Anyway no >>>> strong opinion here so I dropping it... >>>> >>>>> >>>>>> +@param args: initial list of extra arguments >>>>> >>>>> If this is the initial list, then what's the final list? >>>>> >>>> >>>> It's the basic set of arguments which can be modified before the >>>> execution. Do you think it requires additional explanation, or would you >>>> like to improve it somehow? >>> >>> Can this list of extra arguments really be *modified*? Adding more >>> arguments doesn't count for me --- I'd consider them added to the >>> "non-extra" arguments. >>> >> >> Yes, one can remove, shuffle or modify it. > > Bizarre :) > > Who's "one"? > The user, or some inherited classes (eg. iotest.py). Currently nobody does that, but one might... >>> Drop "initial"? >> >> I can do that but it can give false impression that the args will be >> present. Anyway it's probably just a corner case so I'll drop it. > > If it's intended to be modified, then keeping "initial" might be best. > Your choice. > OK, let's not over-complicate it and just say it's list of extra arguments. >>> >>>>>> +@param wrapper: list of arguments used as prefix to qemu binary >>>>>> +@param name: name of this object (used for log/monitor/... file >>>>>> names) >>>>> prefix for socket and log file names (default: qemu-PID) >>>>> >>>> >>>> Sure, both make sense to me. >>>> >>>>>> +@param test_dir: base location to put log/monitor/... files in >>>>> >>>>> where to create socket and log file >>>>> >>>>> Aside: test_dir is a lousy name. >>>> >>>> Agree but changing names is tricky as people might be using kwargs to set >>>> it. Anyway using your description here, keeping the possible rename for a >>>> separate patchset (if needed). >>> >>> I'm merely observing the lousiness of this name. I'm not asking you to >>> do anything about it :) >>> >>>>>>
[Qemu-devel] [PATCH v6 00/10] qemu.py: Pylint/style fixes
Hello guys, I'm reading the available python modules to exercise qemu and while reading them I fixed some issues that caught my attention. It usually starts with a simple pylint/docstring fixes and slowly graduates to more controversial ones so I'm open to suggestion to remove some of them. Kind regards, Lukáš Changes in v2 - Squashed 2nd and 10th patches into 2nd one - Use repr() in MonitorResponseError's description - Improved commit message of the 6th patch - Two tweaks to docstrings changed in the 6th patch - Also updated qmp-shell to use new-style super calls (7th patch) - Fixed the accidental rename of qmp `cmd_id` (kept the id->cmd_id patch) - Changed the style of the style-fix in the 10th commit Changes in v3 - Don't use repr in the 5th patch in MonitorResponseError Changes in v4 - Use correct git base (remove unwanted commits) Changes in v5 - Avoid bool comparison - Change report to return in one docstring - Removed the unnecessary spaces around single-line docstring Changes in v6 - Bunch of docstring tweaks by Markus Armbruster - Line break in <80 chars - result dict => response dict - Removed the "event_match" rename Lukáš Doktor (10): qemu.py: Pylint/style fixes qemu|qtest: Avoid dangerous arguments qemu.py: Use iteritems rather than keys() qemu.py: Simplify QMP key-conversion qemu.py: Use custom exceptions rather than Exception qmp.py: Couple of pylint/style fixes qmp.py: Use object-based class for QEMUMonitorProtocol qmp.py: Avoid "has_key" usage qmp.py: Avoid overriding a builtin object qtest.py: Few pylint/style fixes scripts/qemu.py | 95 +++ scripts/qmp/qmp-shell | 4 +-- scripts/qmp/qmp.py| 49 +++--- scripts/qtest.py | 13 --- 4 files changed, 111 insertions(+), 50 deletions(-) -- 2.9.4
[Qemu-devel] [PATCH v6 02/10] qemu|qtest: Avoid dangerous arguments
The list object is mutable in python and potentially might modify other
object's arguments when used as default argument. Reproducer:
>>> vm1 = QEMUMachine("qemu")
>>> vm2 = QEMUMachine("qemu")
>>> vm1._wrapper.append("foo")
>>> print vm2._wrapper
['foo']
In this case the `args` is actually copied so it would be safe to keep
it, but it's not a good practice to keep it. The same issue applies in
inherited qtest module.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
Reviewed-by: John Snow
---
scripts/qemu.py | 6 +-
scripts/qtest.py | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index dd679f1..5d09de4 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,7 +23,7 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
+def __init__(self, binary, args=None, wrapper=None, name=None,
test_dir="/var/tmp", monitor_address=None,
socket_scm_helper=None, debug=False):
'''
@@ -39,6 +39,10 @@ class QEMUMachine(object):
@param debug: enable debug mode
@note: Qemu process is not started until launch() is used.
'''
+if args is None:
+args = []
+if wrapper is None:
+wrapper = []
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
diff --git a/scripts/qtest.py b/scripts/qtest.py
index d5aecb5..ab183c0 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -79,7 +79,7 @@ class QEMUQtestProtocol(object):
class QEMUQtestMachine(qemu.QEMUMachine):
'''A QEMU VM'''
-def __init__(self, binary, args=[], name=None, test_dir="/var/tmp",
+def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
socket_scm_helper=None):
if name is None:
name = "qemu-%d" % os.getpid()
--
2.9.4
[Qemu-devel] [PATCH v6 01/10] qemu.py: Pylint/style fixes
No actual code changes, just several pylint/style fixes and docstring
clarifications.
Signed-off-by: Lukáš Doktor
Reviewed-by: Stefan Hajnoczi
---
scripts/qemu.py | 73 +++--
1 file changed, 55 insertions(+), 18 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8..dd679f1 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,8 +23,22 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
test_dir="/var/tmp",
- monitor_address=None, socket_scm_helper=None, debug=False):
+def __init__(self, binary, args=[], wrapper=[], name=None,
+ test_dir="/var/tmp", monitor_address=None,
+ socket_scm_helper=None, debug=False):
+'''
+Initialize a QEMUMachine
+
+@param binary: path to the qemu binary
+@param args: list of extra arguments
+@param wrapper: list of arguments used as prefix to qemu binary
+@param name: prefix for socket and log file names (default: qemu-PID)
+@param test_dir: where to create socket and log file
+@param monitor_address: address for QMP monitor
+@param socket_scm_helper: helper program, required for send_fd_scm()"
+@param debug: enable debug mode
+@note: Qemu process is not started until launch() is used.
+'''
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
@@ -33,12 +47,13 @@ class QEMUMachine(object):
self._qemu_log_path = os.path.join(test_dir, name + ".log")
self._popen = None
self._binary = binary
-self._args = list(args) # Force copy args in case we modify them
+self._args = list(args) # Force copy args in case we modify them
self._wrapper = wrapper
self._events = []
self._iolog = None
self._socket_scm_helper = socket_scm_helper
self._debug = debug
+self._qmp = None
# This can be used to add an unused monitor instance.
def add_monitor_telnet(self, ip, port):
@@ -64,16 +79,16 @@ class QEMUMachine(object):
if self._socket_scm_helper is None:
print >>sys.stderr, "No path to socket_scm_helper set"
return -1
-if os.path.exists(self._socket_scm_helper) == False:
+if not os.path.exists(self._socket_scm_helper):
print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
return -1
fd_param = ["%s" % self._socket_scm_helper,
"%d" % self._qmp.get_sock_fd(),
"%s" % fd_file_path]
devnull = open('/dev/null', 'rb')
-p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
- stderr=sys.stderr)
-return p.wait()
+proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
+stderr=sys.stderr)
+return proc.wait()
@staticmethod
def _remove_if_exists(path):
@@ -99,8 +114,8 @@ class QEMUMachine(object):
return self._popen.pid
def _load_io_log(self):
-with open(self._qemu_log_path, "r") as fh:
-self._iolog = fh.read()
+with open(self._qemu_log_path, "r") as iolog:
+self._iolog = iolog.read()
def _base_args(self):
if isinstance(self._monitor_address, tuple):
@@ -114,7 +129,8 @@ class QEMUMachine(object):
'-display', 'none', '-vga', 'none']
def _pre_launch(self):
-self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
server=True,
+self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
+server=True,
debug=self._debug)
def _post_launch(self):
@@ -131,9 +147,11 @@ class QEMUMachine(object):
qemulog = open(self._qemu_log_path, 'wb')
try:
self._pre_launch()
-args = self._wrapper + [self._binary] + self._base_args() +
self._args
+args = (self._wrapper + [self._binary] + self._base_args() +
+self._args)
self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
- stderr=subprocess.STDOUT,
shell=False)
+ stderr=subprocess.STDOUT,
+ shell=False)
self._post_launch()
except:
if self.is_running():
@@ -154,23 +172,30 @@ class QEMUMac
[Qemu-devel] [PATCH v6 06/10] qmp.py: Couple of pylint/style fixes
No actual code changes, just initializing attributes earlier to avoid
AttributeError on early introspection, a few pylint/style fixes and
docstring clarifications.
Signed-off-by: Lukáš Doktor
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qmp/qmp.py | 37 -
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651..782d1ac 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -13,19 +13,30 @@ import errno
import socket
import sys
+
class QMPError(Exception):
pass
+
class QMPConnectError(QMPError):
pass
+
class QMPCapabilitiesError(QMPError):
pass
+
class QMPTimeoutError(QMPError):
pass
+
class QEMUMonitorProtocol:
+
+#: Socket's error class
+error = socket.error
+#: Socket's timeout
+timeout = socket.timeout
+
def __init__(self, address, server=False, debug=False):
"""
Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
self.__address = address
self._debug = debug
self.__sock = self.__get_sock()
+self.__sockfile = None
if server:
self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
self.__sock.bind(self.__address)
@@ -56,7 +68,7 @@ class QEMUMonitorProtocol:
def __negotiate_capabilities(self):
greeting = self.__json_read()
-if greeting is None or not greeting.has_key('QMP'):
+if greeting is None or "QMP" not in greeting:
raise QMPConnectError
# Greeting seems ok, negotiate capabilities
resp = self.cmd('qmp_capabilities')
@@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
continue
return resp
-error = socket.error
-
def __get_events(self, wait=False):
"""
Check for new events in the stream and cache them in __events.
@@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
"""
# Check for new events regardless and pull them into the cache:
@@ -175,7 +185,7 @@ class QEMUMonitorProtocol:
@param args: command arguments (dict)
@param id: command id (dict, list, string or int)
"""
-qmp_cmd = { 'execute': name }
+qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
if id:
@@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
+"""
+Build and send a QMP command to the monitor, report errors if any
+"""
ret = self.cmd(cmd, kwds)
if ret.has_key('error'):
raise Exception(ret['error']['desc'])
@@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
def pull_event(self, wait=False):
"""
-Get and delete the first available QMP event.
+Pulls a single event.
@param wait (bool): block until an event is available.
@param wait (float): If wait is a float, treat it as a timeout value.
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The first available QMP event, or None.
"""
@@ -217,8 +230,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The list of available QMP events.
"""
@@ -235,8 +248,6 @@ class QEMUMonitorProtocol:
self.__sock.close()
self.__sockfile.close()
-timeout = socket.timeout
-
def settimeout(self, timeout):
self.__sock.settimeout(timeout)
--
2.9.4
[Qemu-devel] [PATCH v6 03/10] qemu.py: Use iteritems rather than keys()
Let's avoid creating an in-memory list of keys and query for each value and use `iteritems` which is an iterator of key-value pairs. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Reviewed-by: Philippe Mathieu-Daudé --- scripts/qemu.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 5d09de4..db21407 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -186,11 +186,11 @@ class QEMUMachine(object): def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the response dict''' qmp_args = dict() -for key in args.keys(): +for key, value in args.iteritems(): if conv_keys: -qmp_args[key.translate(self.underscore_to_dash)] = args[key] +qmp_args[key.translate(self.underscore_to_dash)] = value else: -qmp_args[key] = args[key] +qmp_args[key] = value return self._qmp.cmd(cmd, args=qmp_args) -- 2.9.4
[Qemu-devel] [PATCH v6 04/10] qemu.py: Simplify QMP key-conversion
The QMP key conversion consist of '_'s to be replaced with '-'s, which
can easily be done by a single `str.replace` method which is faster and
does not require `string` module import.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qemu.py | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index db21407..19cbd34 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,7 +13,6 @@
#
import errno
-import string
import os
import sys
import subprocess
@@ -181,14 +180,12 @@ class QEMUMachine(object):
self._load_io_log()
self._post_shutdown()
-underscore_to_dash = string.maketrans('_', '-')
-
def qmp(self, cmd, conv_keys=True, **args):
'''Invoke a QMP command and return the response dict'''
qmp_args = dict()
for key, value in args.iteritems():
if conv_keys:
-qmp_args[key.translate(self.underscore_to_dash)] = value
+qmp_args[key.replace('_', '-')] = value
else:
qmp_args[key] = value
--
2.9.4
[Qemu-devel] [PATCH v6 07/10] qmp.py: Use object-based class for QEMUMonitorProtocol
There is no need to define QEMUMonitorProtocol as old-style class. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost --- scripts/qmp/qmp-shell | 4 ++-- scripts/qmp/qmp.py| 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell index 860ffb2..be449de 100755 --- a/scripts/qmp/qmp-shell +++ b/scripts/qmp/qmp-shell @@ -106,7 +106,7 @@ class FuzzyJSON(ast.NodeTransformer): # _execute_cmd()). Let's design a better one. class QMPShell(qmp.QEMUMonitorProtocol): def __init__(self, address, pretty=False): -qmp.QEMUMonitorProtocol.__init__(self, self.__get_address(address)) +super(QMPShell, self).__init__(self.__get_address(address)) self._greeting = None self._completer = None self._pretty = pretty @@ -281,7 +281,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): return True def connect(self, negotiate): -self._greeting = qmp.QEMUMonitorProtocol.connect(self, negotiate) +self._greeting = super(QMPShell, self).connect(negotiate) self.__completer_setup() def show_banner(self, msg='Welcome to the QMP low-level shell!'): diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index 782d1ac..95ff5cc 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError): pass -class QEMUMonitorProtocol: +class QEMUMonitorProtocol(object): #: Socket's error class error = socket.error -- 2.9.4
[Qemu-devel] [PATCH v6 05/10] qemu.py: Use custom exceptions rather than Exception
The naked Exception should not be widely used. It makes sense to be a
bit more specific and use better-suited custom exceptions. As a benefit
we can store the full reply in the exception in case someone needs it
when catching the exception.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qemu.py | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 19cbd34..97ecb63 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -19,6 +19,19 @@ import subprocess
import qmp.qmp
+class MonitorResponseError(qmp.qmp.QMPError):
+'''
+Represents erroneous QMP monitor reply
+'''
+def __init__(self, reply):
+try:
+desc = reply["error"]["desc"]
+except KeyError:
+desc = reply
+super(MonitorResponseError, self).__init__(desc)
+self.reply = reply
+
+
class QEMUMachine(object):
'''A QEMU VM'''
@@ -199,9 +212,9 @@ class QEMUMachine(object):
'''
reply = self.qmp(cmd, conv_keys, **args)
if reply is None:
-raise Exception("Monitor is closed")
+raise qmp.qmp.QMPError("Monitor is closed")
if "error" in reply:
-raise Exception(reply["error"]["desc"])
+raise MonitorResponseError(reply)
return reply["return"]
def get_qmp_event(self, wait=False):
--
2.9.4
[Qemu-devel] [PATCH v6 08/10] qmp.py: Avoid "has_key" usage
The "has_key" is deprecated in favor of "__in__" operator.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qmp/qmp.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 95ff5cc..f2f5a9b 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -197,7 +197,7 @@ class QEMUMonitorProtocol(object):
Build and send a QMP command to the monitor, report errors if any
"""
ret = self.cmd(cmd, kwds)
-if ret.has_key('error'):
+if "error" in ret:
raise Exception(ret['error']['desc'])
return ret['return']
--
2.9.4
[Qemu-devel] [PATCH v6 09/10] qmp.py: Avoid overriding a builtin object
The "id" is a builtin method to get object's identity and should not be
overridden. This might bring some issues in case someone was directly
calling "cmd(..., id=id)" but I haven't found such usage on brief search
for "cmd\(.*id=".
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qmp/qmp.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index f2f5a9b..ef12e8a 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
print >>sys.stderr, "QMP:<<< %s" % resp
return resp
-def cmd(self, name, args=None, id=None):
+def cmd(self, name, args=None, cmd_id=None):
"""
Build a QMP command and send it to the QMP Monitor.
@param name: command name (string)
@param args: command arguments (dict)
-@param id: command id (dict, list, string or int)
+@param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
-if id:
-qmp_cmd['id'] = id
+if cmd_id:
+qmp_cmd['id'] = cmd_id
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
--
2.9.4
[Qemu-devel] [PATCH v6 10/10] qtest.py: Few pylint/style fixes
No actual code changes, just few pylint/style fixes. Signed-off-by: Lukáš Doktor Reviewed-by: John Snow --- scripts/qtest.py | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/scripts/qtest.py b/scripts/qtest.py index ab183c0..df0daf2 100644 --- a/scripts/qtest.py +++ b/scripts/qtest.py @@ -11,14 +11,11 @@ # Based on qmp.py. # -import errno import socket -import string import os -import subprocess -import qmp.qmp import qemu + class QEMUQtestProtocol(object): def __init__(self, address, server=False): """ @@ -83,8 +80,10 @@ class QEMUQtestMachine(qemu.QEMUMachine): socket_scm_helper=None): if name is None: name = "qemu-%d" % os.getpid() -super(QEMUQtestMachine, self).__init__(binary, args, name=name, test_dir=test_dir, - socket_scm_helper=socket_scm_helper) +super(QEMUQtestMachine, + self).__init__(binary, args, name=name, test_dir=test_dir, + socket_scm_helper=socket_scm_helper) +self._qtest = None self._qtest_path = os.path.join(test_dir, name + "-qtest.sock") def _base_args(self): -- 2.9.4
Re: [Qemu-devel] make check speed
Dne 23.8.2017 v 10:35 Thomas Huth napsal(a): > On 23.08.2017 10:01, Paolo Bonzini wrote: >> On 23/08/2017 09:49, Thomas Huth wrote: >>> While we're at it: I'd like to have a "make check-fast", too. Sometimes >>> the normal "make check" is already too slow, e.g. while developing new >>> patches, I sometimes just want to do a very quick sanity test to see >>> whether I broke some basic things or not, and only do the "make check" >>> before I submit my patches. >>> So we would get three stages: >>> >>> - make check-fast => For very, very quick sanity tests only >>> >>> - make check => E.g. has to be run before submitting patches >>> >>> - make check-harder => might run a very long time, so best suited for >>>nightly regression tests etc.? >>> >>> Does that sound reasonable? And the crucial question: Who is going to >>> implement the basic framework for this? >> >> There's already make check-unit or make check-qtest-x86_64 depending on >> what you're working on. > > True. And I just learned that you can also already set the SPEED > variable to either "quick" or "slow" and that we're already using > g_test_quick() and g_test_slow() in a couple of places to check this. So > the framework for running quick vs. thorough tests is already there ... > we just might want to add this to some more tests, I guess... > > Question for the maintainers and the test automation folks: Is anybody > already running "make check SPEED=slow" or is this just rather an > unheard-of way of running the tests? > >> If you have a many-core machine, of course, there's no simpler solution >> than throwing more CPUs at it. :) > > Is it safe nowadays to run "make check -j4" for example? Last time I > tried (maybe 1 or 2 years ago), there were still issues since some tests > were using hard-coded temporary file names, so the parallel tests were > disturbing each other, for example... > Actually the `.travis.yml` defines `MAKEFLAGS="-j3"`, which results in `make check` being executed with 3 threads... I was actually looking at the increasing number of failed travis builds and it seems to be related to the fluctuating performance. Running `nice -n 20 make check -j 12` with `nice -n 5 stress -c 20` in background results in the same kind of failures: File '/tmp/qemu/include/qapi/qmp/qobject.h' Lines executed:0.00% of 9 ** ERROR:tests/prom-env-test.c:42:check_guest_memory: assertion failed (signature == MAGIC): (0x7c7f1b78 == 0xcafec0de) GTester: last random seed: R02S117a2b838f1fd17c65a134629577b996 make: *** [check-qtest-ppc] Chyba 1 /tmp/qemu/tests/Makefile.include:836: návod pro cíl „check-qtest-ppc“ selhal make: *** [check-qtest-sparc] Chyba 1 /tmp/qemu/tests/Makefile.include:836: návod pro cíl „check-qtest-sparc“ selhal ** ERROR:tests/rtc-test.c:173:check_time: assertion failed (ABS(t - s) <= wiggle): (3 <= 2) GTester: last random seed: R02S2ca82eb2e7f63ec62c8433b715d9fe12 Vhost user backend fails to broadcast fake RARP ** ERROR:tests/vhost-user-test.c:779:test_reconnect: child process (/i386/vhost-user/reconnect/subprocess [5264]) failed unexpectedly GTester: last random seed: R02S6e728ccb5384a4d856cacc4166be8052 Gcov report for hw/misc/tmp105.c: File 'hw/misc/tmp105.c' Lines executed:30.40% of 125 Gcov report for arm-softmmu/hw/block/virtio-blk.c: File '/tmp/qemu/hw/block/virtio-blk.c' and all ERROR lines: ERROR:tests/prom-env-test.c:42:check_guest_memory: assertion failed (signature == MAGIC): (0x == 0xcafec0de) ERROR:tests/rtc-test.c:173:check_time: assertion failed (ABS(t - s) <= wiggle): (3 <= 2) ERROR:tests/vhost-user-test.c:779:test_reconnect: child process (/i386/vhost-user/reconnect/subprocess [5264]) failed unexpectedly ERROR:tests/vhost-user-test.c:807:test_connect_fail: child process (/x86_64/vhost-user/connect-fail/subprocess [7751]) failed unexpectedly ERROR:tests/vhost-user-test.c:835:test_flags_mismatch: child process (/x86_64/vhost-user/flags-mismatch/subprocess [7892]) failed unexpectedly ERROR:tests/boot-sector.c:127:boot_sector_test: assertion failed (signature == SIGNATURE): (0x == 0xdead) on my machine. Using `make -j 1` or at least `make -j 2` could improve (but I haven't checked that) Lukáš > Thomas > signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] make check speed
Dne 23.8.2017 v 14:01 Thomas Huth napsal(a): > On 23.08.2017 13:51, Lukáš Doktor wrote: >> Dne 23.8.2017 v 10:35 Thomas Huth napsal(a): >>> On 23.08.2017 10:01, Paolo Bonzini wrote: >>>> On 23/08/2017 09:49, Thomas Huth wrote: >>>>> While we're at it: I'd like to have a "make check-fast", too. Sometimes >>>>> the normal "make check" is already too slow, e.g. while developing new >>>>> patches, I sometimes just want to do a very quick sanity test to see >>>>> whether I broke some basic things or not, and only do the "make check" >>>>> before I submit my patches. >>>>> So we would get three stages: >>>>> >>>>> - make check-fast => For very, very quick sanity tests only >>>>> >>>>> - make check => E.g. has to be run before submitting patches >>>>> >>>>> - make check-harder => might run a very long time, so best suited for >>>>>nightly regression tests etc.? >>>>> >>>>> Does that sound reasonable? And the crucial question: Who is going to >>>>> implement the basic framework for this? >>>> >>>> There's already make check-unit or make check-qtest-x86_64 depending on >>>> what you're working on. >>> >>> True. And I just learned that you can also already set the SPEED >>> variable to either "quick" or "slow" and that we're already using >>> g_test_quick() and g_test_slow() in a couple of places to check this. So >>> the framework for running quick vs. thorough tests is already there ... >>> we just might want to add this to some more tests, I guess... >>> >>> Question for the maintainers and the test automation folks: Is anybody >>> already running "make check SPEED=slow" or is this just rather an >>> unheard-of way of running the tests? >>> >>>> If you have a many-core machine, of course, there's no simpler solution >>>> than throwing more CPUs at it. :) >>> >>> Is it safe nowadays to run "make check -j4" for example? Last time I >>> tried (maybe 1 or 2 years ago), there were still issues since some tests >>> were using hard-coded temporary file names, so the parallel tests were >>> disturbing each other, for example... >>> >> >> Actually the `.travis.yml` defines `MAKEFLAGS="-j3"`, which results in `make >> check` being executed with 3 threads... >> >> I was actually looking at the increasing number of failed travis builds and >> it seems to be related to the fluctuating performance. Running `nice -n 20 >> make check -j 12` with `nice -n 5 stress -c 20` in background results in the >> same kind of failures: >> >> File '/tmp/qemu/include/qapi/qmp/qobject.h' >> Lines executed:0.00% of 9 >> ** >> ERROR:tests/prom-env-test.c:42:check_guest_memory: assertion failed >> (signature == MAGIC): (0x7c7f1b78 == 0xcafec0de) > > I think you're simply running into timeout issues here since you're > likely overloading the host, I guess. Or does your host have that many CPUs? > If the timeouts are really an issue here, we simply might have to > increase the timeout values a bit again... > The reason I did this is that the `make check -j 12` runs for ~ 4 minutes on my machine (8 cores) while in travis it sometimes runs even more than 40 minutes. I wanted to get closer to see why is it failing and this might be the reason and yes, it's most certainly timeout issue. The problem with Travis is it gives quite decent power, but sometimes it's slowed for couple of seconds, or even minutes. This affects many of our (Avocado) selftests so we usually have timeouts between 10-60s for operations that usually take less than a second. Lukáš PS: Usually the `make check -j 12` works well on my machine... > Thomas > signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] make check speed
Dne 23.8.2017 v 14:35 Thomas Huth napsal(a): > On 23.08.2017 13:51, Lukáš Doktor wrote: > [...] >> and all ERROR lines: >> >> ERROR:tests/prom-env-test.c:42:check_guest_memory: assertion failed >> (signature == MAGIC): (0x == 0xcafec0de) > > This test uses a timeout of 120 s, see check_guest_memory() in > tests/prom-env-test.c ... that's a lot already, but if you hit this on a > real system, feel free to send a patch to increase the timeout. > >> ERROR:tests/rtc-test.c:173:check_time: assertion failed (ABS(t - s) <= >> wiggle): (3 <= 2) > > You might need to increase the "wiggle" variable here. > >> ERROR:tests/vhost-user-test.c:779:test_reconnect: child process >> (/i386/vhost-user/reconnect/subprocess [5264]) failed unexpectedly >> ERROR:tests/vhost-user-test.c:807:test_connect_fail: child process >> (/x86_64/vhost-user/connect-fail/subprocess [7751]) failed unexpectedly >> ERROR:tests/vhost-user-test.c:835:test_flags_mismatch: child process >> (/x86_64/vhost-user/flags-mismatch/subprocess [7892]) failed unexpectedly > > I've got no clue about what could be wrong here ... maybe Marc-André can > advise here (who wrote these tests if I got "git blame" right). > >> ERROR:tests/boot-sector.c:127:boot_sector_test: assertion failed >> (signature == SIGNATURE): (0x == 0xdead) > > Uses a timeout of 90 s ... feel free to send a patch to increase it if > it's necessary (the timeout can be found in boot_sector_test() in > tests/boot-sector.c). > > Thomas > Well this one was produced by the `stress` command, so it does not reflect the reality. So this time I took the time and looked at the actual failures in David's travis results (his builds were already migrated to trusty and are failing massively) https://travis-ci.org/dgibson/qemu/builds I see only the `vhost-user-test` failing: ERROR:tests/vhost-user-test.c:779:test_reconnect: child process (/x86_64/vhost-user/reconnect/subprocess [58907]) failed unexpectedly ERROR:tests/vhost-user-test.c:807:test_connect_fail: child process (/x86_64/vhost-user/connect-fail/subprocess [58924]) failed unexpectedly ERROR:tests/vhost-user-test.c:835:test_flags_mismatch: child process (/i386/vhost-user/flags-mismatch/subprocess [56261]) failed unexpectedly I actually looked there but I have no clue of how to fix this issue. But most probably it'd make Travis happier... Lukáš signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH 04/11] qemu.py: Simplify QMP key-conversion
The QMP key conversion consist of '_'s to be replaced with '-'s, which
can easily be done by a single `str.replace` method which is faster and
does not require `string` module import.
Signed-off-by: Lukáš Doktor
---
scripts/qemu.py | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7e95c25..5948e19 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,7 +13,6 @@
#
import errno
-import string
import os
import sys
import subprocess
@@ -180,14 +179,12 @@ class QEMUMachine(object):
self._load_io_log()
self._post_shutdown()
-underscore_to_dash = string.maketrans('_', '-')
-
def qmp(self, cmd, conv_keys=True, **args):
'''Invoke a QMP command and return the result dict'''
qmp_args = dict()
for key, value in args.iteritems():
if conv_keys:
-qmp_args[key.translate(self.underscore_to_dash)] = value
+qmp_args[key.replace('_', '-')] = value
else:
qmp_args[key] = value
--
2.9.4
[Qemu-devel] [PATCH 11/11] qtest.py: Avoid using mutable list as default argument
The list is a mutable object and is dangerous to use it. Recently the QEMUMachine was adjusted to allow None as default, let's use it here as well. Signed-off-by: Lukáš Doktor --- scripts/qtest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qtest.py b/scripts/qtest.py index e4b5788..ba3233c 100644 --- a/scripts/qtest.py +++ b/scripts/qtest.py @@ -76,7 +76,7 @@ class QEMUQtestProtocol(object): class QEMUQtestMachine(qemu.QEMUMachine): '''A QEMU VM''' -def __init__(self, binary, args=[], name=None, test_dir="/var/tmp", +def __init__(self, binary, args=None, name=None, test_dir="/var/tmp", socket_scm_helper=None): if name is None: name = "qemu-%d" % os.getpid() -- 2.9.4
[Qemu-devel] [PATCH 02/11] qemu.py: Avoid dangerous arguments
The list object is mutable in python and potentially might modify other
object's arguments when used as default argument. Reproducer:
>>> vm1 = QEMUMachine("qemu")
>>> vm2 = QEMUMachine("qemu")
>>> vm1._wrapper.append("foo")
>>> print vm2._wrapper
['foo']
In this case the `args` is actually copied so it would be safe to keep
it, but it's not a good practice to keep it.
Signed-off-by: Lukáš Doktor
---
scripts/qemu.py | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 191c916..66fd863 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,7 +23,7 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
+def __init__(self, binary, args=None, wrapper=None, name=None,
test_dir="/var/tmp", monitor_address=None,
socket_scm_helper=None, debug=False):
'''
@@ -39,6 +39,10 @@ class QEMUMachine(object):
@param debug: enable debug mode (forwarded to QMP helper and such)
@note: Qemu process is not started until launch() is used.
'''
+if args is None:
+args = []
+if wrapper is None:
+wrapper = []
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
--
2.9.4
[Qemu-devel] [PATCH 03/11] qemu.py: Use iteritems rather than keys()
Let's avoid creating an in-memory list of keys and query for each value and use `iteritems` which is an iterator of key-value pairs. Signed-off-by: Lukáš Doktor --- scripts/qemu.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 66fd863..7e95c25 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -185,11 +185,11 @@ class QEMUMachine(object): def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the result dict''' qmp_args = dict() -for key in args.keys(): +for key, value in args.iteritems(): if conv_keys: -qmp_args[key.translate(self.underscore_to_dash)] = args[key] +qmp_args[key.translate(self.underscore_to_dash)] = value else: -qmp_args[key] = args[key] +qmp_args[key] = value return self._qmp.cmd(cmd, args=qmp_args) -- 2.9.4
[Qemu-devel] [PATCH 08/11] qmp.py: Avoid "has_key" usage
The "has_key" is deprecated in favor of "__in__" operator.
Signed-off-by: Lukáš Doktor
---
scripts/qmp/qmp.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 68f3420..a14b001 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -197,7 +197,7 @@ class QEMUMonitorProtocol(object):
Build and send a QMP command to the monitor, report errors when any
"""
ret = self.cmd(cmd, kwds)
-if ret.has_key('error'):
+if "error" in ret:
raise Exception(ret['error']['desc'])
return ret['return']
--
2.9.4
[Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception
The naked Exception should not be widely used. It makes sense to be a
bit more specific and use better-suited custom exceptions. As a benefit
we can store the full reply in the exception in case someone needs it
when catching the exception.
Signed-off-by: Lukáš Doktor
---
scripts/qemu.py | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 5948e19..2bd522f 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -19,6 +19,19 @@ import subprocess
import qmp.qmp
+class MonitorResponseError(qmp.qmp.QMPError):
+'''
+Represents erroneous QMP monitor reply
+'''
+def __init__(self, reply):
+try:
+desc = reply["error"]["desc"]
+except KeyError:
+desc = reply
+super(MonitorResponseError, self).__init__(desc)
+self.reply = reply
+
+
class QEMUMachine(object):
'''A QEMU VM'''
@@ -197,9 +210,9 @@ class QEMUMachine(object):
'''
reply = self.qmp(cmd, conv_keys, **args)
if reply is None:
-raise Exception("Monitor is closed")
+raise qmp.qmp.QMPError("Monitor is closed")
if "error" in reply:
-raise Exception(reply["error"]["desc"])
+raise MonitorResponseError(reply)
return reply["return"]
def get_qmp_event(self, wait=False):
--
2.9.4
[Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object
The "id" is a builtin method to get object's identity and should not be
overridden. This might bring some issues in case someone was directly
calling "cmd(..., id=id)" but I haven't found such usage on brief search
for "cmd\(.*id=".
Signed-off-by: Lukáš Doktor
---
scripts/qmp/qmp.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index a14b001..c3e0206 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
print >>sys.stderr, "QMP:<<< %s" % resp
return resp
-def cmd(self, name, args=None, id=None):
+def cmd(self, name, args=None, cmd_id=None):
"""
Build a QMP command and send it to the QMP Monitor.
@param name: command name (string)
@param args: command arguments (dict)
-@param id: command id (dict, list, string or int)
+@param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
-if id:
-qmp_cmd['id'] = id
+if cmd_id:
+qmp_cmd['cmd_id'] = cmd_id
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
--
2.9.4
[Qemu-devel] [PATCH 00/11] qemu.py: Pylint/style fixes
Hello guys, I'm reading the available python modules to exercise qemu and while reading them I fixed some issues that caught my attention. It usually starts with a simple pylint/docstring fixes and slowly graduates to more controversial ones so I'm open to suggestion to remove some of them. Kind regards, Lukáš Lukáš Doktor (11): qemu.py: Pylint/style fixes qemu.py: Avoid dangerous arguments qemu.py: Use iteritems rather than keys() qemu.py: Simplify QMP key-conversion qemu.py: Use custom exceptions rather than Exception qmp.py: Couple of pylint/style fixes qmp.py: Use object-based class for QEMUMonitorProtocol qmp.py: Avoid "has_key" usage qmp.py: Avoid overriding a builtin object qtest.py: Few pylint/style fixes qtest.py: Avoid using mutable list as default argument scripts/qemu.py| 98 +++--- scripts/qmp/qmp.py | 49 --- scripts/qtest.py | 14 3 files changed, 108 insertions(+), 53 deletions(-) -- 2.9.4
[Qemu-devel] [PATCH 01/11] qemu.py: Pylint/style fixes
No actual code changes, just several pylint/style fixes and docstring
clarifications.
Signed-off-by: Lukáš Doktor
---
scripts/qemu.py | 76 -
1 file changed, 53 insertions(+), 23 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8..191c916 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,8 +23,22 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
test_dir="/var/tmp",
- monitor_address=None, socket_scm_helper=None, debug=False):
+def __init__(self, binary, args=[], wrapper=[], name=None,
+ test_dir="/var/tmp", monitor_address=None,
+ socket_scm_helper=None, debug=False):
+'''
+Create a QEMUMachine object
+
+@param binary: path to the qemu binary (str)
+@param args: initial list of extra arguments
+@param wrapper: list of arguments used as prefix to qemu binary
+@param name: name of this object (used for log/monitor/... file names)
+@param test_dir: base location to put log/monitor/... files in
+@param monitor_address: custom address for QMP monitor
+@param socket_scm_helper: path to scm_helper binary (to forward fds)
+@param debug: enable debug mode (forwarded to QMP helper and such)
+@note: Qemu process is not started until launch() is used.
+'''
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
@@ -33,12 +47,13 @@ class QEMUMachine(object):
self._qemu_log_path = os.path.join(test_dir, name + ".log")
self._popen = None
self._binary = binary
-self._args = list(args) # Force copy args in case we modify them
+self._args = list(args) # Force copy args in case we modify them
self._wrapper = wrapper
self._events = []
self._iolog = None
self._socket_scm_helper = socket_scm_helper
self._debug = debug
+self._qmp = None
# This can be used to add an unused monitor instance.
def add_monitor_telnet(self, ip, port):
@@ -64,16 +79,16 @@ class QEMUMachine(object):
if self._socket_scm_helper is None:
print >>sys.stderr, "No path to socket_scm_helper set"
return -1
-if os.path.exists(self._socket_scm_helper) == False:
+if os.path.exists(self._socket_scm_helper) is False:
print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
return -1
fd_param = ["%s" % self._socket_scm_helper,
"%d" % self._qmp.get_sock_fd(),
"%s" % fd_file_path]
devnull = open('/dev/null', 'rb')
-p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
- stderr=sys.stderr)
-return p.wait()
+proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
+stderr=sys.stderr)
+return proc.wait()
@staticmethod
def _remove_if_exists(path):
@@ -99,8 +114,8 @@ class QEMUMachine(object):
return self._popen.pid
def _load_io_log(self):
-with open(self._qemu_log_path, "r") as fh:
-self._iolog = fh.read()
+with open(self._qemu_log_path, "r") as iolog:
+self._iolog = iolog.read()
def _base_args(self):
if isinstance(self._monitor_address, tuple):
@@ -114,8 +129,8 @@ class QEMUMachine(object):
'-display', 'none', '-vga', 'none']
def _pre_launch(self):
-self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
server=True,
-debug=self._debug)
+self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
+server=True, debug=self._debug)
def _post_launch(self):
self._qmp.accept()
@@ -131,9 +146,11 @@ class QEMUMachine(object):
qemulog = open(self._qemu_log_path, 'wb')
try:
self._pre_launch()
-args = self._wrapper + [self._binary] + self._base_args() +
self._args
+args = (self._wrapper + [self._binary] + self._base_args() +
+self._args)
self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
- stderr=subprocess.STDOUT,
shell=False)
+ stderr=subprocess.STDOUT,
+ shell=False)
self._post_launch()
except:
[Qemu-devel] [PATCH 07/11] qmp.py: Use object-based class for QEMUMonitorProtocol
There is no need to define QEMUMonitorProtocol as old-style class. Signed-off-by: Lukáš Doktor --- scripts/qmp/qmp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index bb4d614..68f3420 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError): pass -class QEMUMonitorProtocol: +class QEMUMonitorProtocol(object): #: Socket's error class error = socket.error -- 2.9.4
[Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes
No actual code changes, just few pylint/style fixes. Signed-off-by: Lukáš Doktor --- scripts/qtest.py | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/qtest.py b/scripts/qtest.py index d5aecb5..e4b5788 100644 --- a/scripts/qtest.py +++ b/scripts/qtest.py @@ -11,14 +11,11 @@ # Based on qmp.py. # -import errno import socket -import string import os -import subprocess -import qmp.qmp import qemu + class QEMUQtestProtocol(object): def __init__(self, address, server=False): """ @@ -83,8 +80,11 @@ class QEMUQtestMachine(qemu.QEMUMachine): socket_scm_helper=None): if name is None: name = "qemu-%d" % os.getpid() -super(QEMUQtestMachine, self).__init__(binary, args, name=name, test_dir=test_dir, - socket_scm_helper=socket_scm_helper) +scm_helper = socket_scm_helper +super(QEMUQtestMachine, self).__init__(binary, args, name=name, + test_dir=test_dir, + socket_scm_helper=scm_helper) +self._qtest = None self._qtest_path = os.path.join(test_dir, name + "-qtest.sock") def _base_args(self): -- 2.9.4
[Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes
No actual code changes, just a few pylint/style fixes and docstring
clarifications.
Signed-off-by: Lukáš Doktor
---
scripts/qmp/qmp.py | 37 -
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651..bb4d614 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -13,19 +13,30 @@ import errno
import socket
import sys
+
class QMPError(Exception):
pass
+
class QMPConnectError(QMPError):
pass
+
class QMPCapabilitiesError(QMPError):
pass
+
class QMPTimeoutError(QMPError):
pass
+
class QEMUMonitorProtocol:
+
+#: Socket's error class
+error = socket.error
+#: Socket's timeout
+timeout = socket.timeout
+
def __init__(self, address, server=False, debug=False):
"""
Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
self.__address = address
self._debug = debug
self.__sock = self.__get_sock()
+self.__sockfile = None
if server:
self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
self.__sock.bind(self.__address)
@@ -56,7 +68,7 @@ class QEMUMonitorProtocol:
def __negotiate_capabilities(self):
greeting = self.__json_read()
-if greeting is None or not greeting.has_key('QMP'):
+if greeting is None or "QMP" not in greeting:
raise QMPConnectError
# Greeting seems ok, negotiate capabilities
resp = self.cmd('qmp_capabilities')
@@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
continue
return resp
-error = socket.error
-
def __get_events(self, wait=False):
"""
Check for new events in the stream and cache them in __events.
@@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
"""
# Check for new events regardless and pull them into the cache:
@@ -175,7 +185,7 @@ class QEMUMonitorProtocol:
@param args: command arguments (dict)
@param id: command id (dict, list, string or int)
"""
-qmp_cmd = { 'execute': name }
+qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
if id:
@@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
+"""
+Build and send a QMP command to the monitor, report errors when any
+"""
ret = self.cmd(cmd, kwds)
if ret.has_key('error'):
raise Exception(ret['error']['desc'])
@@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
def pull_event(self, wait=False):
"""
-Get and delete the first available QMP event.
+Get and pop the first available QMP event.
@param wait (bool): block until an event is available.
@param wait (float): If wait is a float, treat it as a timeout value.
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The first available QMP event, or None.
"""
@@ -217,8 +230,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The list of available QMP events.
"""
@@ -235,8 +248,6 @@ class QEMUMonitorProtocol:
self.__sock.close()
self.__sockfile.close()
-timeout = socket.timeout
-
def settimeout(self, timeout):
self.__sock.settimeout(timeout)
--
2.9.4
Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception
Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
> On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
>> The naked Exception should not be widely used. It makes sense to be a
>> bit more specific and use better-suited custom exceptions. As a benefit
>> we can store the full reply in the exception in case someone needs it
>> when catching the exception.
>>
>> Signed-off-by: Lukáš Doktor
>> ---
>> scripts/qemu.py | 17 +++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index 5948e19..2bd522f 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -19,6 +19,19 @@ import subprocess
>> import qmp.qmp
>>
>>
>> +class MonitorResponseError(qmp.qmp.QMPError):
>> +'''
>> +Represents erroneous QMP monitor reply
>> +'''
>> +def __init__(self, reply):
>> +try:
>> +desc = reply["error"]["desc"]
>> +except KeyError:
>> +desc = reply
>> +super(MonitorResponseError, self).__init__(desc)
>> +self.reply = reply
>
> This would require every user of self.reply to first check if
> it's a string or dictionary. All because of the "Monitor is
> closed" case below:
>
I haven't used it for the `Monitor is closed` exception, so it's just to be
able to store really broken reply safely. Anyway people can check whether the
reply is a dict, or I can add `is_valid_reply` property which would check for
`[error][desc]` presence (which is a bit more precise than just plain dict type
checking).
>> +
>> +
>> class QEMUMachine(object):
>> '''A QEMU VM'''
>>
>> @@ -197,9 +210,9 @@ class QEMUMachine(object):
>> '''
>> reply = self.qmp(cmd, conv_keys, **args)
>> if reply is None:
>> -raise Exception("Monitor is closed")
>> +raise qmp.qmp.QMPError("Monitor is closed")
>
> Should we really use the same exception type for this, if it's
> not really a QMP monitor error reply, and won't even have a real
> QMP reply in self.reply?
>
I wasn't sure but the same exception can be caused by other failures when
obtaining replies so I think in case the monitor is closed we might expect the
same exception. Would importing it in the top level of this module to become
`qemu.QMPError` work for you better, or would you prefer IOError, RuntimeError
or another custom exception?
Lukáš
>
>> if "error" in reply:
>> -raise Exception(reply["error"]["desc"])
>> +raise MonitorResponseError(reply)
>> return reply["return"]
>>
>> def get_qmp_event(self, wait=False):
>> --
>> 2.9.4
>>
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 07/11] qmp.py: Use object-based class for QEMUMonitorProtocol
Dne 20.7.2017 v 20:35 Eduardo Habkost napsal(a): > On Thu, Jul 20, 2017 at 06:28:11PM +0200, Lukáš Doktor wrote: >> There is no need to define QEMUMonitorProtocol as old-style class. >> >> Signed-off-by: Lukáš Doktor >> --- >> scripts/qmp/qmp.py | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py >> index bb4d614..68f3420 100644 >> --- a/scripts/qmp/qmp.py >> +++ b/scripts/qmp/qmp.py >> @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError): >> pass >> >> >> -class QEMUMonitorProtocol: >> +class QEMUMonitorProtocol(object): > > I don't fully understand the consequences of changing to > new-style classes when using old-style SuperClass.__init__() > calls in the __init__(). Should we change QMPShell.__init__() to > use super()? > The consequence is it becomes a proper object with full object model and less workarounds. It also consumes a bit more memory but are the only available mode in py3. As for the old-style superclass, it works, but the correct approach is to replace it with `super` call. I'll address that in the v2 (I only checked for `.py` files but there are many python sources in qemu tree without the proper extension. I still need to get used to this.). Lukáš > >> >> #: Socket's error class >> error = socket.error >> -- >> 2.9.4 >> > signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object
Dne 20.7.2017 v 20:38 Eduardo Habkost napsal(a):
> On Thu, Jul 20, 2017 at 06:28:13PM +0200, Lukáš Doktor wrote:
>> The "id" is a builtin method to get object's identity and should not be
>> overridden. This might bring some issues in case someone was directly
>> calling "cmd(..., id=id)" but I haven't found such usage on brief search
>> for "cmd\(.*id=".
>>
>> Signed-off-by: Lukáš Doktor
>> ---
>> scripts/qmp/qmp.py | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index a14b001..c3e0206 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
>> print >>sys.stderr, "QMP:<<< %s" % resp
>> return resp
>>
>> -def cmd(self, name, args=None, id=None):
>> +def cmd(self, name, args=None, cmd_id=None):
>> """
>> Build a QMP command and send it to the QMP Monitor.
>>
>> @param name: command name (string)
>> @param args: command arguments (dict)
>> -@param id: command id (dict, list, string or int)
>> +@param cmd_id: command id (dict, list, string or int)
>> """
>> qmp_cmd = {'execute': name}
>> if args:
>> qmp_cmd['arguments'] = args
>> -if id:
>> -qmp_cmd['id'] = id
>> +if cmd_id:
>> +qmp_cmd['cmd_id'] = cmd_id
>
> The member sent through the monitor should still be called "id".
> i.e.:
>
> qmp_cmd['id'] = cmd_id
>
Oups, sorry, automatic rename changed it and I forgot to fix this one back.
I'll address this in v2. The main problem with this patch is it could break
named arguments (`cmd(..., id=id)` calls) so I'm not sure it's worth including.
But as mentioned in commit message I grepped full sources so we better fix this
before someone starts using it.
Lukáš
>
>> return self.cmd_obj(qmp_cmd)
>>
>> def command(self, cmd, **kwds):
>> --
>> 2.9.4
>>
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes
Dne 20.7.2017 v 20:42 Eduardo Habkost napsal(a): > On Thu, Jul 20, 2017 at 06:28:14PM +0200, Lukáš Doktor wrote: > [...] >> @@ -83,8 +80,11 @@ class QEMUQtestMachine(qemu.QEMUMachine): >> socket_scm_helper=None): >> if name is None: >> name = "qemu-%d" % os.getpid() >> -super(QEMUQtestMachine, self).__init__(binary, args, name=name, >> test_dir=test_dir, >> - >> socket_scm_helper=socket_scm_helper) >> +scm_helper = socket_scm_helper > > Why is this necessary? > to avoid > 80 chars line. It should be optimized-out by the python compiler so it should not slow down the execution. Alternative solution is to use: super(QEMUQtestMachine, self.__init__(...) which looks IMO uglier, but I can use that in v2, should that be your preferred style. Lukáš >> +super(QEMUQtestMachine, self).__init__(binary, args, name=name, >> + test_dir=test_dir, >> + socket_scm_helper=scm_helper) >> +self._qtest = None >> self._qtest_path = os.path.join(test_dir, name + "-qtest.sock") >> >> def _base_args(self): >> -- >> 2.9.4 >> > signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 11/11] qtest.py: Avoid using mutable list as default argument
Dne 20.7.2017 v 20:44 Eduardo Habkost napsal(a): > On Thu, Jul 20, 2017 at 06:28:15PM +0200, Lukáš Doktor wrote: >> The list is a mutable object and is dangerous to use it. Recently the >> QEMUMachine was adjusted to allow None as default, let's use it here as >> well. >> >> Signed-off-by: Lukáš Doktor > > This patch requires patch 02/11 and both are pretty small. > I suggest squashing them together. > Sure, I'll change it in the v2. Lukáš signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 1/3] qemu.py: fix is_running()
Dne 20.7.2017 v 22:14 Amador Pahim napsal(a):
> On Thu, Jul 20, 2017 at 7:49 PM, Eduardo Habkost wrote:
>> On Thu, Jul 20, 2017 at 05:09:11PM +0200, Markus Armbruster wrote:
>>> Amador Pahim writes:
>>>
On Thu, Jul 20, 2017 at 1:49 PM, Markus Armbruster
wrote:
> Amador Pahim writes:
>
>> Current implementation is broken. It does not really test if the child
>> process is running.
>
> What usage exactly is broken by this? Got a reproducer for me?
Problem is that 'returncode' is not set without a calling
poll()/wait()/communicate(), so it's only useful to test if the
process is running after such calls. But if we use 'poll()' instead,
it will, according to the docs, "Check if child process has
terminated. Set and return returncode attribute."
Reproducer is:
>>> import subprocess
>>> devnull = open('/dev/null', 'rb')
>>> p = subprocess.Popen(['qemu-system-x86_64', '-broken'],
stdin=devnull, stdout=devnull, stderr=devnull, shell=False)
>>> print p.returncode
None
>>> print p.poll()
1
>>> print p.returncode
1
>> The Popen.returncode will only be set after by a poll(), wait() or
>> communicate(). If the Popen fails to launch a VM, the Popen.returncode
>> will not turn to None by itself.
>
> Hmm. What is the value of .returncode then?
returncode starts with None and becomes the process exit code when the
process is over and one of that three methods is called (poll(),
wait() or communicate()).
There's an error in my description though. The correct would be: "The
Popen.returncode will only be set after a call to poll(), wait() or
communicate(). If the Popen fails to launch a VM, the Popen.returncode
will not turn from None to the actual return code by itself."
>>>
>>> Suggest to add ", and is_running() continues to report True".
>>>
>> Instead of using Popen.returncode, let's use Popen.poll(), which
>> actually checks if child process has terminated.
>>
>> Signed-off-by: Amador Pahim
>> Reviewed-by: Eduardo Habkost
>> Reviewed-by: Fam Zheng
>> ---
>> scripts/qemu.py | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index 880e3e8219..f0fade32bd 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -86,7 +86,7 @@ class QEMUMachine(object):
>> raise
>>
>> def is_running(self):
>> -return self._popen and (self._popen.returncode is None)
>> +return self._popen and (self._popen.poll() is None)
>>
>>
>> After re-reading shutdown(), I think this is _not_ OK: if
>> is_running() return False before we call .wait(), we will never
>> load the log file or run _post_shutdown() if QEMU exits between
>> the launch() and shutdown() calls.
>
> Yes, I just noticed that while cleaning up the code.
>
>>
>> Yes, it's fragile.
>>
>> The root problem on both launch() and shutdown() seems to be
>> coupling the external "is QEMU running?" state with the internal
>> "did we load the log file and ran _post_shutdown() already?"
>> state.
>>
>> I see two possible approaches for this:
>>
>> 1) Benefit from the fact that the internal Popen state will not
>>change under our feet unless we explicitly call
>>poll()/wait()/etc, and keep the existing code. (Not my
>>favorite option)
>>
>> 2) Rewrite the code so that we don't depend on the subtle Popen
>>internal state rules, and track our own internal state in
>>a QEMUMachine attribute. e.g.:
>
> +1 for this approach. I'm working on something similar, thanks for the
> detailed "e.g." code here.
>
>>
>> def _handle_shutdown(self):
>> '''Load log file and call _post_shutdown() hook if necessary'''
>> # Must be called only after QEMU actually exited.
>> assert not self.is_running()
>> if self._shutdown_pending:
>> if self.exitcode() < 0:
>> sys.stderr.write('qemu received signal %i: %s\n' %
>> (-exitcode, ' '.join(self._args)))
>> self._load_io_log()
>> self._post_shutdown()
>> self._shutdown_pending = False
>>
>> def _terminate(self):
>> '''Terminate QEMU if it's still running'''
>> if self.is_running():
>> try:
>> self._qmp.cmd('quit')
>> self._qmp.close()
>> except:
>> self._popen.kill()
>> self._popen.wait()
>>
>> def _launch(self):
>> '''Launch the VM and establish a QMP connection'''
>> devnull = open('/dev/null', 'rb')
>> qemulog = open(self._qemu_log_path, 'wb')
>> self._shutdown_pending = True
>> self._pre_launch()
>> args = self._wrapper + [self._binary] + self._base_args() +
>> self._args
>> self._pop
Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception
Dne 21.7.2017 v 20:42 Eduardo Habkost napsal(a):
> On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote:
>> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
>>> On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
>>>> The naked Exception should not be widely used. It makes sense to be a
>>>> bit more specific and use better-suited custom exceptions. As a benefit
>>>> we can store the full reply in the exception in case someone needs it
>>>> when catching the exception.
>>>>
>>>> Signed-off-by: Lukáš Doktor
>>>> ---
>>>> scripts/qemu.py | 17 +++--
>>>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>>>> index 5948e19..2bd522f 100644
>>>> --- a/scripts/qemu.py
>>>> +++ b/scripts/qemu.py
>>>> @@ -19,6 +19,19 @@ import subprocess
>>>> import qmp.qmp
>>>>
>>>>
>>>> +class MonitorResponseError(qmp.qmp.QMPError):
>>>> +'''
>>>> +Represents erroneous QMP monitor reply
>>>> +'''
>>>> +def __init__(self, reply):
>>>> +try:
>>>> +desc = reply["error"]["desc"]
>>>> +except KeyError:
>>>> +desc = reply
>>>> +super(MonitorResponseError, self).__init__(desc)
>>>> +self.reply = reply
>>>
>>> This would require every user of self.reply to first check if
>>> it's a string or dictionary. All because of the "Monitor is
>>> closed" case below:
>>>
>> I haven't used it for the `Monitor is closed` exception, so
>> it's just to be able to store really broken reply safely.
>> Anyway people can check whether the reply is a dict, or I can
>> add `is_valid_reply` property which would check for
>> `[error][desc]` presence (which is a bit more precise than just
>> plain dict type checking).
>
>
> Oops, I wasn't paying enough attention, sorry. self.reply is
> actually always set to the response from the monitor.
>
> If you are just trying a safe fallback for 'desc' if the response
> broken, what about using repr(reply) or json.dumps(reply) if
> reply['error']['desc'] isn't set?
>
I could use repr, but I'd prefer keeping it the way it is as you could use
`isinstance` to see whether it's dict and interact with it (if needed, eg. on
negative testing, which is my motivation for storing the full response).
Lukáš
>>
>>>> +
>>>> +
>>>> class QEMUMachine(object):
>>>> '''A QEMU VM'''
>>>>
>>>> @@ -197,9 +210,9 @@ class QEMUMachine(object):
>>>> '''
>>>> reply = self.qmp(cmd, conv_keys, **args)
>>>> if reply is None:
>>>> -raise Exception("Monitor is closed")
>>>> +raise qmp.qmp.QMPError("Monitor is closed")
>>>
>>> Should we really use the same exception type for this, if it's
>>> not really a QMP monitor error reply, and won't even have a real
>>> QMP reply in self.reply?
>>>
>> I wasn't sure but the same exception can be caused by other
>> failures when obtaining replies so I think in case the monitor
>> is closed we might expect the same exception. Would importing
>> it in the top level of this module to become `qemu.QMPError`
>> work for you better, or would you prefer IOError, RuntimeError
>> or another custom exception?
>
> I was not paying enough attention. QMPError sounds good to me.
>
> Reviewed-by: Eduardo Habkost
>
>>
>> Lukáš
>>
>>>
>>>> if "error" in reply:
>>>> -raise Exception(reply["error"]["desc"])
>>>> +raise MonitorResponseError(reply)
>>>> return reply["return"]
>>>>
>>>> def get_qmp_event(self, wait=False):
>>>> --
>>>> 2.9.4
>>>>
>>>
>>
>>
>
>
>
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes
Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
> Hi Lukáš,
>
> Since comment/indent fixes and code changes are not related I'd rather see
> this split in at least 2 patches.
>
Hello Philippe, thank you for the review, I'm wondering what code changes you
have in mind? This is commit should not bring any code changes, just code
reorganization (like including the self.* attributes on top of the file)
> On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
>> No actual code changes, just a few pylint/style fixes and docstring
>> clarifications.
>>
>> Signed-off-by: Lukáš Doktor
>> ---
>> scripts/qmp/qmp.py | 37 -
>> 1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index 62d3651..bb4d614 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -13,19 +13,30 @@ import errno
>> import socket
>> import sys
>> +
>> class QMPError(Exception):
>> pass
>> +
>> class QMPConnectError(QMPError):
>> pass
>> +
>> class QMPCapabilitiesError(QMPError):
>> pass
>> +
>> class QMPTimeoutError(QMPError):
>> pass
>> +
>> class QEMUMonitorProtocol:
>> +
>> +#: Socket's error class
>> +error = socket.error
>> +#: Socket's timeout
>> +timeout = socket.timeout
>> +
>
> ok
>
>> def __init__(self, address, server=False, debug=False):
>> """
>> Create a QEMUMonitorProtocol class.
>> @@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
>> self.__address = address
>> self._debug = debug
>> self.__sock = self.__get_sock()
>> +self.__sockfile = None
>> if server:
>> self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR,
>> 1)
>> self.__sock.bind(self.__address)
>> @@ -56,7 +68,7 @@ class QEMUMonitorProtocol:
>> def __negotiate_capabilities(self):
>> greeting = self.__json_read()
>> -if greeting is None or not greeting.has_key('QMP'):
>> +if greeting is None or "QMP" not in greeting:
>
> ok
>
>> raise QMPConnectError
>> # Greeting seems ok, negotiate capabilities
>> resp = self.cmd('qmp_capabilities')
>> @@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
>> continue
>> return resp
>> -error = socket.error
>> -
>> def __get_events(self, wait=False):
>> """
>> Check for new events in the stream and cache them in __events.
>> @@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
>> @raise QMPTimeoutError: If a timeout float is provided and the
>> timeout
>> period elapses.
>> -@raise QMPConnectError: If wait is True but no events could be
>> retrieved
>> -or if some other error occurred.
>> +@raise QMPConnectError: If wait is True but no events could be
>> +retrieved or if some other error occurred.
>> """
>> # Check for new events regardless and pull them into the cache:
>> @@ -175,7 +185,7 @@ class QEMUMonitorProtocol:
>> @param args: command arguments (dict)
>> @param id: command id (dict, list, string or int)
>> """
>> -qmp_cmd = { 'execute': name }
>> +qmp_cmd = {'execute': name}
>> if args:
>> qmp_cmd['arguments'] = args
>> if id:
>> @@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
>> return self.cmd_obj(qmp_cmd)
>> def command(self, cmd, **kwds):
>> +"""
>> +Build and send a QMP command to the monitor, report errors when any
>
> I'm not native english speaker but I think "if any" sounds better.
>
Yep, you are right.
>> +"""
>> ret = self.cmd(cmd, kwds)
>> if ret.has_key('error'):
>> raise Exception(ret['error']['desc'])
>> @@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
>> def pull_event(self, wait=False):
>> """
>> -Get and delete the first available QMP event.
Re: [Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object
Dne 21.7.2017 v 20:46 Eduardo Habkost napsal(a):
> On Fri, Jul 21, 2017 at 08:53:49AM +0200, Lukáš Doktor wrote:
>> Dne 20.7.2017 v 20:38 Eduardo Habkost napsal(a):
>>> On Thu, Jul 20, 2017 at 06:28:13PM +0200, Lukáš Doktor wrote:
>>>> The "id" is a builtin method to get object's identity and should not be
>>>> overridden. This might bring some issues in case someone was directly
>>>> calling "cmd(..., id=id)" but I haven't found such usage on brief search
>>>> for "cmd\(.*id=".
>>>>
>>>> Signed-off-by: Lukáš Doktor
>>>> ---
>>>> scripts/qmp/qmp.py | 8
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>>>> index a14b001..c3e0206 100644
>>>> --- a/scripts/qmp/qmp.py
>>>> +++ b/scripts/qmp/qmp.py
>>>> @@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
>>>> print >>sys.stderr, "QMP:<<< %s" % resp
>>>> return resp
>>>>
>>>> -def cmd(self, name, args=None, id=None):
>>>> +def cmd(self, name, args=None, cmd_id=None):
>>>> """
>>>> Build a QMP command and send it to the QMP Monitor.
>>>>
>>>> @param name: command name (string)
>>>> @param args: command arguments (dict)
>>>> -@param id: command id (dict, list, string or int)
>>>> +@param cmd_id: command id (dict, list, string or int)
>>>> """
>>>> qmp_cmd = {'execute': name}
>>>> if args:
>>>> qmp_cmd['arguments'] = args
>>>> -if id:
>>>> -qmp_cmd['id'] = id
>>>> +if cmd_id:
>>>> +qmp_cmd['cmd_id'] = cmd_id
>>>
>>> The member sent through the monitor should still be called "id".
>>> i.e.:
>>>
>>> qmp_cmd['id'] = cmd_id
>>>
>> Oups, sorry, automatic rename changed it and I forgot to fix
>> this one back. I'll address this in v2. The main problem with
>> this patch is it could break named arguments (`cmd(..., id=id)`
>> calls) so I'm not sure it's worth including. But as mentioned
>> in commit message I grepped full sources so we better fix this
>> before someone starts using it.
>
> I'm not convinced we need this patch, either. What exactly
> breaks if we don't apply this patch and somebody tries to use
> cmd(..., id=id)?
>
It'll work properly. The only issue is the `id` method will be unavailable
inside that function. So let's say you want to add `print` method to debug
issue with duplicate arguments, you'd be unable to use `id` to distinguish
between them. The other issue is that people will see it and copy&paste this
ugliness to other projects poisoning the world :-)
Therefor I'd like to include it, but I'm prepared to yield.
Lukáš
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes
Dne 21.7.2017 v 20:56 Eduardo Habkost napsal(a): > On Fri, Jul 21, 2017 at 08:57:34AM +0200, Lukáš Doktor wrote: >> Dne 20.7.2017 v 20:42 Eduardo Habkost napsal(a): >>> On Thu, Jul 20, 2017 at 06:28:14PM +0200, Lukáš Doktor wrote: >>> [...] >>>> @@ -83,8 +80,11 @@ class QEMUQtestMachine(qemu.QEMUMachine): >>>> socket_scm_helper=None): >>>> if name is None: >>>> name = "qemu-%d" % os.getpid() >>>> -super(QEMUQtestMachine, self).__init__(binary, args, name=name, >>>> test_dir=test_dir, >>>> - >>>> socket_scm_helper=socket_scm_helper) >>>> +scm_helper = socket_scm_helper >>> >>> Why is this necessary? >>> >> to avoid > 80 chars line. It should be optimized-out by the >> python compiler so it should not slow down the execution. >> Alternative solution is to use: >> >> super(QEMUQtestMachine, >> self.__init__(...) >> >> which looks IMO uglier, but I can use that in v2, should that be your >> preferred style. > > I think that would be better. The purpose of the extra variable > isn't clear when reading the code, making it more confusing. > OK, will fix in v2. Lukáš signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception
Dne 24.7.2017 v 17:32 Eduardo Habkost napsal(a): > On Mon, Jul 24, 2017 at 02:13:09PM +0200, Lukáš Doktor wrote: >> Dne 21.7.2017 v 20:42 Eduardo Habkost napsal(a): >>> On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote: >>>> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a): >>>>> On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote: >>>>>> The naked Exception should not be widely used. It makes sense to be a >>>>>> bit more specific and use better-suited custom exceptions. As a benefit >>>>>> we can store the full reply in the exception in case someone needs it >>>>>> when catching the exception. >>>>>> >>>>>> Signed-off-by: Lukáš Doktor >>>>>> --- >>>>>> scripts/qemu.py | 17 +++-- >>>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/scripts/qemu.py b/scripts/qemu.py >>>>>> index 5948e19..2bd522f 100644 >>>>>> --- a/scripts/qemu.py >>>>>> +++ b/scripts/qemu.py >>>>>> @@ -19,6 +19,19 @@ import subprocess >>>>>> import qmp.qmp >>>>>> >>>>>> >>>>>> +class MonitorResponseError(qmp.qmp.QMPError): >>>>>> +''' >>>>>> +Represents erroneous QMP monitor reply >>>>>> +''' >>>>>> +def __init__(self, reply): >>>>>> +try: >>>>>> +desc = reply["error"]["desc"] >>>>>> +except KeyError: >>>>>> +desc = reply >>>>>> +super(MonitorResponseError, self).__init__(desc) >>>>>> +self.reply = reply >>>>> >>>>> This would require every user of self.reply to first check if >>>>> it's a string or dictionary. All because of the "Monitor is >>>>> closed" case below: >>>>> >>>> I haven't used it for the `Monitor is closed` exception, so >>>> it's just to be able to store really broken reply safely. >>>> Anyway people can check whether the reply is a dict, or I can >>>> add `is_valid_reply` property which would check for >>>> `[error][desc]` presence (which is a bit more precise than just >>>> plain dict type checking). >>> >>> >>> Oops, I wasn't paying enough attention, sorry. self.reply is >>> actually always set to the response from the monitor. >>> >>> If you are just trying a safe fallback for 'desc' if the response >>> broken, what about using repr(reply) or json.dumps(reply) if >>> reply['error']['desc'] isn't set? >>> >> I could use repr, but I'd prefer keeping it the way it is as >> you could use `isinstance` to see whether it's dict and >> interact with it (if needed, eg. on negative testing, which is >> my motivation for storing the full response). > > This makes sense for self.reply, but I'm thinking about the > argument to Exception.__init__(). I'm worried that things might > break if the argument is not a string. > I see. It's python so it'll simply use `__str__` method, but you are right that for the exception description the use of `repr` is better. I'll change it in v2 (keeping the `self.reply` as is). Lukáš signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes
Dne 25.7.2017 v 08:04 Philippe Mathieu-Daudé napsal(a): > Hi Lukáš, > > On 07/24/2017 09:36 AM, Lukáš Doktor wrote: >> Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a): >>> Hi Lukáš, >>> >>> Since comment/indent fixes and code changes are not related I'd rather see >>> this split in at least 2 patches. >>> >> Hello Philippe, thank you for the review, I'm wondering what code changes >> you have in mind? This is commit should not bring any code changes, just >> code reorganization (like including the self.* attributes on top of the file) >> >>> On 07/20/2017 01:28 PM, Lukáš Doktor wrote: >>>> No actual code changes, just a few pylint/style fixes and docstring >>>> clarifications. >>>> >>>> Signed-off-by: Lukáš Doktor >>>> --- >>>>scripts/qmp/qmp.py | 37 - >>>>1 file changed, 24 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py > [...] >>>>def __init__(self, address, server=False, debug=False): >>>>""" >>>>Create a QEMUMonitorProtocol class. >>>> @@ -42,6 +53,7 @@ class QEMUMonitorProtocol: >>>>self.__address = address >>>>self._debug = debug >>>>self.__sock = self.__get_sock() >>>> +self.__sockfile = None > > I was thinking about this line which is new. Now the declaration and > initialization are done in __init__() while before it was only > declared/initialized in connect() or accept(). > > I'm not expert of python interpreter/jit internals but expect the generated > code to be slightly different, even if achieving the same. > > not a bit deal, just about wording ;) > Well the difference is that before `connect` you get `AttributeError` when looking for `self.__sockfile` while with this patch you'll get `None`. In this code nobody queries for `self.__sockfile` before the `connect` so it should not change the behavior of this code so I consider that as a `style` fix as it's ugly to extend attributes later in code (with some exceptions like Namespace-objects..). Anyway if you insist I can split those patches. Lukáš >>>>if server: >>>>self.__sock.setsockopt(socket.SOL_SOCKET, >>>> socket.SO_REUSEADDR, 1) >>>>self.__sock.bind(self.__address) signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 01/10] qemu.py: Pylint/style fixes
No actual code changes, just several pylint/style fixes and docstring
clarifications.
Signed-off-by: Lukáš Doktor
---
scripts/qemu.py | 76 -
1 file changed, 53 insertions(+), 23 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8..191c916 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,8 +23,22 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
test_dir="/var/tmp",
- monitor_address=None, socket_scm_helper=None, debug=False):
+def __init__(self, binary, args=[], wrapper=[], name=None,
+ test_dir="/var/tmp", monitor_address=None,
+ socket_scm_helper=None, debug=False):
+'''
+Create a QEMUMachine object
+
+@param binary: path to the qemu binary (str)
+@param args: initial list of extra arguments
+@param wrapper: list of arguments used as prefix to qemu binary
+@param name: name of this object (used for log/monitor/... file names)
+@param test_dir: base location to put log/monitor/... files in
+@param monitor_address: custom address for QMP monitor
+@param socket_scm_helper: path to scm_helper binary (to forward fds)
+@param debug: enable debug mode (forwarded to QMP helper and such)
+@note: Qemu process is not started until launch() is used.
+'''
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
@@ -33,12 +47,13 @@ class QEMUMachine(object):
self._qemu_log_path = os.path.join(test_dir, name + ".log")
self._popen = None
self._binary = binary
-self._args = list(args) # Force copy args in case we modify them
+self._args = list(args) # Force copy args in case we modify them
self._wrapper = wrapper
self._events = []
self._iolog = None
self._socket_scm_helper = socket_scm_helper
self._debug = debug
+self._qmp = None
# This can be used to add an unused monitor instance.
def add_monitor_telnet(self, ip, port):
@@ -64,16 +79,16 @@ class QEMUMachine(object):
if self._socket_scm_helper is None:
print >>sys.stderr, "No path to socket_scm_helper set"
return -1
-if os.path.exists(self._socket_scm_helper) == False:
+if os.path.exists(self._socket_scm_helper) is False:
print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
return -1
fd_param = ["%s" % self._socket_scm_helper,
"%d" % self._qmp.get_sock_fd(),
"%s" % fd_file_path]
devnull = open('/dev/null', 'rb')
-p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
- stderr=sys.stderr)
-return p.wait()
+proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
+stderr=sys.stderr)
+return proc.wait()
@staticmethod
def _remove_if_exists(path):
@@ -99,8 +114,8 @@ class QEMUMachine(object):
return self._popen.pid
def _load_io_log(self):
-with open(self._qemu_log_path, "r") as fh:
-self._iolog = fh.read()
+with open(self._qemu_log_path, "r") as iolog:
+self._iolog = iolog.read()
def _base_args(self):
if isinstance(self._monitor_address, tuple):
@@ -114,8 +129,8 @@ class QEMUMachine(object):
'-display', 'none', '-vga', 'none']
def _pre_launch(self):
-self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
server=True,
-debug=self._debug)
+self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
+server=True, debug=self._debug)
def _post_launch(self):
self._qmp.accept()
@@ -131,9 +146,11 @@ class QEMUMachine(object):
qemulog = open(self._qemu_log_path, 'wb')
try:
self._pre_launch()
-args = self._wrapper + [self._binary] + self._base_args() +
self._args
+args = (self._wrapper + [self._binary] + self._base_args() +
+self._args)
self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
- stderr=subprocess.STDOUT,
shell=False)
+ stderr=subprocess.STDOUT,
+ shell=False)
self._post_launch()
except:
[Qemu-devel] [PATCH v2 04/10] qemu.py: Simplify QMP key-conversion
The QMP key conversion consist of '_'s to be replaced with '-'s, which
can easily be done by a single `str.replace` method which is faster and
does not require `string` module import.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qemu.py | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7e95c25..5948e19 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,7 +13,6 @@
#
import errno
-import string
import os
import sys
import subprocess
@@ -180,14 +179,12 @@ class QEMUMachine(object):
self._load_io_log()
self._post_shutdown()
-underscore_to_dash = string.maketrans('_', '-')
-
def qmp(self, cmd, conv_keys=True, **args):
'''Invoke a QMP command and return the result dict'''
qmp_args = dict()
for key, value in args.iteritems():
if conv_keys:
-qmp_args[key.translate(self.underscore_to_dash)] = value
+qmp_args[key.replace('_', '-')] = value
else:
qmp_args[key] = value
--
2.9.4
[Qemu-devel] [PATCH v2 08/10] qmp.py: Avoid "has_key" usage
The "has_key" is deprecated in favor of "__in__" operator.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qmp/qmp.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 95ff5cc..f2f5a9b 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -197,7 +197,7 @@ class QEMUMonitorProtocol(object):
Build and send a QMP command to the monitor, report errors if any
"""
ret = self.cmd(cmd, kwds)
-if ret.has_key('error'):
+if "error" in ret:
raise Exception(ret['error']['desc'])
return ret['return']
--
2.9.4
[Qemu-devel] [PATCH v2 00/10] qemu.py: Pylint/style fixes
Hello guys, I'm reading the available python modules to exercise qemu and while reading them I fixed some issues that caught my attention. It usually starts with a simple pylint/docstring fixes and slowly graduates to more controversial ones so I'm open to suggestion to remove some of them. Kind regards, Lukáš Changes v1->v2 - Squashed 2nd and 10th patches into 2nd one - Use repr() in MonitorResponseError's description - Improved commit message of the 6th patch - Two tweaks to docstrings changed in the 6th patch - Also updated qmp-shell to use new-style super calls (7th patch) - Fixed the accidental rename of qmp `cmd_id` (kept the id->cmd_id patch) - Changed the style of the style-fix in the 10th commit Lukáš Doktor (10): qemu.py: Pylint/style fixes qemu|qtest: Avoid dangerous arguments qemu.py: Use iteritems rather than keys() qemu.py: Simplify QMP key-conversion qemu.py: Use custom exceptions rather than Exception qmp.py: Couple of pylint/style fixes qmp.py: Use object-based class for QEMUMonitorProtocol qmp.py: Avoid "has_key" usage qmp.py: Avoid overriding a builtin object qtest.py: Few pylint/style fixes scripts/qemu.py | 98 +-- scripts/qmp/qmp-shell | 4 +-- scripts/qmp/qmp.py| 49 -- scripts/qtest.py | 13 --- 4 files changed, 109 insertions(+), 55 deletions(-) -- 2.9.4
[Qemu-devel] [PATCH v2 02/10] qemu|qtest: Avoid dangerous arguments
The list object is mutable in python and potentially might modify other
object's arguments when used as default argument. Reproducer:
>>> vm1 = QEMUMachine("qemu")
>>> vm2 = QEMUMachine("qemu")
>>> vm1._wrapper.append("foo")
>>> print vm2._wrapper
['foo']
In this case the `args` is actually copied so it would be safe to keep
it, but it's not a good practice to keep it. The same issue applies in
inherited qtest module.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qemu.py | 6 +-
scripts/qtest.py | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 191c916..66fd863 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,7 +23,7 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
+def __init__(self, binary, args=None, wrapper=None, name=None,
test_dir="/var/tmp", monitor_address=None,
socket_scm_helper=None, debug=False):
'''
@@ -39,6 +39,10 @@ class QEMUMachine(object):
@param debug: enable debug mode (forwarded to QMP helper and such)
@note: Qemu process is not started until launch() is used.
'''
+if args is None:
+args = []
+if wrapper is None:
+wrapper = []
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
diff --git a/scripts/qtest.py b/scripts/qtest.py
index d5aecb5..ab183c0 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -79,7 +79,7 @@ class QEMUQtestProtocol(object):
class QEMUQtestMachine(qemu.QEMUMachine):
'''A QEMU VM'''
-def __init__(self, binary, args=[], name=None, test_dir="/var/tmp",
+def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
socket_scm_helper=None):
if name is None:
name = "qemu-%d" % os.getpid()
--
2.9.4
[Qemu-devel] [PATCH v2 10/10] qtest.py: Few pylint/style fixes
No actual code changes, just few pylint/style fixes. Signed-off-by: Lukáš Doktor --- scripts/qtest.py | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/scripts/qtest.py b/scripts/qtest.py index ab183c0..df0daf2 100644 --- a/scripts/qtest.py +++ b/scripts/qtest.py @@ -11,14 +11,11 @@ # Based on qmp.py. # -import errno import socket -import string import os -import subprocess -import qmp.qmp import qemu + class QEMUQtestProtocol(object): def __init__(self, address, server=False): """ @@ -83,8 +80,10 @@ class QEMUQtestMachine(qemu.QEMUMachine): socket_scm_helper=None): if name is None: name = "qemu-%d" % os.getpid() -super(QEMUQtestMachine, self).__init__(binary, args, name=name, test_dir=test_dir, - socket_scm_helper=socket_scm_helper) +super(QEMUQtestMachine, + self).__init__(binary, args, name=name, test_dir=test_dir, + socket_scm_helper=socket_scm_helper) +self._qtest = None self._qtest_path = os.path.join(test_dir, name + "-qtest.sock") def _base_args(self): -- 2.9.4
[Qemu-devel] [PATCH v2 03/10] qemu.py: Use iteritems rather than keys()
Let's avoid creating an in-memory list of keys and query for each value and use `iteritems` which is an iterator of key-value pairs. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Reviewed-by: Philippe Mathieu-Daudé --- scripts/qemu.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 66fd863..7e95c25 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -185,11 +185,11 @@ class QEMUMachine(object): def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the result dict''' qmp_args = dict() -for key in args.keys(): +for key, value in args.iteritems(): if conv_keys: -qmp_args[key.translate(self.underscore_to_dash)] = args[key] +qmp_args[key.translate(self.underscore_to_dash)] = value else: -qmp_args[key] = args[key] +qmp_args[key] = value return self._qmp.cmd(cmd, args=qmp_args) -- 2.9.4
[Qemu-devel] [PATCH v2 07/10] qmp.py: Use object-based class for QEMUMonitorProtocol
There is no need to define QEMUMonitorProtocol as old-style class. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost --- scripts/qmp/qmp-shell | 4 ++-- scripts/qmp/qmp.py| 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell index 860ffb2..be449de 100755 --- a/scripts/qmp/qmp-shell +++ b/scripts/qmp/qmp-shell @@ -106,7 +106,7 @@ class FuzzyJSON(ast.NodeTransformer): # _execute_cmd()). Let's design a better one. class QMPShell(qmp.QEMUMonitorProtocol): def __init__(self, address, pretty=False): -qmp.QEMUMonitorProtocol.__init__(self, self.__get_address(address)) +super(QMPShell, self).__init__(self.__get_address(address)) self._greeting = None self._completer = None self._pretty = pretty @@ -281,7 +281,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): return True def connect(self, negotiate): -self._greeting = qmp.QEMUMonitorProtocol.connect(self, negotiate) +self._greeting = super(QMPShell, self).connect(negotiate) self.__completer_setup() def show_banner(self, msg='Welcome to the QMP low-level shell!'): diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index 782d1ac..95ff5cc 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError): pass -class QEMUMonitorProtocol: +class QEMUMonitorProtocol(object): #: Socket's error class error = socket.error -- 2.9.4
[Qemu-devel] [PATCH v2 05/10] qemu.py: Use custom exceptions rather than Exception
The naked Exception should not be widely used. It makes sense to be a
bit more specific and use better-suited custom exceptions. As a benefit
we can store the full reply in the exception in case someone needs it
when catching the exception.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qemu.py | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 5948e19..e6df54c 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -19,6 +19,19 @@ import subprocess
import qmp.qmp
+class MonitorResponseError(qmp.qmp.QMPError):
+'''
+Represents erroneous QMP monitor reply
+'''
+def __init__(self, reply):
+try:
+desc = reply["error"]["desc"]
+except KeyError:
+desc = reply
+super(MonitorResponseError, self).__init__(repr(desc))
+self.reply = reply
+
+
class QEMUMachine(object):
'''A QEMU VM'''
@@ -197,9 +210,9 @@ class QEMUMachine(object):
'''
reply = self.qmp(cmd, conv_keys, **args)
if reply is None:
-raise Exception("Monitor is closed")
+raise qmp.qmp.QMPError("Monitor is closed")
if "error" in reply:
-raise Exception(reply["error"]["desc"])
+raise MonitorResponseError(reply)
return reply["return"]
def get_qmp_event(self, wait=False):
--
2.9.4
[Qemu-devel] [PATCH v2 06/10] qmp.py: Couple of pylint/style fixes
No actual code changes, just initializing attributes earlier to avoid
AttributeError on early introspection, a few pylint/style fixes and
docstring clarifications.
Signed-off-by: Lukáš Doktor
---
scripts/qmp/qmp.py | 37 -
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651..782d1ac 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -13,19 +13,30 @@ import errno
import socket
import sys
+
class QMPError(Exception):
pass
+
class QMPConnectError(QMPError):
pass
+
class QMPCapabilitiesError(QMPError):
pass
+
class QMPTimeoutError(QMPError):
pass
+
class QEMUMonitorProtocol:
+
+#: Socket's error class
+error = socket.error
+#: Socket's timeout
+timeout = socket.timeout
+
def __init__(self, address, server=False, debug=False):
"""
Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
self.__address = address
self._debug = debug
self.__sock = self.__get_sock()
+self.__sockfile = None
if server:
self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
self.__sock.bind(self.__address)
@@ -56,7 +68,7 @@ class QEMUMonitorProtocol:
def __negotiate_capabilities(self):
greeting = self.__json_read()
-if greeting is None or not greeting.has_key('QMP'):
+if greeting is None or "QMP" not in greeting:
raise QMPConnectError
# Greeting seems ok, negotiate capabilities
resp = self.cmd('qmp_capabilities')
@@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
continue
return resp
-error = socket.error
-
def __get_events(self, wait=False):
"""
Check for new events in the stream and cache them in __events.
@@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
"""
# Check for new events regardless and pull them into the cache:
@@ -175,7 +185,7 @@ class QEMUMonitorProtocol:
@param args: command arguments (dict)
@param id: command id (dict, list, string or int)
"""
-qmp_cmd = { 'execute': name }
+qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
if id:
@@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
+"""
+Build and send a QMP command to the monitor, report errors if any
+"""
ret = self.cmd(cmd, kwds)
if ret.has_key('error'):
raise Exception(ret['error']['desc'])
@@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
def pull_event(self, wait=False):
"""
-Get and delete the first available QMP event.
+Pulls a single event.
@param wait (bool): block until an event is available.
@param wait (float): If wait is a float, treat it as a timeout value.
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The first available QMP event, or None.
"""
@@ -217,8 +230,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The list of available QMP events.
"""
@@ -235,8 +248,6 @@ class QEMUMonitorProtocol:
self.__sock.close()
self.__sockfile.close()
-timeout = socket.timeout
-
def settimeout(self, timeout):
self.__sock.settimeout(timeout)
--
2.9.4
[Qemu-devel] [PATCH v2 09/10] qmp.py: Avoid overriding a builtin object
The "id" is a builtin method to get object's identity and should not be
overridden. This might bring some issues in case someone was directly
calling "cmd(..., id=id)" but I haven't found such usage on brief search
for "cmd\(.*id=".
Signed-off-by: Lukáš Doktor
---
scripts/qmp/qmp.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index f2f5a9b..ef12e8a 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
print >>sys.stderr, "QMP:<<< %s" % resp
return resp
-def cmd(self, name, args=None, id=None):
+def cmd(self, name, args=None, cmd_id=None):
"""
Build a QMP command and send it to the QMP Monitor.
@param name: command name (string)
@param args: command arguments (dict)
-@param id: command id (dict, list, string or int)
+@param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
-if id:
-qmp_cmd['id'] = id
+if cmd_id:
+qmp_cmd['id'] = cmd_id
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
--
2.9.4
Re: [Qemu-devel] [PATCH v2 05/10] qemu.py: Use custom exceptions rather than Exception
Dne 25.7.2017 v 18:06 Eduardo Habkost napsal(a):
> On Tue, Jul 25, 2017 at 05:09:46PM +0200, Lukáš Doktor wrote:
>> The naked Exception should not be widely used. It makes sense to be a
>> bit more specific and use better-suited custom exceptions. As a benefit
>> we can store the full reply in the exception in case someone needs it
>> when catching the exception.
>>
>> Signed-off-by: Lukáš Doktor
>> Reviewed-by: Eduardo Habkost
>> ---
>> scripts/qemu.py | 17 +++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index 5948e19..e6df54c 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -19,6 +19,19 @@ import subprocess
>> import qmp.qmp
>>
>>
>> +class MonitorResponseError(qmp.qmp.QMPError):
>> +'''
>> +Represents erroneous QMP monitor reply
>> +'''
>> +def __init__(self, reply):
>> +try:
>> +desc = reply["error"]["desc"]
>> +except KeyError:
>> +desc = reply
> ^^^ (*)
>> +super(MonitorResponseError, self).__init__(repr(desc))
>
> This will end up calling Exception.__init__. I previously
> suggested repr(desc) above(*) because I didn't know what happened
> when the Exception.__init__ argument is not a string.
>
> I couldn't find any documentation on the right argument types for
> Exception.__init__. The examples in the Python documentation[1]
> don't call Exception.__init__ at all: they simply implement
> __str__().
>
> However, based on my testing and on the BaseException
> documentation[2], I believe repr() isn't really necessary:
>
> "If str() or unicode() is called on an instance of this class,
> the representation of the argument(s) to the instance are
> returned, or the empty string when there were no arguments."
>
> So your previous version was good, already.
>
> But maybe we could do this instead, to make the constructor as
> simple as possible:
>
> class MonitorResponseError(qmp.qmp.QMPError):
> def __init__(self, reply):
> self.reply = reply
>
> def __str__(self):
> return self.reply.get('error', {}).get('desc', repr(self.reply))
>
Well I know I can implement custom `__str__` class, but the default one simply
stores the argument as string and reports it, so for simple strings I usually
go with super call and with complex ones with custom `__str__`. Anyway if this
suits this project style better I'll change it in v2.
Lukáš
>
> [1] https://docs.python.org/2/tutorial/errors.html#user-defined-exceptions
> [2] https://docs.python.org/2/library/exceptions.html#exceptions.BaseException
>
>> +self.reply = reply
>> +
>> +
>> class QEMUMachine(object):
>> '''A QEMU VM'''
>>
>> @@ -197,9 +210,9 @@ class QEMUMachine(object):
>> '''
>> reply = self.qmp(cmd, conv_keys, **args)
>> if reply is None:
>> -raise Exception("Monitor is closed")
>> +raise qmp.qmp.QMPError("Monitor is closed")
>> if "error" in reply:
>> -raise Exception(reply["error"]["desc"])
>> +raise MonitorResponseError(reply)
>> return reply["return"]
>>
>> def get_qmp_event(self, wait=False):
>> --
>> 2.9.4
>>
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 09/10] qmp.py: Avoid overriding a builtin object
Dne 25.7.2017 v 21:34 Eduardo Habkost napsal(a):
> On Tue, Jul 25, 2017 at 05:09:50PM +0200, Lukáš Doktor wrote:
>> The "id" is a builtin method to get object's identity and should not be
>> overridden. This might bring some issues in case someone was directly
>> calling "cmd(..., id=id)" but I haven't found such usage on brief search
>> for "cmd\(.*id=".
>>
>> Signed-off-by: Lukáš Doktor
>
> I don't see the benefit of the patch, as the function is very
> short and unlikely to ever use the id() builtin. But I am not
> against it if it will silence code analyzer warnings.
>
For me the biggest benefit is it decreases the probability of someone copying
the same "idea" to other pieces of code, because, you know, developers are
using copy&paste way too often.
Anyway pylint does complain about this issue. I can either ignore it, skip it
with an informative comment `# use id for backward compatibility pylint:
disable=W0622` or use this patch to fix it properly. I'm inclined towards
fixing it (until it poisons other parts of the code), you are in favor of
ignoring it, so let's see whether someone else has another opinion, otherwise
I'll remove it in v3.
Lukáš
>
>> ---
>> scripts/qmp/qmp.py | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index f2f5a9b..ef12e8a 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
>> print >>sys.stderr, "QMP:<<< %s" % resp
>> return resp
>>
>> -def cmd(self, name, args=None, id=None):
>> +def cmd(self, name, args=None, cmd_id=None):
>> """
>> Build a QMP command and send it to the QMP Monitor.
>>
>> @param name: command name (string)
>> @param args: command arguments (dict)
>> -@param id: command id (dict, list, string or int)
>> +@param cmd_id: command id (dict, list, string or int)
>> """
>> qmp_cmd = {'execute': name}
>> if args:
>> qmp_cmd['arguments'] = args
>> -if id:
>> -qmp_cmd['id'] = id
>> +if cmd_id:
>> +qmp_cmd['id'] = cmd_id
>> return self.cmd_obj(qmp_cmd)
>>
>> def command(self, cmd, **kwds):
>> --
>> 2.9.4
>>
>
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v3 00/14] qemu.py: Pylint/style fixes
Hello guys, I'm reading the available python modules to exercise qemu and while reading them I fixed some issues that caught my attention. It usually starts with a simple pylint/docstring fixes and slowly graduates to more controversial ones so I'm open to suggestion to remove some of them. Kind regards, Lukáš Changes v1->v2 - Squashed 2nd and 10th patches into 2nd one - Use repr() in MonitorResponseError's description - Improved commit message of the 6th patch - Two tweaks to docstrings changed in the 6th patch - Also updated qmp-shell to use new-style super calls (7th patch) - Fixed the accidental rename of qmp `cmd_id` (kept the id->cmd_id patch) - Changed the style of the style-fix in the 10th commit Changes v2->v3 - Don't use repr in the 5th patch in MonitorResponseError Daniel P. Berrange (1): qcow: fix memory leaks related to encryption Kevin Wolf (1): qemu-iotests: Fix reference output for 186 Lukáš Doktor (10): qemu.py: Pylint/style fixes qemu|qtest: Avoid dangerous arguments qemu.py: Use iteritems rather than keys() qemu.py: Simplify QMP key-conversion qemu.py: Use custom exceptions rather than Exception qmp.py: Couple of pylint/style fixes qmp.py: Use object-based class for QEMUMonitorProtocol qmp.py: Avoid "has_key" usage qmp.py: Avoid overriding a builtin object qtest.py: Few pylint/style fixes Peter Maydell (1): Update version for v2.10.0-rc0 release Vladimir Sementsov-Ogievskiy (1): qcow2-bitmap: fix bitmap_free
[Qemu-devel] [PATCH v3 01/14] qcow: fix memory leaks related to encryption
From: "Daniel P. Berrange"
Fix leak of the 'encryptopts' string, which was mistakenly
declared const.
Fix leak of QemuOpts entry which should not have been deleted
from the opts array.
Reported by: coverity
Signed-off-by: Daniel P. Berrange
Message-id: 20170714103105.5781-1-berra...@redhat.com
Reviewed-by: Eric Blake
Signed-off-by: Max Reitz
---
block/qcow.c | 5 +++--
block/qcow2.c | 7 ---
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/block/qcow.c b/block/qcow.c
index 66827d6..c08cdc4 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -768,7 +768,7 @@ static int qcow_create(const char *filename, QemuOpts
*opts, Error **errp)
Error *local_err = NULL;
int ret;
BlockBackend *qcow_blk;
-const char *encryptfmt = NULL;
+char *encryptfmt = NULL;
QDict *options;
QDict *encryptopts = NULL;
QCryptoBlockCreateOptions *crypto_opts = NULL;
@@ -793,7 +793,7 @@ static int qcow_create(const char *filename, QemuOpts
*opts, Error **errp)
goto cleanup;
}
} else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-encryptfmt = "aes";
+encryptfmt = g_strdup("aes");
}
ret = bdrv_create_file(filename, opts, &local_err);
@@ -908,6 +908,7 @@ exit:
blk_unref(qcow_blk);
cleanup:
QDECREF(encryptopts);
+g_free(encryptfmt);
qcrypto_block_free(crypto);
qapi_free_QCryptoBlockCreateOptions(crypto_opts);
g_free(backing_file);
diff --git a/block/qcow2.c b/block/qcow2.c
index 90efa44..d7c600b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2905,7 +2905,7 @@ static int qcow2_create(const char *filename, QemuOpts
*opts, Error **errp)
int version;
uint64_t refcount_bits;
int refcount_order;
-const char *encryptfmt = NULL;
+char *encryptfmt = NULL;
Error *local_err = NULL;
int ret;
@@ -2916,14 +2916,14 @@ static int qcow2_create(const char *filename, QemuOpts
*opts, Error **errp)
backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
if (encryptfmt) {
-if (qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT)) {
+if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
ret = -EINVAL;
goto finish;
}
} else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-encryptfmt = "aes";
+encryptfmt = g_strdup("aes");
}
cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
if (local_err) {
@@ -2983,6 +2983,7 @@ static int qcow2_create(const char *filename, QemuOpts
*opts, Error **errp)
finish:
g_free(backing_file);
g_free(backing_fmt);
+g_free(encryptfmt);
g_free(buf);
return ret;
}
--
2.9.4
[Qemu-devel] [PATCH v3 02/14] qcow2-bitmap: fix bitmap_free
From: Vladimir Sementsov-Ogievskiy
Fix possible crash on error path in
qcow2_remove_persistent_dirty_bitmap. Although bitmap_free was added in
88ddffae8fc the bug was introduced later in commit 469c71edc72 (when
qcow2_remove_persistent_dirty_bitmap was added).
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Eric Blake
Message-id: 20170714123341.373857-1-vsement...@virtuozzo.com
Signed-off-by: Max Reitz
---
block/qcow2-bitmap.c | 4
1 file changed, 4 insertions(+)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3e8735a..e8d3bdb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -487,6 +487,10 @@ static inline void bitmap_directory_to_be(uint8_t *dir,
size_t size)
static void bitmap_free(Qcow2Bitmap *bm)
{
+if (bm == NULL) {
+return;
+}
+
g_free(bm->name);
g_free(bm);
}
--
2.9.4
[Qemu-devel] [PATCH v3 08/14] qemu.py: Simplify QMP key-conversion
The QMP key conversion consist of '_'s to be replaced with '-'s, which
can easily be done by a single `str.replace` method which is faster and
does not require `string` module import.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qemu.py | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7e95c25..5948e19 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,7 +13,6 @@
#
import errno
-import string
import os
import sys
import subprocess
@@ -180,14 +179,12 @@ class QEMUMachine(object):
self._load_io_log()
self._post_shutdown()
-underscore_to_dash = string.maketrans('_', '-')
-
def qmp(self, cmd, conv_keys=True, **args):
'''Invoke a QMP command and return the result dict'''
qmp_args = dict()
for key, value in args.iteritems():
if conv_keys:
-qmp_args[key.translate(self.underscore_to_dash)] = value
+qmp_args[key.replace('_', '-')] = value
else:
qmp_args[key] = value
--
2.9.4
[Qemu-devel] [PATCH v3 04/14] Update version for v2.10.0-rc0 release
From: Peter Maydell Signed-off-by: Peter Maydell --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 12ca1d1..a0700e6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.9.50 +2.9.90 -- 2.9.4
[Qemu-devel] [PATCH v3 09/14] qemu.py: Use custom exceptions rather than Exception
The naked Exception should not be widely used. It makes sense to be a
bit more specific and use better-suited custom exceptions. As a benefit
we can store the full reply in the exception in case someone needs it
when catching the exception.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qemu.py | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 5948e19..2bd522f 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -19,6 +19,19 @@ import subprocess
import qmp.qmp
+class MonitorResponseError(qmp.qmp.QMPError):
+'''
+Represents erroneous QMP monitor reply
+'''
+def __init__(self, reply):
+try:
+desc = reply["error"]["desc"]
+except KeyError:
+desc = reply
+super(MonitorResponseError, self).__init__(desc)
+self.reply = reply
+
+
class QEMUMachine(object):
'''A QEMU VM'''
@@ -197,9 +210,9 @@ class QEMUMachine(object):
'''
reply = self.qmp(cmd, conv_keys, **args)
if reply is None:
-raise Exception("Monitor is closed")
+raise qmp.qmp.QMPError("Monitor is closed")
if "error" in reply:
-raise Exception(reply["error"]["desc"])
+raise MonitorResponseError(reply)
return reply["return"]
def get_qmp_event(self, wait=False):
--
2.9.4
[Qemu-devel] [PATCH v4 08/10] qmp.py: Avoid "has_key" usage
The "has_key" is deprecated in favor of "__in__" operator.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qmp/qmp.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 95ff5cc..f2f5a9b 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -197,7 +197,7 @@ class QEMUMonitorProtocol(object):
Build and send a QMP command to the monitor, report errors if any
"""
ret = self.cmd(cmd, kwds)
-if ret.has_key('error'):
+if "error" in ret:
raise Exception(ret['error']['desc'])
return ret['return']
--
2.9.4
[Qemu-devel] [PATCH v3 10/14] qmp.py: Couple of pylint/style fixes
No actual code changes, just initializing attributes earlier to avoid
AttributeError on early introspection, a few pylint/style fixes and
docstring clarifications.
Signed-off-by: Lukáš Doktor
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qmp/qmp.py | 37 -
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651..782d1ac 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -13,19 +13,30 @@ import errno
import socket
import sys
+
class QMPError(Exception):
pass
+
class QMPConnectError(QMPError):
pass
+
class QMPCapabilitiesError(QMPError):
pass
+
class QMPTimeoutError(QMPError):
pass
+
class QEMUMonitorProtocol:
+
+#: Socket's error class
+error = socket.error
+#: Socket's timeout
+timeout = socket.timeout
+
def __init__(self, address, server=False, debug=False):
"""
Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
self.__address = address
self._debug = debug
self.__sock = self.__get_sock()
+self.__sockfile = None
if server:
self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
self.__sock.bind(self.__address)
@@ -56,7 +68,7 @@ class QEMUMonitorProtocol:
def __negotiate_capabilities(self):
greeting = self.__json_read()
-if greeting is None or not greeting.has_key('QMP'):
+if greeting is None or "QMP" not in greeting:
raise QMPConnectError
# Greeting seems ok, negotiate capabilities
resp = self.cmd('qmp_capabilities')
@@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
continue
return resp
-error = socket.error
-
def __get_events(self, wait=False):
"""
Check for new events in the stream and cache them in __events.
@@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
"""
# Check for new events regardless and pull them into the cache:
@@ -175,7 +185,7 @@ class QEMUMonitorProtocol:
@param args: command arguments (dict)
@param id: command id (dict, list, string or int)
"""
-qmp_cmd = { 'execute': name }
+qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
if id:
@@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
+"""
+Build and send a QMP command to the monitor, report errors if any
+"""
ret = self.cmd(cmd, kwds)
if ret.has_key('error'):
raise Exception(ret['error']['desc'])
@@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
def pull_event(self, wait=False):
"""
-Get and delete the first available QMP event.
+Pulls a single event.
@param wait (bool): block until an event is available.
@param wait (float): If wait is a float, treat it as a timeout value.
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The first available QMP event, or None.
"""
@@ -217,8 +230,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The list of available QMP events.
"""
@@ -235,8 +248,6 @@ class QEMUMonitorProtocol:
self.__sock.close()
self.__sockfile.close()
-timeout = socket.timeout
-
def settimeout(self, timeout):
self.__sock.settimeout(timeout)
--
2.9.4
[Qemu-devel] [PATCH v3 03/14] qemu-iotests: Fix reference output for 186
From: Kevin Wolf
Commits 70f17a1 ('error: Revert unwanted change of warning messages')
and e1824e5 ('qemu-iotests: Test 'info block'') had a semantic merge
conflict, which results in failure for qemu-iotests case 186. Fix the
reference output to consider the changes of 70f17a1.
Signed-off-by: Kevin Wolf
Message-id: 1500973176-29235-1-git-send-email-kw...@redhat.com
Reviewed-by: Markus Armbruster
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Max Reitz
---
tests/qemu-iotests/186.out | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/186.out b/tests/qemu-iotests/186.out
index b963b12..b8bf9a2 100644
--- a/tests/qemu-iotests/186.out
+++ b/tests/qemu-iotests/186.out
@@ -442,7 +442,7 @@ ide0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
Cache mode: writeback
(qemu) quit
-warning: qemu-system-x86_64: -drive if=scsi,driver=null-co: bus=0,unit=0 is
deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is
deprecated with this machine type
Testing: -drive if=scsi,driver=null-co
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) info block
@@ -451,7 +451,7 @@ scsi0-hd0 (NODE_NAME): null-co:// (null-co)
Cache mode: writeback
(qemu) quit
-warning: qemu-system-x86_64: -drive if=scsi,media=cdrom: bus=0,unit=0 is
deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is
deprecated with this machine type
Testing: -drive if=scsi,media=cdrom
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) info block
@@ -460,7 +460,7 @@ scsi0-cd0: [not inserted]
Removable device: not locked, tray closed
(qemu) quit
-warning: qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom:
bus=0,unit=0 is deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: warning:
bus=0,unit=0 is deprecated with this machine type
Testing: -drive if=scsi,driver=null-co,media=cdrom
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) info block
--
2.9.4
[Qemu-devel] [PATCH v3 07/14] qemu.py: Use iteritems rather than keys()
Let's avoid creating an in-memory list of keys and query for each value and use `iteritems` which is an iterator of key-value pairs. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost Reviewed-by: Philippe Mathieu-Daudé --- scripts/qemu.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 66fd863..7e95c25 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -185,11 +185,11 @@ class QEMUMachine(object): def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the result dict''' qmp_args = dict() -for key in args.keys(): +for key, value in args.iteritems(): if conv_keys: -qmp_args[key.translate(self.underscore_to_dash)] = args[key] +qmp_args[key.translate(self.underscore_to_dash)] = value else: -qmp_args[key] = args[key] +qmp_args[key] = value return self._qmp.cmd(cmd, args=qmp_args) -- 2.9.4
[Qemu-devel] [PATCH v3 13/14] qmp.py: Avoid overriding a builtin object
The "id" is a builtin method to get object's identity and should not be
overridden. This might bring some issues in case someone was directly
calling "cmd(..., id=id)" but I haven't found such usage on brief search
for "cmd\(.*id=".
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qmp/qmp.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index f2f5a9b..ef12e8a 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
print >>sys.stderr, "QMP:<<< %s" % resp
return resp
-def cmd(self, name, args=None, id=None):
+def cmd(self, name, args=None, cmd_id=None):
"""
Build a QMP command and send it to the QMP Monitor.
@param name: command name (string)
@param args: command arguments (dict)
-@param id: command id (dict, list, string or int)
+@param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
-if id:
-qmp_cmd['id'] = id
+if cmd_id:
+qmp_cmd['id'] = cmd_id
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
--
2.9.4
[Qemu-devel] [PATCH v3 14/14] qtest.py: Few pylint/style fixes
No actual code changes, just few pylint/style fixes. Signed-off-by: Lukáš Doktor Reviewed-by: John Snow --- scripts/qtest.py | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/scripts/qtest.py b/scripts/qtest.py index ab183c0..df0daf2 100644 --- a/scripts/qtest.py +++ b/scripts/qtest.py @@ -11,14 +11,11 @@ # Based on qmp.py. # -import errno import socket -import string import os -import subprocess -import qmp.qmp import qemu + class QEMUQtestProtocol(object): def __init__(self, address, server=False): """ @@ -83,8 +80,10 @@ class QEMUQtestMachine(qemu.QEMUMachine): socket_scm_helper=None): if name is None: name = "qemu-%d" % os.getpid() -super(QEMUQtestMachine, self).__init__(binary, args, name=name, test_dir=test_dir, - socket_scm_helper=socket_scm_helper) +super(QEMUQtestMachine, + self).__init__(binary, args, name=name, test_dir=test_dir, + socket_scm_helper=socket_scm_helper) +self._qtest = None self._qtest_path = os.path.join(test_dir, name + "-qtest.sock") def _base_args(self): -- 2.9.4
[Qemu-devel] [PATCH v3 06/14] qemu|qtest: Avoid dangerous arguments
The list object is mutable in python and potentially might modify other
object's arguments when used as default argument. Reproducer:
>>> vm1 = QEMUMachine("qemu")
>>> vm2 = QEMUMachine("qemu")
>>> vm1._wrapper.append("foo")
>>> print vm2._wrapper
['foo']
In this case the `args` is actually copied so it would be safe to keep
it, but it's not a good practice to keep it. The same issue applies in
inherited qtest module.
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
Reviewed-by: John Snow
---
scripts/qemu.py | 6 +-
scripts/qtest.py | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 191c916..66fd863 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,7 +23,7 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
+def __init__(self, binary, args=None, wrapper=None, name=None,
test_dir="/var/tmp", monitor_address=None,
socket_scm_helper=None, debug=False):
'''
@@ -39,6 +39,10 @@ class QEMUMachine(object):
@param debug: enable debug mode (forwarded to QMP helper and such)
@note: Qemu process is not started until launch() is used.
'''
+if args is None:
+args = []
+if wrapper is None:
+wrapper = []
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
diff --git a/scripts/qtest.py b/scripts/qtest.py
index d5aecb5..ab183c0 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -79,7 +79,7 @@ class QEMUQtestProtocol(object):
class QEMUQtestMachine(qemu.QEMUMachine):
'''A QEMU VM'''
-def __init__(self, binary, args=[], name=None, test_dir="/var/tmp",
+def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
socket_scm_helper=None):
if name is None:
name = "qemu-%d" % os.getpid()
--
2.9.4
[Qemu-devel] [PATCH v3 11/14] qmp.py: Use object-based class for QEMUMonitorProtocol
There is no need to define QEMUMonitorProtocol as old-style class. Signed-off-by: Lukáš Doktor Reviewed-by: Eduardo Habkost --- scripts/qmp/qmp-shell | 4 ++-- scripts/qmp/qmp.py| 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell index 860ffb2..be449de 100755 --- a/scripts/qmp/qmp-shell +++ b/scripts/qmp/qmp-shell @@ -106,7 +106,7 @@ class FuzzyJSON(ast.NodeTransformer): # _execute_cmd()). Let's design a better one. class QMPShell(qmp.QEMUMonitorProtocol): def __init__(self, address, pretty=False): -qmp.QEMUMonitorProtocol.__init__(self, self.__get_address(address)) +super(QMPShell, self).__init__(self.__get_address(address)) self._greeting = None self._completer = None self._pretty = pretty @@ -281,7 +281,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): return True def connect(self, negotiate): -self._greeting = qmp.QEMUMonitorProtocol.connect(self, negotiate) +self._greeting = super(QMPShell, self).connect(negotiate) self.__completer_setup() def show_banner(self, msg='Welcome to the QMP low-level shell!'): diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index 782d1ac..95ff5cc 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError): pass -class QEMUMonitorProtocol: +class QEMUMonitorProtocol(object): #: Socket's error class error = socket.error -- 2.9.4
[Qemu-devel] [PATCH v4 09/10] qmp.py: Avoid overriding a builtin object
The "id" is a builtin method to get object's identity and should not be
overridden. This might bring some issues in case someone was directly
calling "cmd(..., id=id)" but I haven't found such usage on brief search
for "cmd\(.*id=".
Signed-off-by: Lukáš Doktor
Reviewed-by: Eduardo Habkost
---
scripts/qmp/qmp.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index f2f5a9b..ef12e8a 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
print >>sys.stderr, "QMP:<<< %s" % resp
return resp
-def cmd(self, name, args=None, id=None):
+def cmd(self, name, args=None, cmd_id=None):
"""
Build a QMP command and send it to the QMP Monitor.
@param name: command name (string)
@param args: command arguments (dict)
-@param id: command id (dict, list, string or int)
+@param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
-if id:
-qmp_cmd['id'] = id
+if cmd_id:
+qmp_cmd['id'] = cmd_id
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
--
2.9.4
