Re: [RFC PATCH 2/3] hw/sd/sdhci: Prohibit DMA accesses to devices
On Thu, Dec 16, 2021 at 4:57 AM Philippe Mathieu-Daudé
wrote:
>
> From: Philippe Mathieu-Daudé
>
> The issue reported by OSS-Fuzz produces the following backtrace:
>
> ==447470==ERROR: AddressSanitizer: heap-buffer-overflow
> READ of size 1 at 0x6152a080 thread T0
> #0 0x71766d47 in sdhci_read_dataport hw/sd/sdhci.c:474:18
> #1 0x7175f139 in sdhci_read hw/sd/sdhci.c:1022:19
> #2 0x721b937b in memory_region_read_accessor softmmu/memory.c:440:11
> #3 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
> #4 0x7216f47c in memory_region_dispatch_read1 softmmu/memory.c:1424:16
> #5 0x7216ebb9 in memory_region_dispatch_read softmmu/memory.c:1452:9
> #6 0x7212db5d in flatview_read_continue softmmu/physmem.c:2879:23
> #7 0x7212f958 in flatview_read softmmu/physmem.c:2921:12
> #8 0x7212f418 in address_space_read_full softmmu/physmem.c:2934:18
> #9 0x721305a9 in address_space_rw softmmu/physmem.c:2962:16
> #10 0x7175a392 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
> #11 0x7175a0ea in dma_memory_rw include/sysemu/dma.h:132:12
> #12 0x71759684 in dma_memory_read include/sysemu/dma.h:152:12
> #13 0x7175518c in sdhci_do_adma hw/sd/sdhci.c:823:27
> #14 0x7174bf69 in sdhci_data_transfer hw/sd/sdhci.c:935:13
> #15 0x7176aaa7 in sdhci_send_command hw/sd/sdhci.c:376:9
> #16 0x717629ee in sdhci_write hw/sd/sdhci.c:1212:9
> #17 0x72172513 in memory_region_write_accessor softmmu/memory.c:492:5
> #18 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
> #19 0x72170766 in memory_region_dispatch_write softmmu/memory.c:1504:16
> #20 0x721419ee in flatview_write_continue softmmu/physmem.c:2812:23
> #21 0x721301eb in flatview_write softmmu/physmem.c:2854:12
> #22 0x7212fca8 in address_space_write softmmu/physmem.c:2950:18
> #23 0x721d9a53 in qtest_process_command softmmu/qtest.c:727:9
>
> A DMA descriptor is previously filled in RAM. An I/O access to the
> device (frames #22 to #16) start the DMA engine (frame #13). The
> engine fetch the descriptor and execute the request, which itself
> accesses the SDHCI I/O registers (frame #1 and #0), triggering a
> re-entrancy issue.
>
> Fix by prohibit transactions from the DMA to devices. The DMA engine
> is thus restricted to memories.
>
> Reported-by: OSS-Fuzz (Issue 36391)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/451
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sdhci.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index fe2f21f0c37..0e5e988927e 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -741,6 +741,7 @@ static void sdhci_do_adma(SDHCIState *s)
> {
> unsigned int begin, length;
> const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
> +const MemTxAttrs attrs = { .memory = true };
> ADMADescr dscr = {};
> MemTxResult res;
> int i;
> @@ -794,7 +795,7 @@ static void sdhci_do_adma(SDHCIState *s)
> res = dma_memory_write(s->dma_as, dscr.addr,
> >fifo_buffer[begin],
> s->data_count - begin,
> - MEMTXATTRS_UNSPECIFIED);
> + attrs);
> if (res != MEMTX_OK) {
> break;
> }
> @@ -823,7 +824,7 @@ static void sdhci_do_adma(SDHCIState *s)
> res = dma_memory_read(s->dma_as, dscr.addr,
>>fifo_buffer[begin],
>s->data_count - begin,
> - MEMTXATTRS_UNSPECIFIED);
> + attrs);
> if (res != MEMTX_OK) {
> break;
> }
I wonder how we can fix this for other devices, as this seems to be a
known issue for many years. We've received many reports from the
networking side.
It looks like this patch simply forbids p2p which is probably not the
case for other devices.
I remember there's ideas like using bh from Paolo or detecting
reentrancy in the memory core, both of them seems more general than
this?
Thanks
> --
> 2.33.1
>
[RFC PATCH 3/3] tests/qtest/fuzz-sdcard-test: Add reproducer for OSS-Fuzz (Issue 29225)
Include the qtest reproducer provided by Alexander Bulekov
in https://gitlab.com/qemu-project/qemu/-/issues/451. Without
the previous commit, we get:
$ make check-qtest-i386
...
Running test qtest-i386/fuzz-sdcard-test
==447470==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x6152a080 at pc 0x564c71766d48 bp 0x7ffc126c62b0 sp 0x7ffc126c62a8
READ of size 1 at 0x6152a080 thread T0
#0 0x564c71766d47 in sdhci_read_dataport hw/sd/sdhci.c:474:18
#1 0x564c7175f139 in sdhci_read hw/sd/sdhci.c:1022:19
#2 0x564c721b937b in memory_region_read_accessor softmmu/memory.c:440:11
#3 0x564c72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
#4 0x564c7216f47c in memory_region_dispatch_read1 softmmu/memory.c:1424:16
#5 0x564c7216ebb9 in memory_region_dispatch_read softmmu/memory.c:1452:9
#6 0x564c7212db5d in flatview_read_continue softmmu/physmem.c:2879:23
#7 0x564c7212f958 in flatview_read softmmu/physmem.c:2921:12
#8 0x564c7212f418 in address_space_read_full softmmu/physmem.c:2934:18
#9 0x564c721305a9 in address_space_rw softmmu/physmem.c:2962:16
#10 0x564c7175a392 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
#11 0x564c7175a0ea in dma_memory_rw include/sysemu/dma.h:132:12
#12 0x564c71759684 in dma_memory_read include/sysemu/dma.h:152:12
#13 0x564c7175518c in sdhci_do_adma hw/sd/sdhci.c:823:27
#14 0x564c7174bf69 in sdhci_data_transfer hw/sd/sdhci.c:935:13
#15 0x564c7176aaa7 in sdhci_send_command hw/sd/sdhci.c:376:9
#16 0x564c717629ee in sdhci_write hw/sd/sdhci.c:1212:9
#17 0x564c72172513 in memory_region_write_accessor softmmu/memory.c:492:5
#18 0x564c72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
#19 0x564c72170766 in memory_region_dispatch_write
softmmu/memory.c:1504:16
#20 0x564c721419ee in flatview_write_continue softmmu/physmem.c:2812:23
#21 0x564c721301eb in flatview_write softmmu/physmem.c:2854:12
#22 0x564c7212fca8 in address_space_write softmmu/physmem.c:2950:18
#23 0x564c721d9a53 in qtest_process_command softmmu/qtest.c:727:9
0x6152a080 is located 0 bytes to the right of 512-byte region
[0x61529e80,0x6152a080)
allocated by thread T0 here:
#0 0x564c708e1737 in __interceptor_calloc (qemu-system-i386+0x1e6a737)
#1 0x7ff05567b5e0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5a5e0)
#2 0x564c71774adb in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5
SUMMARY: AddressSanitizer: heap-buffer-overflow hw/sd/sdhci.c:474:18 in
sdhci_read_dataport
Shadow bytes around the buggy address:
0x0c2a7fffd3c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c2a7fffd3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c2a7fffd3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c2a7fffd3f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c2a7fffd400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c2a7fffd410:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c2a7fffd420: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffd430: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffd440: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffd450: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffd460: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Heap left redzone: fa
Freed heap region: fd
==447470==ABORTING
Broken pipe
ERROR qtest-i386/fuzz-sdcard-test - too few tests run (expected 3, got 2)
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/fuzz-sdcard-test.c | 76 ++
1 file changed, 76 insertions(+)
diff --git a/tests/qtest/fuzz-sdcard-test.c b/tests/qtest/fuzz-sdcard-test.c
index ae14305344a..6dfe26e983c 100644
--- a/tests/qtest/fuzz-sdcard-test.c
+++ b/tests/qtest/fuzz-sdcard-test.c
@@ -87,6 +87,81 @@ static void oss_fuzz_36217(void)
qtest_quit(s);
}
+/*
+ * https://gitlab.com/qemu-project/qemu/-/issues/451
+ * Used to trigger a heap buffer overflow.
+ */
+static void oss_fuzz_36391(void)
+{
+QTestState *s;
+
+s = qtest_init(" -display none -m 4G -nodefaults -nographic"
+ " -device sdhci-pci,sd-spec-version=3"
+ " -device sd-card,drive=drv"
+ " -drive
if=none,index=0,file=null-co://,format=raw,id=drv");
+qtest_outl(s, 0xcf8, 0x80001010);
+qtest_outl(s, 0xcfc, 0xe000);
+qtest_outl(s, 0xcf8, 0x80001004);
+qtest_outw(s, 0xcfc, 0x7);
+qtest_bufwrite(s, 0xe005, "\x73", 0x1);
+qtest_bufwrite(s, 0xe028, "\x55", 0x1);
+qtest_bufwrite(s, 0xe02c, "\x55", 0x1);
+qtest_bufwrite(s, 0x0, "\x65", 0x1);
+qtest_bufwrite(s, 0x7, "\x69", 0x1);
+qtest_bufwrite(s, 0x8, "\x65", 0x1);
+qtest_bufwrite(s, 0xf, "\x69", 0x1);
+qtest_bufwrite(s,
[RFC PATCH 2/3] hw/sd/sdhci: Prohibit DMA accesses to devices
From: Philippe Mathieu-Daudé
The issue reported by OSS-Fuzz produces the following backtrace:
==447470==ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 1 at 0x6152a080 thread T0
#0 0x71766d47 in sdhci_read_dataport hw/sd/sdhci.c:474:18
#1 0x7175f139 in sdhci_read hw/sd/sdhci.c:1022:19
#2 0x721b937b in memory_region_read_accessor softmmu/memory.c:440:11
#3 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
#4 0x7216f47c in memory_region_dispatch_read1 softmmu/memory.c:1424:16
#5 0x7216ebb9 in memory_region_dispatch_read softmmu/memory.c:1452:9
#6 0x7212db5d in flatview_read_continue softmmu/physmem.c:2879:23
#7 0x7212f958 in flatview_read softmmu/physmem.c:2921:12
#8 0x7212f418 in address_space_read_full softmmu/physmem.c:2934:18
#9 0x721305a9 in address_space_rw softmmu/physmem.c:2962:16
#10 0x7175a392 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
#11 0x7175a0ea in dma_memory_rw include/sysemu/dma.h:132:12
#12 0x71759684 in dma_memory_read include/sysemu/dma.h:152:12
#13 0x7175518c in sdhci_do_adma hw/sd/sdhci.c:823:27
#14 0x7174bf69 in sdhci_data_transfer hw/sd/sdhci.c:935:13
#15 0x7176aaa7 in sdhci_send_command hw/sd/sdhci.c:376:9
#16 0x717629ee in sdhci_write hw/sd/sdhci.c:1212:9
#17 0x72172513 in memory_region_write_accessor softmmu/memory.c:492:5
#18 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
#19 0x72170766 in memory_region_dispatch_write softmmu/memory.c:1504:16
#20 0x721419ee in flatview_write_continue softmmu/physmem.c:2812:23
#21 0x721301eb in flatview_write softmmu/physmem.c:2854:12
#22 0x7212fca8 in address_space_write softmmu/physmem.c:2950:18
#23 0x721d9a53 in qtest_process_command softmmu/qtest.c:727:9
A DMA descriptor is previously filled in RAM. An I/O access to the
device (frames #22 to #16) start the DMA engine (frame #13). The
engine fetch the descriptor and execute the request, which itself
accesses the SDHCI I/O registers (frame #1 and #0), triggering a
re-entrancy issue.
Fix by prohibit transactions from the DMA to devices. The DMA engine
is thus restricted to memories.
Reported-by: OSS-Fuzz (Issue 36391)
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/451
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sdhci.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index fe2f21f0c37..0e5e988927e 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -741,6 +741,7 @@ static void sdhci_do_adma(SDHCIState *s)
{
unsigned int begin, length;
const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
+const MemTxAttrs attrs = { .memory = true };
ADMADescr dscr = {};
MemTxResult res;
int i;
@@ -794,7 +795,7 @@ static void sdhci_do_adma(SDHCIState *s)
res = dma_memory_write(s->dma_as, dscr.addr,
>fifo_buffer[begin],
s->data_count - begin,
- MEMTXATTRS_UNSPECIFIED);
+ attrs);
if (res != MEMTX_OK) {
break;
}
@@ -823,7 +824,7 @@ static void sdhci_do_adma(SDHCIState *s)
res = dma_memory_read(s->dma_as, dscr.addr,
>fifo_buffer[begin],
s->data_count - begin,
- MEMTXATTRS_UNSPECIFIED);
+ attrs);
if (res != MEMTX_OK) {
break;
}
--
2.33.1
[RFC PATCH 1/3] hw/sd/sdhci: Honor failed DMA transactions
From: Philippe Mathieu-Daudé
DMA transactions might fail. The DMA API returns a MemTxResult,
indicating such failures. Do not ignore it. On failure, raise
the ADMA error flag and eventually triggering an IRQ (see spec
chapter 1.13.5: "ADMA2 States").
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sdhci.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index e0bbc903446..fe2f21f0c37 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -742,6 +742,7 @@ static void sdhci_do_adma(SDHCIState *s)
unsigned int begin, length;
const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
ADMADescr dscr = {};
+MemTxResult res;
int i;
if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) {
@@ -790,10 +791,13 @@ static void sdhci_do_adma(SDHCIState *s)
s->data_count = block_size;
length -= block_size - begin;
}
-dma_memory_write(s->dma_as, dscr.addr,
- >fifo_buffer[begin],
- s->data_count - begin,
- MEMTXATTRS_UNSPECIFIED);
+res = dma_memory_write(s->dma_as, dscr.addr,
+ >fifo_buffer[begin],
+ s->data_count - begin,
+ MEMTXATTRS_UNSPECIFIED);
+if (res != MEMTX_OK) {
+break;
+}
dscr.addr += s->data_count - begin;
if (s->data_count == block_size) {
s->data_count = 0;
@@ -816,10 +820,13 @@ static void sdhci_do_adma(SDHCIState *s)
s->data_count = block_size;
length -= block_size - begin;
}
-dma_memory_read(s->dma_as, dscr.addr,
->fifo_buffer[begin],
-s->data_count - begin,
-MEMTXATTRS_UNSPECIFIED);
+res = dma_memory_read(s->dma_as, dscr.addr,
+ >fifo_buffer[begin],
+ s->data_count - begin,
+ MEMTXATTRS_UNSPECIFIED);
+if (res != MEMTX_OK) {
+break;
+}
dscr.addr += s->data_count - begin;
if (s->data_count == block_size) {
sdbus_write_data(>sdbus, s->fifo_buffer,
block_size);
@@ -833,7 +840,16 @@ static void sdhci_do_adma(SDHCIState *s)
}
}
}
-s->admasysaddr += dscr.incr;
+if (res != MEMTX_OK) {
+if (s->errintstsen & SDHC_EISEN_ADMAERR) {
+trace_sdhci_error("Set ADMA error flag");
+s->errintsts |= SDHC_EIS_ADMAERR;
+s->norintsts |= SDHC_NIS_ERR;
+}
+sdhci_update_irq(s);
+} else {
+s->admasysaddr += dscr.incr;
+}
break;
case SDHC_ADMA_ATTR_ACT_LINK: /* link to next descriptor table */
s->admasysaddr = dscr.addr;
--
2.33.1
Re: [PATCH 0/3] hw/sd/sdhci: Fix DMA re-entrancy issue
On 12/15/21 21:55, Philippe Mathieu-Daudé wrote: > Hi, > > This series is an attempt to fix the DMA re-entrancy problem > on the SDHCI device. OSS-Fuzz found it and Alexander generated > a helpful reproducer. > > By setting the MemTxAttrs::memory bit before doing DMA transactions, > the flatview API will return MEMTX_BUS_ERROR if the transaction > targets a non-memory (a device), which is usually how DMA-reentrancy > bugs are exploited. > > On real hardware, the checks are on the interconnect bus, not in > the SDHCI block. However QEMU blocks aren't modelled that way. > Using the flatview API seems (to me) the simplest and closer > to hardware, it is a generic API and we can use it to trace > bus transactions on all blocks. > > Note this series is simply one example to fix the generic > issues. The important changes are in the previous series: > https://lore.kernel.org/qemu-devel/20211215182421.418374-1-phi...@redhat.com/ > Based-on: <20211215182421.418374-1-phi...@redhat.com> > "physmem: Have flatview API check bus permission from MemTxAttrs" Please disregard this cover, I forgot to post as RFC...
[RFC PATCH 0/3] hw/sd/sdhci: Fix DMA re-entrancy issue
Hi, This series is an attempt to fix the DMA re-entrancy problem on the SDHCI device. OSS-Fuzz found it and Alexander generated a helpful reproducer. By setting the MemTxAttrs::memory bit before doing DMA transactions, the flatview API will return MEMTX_BUS_ERROR if the transaction targets a non-memory (a device), which is usually how DMA-reentrancy bugs are exploited. On real hardware, the checks are on the interconnect bus, not in the SDHCI block. However QEMU blocks aren't modelled that way. Using the flatview API seems (to me) the simplest and closer to hardware, it is a generic API and we can use it to trace bus transactions on all blocks. Note this series is simply one example to fix the generic issues. The important changes are in the previous series: https://lore.kernel.org/qemu-devel/20211215182421.418374-1-phi...@redhat.com/ Based-on: <20211215182421.418374-1-phi...@redhat.com> "physmem: Have flatview API check bus permission from MemTxAttrs" Cc: Mauro Matteo Cascella Cc: Qiuhao Li Cc: Peter Xu Cc: Jason Wang Cc: David Hildenbrand Cc: Gerd Hoffmann Cc: Peter Maydell Cc: Li Qiang Cc: Thomas Huth Cc: Laurent Vivier Cc: Bandan Das Cc: Edgar E. Iglesias Cc: Darren Kenny Cc: Bin Meng Cc: Paolo Bonzini Cc: Alexander Bulekov Cc: Stefan Hajnoczi Philippe Mathieu-Daudé (3): hw/sd/sdhci: Honor failed DMA transactions hw/sd/sdhci: Prohibit DMA accesses to devices tests/qtest/fuzz-sdcard-test: Add reproducer for OSS-Fuzz (Issue 29225) hw/sd/sdhci.c | 35 tests/qtest/fuzz-sdcard-test.c | 76 ++ 2 files changed, 102 insertions(+), 9 deletions(-) -- 2.33.1
[PATCH 0/3] hw/sd/sdhci: Fix DMA re-entrancy issue
Hi, This series is an attempt to fix the DMA re-entrancy problem on the SDHCI device. OSS-Fuzz found it and Alexander generated a helpful reproducer. By setting the MemTxAttrs::memory bit before doing DMA transactions, the flatview API will return MEMTX_BUS_ERROR if the transaction targets a non-memory (a device), which is usually how DMA-reentrancy bugs are exploited. On real hardware, the checks are on the interconnect bus, not in the SDHCI block. However QEMU blocks aren't modelled that way. Using the flatview API seems (to me) the simplest and closer to hardware, it is a generic API and we can use it to trace bus transactions on all blocks. Note this series is simply one example to fix the generic issues. The important changes are in the previous series: https://lore.kernel.org/qemu-devel/20211215182421.418374-1-phi...@redhat.com/ Based-on: <20211215182421.418374-1-phi...@redhat.com> "physmem: Have flatview API check bus permission from MemTxAttrs" Cc: Mauro Matteo Cascella Cc: Qiuhao Li Cc: Peter Xu Cc: Jason Wang Cc: David Hildenbrand Cc: Gerd Hoffmann Cc: Peter Maydell Cc: Li Qiang Cc: Thomas Huth Cc: Laurent Vivier Cc: Bandan Das Cc: Edgar E. Iglesias Cc: Darren Kenny Cc: Bin Meng Cc: Paolo Bonzini Cc: Alexander Bulekov Cc: Stefan Hajnoczi Philippe Mathieu-Daudé (3): hw/sd/sdhci: Honor failed DMA transactions hw/sd/sdhci: Prohibit DMA accesses to devices tests/qtest/fuzz-sdcard-test: Add reproducer for OSS-Fuzz (Issue 29225) hw/sd/sdhci.c | 35 tests/qtest/fuzz-sdcard-test.c | 76 ++ 2 files changed, 102 insertions(+), 9 deletions(-) -- 2.33.1
[PATCH v2 23/25] python: remove the old QMP package
Thank you for your service!
Signed-off-by: John Snow
---
python/PACKAGE.rst | 4 +-
python/README.rst | 2 +-
python/qemu/qmp/README.rst | 9 -
python/qemu/qmp/__init__.py | 396
python/qemu/qmp/py.typed| 0
python/setup.cfg| 3 +-
6 files changed, 4 insertions(+), 410 deletions(-)
delete mode 100644 python/qemu/qmp/README.rst
delete mode 100644 python/qemu/qmp/__init__.py
delete mode 100644 python/qemu/qmp/py.typed
diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index b0b86cc4c3..ddfa9ba3f5 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
Usage
-
-The ``qemu.qmp`` subpackage provides a library for communicating with
+The ``qemu.aqmp`` subpackage provides a library for communicating with
QMP servers. The ``qemu.machine`` subpackage offers rudimentary
facilities for launching and managing QEMU processes. Refer to each
package's documentation
-(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
for more information.
Contributing
diff --git a/python/README.rst b/python/README.rst
index fcf74f69ea..eb5213337d 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
This directory houses Python tooling used by the QEMU project to build,
configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
``setup.py`` is used by ``pip`` to install this tooling to the current
environment. ``setup.cfg`` provides the packaging configuration used by
diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
deleted file mode 100644
index 5bfb82535f..00
--- a/python/qemu/qmp/README.rst
+++ /dev/null
@@ -1,9 +0,0 @@
-qemu.qmp package
-
-
-This package provides a library used for connecting to and communicating
-with QMP servers. It is used extensively by iotests, vm tests,
-avocado tests, and other utilities in the ./scripts directory. It is
-not a fully-fledged SDK and is subject to change at any time.
-
-See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
deleted file mode 100644
index 4e08641154..00
--- a/python/qemu/qmp/__init__.py
+++ /dev/null
@@ -1,396 +0,0 @@
-"""
-QEMU Monitor Protocol (QMP) development library & tooling.
-
-This package provides a fairly low-level class for communicating to QMP
-protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
-QEMU Storage Daemon. This library is not intended for production use.
-
-`QEMUMonitorProtocol` is the primary class of interest, and all errors
-raised derive from `QMPError`.
-"""
-
-# Copyright (C) 2009, 2010 Red Hat Inc.
-#
-# Authors:
-# Luiz Capitulino
-#
-# This work is licensed under the terms of the GNU GPL, version 2. See
-# the COPYING file in the top-level directory.
-
-import errno
-import json
-import logging
-import socket
-import struct
-from types import TracebackType
-from typing import (
-Any,
-Dict,
-List,
-Optional,
-TextIO,
-Tuple,
-Type,
-TypeVar,
-Union,
-cast,
-)
-
-
-#: QMPMessage is an entire QMP message of any kind.
-QMPMessage = Dict[str, Any]
-
-#: QMPReturnValue is the 'return' value of a command.
-QMPReturnValue = object
-
-#: QMPObject is any object in a QMP message.
-QMPObject = Dict[str, object]
-
-# QMPMessage can be outgoing commands or incoming events/returns.
-# QMPReturnValue is usually a dict/json object, but due to QAPI's
-# 'returns-whitelist', it can actually be anything.
-#
-# {'return': {}} is a QMPMessage,
-# {} is the QMPReturnValue.
-
-
-InternetAddrT = Tuple[str, int]
-UnixAddrT = str
-SocketAddrT = Union[InternetAddrT, UnixAddrT]
-
-
-class QMPError(Exception):
-"""
-QMP base exception
-"""
-
-
-class QMPConnectError(QMPError):
-"""
-QMP connection exception
-"""
-
-
-class QMPCapabilitiesError(QMPError):
-"""
-QMP negotiate capabilities exception
-"""
-
-
-class QMPTimeoutError(QMPError):
-"""
-QMP timeout exception
-"""
-
-
-class QMPProtocolError(QMPError):
-"""
-QMP protocol error; unexpected response
-"""
-
-
-class QMPResponseError(QMPError):
-"""
-Represents erroneous QMP monitor reply
-"""
-def __init__(self, reply: QMPMessage):
-try:
-desc = reply['error']['desc']
-except KeyError:
-desc = reply
-super().__init__(desc)
-self.reply = reply
-
-
-class QEMUMonitorProtocol:
-"""
-Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
-allow to handle commands and events.
-"""
-
-#: Logger object for debugging messages
-logger = logging.getLogger('QMP')
-
-def __init__(self,
[PATCH v2 20/25] python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
Shift these definitions over from the qmp package to the async qmp
package.
Signed-off-by: John Snow
---
python/qemu/aqmp/aqmp_tui.py | 2 +-
python/qemu/aqmp/legacy.py | 30 ++
python/qemu/qmp/__init__.py | 26 --
3 files changed, 27 insertions(+), 31 deletions(-)
diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index a2929f771c..184a3e4690 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -35,8 +35,8 @@
import urwid
import urwid_readline
-from ..qmp import QEMUMonitorProtocol, QMPBadPortError
from .error import ProtocolError
+from .legacy import QEMUMonitorProtocol, QMPBadPortError
from .message import DeserializationError, Message, UnexpectedTypeError
from .protocol import ConnectError, Runstate
from .qmp_client import ExecInterruptedError, QMPClient
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 0890f95b16..76b09671cc 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -22,9 +22,6 @@
from .qmp_client import QMPClient
-# (Temporarily) Re-export QMPBadPortError
-QMPBadPortError = qemu.qmp.QMPBadPortError
-
#: QMPMessage is an entire QMP message of any kind.
QMPMessage = Dict[str, Any]
@@ -45,6 +42,12 @@
# pylint: disable=missing-docstring
+class QMPBadPortError(QMPError):
+"""
+Unable to parse socket address: Port was non-numerical.
+"""
+
+
class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
def __init__(self, address: SocketAddrT,
server: bool = False,
@@ -72,7 +75,26 @@ def _get_greeting(self) -> Optional[QMPMessage]:
return None
# __enter__ and __exit__ need no changes
-# parse_address needs no changes
+
+@classmethod
+def parse_address(cls, address: str) -> SocketAddrT:
+"""
+Parse a string into a QMP address.
+
+Figure out if the argument is in the port:host form.
+If it's not, it's probably a file path.
+"""
+components = address.split(':')
+if len(components) == 2:
+try:
+port = int(components[1])
+except ValueError:
+msg = f"Bad port: '{components[1]}' in '{address}'."
+raise QMPBadPortError(msg) from None
+return (components[0], port)
+
+# Treat as filepath.
+return address
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
self._aqmp.await_greeting = negotiate
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index 358c0971d0..4e08641154 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -102,12 +102,6 @@ def __init__(self, reply: QMPMessage):
self.reply = reply
-class QMPBadPortError(QMPError):
-"""
-Unable to parse socket address: Port was non-numerical.
-"""
-
-
class QEMUMonitorProtocol:
"""
Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
@@ -237,26 +231,6 @@ def __exit__(self,
# Implement context manager exit function.
self.close()
-@classmethod
-def parse_address(cls, address: str) -> SocketAddrT:
-"""
-Parse a string into a QMP address.
-
-Figure out if the argument is in the port:host form.
-If it's not, it's probably a file path.
-"""
-components = address.split(':')
-if len(components) == 2:
-try:
-port = int(components[1])
-except ValueError:
-msg = f"Bad port: '{components[1]}' in '{address}'."
-raise QMPBadPortError(msg) from None
-return (components[0], port)
-
-# Treat as filepath.
-return address
-
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
"""
Connect to the QMP Monitor and perform capabilities negotiation.
--
2.31.1
[PATCH v2 21/25] python/aqmp: fully separate from qmp.QEMUMonitorProtocol
After this patch, qemu.aqmp.legacy.QEMUMonitorProtocol no longer
inherits from qemu.qmp.QEMUMonitorProtocol. To do this, several
inherited methods need to be explicitly re-defined.
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 38 --
1 file changed, 32 insertions(+), 6 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 76b09671cc..8f38e7d912 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -5,18 +5,18 @@
"""
import asyncio
+from types import TracebackType
from typing import (
Any,
Awaitable,
Dict,
List,
Optional,
+Type,
TypeVar,
Union,
)
-import qemu.qmp
-
from .error import QMPError
from .protocol import Runstate, SocketAddrT
from .qmp_client import QMPClient
@@ -48,9 +48,9 @@ class QMPBadPortError(QMPError):
"""
-class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+class QEMUMonitorProtocol:
def __init__(self, address: SocketAddrT,
- server: bool = False,
+ server: bool = False, # pylint: disable=unused-argument
nickname: Optional[str] = None):
# pylint: disable=super-init-not-called
@@ -74,7 +74,18 @@ def _get_greeting(self) -> Optional[QMPMessage]:
return self._aqmp.greeting._asdict()
return None
-# __enter__ and __exit__ need no changes
+def __enter__(self: _T) -> _T:
+# Implement context manager enter function.
+return self
+
+def __exit__(self,
+ # pylint: disable=duplicate-code
+ # see https://github.com/PyCQA/pylint/issues/3619
+ exc_type: Optional[Type[BaseException]],
+ exc_val: Optional[BaseException],
+ exc_tb: Optional[TracebackType]) -> None:
+# Implement context manager exit function.
+self.close()
@classmethod
def parse_address(cls, address: str) -> SocketAddrT:
@@ -131,7 +142,22 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
)
)
-# Default impl of cmd() delegates to cmd_obj
+def cmd(self, name: str,
+args: Optional[Dict[str, object]] = None,
+cmd_id: Optional[object] = None) -> QMPMessage:
+"""
+Build a QMP command and send it to the QMP Monitor.
+
+@param name: command name (string)
+@param args: command arguments (dict)
+@param cmd_id: command id (dict, list, string or int)
+"""
+qmp_cmd: QMPMessage = {'execute': name}
+if args:
+qmp_cmd['arguments'] = args
+if cmd_id:
+qmp_cmd['id'] = cmd_id
+return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
return self._sync(
--
2.31.1
[PATCH v2 25/25] python: rename qemu.aqmp to qemu.qmp
Now that we are fully switched over to the new QMP library, move it back
over the old namespace. This is being done primarily so that we may
upload this package simply as "qemu.qmp" without introducing confusion
over whether or not "aqmp" is a new protocol or not.
The trade-off is increased confusion inside the QEMU developer
tree. Sorry!
Signed-off-by: John Snow
---
python/PACKAGE.rst| 4 ++--
python/README.rst | 4 ++--
python/qemu/machine/machine.py| 4 ++--
python/qemu/machine/qtest.py | 2 +-
python/qemu/{aqmp => qmp}/__init__.py | 6 +++---
python/qemu/{aqmp => qmp}/aqmp_tui.py | 0
python/qemu/{aqmp => qmp}/error.py| 0
python/qemu/{aqmp => qmp}/events.py | 2 +-
python/qemu/{aqmp => qmp}/legacy.py | 0
python/qemu/{aqmp => qmp}/message.py | 0
python/qemu/{aqmp => qmp}/models.py | 0
python/qemu/{aqmp => qmp}/protocol.py | 4 ++--
python/qemu/{aqmp => qmp}/py.typed| 0
python/qemu/{aqmp => qmp}/qmp_client.py | 16
python/qemu/{aqmp => qmp}/qmp_shell.py| 4 ++--
python/qemu/{aqmp => qmp}/util.py | 0
python/qemu/utils/qemu_ga_client.py | 4 ++--
python/qemu/utils/qom.py | 2 +-
python/qemu/utils/qom_common.py | 4 ++--
python/qemu/utils/qom_fuse.py | 2 +-
python/setup.cfg | 8
python/tests/protocol.py | 14 +++---
scripts/cpu-x86-uarch-abi.py | 2 +-
scripts/device-crash-test | 4 ++--
scripts/qmp/qmp-shell | 2 +-
scripts/render_block_graph.py | 4 ++--
scripts/simplebench/bench_block_job.py| 2 +-
tests/qemu-iotests/iotests.py | 2 +-
tests/qemu-iotests/tests/mirror-top-perms | 6 +++---
29 files changed, 51 insertions(+), 51 deletions(-)
rename python/qemu/{aqmp => qmp}/__init__.py (87%)
rename python/qemu/{aqmp => qmp}/aqmp_tui.py (100%)
rename python/qemu/{aqmp => qmp}/error.py (100%)
rename python/qemu/{aqmp => qmp}/events.py (99%)
rename python/qemu/{aqmp => qmp}/legacy.py (100%)
rename python/qemu/{aqmp => qmp}/message.py (100%)
rename python/qemu/{aqmp => qmp}/models.py (100%)
rename python/qemu/{aqmp => qmp}/protocol.py (99%)
rename python/qemu/{aqmp => qmp}/py.typed (100%)
rename python/qemu/{aqmp => qmp}/qmp_client.py (97%)
rename python/qemu/{aqmp => qmp}/qmp_shell.py (99%)
rename python/qemu/{aqmp => qmp}/util.py (100%)
diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index ddfa9ba3f5..b0b86cc4c3 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
Usage
-
-The ``qemu.aqmp`` subpackage provides a library for communicating with
+The ``qemu.qmp`` subpackage provides a library for communicating with
QMP servers. The ``qemu.machine`` subpackage offers rudimentary
facilities for launching and managing QEMU processes. Refer to each
package's documentation
-(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
for more information.
Contributing
diff --git a/python/README.rst b/python/README.rst
index eb5213337d..9c1fceaee7 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
This directory houses Python tooling used by the QEMU project to build,
configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
``setup.py`` is used by ``pip`` to install this tooling to the current
environment. ``setup.cfg`` provides the packaging configuration used by
@@ -59,7 +59,7 @@ Package installation also normally provides executable
console scripts,
so that tools like ``qmp-shell`` are always available via $PATH. To
invoke them without installation, you can invoke e.g.:
-``> PYTHONPATH=~/src/qemu/python python3 -m qemu.aqmp.qmp_shell``
+``> PYTHONPATH=~/src/qemu/python python3 -m qemu.qmp.qmp_shell``
The mappings between console script name and python module path can be
found in ``setup.cfg``.
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 21fb4a4f30..6e4bf6520c 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,8 +40,8 @@
TypeVar,
)
-from qemu.aqmp import SocketAddrT
-from qemu.aqmp.legacy import (
+from qemu.qmp import SocketAddrT
+from qemu.qmp.legacy import (
QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index 817c8a5425..f29d5c1042 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
TextIO,
)
-from qemu.aqmp.protocol import SocketAddrT
+from qemu.qmp.protocol import SocketAddrT
from .machine import
[PATCH v2 22/25] python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
Copy the docstrings out of qemu.qmp, adjusting them as necessary to
more accurately reflect the current state of this class.
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 110 ++---
1 file changed, 102 insertions(+), 8 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 8f38e7d912..6c250cd46a 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -1,9 +1,23 @@
"""
-Sync QMP Wrapper
+(Legacy) Sync QMP Wrapper
-This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+This module provides the `QEMUMonitorProtocol` class, which is a
+synchronous wrapper around `QMPClient`.
+
+Its design closely resembles that of the original QEMUMonitorProtocol
+class, originally written by Luiz Capitulino.
"""
+# Copyright (C) 2009, 2010, 2021 Red Hat Inc.
+#
+# Authors:
+# Luiz Capitulino
+# John Snow
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+
import asyncio
from types import TracebackType
from typing import (
@@ -39,9 +53,6 @@
# {} is the QMPReturnValue.
-# pylint: disable=missing-docstring
-
-
class QMPBadPortError(QMPError):
"""
Unable to parse socket address: Port was non-numerical.
@@ -49,6 +60,21 @@ class QMPBadPortError(QMPError):
class QEMUMonitorProtocol:
+"""
+Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP)
+and then allow to handle commands and events.
+
+:param address: QEMU address, can be either a unix socket path (string)
+ or a tuple in the form ( address, port ) for a TCP
+ connection
+:param server: Deprecated, ignored. (See 'accept')
+:param nickname: Optional nickname used for logging.
+
+..note::
+No connection is established during `__init__`, this is done by
+the `connect()` or `accept()` methods.
+"""
+
def __init__(self, address: SocketAddrT,
server: bool = False, # pylint: disable=unused-argument
nickname: Optional[str] = None):
@@ -108,6 +134,12 @@ def parse_address(cls, address: str) -> SocketAddrT:
return address
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+"""
+Connect to the QMP Monitor and perform capabilities negotiation.
+
+:return: QMP greeting dict, or None if negotiate is false
+:raise ConnectError: on connection errors
+"""
self._aqmp.await_greeting = negotiate
self._aqmp.negotiate = negotiate
@@ -117,6 +149,16 @@ def connect(self, negotiate: bool = True) ->
Optional[QMPMessage]:
return self._get_greeting()
def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+"""
+Await connection from QMP Monitor and perform capabilities negotiation.
+
+:param timeout:
+timeout in seconds (nonnegative float number, or None).
+If None, there is no timeout, and this may block forever.
+
+:return: QMP greeting dict
+:raise ConnectError: on connection errors
+"""
self._aqmp.await_greeting = True
self._aqmp.negotiate = True
@@ -130,6 +172,12 @@ def accept(self, timeout: Optional[float] = 15.0) ->
QMPMessage:
return ret
def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+"""
+Send a QMP command to the QMP Monitor.
+
+:param qmp_cmd: QMP command to be sent as a Python dict
+:return: QMP response as a Python dict
+"""
return dict(
self._sync(
# pylint: disable=protected-access
@@ -148,9 +196,9 @@ def cmd(self, name: str,
"""
Build a QMP command and send it to the QMP Monitor.
-@param name: command name (string)
-@param args: command arguments (dict)
-@param cmd_id: command id (dict, list, string or int)
+:param name: command name (string)
+:param args: command arguments (dict)
+:param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd: QMPMessage = {'execute': name}
if args:
@@ -160,6 +208,9 @@ def cmd(self, name: str,
return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+"""
+Build and send a QMP command to the monitor, report errors if any
+"""
return self._sync(
self._aqmp.execute(cmd, kwds),
self._timeout
@@ -167,6 +218,19 @@ def command(self, cmd: str, **kwds: object) ->
QMPReturnValue:
def pull_event(self,
wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+"""
+Pulls a single event.
+
+:param wait:
+If False or 0, do not wait. Return None if no events ready.
+If True, wait forever until the next event.
+Otherwise,
[PATCH v2 19/25] python: temporarily silence pylint duplicate-code warnings
The next several commits copy some code from qemu.qmp to qemu.aqmp, then delete qemu.qmp. In the interim, to prevent test failures, the duplicate code detection needs to be silenced to prevent bisect problems with CI testing. Signed-off-by: John Snow --- python/setup.cfg | 1 + 1 file changed, 1 insertion(+) diff --git a/python/setup.cfg b/python/setup.cfg index 168a79c867..510df23698 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -115,6 +115,7 @@ ignore_missing_imports = True disable=consider-using-f-string, too-many-function-args, # mypy handles this with less false positives. no-member, # mypy also handles this better. +duplicate-code, # To be removed by the end of this patch series. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.31.1
[PATCH v2 18/25] iotests: switch to AQMP
Simply import the type defition from the new location.
Signed-off-by: John Snow
---
tests/qemu-iotests/iotests.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 83bfedb902..cb21aebe36 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,7 +37,7 @@
from contextlib import contextmanager
from qemu.machine import qtest
-from qemu.qmp import QMPMessage
+from qemu.aqmp.legacy import QMPMessage
# Use this logger for logging messages directly from the iotests module
logger = logging.getLogger('qemu.iotests')
--
2.31.1
[PATCH v2 15/25] scripts/render-block-graph: switch to AQMP
Creating an instance of qemu.aqmp.ExecuteError is too involved here, so just drop the specificity down to a generic AQMPError. Signed-off-by: John Snow --- scripts/render_block_graph.py | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py index da6acf050d..97778927f3 100755 --- a/scripts/render_block_graph.py +++ b/scripts/render_block_graph.py @@ -25,10 +25,8 @@ from graphviz import Digraph sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python')) -from qemu.qmp import ( -QEMUMonitorProtocol, -QMPResponseError, -) +from qemu.aqmp import QMPError +from qemu.aqmp.legacy import QEMUMonitorProtocol def perm(arr): @@ -105,7 +103,7 @@ def command(self, cmd): reply = json.loads(subprocess.check_output(ar)) if 'error' in reply: -raise QMPResponseError(reply) +raise QMPError(reply) return reply['return'] -- 2.31.1
[PATCH v2 16/25] scripts/bench-block-job: switch to AQMP
For this commit, we only need to remove accommodations for the
synchronous QMP library.
Signed-off-by: John Snow
---
scripts/simplebench/bench_block_job.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index a403c35b08..af9d1646a4 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -27,7 +27,6 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
-from qemu.qmp import QMPConnectError
from qemu.aqmp import ConnectError
@@ -50,7 +49,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.launch()
except OSError as e:
return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, ConnectError, socket.timeout):
+except (ConnectError, socket.timeout):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
--
2.31.1
[PATCH v2 10/25] python: move qmp utilities to python/qemu/utils
In order to upload a QMP package to PyPI, I want to remove any scripts
that I am not 100% confident I want to support upstream, beyond our
castle walls.
Move most of our QMP utilities into the utils package so we can split
them out from the PyPI upload.
Signed-off-by: John Snow
---
python/qemu/{qmp => utils}/qemu_ga_client.py | 0
python/qemu/{qmp => utils}/qom.py| 0
python/qemu/{qmp => utils}/qom_common.py | 0
python/qemu/{qmp => utils}/qom_fuse.py | 0
python/setup.cfg | 16
scripts/qmp/qemu-ga-client | 2 +-
scripts/qmp/qom-fuse | 2 +-
scripts/qmp/qom-get | 2 +-
scripts/qmp/qom-list | 2 +-
scripts/qmp/qom-set | 2 +-
scripts/qmp/qom-tree | 2 +-
11 files changed, 14 insertions(+), 14 deletions(-)
rename python/qemu/{qmp => utils}/qemu_ga_client.py (100%)
rename python/qemu/{qmp => utils}/qom.py (100%)
rename python/qemu/{qmp => utils}/qom_common.py (100%)
rename python/qemu/{qmp => utils}/qom_fuse.py (100%)
diff --git a/python/qemu/qmp/qemu_ga_client.py
b/python/qemu/utils/qemu_ga_client.py
similarity index 100%
rename from python/qemu/qmp/qemu_ga_client.py
rename to python/qemu/utils/qemu_ga_client.py
diff --git a/python/qemu/qmp/qom.py b/python/qemu/utils/qom.py
similarity index 100%
rename from python/qemu/qmp/qom.py
rename to python/qemu/utils/qom.py
diff --git a/python/qemu/qmp/qom_common.py b/python/qemu/utils/qom_common.py
similarity index 100%
rename from python/qemu/qmp/qom_common.py
rename to python/qemu/utils/qom_common.py
diff --git a/python/qemu/qmp/qom_fuse.py b/python/qemu/utils/qom_fuse.py
similarity index 100%
rename from python/qemu/qmp/qom_fuse.py
rename to python/qemu/utils/qom_fuse.py
diff --git a/python/setup.cfg b/python/setup.cfg
index 417e937839..78421411d2 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -60,13 +60,13 @@ tui =
[options.entry_points]
console_scripts =
-qom = qemu.qmp.qom:main
-qom-set = qemu.qmp.qom:QOMSet.entry_point
-qom-get = qemu.qmp.qom:QOMGet.entry_point
-qom-list = qemu.qmp.qom:QOMList.entry_point
-qom-tree = qemu.qmp.qom:QOMTree.entry_point
-qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
-qemu-ga-client = qemu.qmp.qemu_ga_client:main
+qom = qemu.utils.qom:main
+qom-set = qemu.utils.qom:QOMSet.entry_point
+qom-get = qemu.utils.qom:QOMGet.entry_point
+qom-list = qemu.utils.qom:QOMList.entry_point
+qom-tree = qemu.utils.qom:QOMTree.entry_point
+qom-fuse = qemu.utils.qom_fuse:QOMFuse.entry_point [fuse]
+qemu-ga-client = qemu.utils.qemu_ga_client:main
qmp-shell = qemu.qmp.qmp_shell:main
aqmp-tui = qemu.aqmp.aqmp_tui:main [tui]
@@ -80,7 +80,7 @@ python_version = 3.6
warn_unused_configs = True
namespace_packages = True
-[mypy-qemu.qmp.qom_fuse]
+[mypy-qemu.utils.qom_fuse]
# fusepy has no type stubs:
allow_subclassing_any = True
diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client
index 102fd2cad9..56edd0234a 100755
--- a/scripts/qmp/qemu-ga-client
+++ b/scripts/qmp/qemu-ga-client
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import qemu_ga_client
+from qemu.utils import qemu_ga_client
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index a58c8ef979..d453807b27 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom_fuse import QOMFuse
+from qemu.utils.qom_fuse import QOMFuse
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index e4f3e0c013..04ebe052e8 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMGet
+from qemu.utils.qom import QOMGet
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index 7a071a54e1..853b85a8d3 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMList
+from qemu.utils.qom import QOMList
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index 9ca9e2ba10..06820feec4 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMSet
+from qemu.utils.qom import QOMSet
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index
[PATCH v2 09/25] python/qmp: switch qmp-shell to AQMP
We have a replacement for async QMP, but it doesn't have feature parity
yet. For now, then, port the old tool onto the new backend.
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 3 +++
python/qemu/qmp/qmp_shell.py | 31 +--
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 27df22818a..0890f95b16 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -22,6 +22,9 @@
from .qmp_client import QMPClient
+# (Temporarily) Re-export QMPBadPortError
+QMPBadPortError = qemu.qmp.QMPBadPortError
+
#: QMPMessage is an entire QMP message of any kind.
QMPMessage = Dict[str, Any]
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
index e7d7eb18f1..d11bf54b00 100644
--- a/python/qemu/qmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -95,8 +95,13 @@
Sequence,
)
-from qemu import qmp
-from qemu.qmp import QMPMessage
+from qemu.aqmp import ConnectError, QMPError, SocketAddrT
+from qemu.aqmp.legacy import (
+QEMUMonitorProtocol,
+QMPBadPortError,
+QMPMessage,
+QMPObject,
+)
LOG = logging.getLogger(__name__)
@@ -125,7 +130,7 @@ def complete(self, text: str, state: int) -> Optional[str]:
return None
-class QMPShellError(qmp.QMPError):
+class QMPShellError(QMPError):
"""
QMP Shell Base error class.
"""
@@ -153,7 +158,7 @@ def visit_Name(cls, # pylint: disable=invalid-name
return node
-class QMPShell(qmp.QEMUMonitorProtocol):
+class QMPShell(QEMUMonitorProtocol):
"""
QMPShell provides a basic readline-based QMP shell.
@@ -161,7 +166,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
:param pretty: Pretty-print QMP messages.
:param verbose: Echo outgoing QMP messages to console.
"""
-def __init__(self, address: qmp.SocketAddrT,
+def __init__(self, address: SocketAddrT,
pretty: bool = False, verbose: bool = False):
super().__init__(address)
self._greeting: Optional[QMPMessage] = None
@@ -237,7 +242,7 @@ def _parse_value(cls, val: str) -> object:
def _cli_expr(self,
tokens: Sequence[str],
- parent: qmp.QMPObject) -> None:
+ parent: QMPObject) -> None:
for arg in tokens:
(key, sep, val) = arg.partition('=')
if sep != '=':
@@ -403,7 +408,7 @@ class HMPShell(QMPShell):
:param pretty: Pretty-print QMP messages.
:param verbose: Echo outgoing QMP messages to console.
"""
-def __init__(self, address: qmp.SocketAddrT,
+def __init__(self, address: SocketAddrT,
pretty: bool = False, verbose: bool = False):
super().__init__(address, pretty, verbose)
self._cpu_index = 0
@@ -512,19 +517,17 @@ def main() -> None:
try:
address = shell_class.parse_address(args.qmp_server)
-except qmp.QMPBadPortError:
+except QMPBadPortError:
parser.error(f"Bad port number: {args.qmp_server}")
return # pycharm doesn't know error() is noreturn
with shell_class(address, args.pretty, args.verbose) as qemu:
try:
qemu.connect(negotiate=not args.skip_negotiation)
-except qmp.QMPConnectError:
-die("Didn't get QMP greeting message")
-except qmp.QMPCapabilitiesError:
-die("Couldn't negotiate capabilities")
-except OSError as err:
-die(f"Couldn't connect to {args.qmp_server}: {err!s}")
+except ConnectError as err:
+if isinstance(err.exc, OSError):
+die(f"Couldn't connect to {args.qmp_server}: {err!s}")
+die(str(err))
for _ in qemu.repl():
pass
--
2.31.1
[PATCH v2 24/25] python: re-enable pylint duplicate-code warnings
With the old library gone, there's nothing duplicated in the tree, so the warning suppression can be removed. Signed-off-by: John Snow --- python/setup.cfg | 1 - 1 file changed, 1 deletion(-) diff --git a/python/setup.cfg b/python/setup.cfg index 5140a5b322..c341e922c2 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -114,7 +114,6 @@ ignore_missing_imports = True disable=consider-using-f-string, too-many-function-args, # mypy handles this with less false positives. no-member, # mypy also handles this better. -duplicate-code, # To be removed by the end of this patch series. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.31.1
[PATCH v2 17/25] iotests/mirror-top-perms: switch to AQMP
Signed-off-by: John Snow
---
Note: I still need to adjust the logging. The problem now is that the
logging messages include the PID of the test process, so they need to be
filtered out. I'll investigate that for a follow-up, or for v2.
I could just add yet another filtering function somewhere, but I think
it's getting out of hand with how many filters and loggers there are, so
I want to give it a slightly more serious treatment instead of a
hackjob.
Signed-off-by: John Snow
---
tests/qemu-iotests/tests/mirror-top-perms | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index 0a51a613f3..f394931a00 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -23,7 +23,6 @@ import os
from qemu.aqmp import ConnectError
from qemu.machine import machine
-from qemu.qmp import QMPConnectError
import iotests
from iotests import change_log_level, qemu_img
@@ -101,13 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
try:
# Silence AQMP errors temporarily.
-# TODO: Remove this and just allow the errors to be logged when
-# AQMP fully replaces QMP.
+# TODO: Remove change_log_level and allow the errors to be logged.
+# This necessitates a PID filter on *all* logging output.
with change_log_level('qemu.aqmp'):
self.vm_b.launch()
print('ERROR: VM B launched successfully, '
'this should not have happened')
-except (QMPConnectError, ConnectError):
+except ConnectError:
assert 'Is another process using the image' in self.vm_b.get_log()
result = self.vm.qmp('block-job-cancel',
--
2.31.1
[PATCH v2 08/25] python/qmp: switch qom tools to AQMP
Signed-off-by: John Snow
---
python/qemu/qmp/qom.py| 5 +++--
python/qemu/qmp/qom_common.py | 3 ++-
python/qemu/qmp/qom_fuse.py | 11 ++-
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/python/qemu/qmp/qom.py b/python/qemu/qmp/qom.py
index 8ff28a8343..bb5d1a78f5 100644
--- a/python/qemu/qmp/qom.py
+++ b/python/qemu/qmp/qom.py
@@ -32,7 +32,8 @@
import argparse
-from . import QMPResponseError
+from qemu.aqmp import ExecuteError
+
from .qom_common import QOMCommand
@@ -233,7 +234,7 @@ def _list_node(self, path: str) -> None:
rsp = self.qmp.command('qom-get', path=path,
property=item.name)
print(f" {item.name}: {rsp} ({item.type})")
-except QMPResponseError as err:
+except ExecuteError as err:
print(f" {item.name}: ({item.type})")
print('')
for item in items:
diff --git a/python/qemu/qmp/qom_common.py b/python/qemu/qmp/qom_common.py
index a59ae1a2a1..6f07451dfa 100644
--- a/python/qemu/qmp/qom_common.py
+++ b/python/qemu/qmp/qom_common.py
@@ -27,7 +27,8 @@
TypeVar,
)
-from . import QEMUMonitorProtocol, QMPError
+from qemu.aqmp import QMPError
+from qemu.aqmp.legacy import QEMUMonitorProtocol
# The following is needed only for a type alias.
diff --git a/python/qemu/qmp/qom_fuse.py b/python/qemu/qmp/qom_fuse.py
index 43f4671fdb..653a76b93b 100644
--- a/python/qemu/qmp/qom_fuse.py
+++ b/python/qemu/qmp/qom_fuse.py
@@ -48,7 +48,8 @@
import fuse
from fuse import FUSE, FuseOSError, Operations
-from . import QMPResponseError
+from qemu.aqmp import ExecuteError
+
from .qom_common import QOMCommand
@@ -99,7 +100,7 @@ def is_object(self, path: str) -> bool:
try:
self.qom_list(path)
return True
-except QMPResponseError:
+except ExecuteError:
return False
def is_property(self, path: str) -> bool:
@@ -112,7 +113,7 @@ def is_property(self, path: str) -> bool:
if item.name == prop:
return True
return False
-except QMPResponseError:
+except ExecuteError:
return False
def is_link(self, path: str) -> bool:
@@ -125,7 +126,7 @@ def is_link(self, path: str) -> bool:
if item.name == prop and item.link:
return True
return False
-except QMPResponseError:
+except ExecuteError:
return False
def read(self, path: str, size: int, offset: int, fh: IO[bytes]) -> bytes:
@@ -138,7 +139,7 @@ def read(self, path: str, size: int, offset: int, fh:
IO[bytes]) -> bytes:
try:
data = str(self.qmp.command('qom-get', path=path, property=prop))
data += '\n' # make values shell friendly
-except QMPResponseError as err:
+except ExecuteError as err:
raise FuseOSError(EPERM) from err
if offset > len(data):
--
2.31.1
[PATCH v2 14/25] scripts/cpu-x86-uarch-abi: switch to AQMP
Signed-off-by: John Snow
---
scripts/cpu-x86-uarch-abi.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index 8963d90f0b..c262d2f027 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -6,7 +6,7 @@
# compatibility levels for each CPU model.
#
-from qemu import qmp
+from qemu.aqmp.legacy import QEMUMonitorProtocol
import sys
if len(sys.argv) != 2:
@@ -66,7 +66,7 @@
sock = sys.argv[1]
-shell = qmp.QEMUMonitorProtocol(sock)
+shell = QEMUMonitorProtocol(sock)
shell.connect()
models = shell.cmd("query-cpu-definitions")
--
2.31.1
[PATCH v2 12/25] python/machine: permanently switch to AQMP
Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
switch permanent. Update Exceptions and import paths as necessary.
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 18 +++---
python/qemu/machine/qtest.py | 2 +-
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 67ab06ca2b..21fb4a4f30 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,21 +40,16 @@
TypeVar,
)
-from qemu.qmp import ( # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
+from qemu.aqmp.legacy import (
+QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
-SocketAddrT,
)
from . import console_socket
-if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
-from qemu.qmp import QEMUMonitorProtocol
-else:
-from qemu.aqmp.legacy import QEMUMonitorProtocol
-
-
LOG = logging.getLogger(__name__)
@@ -710,8 +705,9 @@ def events_wait(self,
:param timeout: Optional timeout, in seconds.
See QEMUMonitorProtocol.pull_event.
-:raise QMPTimeoutError: If timeout was non-zero and no matching events
-were found.
+:raise asyncio.TimeoutError:
+If timeout was non-zero and no matching events were found.
+
:return: A QMP event matching the filter criteria.
If timeout was 0 and no event matched, None.
"""
@@ -734,7 +730,7 @@ def _match(event: QMPMessage) -> bool:
event = self._qmp.pull_event(wait=timeout)
if event is None:
# NB: None is only returned when timeout is false-ish.
-# Timeouts raise QMPTimeoutError instead!
+# Timeouts raise asyncio.TimeoutError instead!
break
if _match(event):
return event
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index f2f9aaa5e5..817c8a5425 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
TextIO,
)
-from qemu.qmp import SocketAddrT # pylint: disable=import-error
+from qemu.aqmp.protocol import SocketAddrT
from .machine import QEMUMachine
--
2.31.1
[PATCH v2 11/25] python: move qmp-shell under the AQMP package
Signed-off-by: John Snow
---
python/README.rst | 2 +-
python/qemu/{qmp => aqmp}/qmp_shell.py | 0
python/setup.cfg | 2 +-
scripts/qmp/qmp-shell | 2 +-
4 files changed, 3 insertions(+), 3 deletions(-)
rename python/qemu/{qmp => aqmp}/qmp_shell.py (100%)
diff --git a/python/README.rst b/python/README.rst
index 9c1fceaee7..fcf74f69ea 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -59,7 +59,7 @@ Package installation also normally provides executable
console scripts,
so that tools like ``qmp-shell`` are always available via $PATH. To
invoke them without installation, you can invoke e.g.:
-``> PYTHONPATH=~/src/qemu/python python3 -m qemu.qmp.qmp_shell``
+``> PYTHONPATH=~/src/qemu/python python3 -m qemu.aqmp.qmp_shell``
The mappings between console script name and python module path can be
found in ``setup.cfg``.
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
similarity index 100%
rename from python/qemu/qmp/qmp_shell.py
rename to python/qemu/aqmp/qmp_shell.py
diff --git a/python/setup.cfg b/python/setup.cfg
index 78421411d2..168a79c867 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -67,7 +67,7 @@ console_scripts =
qom-tree = qemu.utils.qom:QOMTree.entry_point
qom-fuse = qemu.utils.qom_fuse:QOMFuse.entry_point [fuse]
qemu-ga-client = qemu.utils.qemu_ga_client:main
-qmp-shell = qemu.qmp.qmp_shell:main
+qmp-shell = qemu.aqmp.qmp_shell:main
aqmp-tui = qemu.aqmp.aqmp_tui:main [tui]
[flake8]
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 4a20f97db7..31b19d73e2 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import qmp_shell
+from qemu.aqmp import qmp_shell
if __name__ == '__main__':
--
2.31.1
[PATCH v2 13/25] scripts/cpu-x86-uarch-abi: fix CLI parsing
Signed-off-by: John Snow
---
scripts/cpu-x86-uarch-abi.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index 08acc52a81..8963d90f0b 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -9,7 +9,7 @@
from qemu import qmp
import sys
-if len(sys.argv) != 1:
+if len(sys.argv) != 2:
print("syntax: %s QMP-SOCK\n\n" % __file__ +
"Where QMP-SOCK points to a QEMU process such as\n\n" +
" # qemu-system-x86_64 -qmp unix:/tmp/qmp,server,nowait " +
@@ -66,7 +66,6 @@
sock = sys.argv[1]
-cmd = sys.argv[2]
shell = qmp.QEMUMonitorProtocol(sock)
shell.connect()
--
2.31.1
[PATCH v2 03/25] python/aqmp: copy type definitions from qmp
Copy the remaining type definitions from QMP into the qemu.aqmp.legacy
module. Now, most users don't need to import anything else but
qemu.aqmp.legacy.
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 22 --
python/qemu/aqmp/protocol.py | 16 ++--
2 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 2ccb136b02..9431fe9330 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -6,7 +6,9 @@
import asyncio
from typing import (
+Any,
Awaitable,
+Dict,
List,
Optional,
TypeVar,
@@ -14,13 +16,29 @@
)
import qemu.qmp
-from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
from .error import AQMPError
-from .protocol import Runstate
+from .protocol import Runstate, SocketAddrT
from .qmp_client import QMPClient
+#: QMPMessage is an entire QMP message of any kind.
+QMPMessage = Dict[str, Any]
+
+#: QMPReturnValue is the 'return' value of a command.
+QMPReturnValue = object
+
+#: QMPObject is any object in a QMP message.
+QMPObject = Dict[str, object]
+
+# QMPMessage can be outgoing commands or incoming events/returns.
+# QMPReturnValue is usually a dict/json object, but due to QAPI's
+# 'returns-whitelist', it can actually be anything.
+#
+# {'return': {}} is a QMPMessage,
+# {} is the QMPReturnValue.
+
+
# pylint: disable=missing-docstring
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 5190b33b13..42a897e2fe 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -46,6 +46,10 @@
_TaskFN = Callable[[], Awaitable[None]] # aka ``async def func() -> None``
_FutureT = TypeVar('_FutureT', bound=Optional['asyncio.Future[Any]'])
+InternetAddrT = Tuple[str, int]
+UnixAddrT = str
+SocketAddrT = Union[UnixAddrT, InternetAddrT]
+
class Runstate(Enum):
"""Protocol session runstate."""
@@ -257,7 +261,7 @@ async def runstate_changed(self) -> Runstate:
@upper_half
@require(Runstate.IDLE)
-async def accept(self, address: Union[str, Tuple[str, int]],
+async def accept(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
"""
Accept a connection and begin processing message queues.
@@ -275,7 +279,7 @@ async def accept(self, address: Union[str, Tuple[str, int]],
@upper_half
@require(Runstate.IDLE)
-async def connect(self, address: Union[str, Tuple[str, int]],
+async def connect(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
"""
Connect to the server and begin processing message queues.
@@ -337,7 +341,7 @@ def _set_state(self, state: Runstate) -> None:
@upper_half
async def _new_session(self,
- address: Union[str, Tuple[str, int]],
+ address: SocketAddrT,
ssl: Optional[SSLContext] = None,
accept: bool = False) -> None:
"""
@@ -397,7 +401,7 @@ async def _new_session(self,
@upper_half
async def _establish_connection(
self,
-address: Union[str, Tuple[str, int]],
+address: SocketAddrT,
ssl: Optional[SSLContext] = None,
accept: bool = False
) -> None:
@@ -424,7 +428,7 @@ async def _establish_connection(
await self._do_connect(address, ssl)
@upper_half
-async def _do_accept(self, address: Union[str, Tuple[str, int]],
+async def _do_accept(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
"""
Acting as the transport server, accept a single connection.
@@ -482,7 +486,7 @@ async def _client_connected_cb(reader: asyncio.StreamReader,
self.logger.debug("Connection accepted.")
@upper_half
-async def _do_connect(self, address: Union[str, Tuple[str, int]],
+async def _do_connect(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
"""
Acting as the transport client, initiate a connection to a server.
--
2.31.1
[PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()
This exception can be injected into any await statement. If we are canceled via timeout, we want to clear the pending execution record on our way out. Signed-off-by: John Snow --- python/qemu/aqmp/qmp_client.py | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py index 8105e29fa8..6a985ffe30 100644 --- a/python/qemu/aqmp/qmp_client.py +++ b/python/qemu/aqmp/qmp_client.py @@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None, str]: msg_id = msg['id'] self._pending[msg_id] = asyncio.Queue(maxsize=1) -await self._outgoing.put(msg) +try: +await self._outgoing.put(msg) +except: +del self._pending[msg_id] +raise return msg_id @@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) -> Message: was lost, or some other problem. """ queue = self._pending[msg_id] -result = await queue.get() try: +result = await queue.get() if isinstance(result, ExecInterruptedError): raise result return result -- 2.31.1
[PATCH v2 04/25] python/aqmp: add SocketAddrT to package root
It's a commonly needed definition, it can be re-exported by the root. Signed-off-by: John Snow --- python/qemu/aqmp/__init__.py | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py index 880d5b6fa7..c6fa2dda58 100644 --- a/python/qemu/aqmp/__init__.py +++ b/python/qemu/aqmp/__init__.py @@ -26,7 +26,12 @@ from .error import AQMPError from .events import EventListener from .message import Message -from .protocol import ConnectError, Runstate, StateError +from .protocol import ( +ConnectError, +Runstate, +SocketAddrT, +StateError, +) from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient @@ -48,4 +53,7 @@ 'ConnectError', 'ExecuteError', 'ExecInterruptedError', + +# Type aliases +'SocketAddrT', ) -- 2.31.1
[PATCH v2 05/25] python/aqmp: rename AQMPError to QMPError
This is in preparation for renaming qemu.aqmp to qemu.qmp. I should have
done this from this from the very beginning, but it's a convenient time
to make sure this churn is taken care of.
Signed-off-by: John Snow
---
python/qemu/aqmp/__init__.py | 6 +++---
python/qemu/aqmp/error.py | 12 ++--
python/qemu/aqmp/events.py | 4 ++--
python/qemu/aqmp/legacy.py | 4 ++--
python/qemu/aqmp/protocol.py | 8
python/qemu/aqmp/qmp_client.py | 8
6 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index c6fa2dda58..e1efab00af 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -6,7 +6,7 @@
QEMU Guest Agent, and the QEMU Storage Daemon.
`QMPClient` provides the main functionality of this package. All errors
-raised by this library dervive from `AQMPError`, see `aqmp.error` for
+raised by this library dervive from `QMPError`, see `aqmp.error` for
additional detail. See `aqmp.events` for an in-depth tutorial on
managing QMP events.
"""
@@ -23,7 +23,7 @@
import logging
-from .error import AQMPError
+from .error import QMPError
from .events import EventListener
from .message import Message
from .protocol import (
@@ -48,7 +48,7 @@
'Runstate',
# Exceptions, most generic to most explicit
-'AQMPError',
+'QMPError',
'StateError',
'ConnectError',
'ExecuteError',
diff --git a/python/qemu/aqmp/error.py b/python/qemu/aqmp/error.py
index 781f49b008..24ba4d5054 100644
--- a/python/qemu/aqmp/error.py
+++ b/python/qemu/aqmp/error.py
@@ -1,21 +1,21 @@
"""
-AQMP Error Classes
+QMP Error Classes
This package seeks to provide semantic error classes that are intended
to be used directly by clients when they would like to handle particular
semantic failures (e.g. "failed to connect") without needing to know the
enumeration of possible reasons for that failure.
-AQMPError serves as the ancestor for all exceptions raised by this
+QMPError serves as the ancestor for all exceptions raised by this
package, and is suitable for use in handling semantic errors from this
library. In most cases, individual public methods will attempt to catch
and re-encapsulate various exceptions to provide a semantic
error-handling interface.
-.. admonition:: AQMP Exception Hierarchy Reference
+.. admonition:: QMP Exception Hierarchy Reference
| `Exception`
- |+-- `AQMPError`
+ |+-- `QMPError`
| +-- `ConnectError`
| +-- `StateError`
| +-- `ExecInterruptedError`
@@ -31,11 +31,11 @@
"""
-class AQMPError(Exception):
+class QMPError(Exception):
"""Abstract error class for all errors originating from this package."""
-class ProtocolError(AQMPError):
+class ProtocolError(QMPError):
"""
Abstract error class for protocol failures.
diff --git a/python/qemu/aqmp/events.py b/python/qemu/aqmp/events.py
index 5f7150c78d..f3d4e2b5e8 100644
--- a/python/qemu/aqmp/events.py
+++ b/python/qemu/aqmp/events.py
@@ -443,7 +443,7 @@ def accept(self, event) -> bool:
Union,
)
-from .error import AQMPError
+from .error import QMPError
from .message import Message
@@ -451,7 +451,7 @@ def accept(self, event) -> bool:
EventFilter = Callable[[Message], bool]
-class ListenerError(AQMPError):
+class ListenerError(QMPError):
"""
Generic error class for `EventListener`-related problems.
"""
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 9431fe9330..27df22818a 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -17,7 +17,7 @@
import qemu.qmp
-from .error import AQMPError
+from .error import QMPError
from .protocol import Runstate, SocketAddrT
from .qmp_client import QMPClient
@@ -168,7 +168,7 @@ def __del__(self) -> None:
# Nothing we can do about it now, but if we don't raise our
# own error, the user will be treated to a lot of traceback
# they might not understand.
-raise AQMPError(
+raise QMPError(
"QEMUMonitorProtocol.close()"
" was not called before object was garbage collected"
)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 42a897e2fe..9ee6fe4ae2 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -29,7 +29,7 @@
cast,
)
-from .error import AQMPError
+from .error import QMPError
from .util import (
bottom_half,
create_task,
@@ -65,7 +65,7 @@ class Runstate(Enum):
DISCONNECTING = 3
-class ConnectError(AQMPError):
+class ConnectError(QMPError):
"""
Raised when the initial connection process has failed.
@@ -90,7 +90,7 @@ def __str__(self) -> str:
return f"{self.error_message}: {cause}"
-class StateError(AQMPError):
+class StateError(QMPError):
"""
An API command (connect, execute, etc) was
[PATCH v2 06/25] python/qemu-ga-client: update instructions to newer CLI syntax
Signed-off-by: John Snow --- python/qemu/qmp/qemu_ga_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/qmp/qemu_ga_client.py index 67ac0b4211..b3e1d98c9e 100644 --- a/python/qemu/qmp/qemu_ga_client.py +++ b/python/qemu/qmp/qemu_ga_client.py @@ -5,7 +5,7 @@ Start QEMU with: -# qemu [...] -chardev socket,path=/tmp/qga.sock,server,wait=off,id=qga0 \ +# qemu [...] -chardev socket,path=/tmp/qga.sock,server=on,wait=off,id=qga0 \ -device virtio-serial \ -device virtserialport,chardev=qga0,name=org.qemu.guest_agent.0 -- 2.31.1
[PATCH v2 07/25] python/qmp: switch qemu-ga-client to AQMP
Signed-off-by: John Snow
---
python/qemu/qmp/qemu_ga_client.py | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/python/qemu/qmp/qemu_ga_client.py
b/python/qemu/qmp/qemu_ga_client.py
index b3e1d98c9e..15ed430c61 100644
--- a/python/qemu/qmp/qemu_ga_client.py
+++ b/python/qemu/qmp/qemu_ga_client.py
@@ -37,8 +37,8 @@
# the COPYING file in the top-level directory.
import argparse
+import asyncio
import base64
-import errno
import os
import random
import sys
@@ -50,8 +50,8 @@
Sequence,
)
-from qemu import qmp
-from qemu.qmp import SocketAddrT
+from qemu.aqmp import ConnectError, SocketAddrT
+from qemu.aqmp.legacy import QEMUMonitorProtocol
# This script has not seen many patches or careful attention in quite
@@ -61,7 +61,7 @@
# pylint: disable=missing-docstring
-class QemuGuestAgent(qmp.QEMUMonitorProtocol):
+class QemuGuestAgent(QEMUMonitorProtocol):
def __getattr__(self, name: str) -> Callable[..., Any]:
def wrapper(**kwds: object) -> object:
return self.command('guest-' + name.replace('_', '-'), **kwds)
@@ -149,7 +149,7 @@ def ping(self, timeout: Optional[float]) -> bool:
self.qga.settimeout(timeout)
try:
self.qga.ping()
-except TimeoutError:
+except asyncio.TimeoutError:
return False
return True
@@ -172,7 +172,7 @@ def suspend(self, mode: str) -> None:
try:
getattr(self.qga, 'suspend' + '_' + mode)()
# On error exception will raise
-except TimeoutError:
+except asyncio.TimeoutError:
# On success command will timed out
return
@@ -182,7 +182,7 @@ def shutdown(self, mode: str = 'powerdown') -> None:
try:
self.qga.shutdown(mode=mode)
-except TimeoutError:
+except asyncio.TimeoutError:
pass
@@ -277,7 +277,7 @@ def _cmd_reboot(client: QemuGuestAgentClient, args:
Sequence[str]) -> None:
def send_command(address: str, cmd: str, args: Sequence[str]) -> None:
if not os.path.exists(address):
-print('%s not found' % address)
+print(f"'{address}' not found. (Is QEMU running?)")
sys.exit(1)
if cmd not in commands:
@@ -287,10 +287,10 @@ def send_command(address: str, cmd: str, args:
Sequence[str]) -> None:
try:
client = QemuGuestAgentClient(address)
-except OSError as err:
+except ConnectError as err:
print(err)
-if err.errno == errno.ECONNREFUSED:
-print('Hint: qemu is not running?')
+if isinstance(err.exc, ConnectionError):
+print('(Is QEMU running?)')
sys.exit(1)
if cmd == 'fsfreeze' and args[0] == 'freeze':
--
2.31.1
[PATCH v2 01/25] python/aqmp: add __del__ method to legacy interface
asyncio can complain *very* loudly if you forget to back out of things gracefully before the garbage collector starts destroying objects that contain live references to asyncio Tasks. The usual fix is just to remember to call aqmp.disconnect(), but for the sake of the legacy wrapper and quick, one-off scripts where a graceful shutdown is not necessarily of paramount imporance, add a courtesy cleanup that will trigger prior to seeing screenfuls of confusing asyncio tracebacks. Note that we can't *always* save you from yourself; depending on when the GC runs, you might just seriously be out of luck. The best we can do in this case is to gently remind you to clean up after yourself. (Still much better than multiple pages of incomprehensible python warnings for the crime of forgetting to put your toys away.) Signed-off-by: John Snow --- python/qemu/aqmp/legacy.py | 18 ++ 1 file changed, 18 insertions(+) diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py index 9e7b9fb80b..2ccb136b02 100644 --- a/python/qemu/aqmp/legacy.py +++ b/python/qemu/aqmp/legacy.py @@ -16,6 +16,8 @@ import qemu.qmp from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT +from .error import AQMPError +from .protocol import Runstate from .qmp_client import QMPClient @@ -136,3 +138,19 @@ def settimeout(self, timeout: Optional[float]) -> None: def send_fd_scm(self, fd: int) -> None: self._aqmp.send_fd_scm(fd) + +def __del__(self) -> None: +if self._aqmp.runstate == Runstate.IDLE: +return + +if not self._aloop.is_running(): +self.close() +else: +# Garbage collection ran while the event loop was running. +# Nothing we can do about it now, but if we don't raise our +# own error, the user will be treated to a lot of traceback +# they might not understand. +raise AQMPError( +"QEMUMonitorProtocol.close()" +" was not called before object was garbage collected" +) -- 2.31.1
[PATCH v2 00/25] Python: delete synchronous qemu.qmp package
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch
CI: https://gitlab.com/jsnow/qemu/-/pipelines/430491195
Hi, this series is part of an effort to publish the qemu.qmp package on
PyPI. It is the first of three series to complete this work:
--> (1) Switch the new Async QMP library in to python/qemu/qmp
(2) Fork python/qemu/qmp out into its own repository,
with updated GitLab CI/CD targets to build packages.
(3) Update qemu.git to install qemu.qmp from PyPI,
and then delete python/qemu/qmp.
This series swaps out qemu.qmp for qemu.aqmp permanently, instead of
hiding it behind an environment variable toggle. This leaves us with
just one QMP library to worry about. It also implements the rename of
"qemu.aqmp" to "qemu.qmp".
Who should review what?
- iotests/block maintainers (Hanna, Vladimir, and Kevin):
please look at patches 15-18.
- Dan, please glance at patches 13-14.
- Anyone who cares about Python: whatever you're willing to
stomach. We're recently down Willian and Eduardo, so there's not
many dedicated Python reviewers...
I suspect the most potential disruption to iotest and avocado
maintainers, as those two subsystems rely on the QMP features the
most. Would appreciate at least an ACK from each of those camps if
you're willing to give benefit-of-the-doubt on the actual Python code.
V2:
- Integrate the renaming of qemu.aqmp to qemu.qmp in this series
- Minor bits and pieces.
John Snow (25):
python/aqmp: add __del__ method to legacy interface
python/aqmp: handle asyncio.TimeoutError on execute()
python/aqmp: copy type definitions from qmp
python/aqmp: add SocketAddrT to package root
python/aqmp: rename AQMPError to QMPError
python/qemu-ga-client: update instructions to newer CLI syntax
python/qmp: switch qemu-ga-client to AQMP
python/qmp: switch qom tools to AQMP
python/qmp: switch qmp-shell to AQMP
python: move qmp utilities to python/qemu/utils
python: move qmp-shell under the AQMP package
python/machine: permanently switch to AQMP
scripts/cpu-x86-uarch-abi: fix CLI parsing
scripts/cpu-x86-uarch-abi: switch to AQMP
scripts/render-block-graph: switch to AQMP
scripts/bench-block-job: switch to AQMP
iotests/mirror-top-perms: switch to AQMP
iotests: switch to AQMP
python: temporarily silence pylint duplicate-code warnings
python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
python/aqmp: fully separate from qmp.QEMUMonitorProtocol
python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
python: remove the old QMP package
python: re-enable pylint duplicate-code warnings
python: rename qemu.aqmp to qemu.qmp
python/qemu/qmp/README.rst | 9 -
python/qemu/aqmp/__init__.py | 51 ---
python/qemu/aqmp/legacy.py | 138 --
python/qemu/aqmp/py.typed| 0
python/qemu/machine/machine.py | 18 +-
python/qemu/machine/qtest.py | 2 +-
python/qemu/qmp/__init__.py | 441 ++-
python/qemu/{aqmp => qmp}/aqmp_tui.py| 2 +-
python/qemu/{aqmp => qmp}/error.py | 12 +-
python/qemu/{aqmp => qmp}/events.py | 6 +-
python/qemu/qmp/legacy.py| 319 ++
python/qemu/{aqmp => qmp}/message.py | 0
python/qemu/{aqmp => qmp}/models.py | 0
python/qemu/{aqmp => qmp}/protocol.py| 28 +-
python/qemu/{aqmp => qmp}/qmp_client.py | 32 +-
python/qemu/qmp/qmp_shell.py | 31 +-
python/qemu/{aqmp => qmp}/util.py| 0
python/qemu/{qmp => utils}/qemu_ga_client.py | 24 +-
python/qemu/{qmp => utils}/qom.py| 5 +-
python/qemu/{qmp => utils}/qom_common.py | 3 +-
python/qemu/{qmp => utils}/qom_fuse.py | 11 +-
python/setup.cfg | 21 +-
python/tests/protocol.py | 14 +-
scripts/cpu-x86-uarch-abi.py | 7 +-
scripts/device-crash-test| 4 +-
scripts/qmp/qemu-ga-client | 2 +-
scripts/qmp/qom-fuse | 2 +-
scripts/qmp/qom-get | 2 +-
scripts/qmp/qom-list | 2 +-
scripts/qmp/qom-set | 2 +-
scripts/qmp/qom-tree | 2 +-
scripts/render_block_graph.py| 8 +-
scripts/simplebench/bench_block_job.py | 5 +-
tests/qemu-iotests/iotests.py| 2 +-
tests/qemu-iotests/tests/mirror-top-perms| 13 +-
35 files changed, 490 insertions(+), 728 deletions(-)
delete mode 100644 python/qemu/qmp/README.rst
delete mode 100644 python/qemu/aqmp/__init__.py
delete mode 100644 python/qemu/aqmp/legacy.py
delete mode 100644 python/qemu/aqmp/py.typed
rename python/qemu/{aqmp => qmp}/aqmp_tui.py (99%)
rename python/qemu/{aqmp => qmp}/error.py (87%)
rename python/qemu/{aqmp
Re: [PATCH v3 00/28] glib: Replace g_memdup() by g_memdup2()
Hi Laurent, On 9/3/21 19:44, Philippe Mathieu-Daudé wrote: > This series provides the safely equivalent g_memdup2() wrapper, > and replace all g_memdup() calls by it. > Philippe Mathieu-Daudé (28): > hw/hyperv/vmbus: Remove unused vmbus_load/save_req() > glib-compat: Introduce g_memdup2() wrapper > qapi: Replace g_memdup() by g_memdup2() > accel/tcg: Replace g_memdup() by g_memdup2() > block/qcow2-bitmap: Replace g_memdup() by g_memdup2() > softmmu: Replace g_memdup() by g_memdup2() > hw/9pfs: Replace g_memdup() by g_memdup2() > hw/acpi: Avoid truncating acpi_data_len() to 32-bit > hw/acpi: Replace g_memdup() by g_memdup2() > hw/core/machine: Replace g_memdup() by g_memdup2() > hw/hppa/machine: Replace g_memdup() by g_memdup2() > hw/i386/multiboot: Replace g_memdup() by g_memdup2() > hw/net/eepro100: Replace g_memdup() by g_memdup2() > hw/nvram/fw_cfg: Replace g_memdup() by g_memdup2() > hw/scsi/mptsas: Replace g_memdup() by g_memdup2() > hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2() > hw/rdma: Replace g_memdup() by g_memdup2() > hw/vfio/pci: Replace g_memdup() by g_memdup2() > hw/virtio: Replace g_memdup() by g_memdup2() > net/colo: Replace g_memdup() by g_memdup2() > ui/clipboard: Replace g_memdup() by g_memdup2() > linux-user: Replace g_memdup() by g_memdup2() > tests/unit: Replace g_memdup() by g_memdup2() > tests/qtest: Replace g_memdup() by g_memdup2() > target/arm: Replace g_memdup() by g_memdup2() > target/ppc: Replace g_memdup() by g_memdup2() > contrib: Replace g_memdup() by g_memdup2() > checkpatch: Do not allow deprecated g_memdup( Is it possible to take the reviewed patches 2, 24 and 28 via qemu-trivial, so the rest can be merged slowly by each submaintainer?
Re: [PATCH v2] block-backend: prevent dangling BDS pointers across aio_poll()
On Wed, Dec 15, 2021 at 04:31:26PM +0100, Kevin Wolf wrote:
> Am 15.12.2021 um 12:28 hat Stefan Hajnoczi geschrieben:
> > On Tue, Dec 14, 2021 at 03:59:49PM +0100, Kevin Wolf wrote:
> > > Am 14.12.2021 um 15:35 hat Stefan Hajnoczi geschrieben:
> > > > The BlockBackend root child can change when aio_poll() is invoked. This
> > > > happens when a temporary filter node is removed upon blockjob
> > > > completion, for example.
> > > >
> > > > Functions in block/block-backend.c must be aware of this when using a
> > > > blk_bs() pointer across aio_poll() because the BlockDriverState refcnt
> > > > may reach 0, resulting in a stale pointer.
> > > >
> > > > One example is scsi_device_purge_requests(), which calls blk_drain() to
> > > > wait for in-flight requests to cancel. If the backup blockjob is active,
> > > > then the BlockBackend root child is a temporary filter BDS owned by the
> > > > blockjob. The blockjob can complete during bdrv_drained_begin() and the
> > > > last reference to the BDS is released when the temporary filter node is
> > > > removed. This results in a use-after-free when blk_drain() calls
> > > > bdrv_drained_end(bs) on the dangling pointer.
> > > >
> > > > Explicitly hold a reference to bs across block APIs that invoke
> > > > aio_poll().
> > > >
> > > > Signed-off-by: Stefan Hajnoczi
> > > > ---
> > > > v2:
> > > > - Audit block/block-backend.c and fix additional cases
> > > > ---
> > > > block/block-backend.c | 11 +++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > index 12ef80ea17..a40ad7fa92 100644
> > > > --- a/block/block-backend.c
> > > > +++ b/block/block-backend.c
> > > > @@ -828,10 +828,12 @@ void blk_remove_bs(BlockBackend *blk)
> > > > notifier_list_notify(>remove_bs_notifiers, blk);
> > > > if (tgm->throttle_state) {
> > > > bs = blk_bs(blk);
> > > > +bdrv_ref(bs);
> > > > bdrv_drained_begin(bs);
> > > > throttle_group_detach_aio_context(tgm);
> > > > throttle_group_attach_aio_context(tgm, qemu_get_aio_context());
> > > > bdrv_drained_end(bs);
> > > > +bdrv_unref(bs);
> > > > }
> > > >
> > > > blk_update_root_state(blk);
> > >
> > > This hunk is unnecessary, we still hold a reference that is only given
> > > up a few lines down with bdrv_root_unref_child(root).
> >
> > That's not the only place where the reference can be dropped:
> > bdrv_drop_filter() removes the filter node from the graph.
> >
> > Here is a case where it happens: block/backup.c:backup_clean() ->
> > bdrv_cbw_drop() -> bdrv_drop_filter() -> bdrv_replace_node_common() ->
> > bdrv_replace_child_commit(). After we reach this bdrv_unref() is called
> > a few times and all references are dropped because the node is no longer
> > in the graph.
> >
> > This happens during blk_remove_bs() -> bdrv_drained_begin(), so the bs
> > pointer in the above hunk can be stale.
>
> Is the scenario that blk->root doesn't go away, but the node it
> references changes? So the unref below will be for a different node than
> we're draining here?
Yes, exactly.
> If so, let's add a comment that blk_bs(blk) can change after the drain,
> and maybe move the BlockDriverState *bs declaration inside the if block
> because the variable is invalid after it anyway.
Will fix.
> Can it also happen that instead of removing a node from the chain, a new
> one is inserted and we actually end up having drained the wrong node
> before switching the context for tgm?
I'll investigate that. There is already some level of support for
draining new nodes but I'm not sure it covers all insert and replace
cases.
Stefan
signature.asc
Description: PGP signature
Re: [PATCH v2] block-backend: prevent dangling BDS pointers across aio_poll()
Am 15.12.2021 um 12:28 hat Stefan Hajnoczi geschrieben:
> On Tue, Dec 14, 2021 at 03:59:49PM +0100, Kevin Wolf wrote:
> > Am 14.12.2021 um 15:35 hat Stefan Hajnoczi geschrieben:
> > > The BlockBackend root child can change when aio_poll() is invoked. This
> > > happens when a temporary filter node is removed upon blockjob
> > > completion, for example.
> > >
> > > Functions in block/block-backend.c must be aware of this when using a
> > > blk_bs() pointer across aio_poll() because the BlockDriverState refcnt
> > > may reach 0, resulting in a stale pointer.
> > >
> > > One example is scsi_device_purge_requests(), which calls blk_drain() to
> > > wait for in-flight requests to cancel. If the backup blockjob is active,
> > > then the BlockBackend root child is a temporary filter BDS owned by the
> > > blockjob. The blockjob can complete during bdrv_drained_begin() and the
> > > last reference to the BDS is released when the temporary filter node is
> > > removed. This results in a use-after-free when blk_drain() calls
> > > bdrv_drained_end(bs) on the dangling pointer.
> > >
> > > Explicitly hold a reference to bs across block APIs that invoke
> > > aio_poll().
> > >
> > > Signed-off-by: Stefan Hajnoczi
> > > ---
> > > v2:
> > > - Audit block/block-backend.c and fix additional cases
> > > ---
> > > block/block-backend.c | 11 +++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > index 12ef80ea17..a40ad7fa92 100644
> > > --- a/block/block-backend.c
> > > +++ b/block/block-backend.c
> > > @@ -828,10 +828,12 @@ void blk_remove_bs(BlockBackend *blk)
> > > notifier_list_notify(>remove_bs_notifiers, blk);
> > > if (tgm->throttle_state) {
> > > bs = blk_bs(blk);
> > > +bdrv_ref(bs);
> > > bdrv_drained_begin(bs);
> > > throttle_group_detach_aio_context(tgm);
> > > throttle_group_attach_aio_context(tgm, qemu_get_aio_context());
> > > bdrv_drained_end(bs);
> > > +bdrv_unref(bs);
> > > }
> > >
> > > blk_update_root_state(blk);
> >
> > This hunk is unnecessary, we still hold a reference that is only given
> > up a few lines down with bdrv_root_unref_child(root).
>
> That's not the only place where the reference can be dropped:
> bdrv_drop_filter() removes the filter node from the graph.
>
> Here is a case where it happens: block/backup.c:backup_clean() ->
> bdrv_cbw_drop() -> bdrv_drop_filter() -> bdrv_replace_node_common() ->
> bdrv_replace_child_commit(). After we reach this bdrv_unref() is called
> a few times and all references are dropped because the node is no longer
> in the graph.
>
> This happens during blk_remove_bs() -> bdrv_drained_begin(), so the bs
> pointer in the above hunk can be stale.
Is the scenario that blk->root doesn't go away, but the node it
references changes? So the unref below will be for a different node than
we're draining here?
If so, let's add a comment that blk_bs(blk) can change after the drain,
and maybe move the BlockDriverState *bs declaration inside the if block
because the variable is invalid after it anyway.
Can it also happen that instead of removing a node from the chain, a new
one is inserted and we actually end up having drained the wrong node
before switching the context for tgm?
Kevin
signature.asc
Description: PGP signature
[PATCH] block/file-posix: Simplify the XFS_IOC_DIOINFO handling
The handling for the XFS_IOC_DIOINFO ioctl is currently quite excessive:
This is not a "real" feature like the other features that we provide with
the "--enable-xxx" and "--disable-xxx" switches for the configure script,
since this does not influence lots of code (it's only about one call to
xfsctl() in file-posix.c), so people don't gain much with the ability to
disable this with "--disable-xfsctl".
It's also unfortunate that the ioctl will be disabled on Linux in case
the user did not install the right xfsprogs-devel package before running
configure. Thus let's simplify this by providing the ioctl definition
on our own, so we can completely get rid of the header dependency and
thus the related code in the configure script.
Suggested-by: Paolo Bonzini
Signed-off-by: Thomas Huth
---
block/file-posix.c | 37 -
configure | 31 ---
meson.build| 1 -
3 files changed, 16 insertions(+), 53 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index b283093e5b..1f1756e192 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -106,10 +106,6 @@
#include
#endif
-#ifdef CONFIG_XFS
-#include
-#endif
-
/* OS X does not have O_DSYNC */
#ifndef O_DSYNC
#ifdef O_SYNC
@@ -156,9 +152,6 @@ typedef struct BDRVRawState {
int perm_change_flags;
BDRVReopenState *reopen_state;
-#ifdef CONFIG_XFS
-bool is_xfs:1;
-#endif
bool has_discard:1;
bool has_write_zeroes:1;
bool discard_zeroes:1;
@@ -409,14 +402,22 @@ static void raw_probe_alignment(BlockDriverState *bs, int
fd, Error **errp)
if (probe_logical_blocksize(fd, >bl.request_alignment) < 0) {
bs->bl.request_alignment = 0;
}
-#ifdef CONFIG_XFS
-if (s->is_xfs) {
-struct dioattr da;
-if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, ) >= 0) {
-bs->bl.request_alignment = da.d_miniosz;
-/* The kernel returns wrong information for d_mem */
-/* s->buf_align = da.d_mem; */
-}
+
+#ifdef __linux__
+/*
+ * The XFS ioctl definitions are shipped in extra packages that might
+ * not always be available. Since we just need the XFS_IOC_DIOINFO ioctl
+ * here, we simply use our own definition instead:
+ */
+struct xfs_dioattr {
+uint32_t d_mem;
+uint32_t d_miniosz;
+uint32_t d_maxiosz;
+} da;
+if (ioctl(fd, _IOR('X', 30, struct xfs_dioattr), ) >= 0) {
+bs->bl.request_alignment = da.d_miniosz;
+/* The kernel returns wrong information for d_mem */
+/* s->buf_align = da.d_mem; */
}
#endif
@@ -798,12 +799,6 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
#endif
s->needs_alignment = raw_needs_alignment(bs);
-#ifdef CONFIG_XFS
-if (platform_test_xfs_fd(s->fd)) {
-s->is_xfs = true;
-}
-#endif
-
bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK;
if (S_ISREG(st.st_mode)) {
/* When extending regular files, we get zeros from the OS */
diff --git a/configure b/configure
index bb99a40ed0..e0d8bedd3a 100755
--- a/configure
+++ b/configure
@@ -291,7 +291,6 @@ EXTRA_CXXFLAGS=""
EXTRA_LDFLAGS=""
xen_ctrl_version="$default_feature"
-xfs="$default_feature"
membarrier="$default_feature"
vhost_kernel="$default_feature"
vhost_net="$default_feature"
@@ -1020,10 +1019,6 @@ for opt do
;;
--enable-opengl) opengl="yes"
;;
- --disable-xfsctl) xfs="no"
- ;;
- --enable-xfsctl) xfs="yes"
- ;;
--disable-zlib-test)
;;
--enable-guest-agent) guest_agent="yes"
@@ -1448,7 +1443,6 @@ cat << EOF
avx512f AVX512F optimization support
replication replication support
opengl opengl support
- xfsctl xfsctl support
qom-cast-debug cast debugging support
tools build qemu-io, qemu-nbd and qemu-img tools
bochs bochs image format support
@@ -2340,28 +2334,6 @@ EOF
fi
fi
-##
-# xfsctl() probe, used for file-posix.c
-if test "$xfs" != "no" ; then
- cat > $TMPC << EOF
-#include /* NULL */
-#include
-int main(void)
-{
-xfsctl(NULL, 0, 0, NULL);
-return 0;
-}
-EOF
- if compile_prog "" "" ; then
-xfs="yes"
- else
-if test "$xfs" = "yes" ; then
- feature_not_found "xfs" "Install xfsprogs/xfslibs devel"
-fi
-xfs=no
- fi
-fi
-
##
# plugin linker support probe
@@ -3478,9 +3450,6 @@ echo "CONFIG_BDRV_RO_WHITELIST=$block_drv_ro_whitelist"
>> $config_host_mak
if test "$block_drv_whitelist_tools" = "yes" ; then
echo "CONFIG_BDRV_WHITELIST_TOOLS=y" >> $config_host_mak
fi
-if test "$xfs" = "yes" ; then
- echo "CONFIG_XFS=y" >> $config_host_mak
-fi
qemu_version=$(head $source_path/VERSION)
echo "PKGVERSION=$pkgversion" >>$config_host_mak
echo "SRC_PATH=$source_path" >> $config_host_mak
diff --git a/meson.build b/meson.build
index
Re: [RFC PATCH 0/6] Removal of Aiocontext lock and usage of subtree drains in aborted transactions
On 14.12.21 19:10, Emanuele Giuseppe Esposito wrote: On 13/12/2021 15:52, Stefan Hajnoczi wrote: Off-topic: I don't understand the difference between the effects of bdrv_drained_begin() and bdrv_subtree_drained_begin(). Both call aio_disable_external(aio_context) and aio_poll(). bdrv_drained_begin() only polls parents and itself, while bdrv_subtree_drained_begin() also polls children. But why does that distinction matter? I wouldn't know when to use one over the other. Good point. Now I am wondering the same, so it would be great if anyone could clarify it. As far as I understand, bdrv_drained_begin() is used to drain and stop requests on a single BDS, whereas bdrv_subtree_drained_begin() drains the BDS and all of its children. So when you don’t care about lingering requests in child nodes, then bdrv_drained_begin() suffices. Hanna
[PATCH v3 1/3] block_int: make bdrv_backing_overridden static
bdrv_backing_overridden is only used in block.c, so there is
no need to leave it in block_int.h
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
block.c | 4 +++-
include/block/block_int.h | 3 ---
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 0ac5b163d2..10346b5011 100644
--- a/block.c
+++ b/block.c
@@ -103,6 +103,8 @@ static int bdrv_reopen_prepare(BDRVReopenState
*reopen_state,
static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
+static bool bdrv_backing_overridden(BlockDriverState *bs);
+
/* If non-zero, use only whitelisted block drivers */
static int use_bdrv_whitelist;
@@ -7475,7 +7477,7 @@ static bool append_strong_runtime_options(QDict *d,
BlockDriverState *bs)
/* Note: This function may return false positives; it may return true
* even if opening the backing file specified by bs's image header
* would result in exactly bs->backing. */
-bool bdrv_backing_overridden(BlockDriverState *bs)
+static bool bdrv_backing_overridden(BlockDriverState *bs)
{
if (bs->backing) {
return strcmp(bs->auto_backing_file,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f4c75e8ba9..27008cfb22 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1122,9 +1122,6 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int
buf_size,
void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
QDict *options);
-bool bdrv_backing_overridden(BlockDriverState *bs);
-
-
/**
* bdrv_add_aio_context_notifier:
*
--
2.31.1
[PATCH v3 0/3] block: minor refactoring in preparation to the block layer API split
These patches are taken from my old patches and feedback of my series "block layer: split block APIs in global state and I/O". The reason for a separate series is that the original one is already too long, and these patches are just refactoring the code, mainly deleting or moving functions in blockdev.h and block_int.h. Signed-off-by: Emanuele Giuseppe Esposito --- v3: * Apply Kevin comments, remove getter method added in v2 and do not touch drive_add(). v2: * Apply Philippe comments, instead of renaming a make if_name public, create a getter method (discard old patch 2). Emanuele Giuseppe Esposito (3): block_int: make bdrv_backing_overridden static include/sysemu/blockdev.h: remove drive_mark_claimed_by_board and inline drive_def include/sysemu/blockdev.h: remove drive_get_max_devs block.c| 4 +++- block/monitor/block-hmp-cmds.c | 2 +- blockdev.c | 24 +--- include/block/block_int.h | 3 --- include/sysemu/blockdev.h | 3 --- softmmu/vl.c | 4 +++- 6 files changed, 8 insertions(+), 32 deletions(-) -- 2.31.1
[PATCH v3 3/3] include/sysemu/blockdev.h: remove drive_get_max_devs
Remove drive_get_max_devs, as it is not used by anyone.
Last use was removed in commit 8f2d75e81d5
("hw: Drop superfluous special checks for orphaned -drive").
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Stefan Hajnoczi
---
blockdev.c| 17 -
include/sysemu/blockdev.h | 1 -
2 files changed, 18 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 9839e234cb..5f332c6ebd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -168,23 +168,6 @@ void blockdev_auto_del(BlockBackend *blk)
}
}
-/**
- * Returns the current mapping of how many units per bus
- * a particular interface can support.
- *
- * A positive integer indicates n units per bus.
- * 0 implies the mapping has not been established.
- * -1 indicates an invalid BlockInterfaceType was given.
- */
-int drive_get_max_devs(BlockInterfaceType type)
-{
-if (type >= IF_IDE && type < IF_COUNT) {
-return if_max_devs[type];
-}
-
-return -1;
-}
-
static int drive_index_to_bus_id(BlockInterfaceType type, int index)
{
int max_devs = if_max_devs[type];
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index b236425c6e..9e4c68b6ca 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -48,7 +48,6 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int
unit);
void drive_check_orphaned(void);
DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
int drive_get_max_bus(BlockInterfaceType type);
-int drive_get_max_devs(BlockInterfaceType type);
DriveInfo *drive_get_next(BlockInterfaceType type);
QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
--
2.31.1
[PATCH v3 2/3] include/sysemu/blockdev.h: remove drive_mark_claimed_by_board and inline drive_def
drive_def is only a particular use case of
qemu_opts_parse_noisily, so it can be inlined.
Also remove drive_mark_claimed_by_board, as it is only defined
but not implemented (nor used) anywhere.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/monitor/block-hmp-cmds.c | 2 +-
blockdev.c | 7 +--
include/sysemu/blockdev.h | 2 --
softmmu/vl.c | 4 +++-
4 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 2ac4aedfff..bfb3c043a0 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -101,7 +101,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
return;
}
-opts = drive_def(optstr);
+opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
if (!opts)
return;
diff --git a/blockdev.c b/blockdev.c
index b35072644e..9839e234cb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -197,17 +197,12 @@ static int drive_index_to_unit_id(BlockInterfaceType
type, int index)
return max_devs ? index % max_devs : index;
}
-QemuOpts *drive_def(const char *optstr)
-{
-return qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
-}
-
QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
const char *optstr)
{
QemuOpts *opts;
-opts = drive_def(optstr);
+opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
if (!opts) {
return NULL;
}
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 32c2d6023c..b236425c6e 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -45,14 +45,12 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
void override_max_devs(BlockInterfaceType type, int max_devs);
DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
-void drive_mark_claimed_by_board(void);
void drive_check_orphaned(void);
DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
int drive_get_max_bus(BlockInterfaceType type);
int drive_get_max_devs(BlockInterfaceType type);
DriveInfo *drive_get_next(BlockInterfaceType type);
-QemuOpts *drive_def(const char *optstr);
QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
const char *optstr);
DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 1159a64bce..fbf12f64d2 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2884,7 +2884,9 @@ void qemu_init(int argc, char **argv, char **envp)
break;
}
case QEMU_OPTION_drive:
-if (drive_def(optarg) == NULL) {
+opts = qemu_opts_parse_noisily(qemu_find_opts("drive"),
+ optarg, false);
+if (opts == NULL) {
exit(1);
}
break;
--
2.31.1
Re: [PATCH v2] block-backend: prevent dangling BDS pointers across aio_poll()
On Tue, Dec 14, 2021 at 03:59:49PM +0100, Kevin Wolf wrote:
> Am 14.12.2021 um 15:35 hat Stefan Hajnoczi geschrieben:
> > The BlockBackend root child can change when aio_poll() is invoked. This
> > happens when a temporary filter node is removed upon blockjob
> > completion, for example.
> >
> > Functions in block/block-backend.c must be aware of this when using a
> > blk_bs() pointer across aio_poll() because the BlockDriverState refcnt
> > may reach 0, resulting in a stale pointer.
> >
> > One example is scsi_device_purge_requests(), which calls blk_drain() to
> > wait for in-flight requests to cancel. If the backup blockjob is active,
> > then the BlockBackend root child is a temporary filter BDS owned by the
> > blockjob. The blockjob can complete during bdrv_drained_begin() and the
> > last reference to the BDS is released when the temporary filter node is
> > removed. This results in a use-after-free when blk_drain() calls
> > bdrv_drained_end(bs) on the dangling pointer.
> >
> > Explicitly hold a reference to bs across block APIs that invoke
> > aio_poll().
> >
> > Signed-off-by: Stefan Hajnoczi
> > ---
> > v2:
> > - Audit block/block-backend.c and fix additional cases
> > ---
> > block/block-backend.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index 12ef80ea17..a40ad7fa92 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -828,10 +828,12 @@ void blk_remove_bs(BlockBackend *blk)
> > notifier_list_notify(>remove_bs_notifiers, blk);
> > if (tgm->throttle_state) {
> > bs = blk_bs(blk);
> > +bdrv_ref(bs);
> > bdrv_drained_begin(bs);
> > throttle_group_detach_aio_context(tgm);
> > throttle_group_attach_aio_context(tgm, qemu_get_aio_context());
> > bdrv_drained_end(bs);
> > +bdrv_unref(bs);
> > }
> >
> > blk_update_root_state(blk);
>
> This hunk is unnecessary, we still hold a reference that is only given
> up a few lines down with bdrv_root_unref_child(root).
That's not the only place where the reference can be dropped:
bdrv_drop_filter() removes the filter node from the graph.
Here is a case where it happens: block/backup.c:backup_clean() ->
bdrv_cbw_drop() -> bdrv_drop_filter() -> bdrv_replace_node_common() ->
bdrv_replace_child_commit(). After we reach this bdrv_unref() is called
a few times and all references are dropped because the node is no longer
in the graph.
This happens during blk_remove_bs() -> bdrv_drained_begin(), so the bs
pointer in the above hunk can be stale.
Stefan
signature.asc
Description: PGP signature
Re: [PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def
On 15/12/2021 11:00, Kevin Wolf wrote: Am 15.12.2021 um 10:19 hat Emanuele Giuseppe Esposito geschrieben: On 14/12/2021 15:35, Kevin Wolf wrote: Am 30.11.2021 um 10:46 hat Emanuele Giuseppe Esposito geschrieben: drive_add is only used in softmmu/vl.c, so it can be a static function there, and drive_def is only a particular use case of qemu_opts_parse_noisily, so it can be inlined. Also remove drive_mark_claimed_by_board, as it is only defined but not implemented (nor used) anywhere. Signed-off-by: Emanuele Giuseppe Esposito I don't think moving drive_add() actually improves anything. Yes, you can make it static, but in order to do that you had to introduce block_if_name() as a new public function and you're moving an obviously block related function to common code in vl.c. So this part doesn't look like a net win to me. The rest of the series looks good to me. So are we going to drop patch 2 and 3? For me it is fine either way, and I saw Stefan added r-b to all patches. If we are, Kevin are you going to apply only patch 1 and 4, or do you want me to send v3? This patch does a bit more than just moving drive_add(). It also inlines drive_def() and deletes drive_mark_claimed_by_board(), which are both things that make sense to me. So this suggests a v3. But if you think I should just apply patches 1 and 4, I'm happy to do that, too. You're right, I forgot about that. I will send v3, thanks. Emanuele
Re: [PATCH-for-6.2? v2 0/3] misc: Spell QEMU all caps
Ping? On 11/19/21 10:16, Philippe Mathieu-Daudé wrote: > Replace Qemu -> QEMU. > > Supersedes: <2028143401.4101497-1-phi...@redhat.com> > > Philippe Mathieu-Daudé (3): > docs: Spell QEMU all caps > misc: Spell QEMU all caps > qga: Spell QEMU all caps > > docs/devel/modules.rst | 2 +- > docs/devel/multi-thread-tcg.rst| 2 +- > docs/devel/style.rst | 2 +- > docs/devel/ui.rst | 4 ++-- > docs/interop/nbd.txt | 6 +++--- > docs/interop/qcow2.txt | 8 > docs/multiseat.txt | 2 +- > docs/system/device-url-syntax.rst.inc | 2 +- > docs/system/i386/sgx.rst | 26 +- > docs/u2f.txt | 2 +- > qapi/block-core.json | 2 +- > python/qemu/machine/machine.py | 2 +- > qga/installer/qemu-ga.wxs | 6 +++--- > scripts/checkpatch.pl | 2 +- > scripts/render_block_graph.py | 2 +- > scripts/simplebench/bench-backup.py| 4 ++-- > scripts/simplebench/bench_block_job.py | 2 +- > target/hexagon/README | 2 +- > tests/guest-debug/run-test.py | 4 ++-- > tests/qemu-iotests/testenv.py | 2 +- > 20 files changed, 42 insertions(+), 42 deletions(-) >
Re: [PATCH-for-6.2 0/2] hw/scsi/megasas: Avoid overflowing the SGL buffer
ping? On 12/13/21 13:32, Philippe Mathieu-Daudé wrote: > Too late for 6.2 now, so Cc'ing qemu-trivial (series reviewed). > > On 11/19/21 21:11, Philippe Mathieu-Daudé wrote: >> Fix issue #521 reported by Alex some months ago: >> https://gitlab.com/qemu-project/qemu/-/issues/521 >> >> Philippe Mathieu-Daudé (2): >> hw/scsi/megasas: Fails command if SGL buffer overflows >> tests/qtest/fuzz-megasas-test: Add test for GitLab issue #521
Re: [PATCH v5 05/31] block-backend: special comments for blk_set/get_perm due to fuse
On 15/12/2021 09:57, Emanuele Giuseppe Esposito wrote:
On 10/12/2021 15:38, Hanna Reitz wrote:
On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
Fuse logic can be classified as I/O, so there is no BQL held
during its execution. And yet, it uses blk_{get/set}_perm
functions, that are classified as BQL and clearly require
the BQL lock. Since there is no easy solution for this,
add a couple of TODOs and FIXME in the relevant sections of the
code.
Well, the problem goes deeper, because we still consider them GS
functions. So it’s fine for the permission function
raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS
function, and then you still get:
qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion
`qemu_in_main_thread()' failed.
Wait... Now that I read it more carefully I am confused about this. I
don't think the above has to do with fuse, right?
Can you share the command that makes qemu-storage-daemon fail?
raw_handle_perm_lock() is currently called by these three callbacks:
.bdrv_check_perm = raw_check_perm,
.bdrv_set_perm = raw_set_perm,
.bdrv_abort_perm_update = raw_abort_perm_update,
all three function pointers are defined as GS functions. So I don't
understand why is this failing.
It looks like in this case making bdrv_get_flags() not a GS function
would be feasible and might solve the problem, but I guess we should
instead think about adding something like
if (!exp->growable && !qemu_in_main_thread()) {
/* Changing permissions like below only works in the main thread */
return -EPERM;
}
to fuse_do_truncate().
Ideally, to make up for this, we should probably take the RESIZE
permission in fuse_export_create() for writable exports in iothreads.
I think I got it. I will send the RESIZE permission fix in a separate
patch.
Thank you,
Emanuele
Re: [PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def
Am 15.12.2021 um 10:19 hat Emanuele Giuseppe Esposito geschrieben: > > > On 14/12/2021 15:35, Kevin Wolf wrote: > > Am 30.11.2021 um 10:46 hat Emanuele Giuseppe Esposito geschrieben: > > > drive_add is only used in softmmu/vl.c, so it can be a static > > > function there, and drive_def is only a particular use case of > > > qemu_opts_parse_noisily, so it can be inlined. > > > > > > Also remove drive_mark_claimed_by_board, as it is only defined > > > but not implemented (nor used) anywhere. > > > > > > Signed-off-by: Emanuele Giuseppe Esposito > > > > I don't think moving drive_add() actually improves anything. Yes, you > > can make it static, but in order to do that you had to introduce > > block_if_name() as a new public function and you're moving an obviously > > block related function to common code in vl.c. > > > > So this part doesn't look like a net win to me. The rest of the series > > looks good to me. > > > > So are we going to drop patch 2 and 3? For me it is fine either way, and I > saw Stefan added r-b to all patches. > > If we are, Kevin are you going to apply only patch 1 and 4, or do you want > me to send v3? This patch does a bit more than just moving drive_add(). It also inlines drive_def() and deletes drive_mark_claimed_by_board(), which are both things that make sense to me. So this suggests a v3. But if you think I should just apply patches 1 and 4, I'm happy to do that, too. Kevin
Re: [PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def
On 14/12/2021 15:35, Kevin Wolf wrote: Am 30.11.2021 um 10:46 hat Emanuele Giuseppe Esposito geschrieben: drive_add is only used in softmmu/vl.c, so it can be a static function there, and drive_def is only a particular use case of qemu_opts_parse_noisily, so it can be inlined. Also remove drive_mark_claimed_by_board, as it is only defined but not implemented (nor used) anywhere. Signed-off-by: Emanuele Giuseppe Esposito I don't think moving drive_add() actually improves anything. Yes, you can make it static, but in order to do that you had to introduce block_if_name() as a new public function and you're moving an obviously block related function to common code in vl.c. So this part doesn't look like a net win to me. The rest of the series looks good to me. So are we going to drop patch 2 and 3? For me it is fine either way, and I saw Stefan added r-b to all patches. If we are, Kevin are you going to apply only patch 1 and 4, or do you want me to send v3? Thank you, Emanuele
Re: [PATCH v5 05/31] block-backend: special comments for blk_set/get_perm due to fuse
On 10/12/2021 15:38, Hanna Reitz wrote:
On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
Fuse logic can be classified as I/O, so there is no BQL held
during its execution. And yet, it uses blk_{get/set}_perm
functions, that are classified as BQL and clearly require
the BQL lock. Since there is no easy solution for this,
add a couple of TODOs and FIXME in the relevant sections of the
code.
Well, the problem goes deeper, because we still consider them GS
functions. So it’s fine for the permission function
raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS function,
and then you still get:
qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion
`qemu_in_main_thread()' failed.
It looks like in this case making bdrv_get_flags() not a GS function
would be feasible and might solve the problem, but I guess we should
instead think about adding something like
if (!exp->growable && !qemu_in_main_thread()) {
/* Changing permissions like below only works in the main thread */
return -EPERM;
}
to fuse_do_truncate().
Ideally, to make up for this, we should probably take the RESIZE
permission in fuse_export_create() for writable exports in iothreads.
I think I got it. I will send the RESIZE permission fix in a separate patch.
Thank you,
Emanuele
