Re: [PATCH] fuzz: avoid building twice, when running on gitlab
+Coiby Xu & qemu-block@ On 8/9/21 9:36 PM, Peter Maydell wrote: > On Mon, 9 Aug 2021 at 20:30, Alexander Bulekov wrote: >> >> On 210809 1506, Alexander Bulekov wrote: >>> On 210809 1925, Peter Maydell wrote: On Mon, 9 Aug 2021 at 12:18, Alexander Bulekov wrote: > > On oss-fuzz, we build twice, to put together a build that is portable to > the runner containers. On gitlab ci, this is wasteful and contributes to > timeouts on the build-oss-fuzz job. Avoid building twice on gitlab, at > the remote cost of potentially missing some cases that break oss-fuzz > builds. > > Signed-off-by: Alexander Bulekov > --- > > From a couple test runs it looks like this can shave off 15-20 minutes. > > scripts/oss-fuzz/build.sh | 24 +--- > 1 file changed, 13 insertions(+), 11 deletions(-) I tried a test run with this, but it still hit the 1 hour timeout: https://gitlab.com/qemu-project/qemu/-/pipelines/350387482 >>> >>> It also timed out for me with a 120 minute timeout: >>> https://gitlab.com/a1xndr/qemu/-/jobs/1488160601 >>> >>> The log has almost exactly the same number of lines as yours, so I'm >>> guessing one of the qtests is timing out with --enable-sanitizers . > >> >> Building locally: >> $ CC=clang-11 CXX=clang++-11 ../configure --enable-fuzzing \ >> --enable-debug --enable-sanitizers >> $ make check-qtest-i386 check-unit >> >> Same as on gitlab, this times out shortly after outputting >> "sh: 1: exec: ./storage-daemon/qemu-storage-daemon: not found" >> >> Manually running qos-test, the same way check-qtest-i386 invokes it: >> >> $ QTEST_QEMU_BINARY=./qemu-system-i386 >> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon >> tests/qtest/qos-test --tap -k -m quick < /dev/null >> >> # starting vhost-user backend: exec ./storage-daemon/qemu-storage-daemon >> --blockdev driver=file,node-name=disk0,filename=qtest.XRAzzu --export >> type=vhost-user-blk,id=disk0,addr.type=unix,addr.path=/tmp/qtest-94561-sock.NdKWpt,node-name=disk0,writable=on,num-queues=1 >> sh: 1: exec: ./storage-daemon/qemu-storage-daemon: not found >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-94561.sock >> -qtest-log /dev/null -chardev socket,path=/tmp/qtest-94561.qmp,id=char0 -mon >> chardev=char0,mode=control -display none -M pc -device >> vhost-user-blk-pci,id=drv0,chardev=char1,addr=4.0 -object >> memory-backend-memfd,id=mem,size=256M,share=on -M memory-backend=mem -m >> 256M -chardev socket id=char1,path=/tmp/qtest-94561-sock.NdKWpt -accel qtest >> >> *timeout* > > vhost-user timing out in realize I suspect. I see that as > an intermittent hang in non-sanitizer configs. > > vhost-user folk: Can we either look at fixing this or else disable > the test ? (Stack backtraces available in the other email thread.) > > thanks > -- PMM >
Re: [PULL 0/2] Block patches for 6.1-rc3
On Mon, 9 Aug 2021 at 18:03, Hanna Reitz wrote: > > Hi Peter, > > Let me prefix this by saying that it's me, Max. I've changed my name > and email address. I understand freeze may not be the best of times for > this, but it looks like I can no longer send mails from the mreitz@ > address for now (only receive them). > > I've tried to create and sign the tag as Max, so I hope this pull > request won't run into any issues from that perspective. > > (For the future, I'll create a new key and hope signing it with my old > key will make it sufficiently trustworthy...) > > > The following changes since commit 632eda54043d6f26ff87dac16233e14b4708b967: > > Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' > into staging (2021-08-09 11:04:27 +0100) > > are available in the Git repository at: > > https://github.com/XanClic/qemu.git tags/pull-block-2021-08-09 > > for you to fetch changes up to a6d2bb25cf945cd16f29a575055c6f1a1f9cf6c9: > > tests: filter out TLS distinguished name in certificate checks (2021-08-09 > 17:32:43 +0200) > > > Block patches for 6.1-rc3: > - Build fix for FUSE block exports > - iotest 233 fix > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1 for any user-visible changes. -- PMM
Re: [PULL 0/2] Block patches for 6.1-rc3
On Mon, 9 Aug 2021 at 18:03, Hanna Reitz wrote: > > Hi Peter, > > Let me prefix this by saying that it's me, Max. I've changed my name > and email address. I understand freeze may not be the best of times for > this, but it looks like I can no longer send mails from the mreitz@ > address for now (only receive them). > > I've tried to create and sign the tag as Max, so I hope this pull > request won't run into any issues from that perspective. > > (For the future, I'll create a new key and hope signing it with my old > key will make it sufficiently trustworthy...) Yep, that's fine. This pullreq is going through OK (will send the usual applied/errors email once the builds have completed). I'm happy to treat signed-by-old-key as trusted enough. (In fact we already are working basically on trust-on-first-use for new keys, but if you're OK with signing your new key with the old one that does simplify things a bit.) Do you plan to send a patch to MAINTAINERS ? It has some mreitz@redhat lines in it currently. thanks -- PMM
[PULL 0/2] Block patches for 6.1-rc3
Hi Peter, Let me prefix this by saying that it's me, Max. I've changed my name and email address. I understand freeze may not be the best of times for this, but it looks like I can no longer send mails from the mreitz@ address for now (only receive them). I've tried to create and sign the tag as Max, so I hope this pull request won't run into any issues from that perspective. (For the future, I'll create a new key and hope signing it with my old key will make it sufficiently trustworthy...) The following changes since commit 632eda54043d6f26ff87dac16233e14b4708b967: Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2021-08-09 11:04:27 +0100) are available in the Git repository at: https://github.com/XanClic/qemu.git tags/pull-block-2021-08-09 for you to fetch changes up to a6d2bb25cf945cd16f29a575055c6f1a1f9cf6c9: tests: filter out TLS distinguished name in certificate checks (2021-08-09 17:32:43 +0200) Block patches for 6.1-rc3: - Build fix for FUSE block exports - iotest 233 fix Daniel P. Berrangé (1): tests: filter out TLS distinguished name in certificate checks Fabrice Fontaine (1): block/export/fuse.c: fix musl build block/export/fuse.c | 8 ++-- tests/qemu-iotests/233 | 2 +- tests/qemu-iotests/233.out | 4 ++-- tests/qemu-iotests/common.filter | 5 + 4 files changed, 14 insertions(+), 5 deletions(-) -- 2.31.1
[PULL 2/2] tests: filter out TLS distinguished name in certificate checks
From: Daniel P. Berrangé
The version of GNUTLS in Fedora 34 has changed the order in which encodes
fields when generating new TLS certificates. This in turn changes the
order seen when querying the distinguished name. This ultimately breaks
the expected output in the NBD TLS iotests. We don't need to be
comparing the exact distinguished name text for the purpose of the test
though, so it is fine to filter it out.
Reported-by: Eric Blake
Signed-off-by: Daniel P. Berrangé
Message-Id: <20210804180330.3469683-1-berra...@redhat.com>
Reviewed-by: Eric Blake
Tested-by: Eric Blake
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/233 | 2 +-
tests/qemu-iotests/233.out | 4 ++--
tests/qemu-iotests/common.filter | 5 +
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
index da150cd27b..9ca7b68f42 100755
--- a/tests/qemu-iotests/233
+++ b/tests/qemu-iotests/233
@@ -148,7 +148,7 @@ $QEMU_IMG info --image-opts \
echo
echo "== final server log =="
-cat "$TEST_DIR/server.log"
+cat "$TEST_DIR/server.log" | _filter_authz_check_tls
rm -f "$TEST_DIR/server.log"
# success, all done
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index c3c344811b..4b1f6a0e15 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -65,6 +65,6 @@ qemu-img: Could not open
'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': F
== final server log ==
qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
-qemu-nbd: option negotiation failed: TLS x509 authz check for
CN=localhost,O=Cthulhu Dark Lord Enterprises client1,L=R'lyeh,C=South Pacific
is denied
-qemu-nbd: option negotiation failed: TLS x509 authz check for
CN=localhost,O=Cthulhu Dark Lord Enterprises client3,L=R'lyeh,C=South Pacific
is denied
+qemu-nbd: option negotiation failed: TLS x509 authz check for
DISTINGUISHED-NAME is denied
+qemu-nbd: option negotiation failed: TLS x509 authz check for
DISTINGUISHED-NAME is denied
*** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 268b749e2f..2b2b53946c 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -332,5 +332,10 @@ for fname in fnames:
sys.stdout.write(result)'
}
+_filter_authz_check_tls()
+{
+$SED -e 's/TLS x509 authz check for .* is denied/TLS x509 authz check for
DISTINGUISHED-NAME is denied/'
+}
+
# make sure this script returns success
true
--
2.31.1
[PULL 1/2] block/export/fuse.c: fix musl build
From: Fabrice Fontaine
Fix the following build failure on musl raised since version 6.0.0 and
https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
because musl does not define FALLOC_FL_ZERO_RANGE:
../block/export/fuse.c: In function 'fuse_fallocate':
../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared (first
use in this function)
563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
| ^~~~
Fixes:
-
http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
Signed-off-by: Fabrice Fontaine
Message-Id: <20210809095101.1101336-1-fontaine.fabr...@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Denis V. Lunev
Signed-off-by: Hanna Reitz
---
block/export/fuse.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/block/export/fuse.c b/block/export/fuse.c
index ada9e263eb..fc7b07d2b5 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
-} else if (mode & FALLOC_FL_ZERO_RANGE) {
+}
+#ifdef CONFIG_FALLOCATE_ZERO_RANGE
+else if (mode & FALLOC_FL_ZERO_RANGE) {
if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
/* No need for zeroes, we are going to write them ourselves */
ret = fuse_do_truncate(exp, offset + length, false,
@@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
-} else if (!mode) {
+}
+#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
+else if (!mode) {
/* We can only fallocate at the EOF with a truncate */
if (offset < blk_len) {
fuse_reply_err(req, EOPNOTSUPP);
--
2.31.1
Re: [PULL for-6.1 0/1] hw/nvme fixes
On Mon, 9 Aug 2021 at 11:56, Klaus Jensen wrote: > > From: Klaus Jensen > > Hi Peter, > > The following changes since commit dee64246ded3aa7dbada68b96ce1c64e5bea327d: > > Merge remote-tracking branch > 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging > (2021-08-06 10:28:33 +0100) > > are available in the Git repository at: > > git://git.infradead.org/qemu-nvme.git tags/nvme-next-pull-request > > for you to fetch changes up to 5f4884c4412318a1adc105dea9cc28f7625ce730: > > hw/nvme: fix missing variable initializers (2021-08-09 12:52:16 +0200) > > > hw/nvme fixes > > * coverity fixes > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1 for any user-visible changes. -- PMM
Re: [PATCH for 6.1] tests: filter out TLS distinguished name in certificate checks
On 04.08.21 20:03, Daniel P. Berrangé wrote: The version of GNUTLS in Fedora 34 has changed the order in which encodes fields when generating new TLS certificates. This in turn changes the order seen when querying the distinguished name. This ultimately breaks the expected output in the NBD TLS iotests. We don't need to be comparing the exact distinguished name text for the purpose of the test though, so it is fine to filter it out. Reported-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- tests/qemu-iotests/233 | 2 +- tests/qemu-iotests/233.out | 4 ++-- tests/qemu-iotests/common.filter | 5 + 3 files changed, 8 insertions(+), 3 deletions(-) Thanks, applied to my block branch: https://github.com/XanClic/qemu/commits/block (Given my email address change today, I don’t know yet how well the pull request will go. Perhaps I’ll have to ask Kevin or Eric to step in on this one.) Hanna
[PATCH] block/rbd: implement bdrv_co_block_status
Signed-off-by: Peter Lieven
---
block/rbd.c | 119
1 file changed, 119 insertions(+)
diff --git a/block/rbd.c b/block/rbd.c
index dcf82b15b8..ef1eaa6af3 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -88,6 +88,7 @@ typedef struct BDRVRBDState {
char *namespace;
uint64_t image_size;
uint64_t object_size;
+uint64_t features;
} BDRVRBDState;
typedef struct RBDTask {
@@ -983,6 +984,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
s->image_size = info.size;
s->object_size = info.obj_size;
+r = rbd_get_features(s->image, >features);
+if (r < 0) {
+error_setg_errno(errp, -r, "error getting image features from %s",
+ s->image_name);
+rbd_close(s->image);
+goto failed_open;
+}
+
/* If we are using an rbd snapshot, we must be r/o, otherwise
* leave as-is */
if (s->snap != NULL) {
@@ -1259,6 +1268,115 @@ static ImageInfoSpecific
*qemu_rbd_get_specific_info(BlockDriverState *bs,
return spec_info;
}
+typedef struct rbd_diff_req {
+uint64_t offs;
+uint64_t bytes;
+int exists;
+} rbd_diff_req;
+
+static int qemu_rbd_co_block_status_cb(uint64_t offs, size_t len,
+ int exists, void *opaque)
+{
+struct rbd_diff_req *req = opaque;
+
+assert(req->offs + req->bytes <= offs);
+assert(offs >= req->offs + req->bytes);
+
+if (req->exists && offs > req->offs + req->bytes) {
+/*
+ * we started in an allocated area and jumped over an unallocated area,
+ * req->bytes contains the length of the allocated area before the
+ * unallocated area. stop further processing.
+ */
+return -9000;
+}
+if (req->exists && !exists) {
+/*
+ * we started in an allocated area and reached a hole. req->bytes
+ * contains the length of the allocated area before the hole.
+ * stop further processing.
+ */
+return -9000;
+}
+if (!req->exists && exists && offs > req->offs) {
+/*
+ * we started in an unallocated area and hit the first allocated
+ * block. req->bytes must be set to the length of the unallocated area
+ * before the allocated area. stop further processing.
+ */
+req->bytes = offs - req->offs;
+return -9000;
+}
+
+/*
+ * assert that we catched all cases above and allocation state has not
+ * changed during callbacks.
+ */
+assert(exists == req->exists || !req->bytes);
+req->exists = exists;
+
+/*
+ * assert that we either return an unallocated block or have got callbacks
+ * for all allocated blocks present.
+ */
+assert(!req->exists || offs == req->offs + req->bytes);
+req->bytes = offs + len - req->offs;
+
+return 0;
+}
+
+static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
+ bool want_zero, int64_t
offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
+{
+BDRVRBDState *s = bs->opaque;
+int ret, r;
+struct rbd_diff_req req = { .offs = offset };
+
+assert(offset + bytes <= s->image_size);
+
+/* default to all sectors allocated */
+ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+if (map) {
+*map = offset;
+}
+*pnum = bytes;
+
+/* RBD image does not support fast-diff */
+if (!(s->features & RBD_FEATURE_FAST_DIFF)) {
+goto out;
+}
+
+r = rbd_diff_iterate2(s->image, NULL, offset, bytes, true, true,
+ qemu_rbd_co_block_status_cb, );
+if (r < 0 && r != -9000) {
+goto out;
+}
+assert(req.bytes <= bytes);
+if (!req.exists) {
+if (r == 0 && !req.bytes) {
+/*
+ * rbd_diff_iterate2 does not invoke callbacks for unallocated
areas
+ * except for the case where an overlay has a hole where the parent
+ * has not. This here catches the case where no callback was
+ * invoked at all.
+ */
+req.bytes = bytes;
+}
+ret &= ~BDRV_BLOCK_DATA;
+ret |= BDRV_BLOCK_ZERO;
+}
+*pnum = req.bytes;
+
+out:
+if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
+*file = bs;
+}
+return ret;
+}
+
static int64_t qemu_rbd_getlength(BlockDriverState *bs)
{
BDRVRBDState *s = bs->opaque;
@@ -1494,6 +1612,7 @@ static BlockDriver bdrv_rbd = {
#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
.bdrv_co_pwrite_zeroes = qemu_rbd_co_pwrite_zeroes,
#endif
+.bdrv_co_block_status = qemu_rbd_co_block_status,
.bdrv_snapshot_create = qemu_rbd_snap_create,
.bdrv_snapshot_delete =
Re: [PATCH-for-6.1 v2] block/export/fuse.c: fix musl build
On 8/9/21 1:27 PM, Philippe Mathieu-Daudé wrote:
> On 8/9/21 11:51 AM, Fabrice Fontaine wrote:
>> Fix the following build failure on musl raised since version 6.0.0 and
>> https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
>> because musl does not define FALLOC_FL_ZERO_RANGE:
>>
>> ../block/export/fuse.c: In function 'fuse_fallocate':
>> ../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared
>> (first use in this function)
>> 563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
>> | ^~~~
>>
>> Fixes:
>> -
>> http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
>>
>> Signed-off-by: Fabrice Fontaine
>> ---
>> Changes v1 -> v2 (after review of Philippe Mathieu-Daudé):
>> - Use CONFIG_FALLOCATE_ZERO_RANGE and make safer #ifdef'ry
>>
>> block/export/fuse.c | 8 ++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/export/fuse.c b/block/export/fuse.c
>> index ada9e263eb..fc7b07d2b5 100644
>> --- a/block/export/fuse.c
>> +++ b/block/export/fuse.c
>> @@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
>> inode, int mode,
>> offset += size;
>> length -= size;
>> } while (ret == 0 && length > 0);
>> -} else if (mode & FALLOC_FL_ZERO_RANGE) {
>> +}
>> +#ifdef CONFIG_FALLOCATE_ZERO_RANGE
>> +else if (mode & FALLOC_FL_ZERO_RANGE) {
>> if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
>> /* No need for zeroes, we are going to write them ourselves */
>> ret = fuse_do_truncate(exp, offset + length, false,
>> @@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
>> inode, int mode,
>> offset += size;
>> length -= size;
>> } while (ret == 0 && length > 0);
>> -} else if (!mode) {
>> +}
>> +#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
>> +else if (!mode) {
>> /* We can only fallocate at the EOF with a truncate */
>> if (offset < blk_len) {
>> fuse_reply_err(req, EOPNOTSUPP);
>>
> Reviewed-by: Philippe Mathieu-Daudé
>
Reviewed-by: Denis V. Lunev
[PULL for-6.1 1/1] hw/nvme: fix missing variable initializers
From: Klaus Jensen
Coverity found that 'uuid', 'csi' and 'eui64' are uninitialized. While
we set most of the fields, we do not explicitly set the rsvd2 field in
the NvmeIdNsDescr header.
Fix this by explicitly zero-initializing the variables.
Reported-by: Coverity (CID 1458835, 1459295 and 1459580)
Fixes: 6870cfb8140d ("hw/nvme: namespace parameter for EUI-64")
Suggested-by: Peter Maydell
Signed-off-by: Klaus Jensen
Reviewed-by: Philippe Mathieu-Daudé
---
hw/nvme/ctrl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 43dfaeac9f54..6baf9e0420d5 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4663,15 +4663,15 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
*n, NvmeRequest *req)
struct {
NvmeIdNsDescr hdr;
uint8_t v[NVME_NIDL_UUID];
-} QEMU_PACKED uuid;
+} QEMU_PACKED uuid = {};
struct {
NvmeIdNsDescr hdr;
uint64_t v;
-} QEMU_PACKED eui64;
+} QEMU_PACKED eui64 = {};
struct {
NvmeIdNsDescr hdr;
uint8_t v;
-} QEMU_PACKED csi;
+} QEMU_PACKED csi = {};
trace_pci_nvme_identify_ns_descr_list(nsid);
--
2.32.0
[PULL for-6.1 0/1] hw/nvme fixes
From: Klaus Jensen Hi Peter, The following changes since commit dee64246ded3aa7dbada68b96ce1c64e5bea327d: Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging (2021-08-06 10:28:33 +0100) are available in the Git repository at: git://git.infradead.org/qemu-nvme.git tags/nvme-next-pull-request for you to fetch changes up to 5f4884c4412318a1adc105dea9cc28f7625ce730: hw/nvme: fix missing variable initializers (2021-08-09 12:52:16 +0200) hw/nvme fixes * coverity fixes Klaus Jensen (1): hw/nvme: fix missing variable initializers hw/nvme/ctrl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.32.0
Re: [PATCH-for-6.1?] hw/nvme: fix missing variable initializers
On Aug 9 12:47, Philippe Mathieu-Daudé wrote:
> On 8/9/21 12:43 PM, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Coverity found that 'uuid', 'csi' and 'eui64' are uninitialized. While
> > we set most of the fields, we do not explicitly set the rsvd2 field in
> > the NvmeIdNsDescr header.
> >
> > Fix this by explicitly zero-initializing the variables.
> >
> > Reported-by: Coverity (CID 1458835, 1459295 and 1459580)
> > Fixes: 6870cfb8140d ("hw/nvme: namespace parameter for EUI-64")
> > Suggested-by: Peter Maydell
> > Signed-off-by: Klaus Jensen
> > ---
> > hw/nvme/ctrl.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
>
> Reviewed-by: Philippe Mathieu-Daudé
>
Swift as always Philippe, thanks!
Yes, I'll PR this for -rc3 immediately.
signature.asc
Description: PGP signature
Re: [PATCH-for-6.1?] hw/nvme: fix missing variable initializers
On 8/9/21 12:43 PM, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Coverity found that 'uuid', 'csi' and 'eui64' are uninitialized. While
> we set most of the fields, we do not explicitly set the rsvd2 field in
> the NvmeIdNsDescr header.
>
> Fix this by explicitly zero-initializing the variables.
>
> Reported-by: Coverity (CID 1458835, 1459295 and 1459580)
> Fixes: 6870cfb8140d ("hw/nvme: namespace parameter for EUI-64")
> Suggested-by: Peter Maydell
> Signed-off-by: Klaus Jensen
> ---
> hw/nvme/ctrl.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PULL 06/23] hw/nvme: namespace parameter for EUI-64
On Aug 9 11:18, Peter Maydell wrote:
> On Tue, 29 Jun 2021 at 19:48, Klaus Jensen wrote:
> >
> > From: Heinrich Schuchardt
> >
> > The EUI-64 field is the only identifier for NVMe namespaces in UEFI device
> > paths. Add a new namespace property "eui64", that provides the user the
> > option to specify the EUI-64.
>
> Hi; Coverity complains about some uses of uninitialized data in this
> code (CID 1458835 1459295 1459580). I think the bug was present in
> the previous version of this function, but this was the last change
> to touch it...
>
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 7dea64b72e6a..762bb82e3cac 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -4426,19 +4426,19 @@ static uint16_t
> > nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
> > NvmeIdentify *c = (NvmeIdentify *)>cmd;
> > uint32_t nsid = le32_to_cpu(c->nsid);
> > uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> > -
> > -struct data {
> > -struct {
> > -NvmeIdNsDescr hdr;
> > -uint8_t v[NVME_NIDL_UUID];
> > -} uuid;
> > -struct {
> > -NvmeIdNsDescr hdr;
> > -uint8_t v;
> > -} csi;
> > -};
> > -
> > -struct data *ns_descrs = (struct data *)list;
> > +uint8_t *pos = list;
> > +struct {
> > +NvmeIdNsDescr hdr;
> > +uint8_t v[NVME_NIDL_UUID];
> > +} QEMU_PACKED uuid;
> > +struct {
> > +NvmeIdNsDescr hdr;
> > +uint64_t v;
> > +} QEMU_PACKED eui64;
> > +struct {
> > +NvmeIdNsDescr hdr;
> > +uint8_t v;
> > +} QEMU_PACKED csi;
>
> Here we define locals 'uuid', 'eui64', 'csi', without an initializer.
>
> > trace_pci_nvme_identify_ns_descr_list(nsid);
> >
> > @@ -4452,17 +4452,29 @@ static uint16_t
> > nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
> > }
> >
> > /*
> > - * Because the NGUID and EUI64 fields are 0 in the Identify Namespace
> > data
> > - * structure, a Namespace UUID (nidt = 3h) must be reported in the
> > - * Namespace Identification Descriptor. Add the namespace UUID here.
> > + * If the EUI-64 field is 0 and the NGUID field is 0, the namespace
> > must
> > + * provide a valid Namespace UUID in the Namespace Identification
> > Descriptor
> > + * data structure. QEMU does not yet support setting NGUID.
> > */
> > -ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
> > -ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
> > -memcpy(_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
> > +uuid.hdr.nidt = NVME_NIDT_UUID;
> > +uuid.hdr.nidl = NVME_NIDL_UUID;
> > +memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
>
> Here we fill in some fields of uuid, but we don't touch uuid.hdr.rsvd2[],
> which remains thus 2 bytes of uninitialized junk from our stack.
>
> > +memcpy(pos, , sizeof(uuid));
>
> Here we copy all of uuid to a buffer which we're going to hand
> to the guest, so we've just given it two bytes of QEMU stack data
> that it shouldn't really be able to look at.
>
> > +pos += sizeof(uuid);
>
> >
> > -ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
> > -ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
> > -ns_descrs->csi.v = ns->csi;
> > +if (ns->params.eui64) {
> > +eui64.hdr.nidt = NVME_NIDT_EUI64;
> > +eui64.hdr.nidl = NVME_NIDL_EUI64;
> > +eui64.v = cpu_to_be64(ns->params.eui64);
> > +memcpy(pos, , sizeof(eui64));
> > +pos += sizeof(eui64);
> > +}
> > +
> > +csi.hdr.nidt = NVME_NIDT_CSI;
> > +csi.hdr.nidl = NVME_NIDL_CSI;
> > +csi.v = ns->csi;
> > +memcpy(pos, , sizeof(csi));
> > +pos += sizeof(csi);
>
> We do the same thing for the rsvd2[] bytes in csi.hdr and eui64.hdr.
>
> > return nvme_c2h(n, list, sizeof(list), req);
> > }
>
> Explicitly zero-initializing uuid, csi, eui64 with "= { }" would
> be the most robust fix. If you think it's worth avoiding "zero
> init and then overwrite 90% of the fields anyway" then you could
> explicitly zero the .hdr.rsvd2 bytes.
>
Thanks Peter,
Fix posted!
signature.asc
Description: PGP signature
[PATCH] hw/nvme: fix missing variable initializers
From: Klaus Jensen
Coverity found that 'uuid', 'csi' and 'eui64' are uninitialized. While
we set most of the fields, we do not explicitly set the rsvd2 field in
the NvmeIdNsDescr header.
Fix this by explicitly zero-initializing the variables.
Reported-by: Coverity (CID 1458835, 1459295 and 1459580)
Fixes: 6870cfb8140d ("hw/nvme: namespace parameter for EUI-64")
Suggested-by: Peter Maydell
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 43dfaeac9f54..6baf9e0420d5 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4663,15 +4663,15 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
*n, NvmeRequest *req)
struct {
NvmeIdNsDescr hdr;
uint8_t v[NVME_NIDL_UUID];
-} QEMU_PACKED uuid;
+} QEMU_PACKED uuid = {};
struct {
NvmeIdNsDescr hdr;
uint64_t v;
-} QEMU_PACKED eui64;
+} QEMU_PACKED eui64 = {};
struct {
NvmeIdNsDescr hdr;
uint8_t v;
-} QEMU_PACKED csi;
+} QEMU_PACKED csi = {};
trace_pci_nvme_identify_ns_descr_list(nsid);
--
2.32.0
Re: [PATCH-for-6.1 v2] block/export/fuse.c: fix musl build
On 8/9/21 11:51 AM, Fabrice Fontaine wrote:
> Fix the following build failure on musl raised since version 6.0.0 and
> https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
> because musl does not define FALLOC_FL_ZERO_RANGE:
>
> ../block/export/fuse.c: In function 'fuse_fallocate':
> ../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared
> (first use in this function)
> 563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
> | ^~~~
>
> Fixes:
> -
> http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
>
> Signed-off-by: Fabrice Fontaine
> ---
> Changes v1 -> v2 (after review of Philippe Mathieu-Daudé):
> - Use CONFIG_FALLOCATE_ZERO_RANGE and make safer #ifdef'ry
>
> block/export/fuse.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index ada9e263eb..fc7b07d2b5 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
> inode, int mode,
> offset += size;
> length -= size;
> } while (ret == 0 && length > 0);
> -} else if (mode & FALLOC_FL_ZERO_RANGE) {
> +}
> +#ifdef CONFIG_FALLOCATE_ZERO_RANGE
> +else if (mode & FALLOC_FL_ZERO_RANGE) {
> if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
> /* No need for zeroes, we are going to write them ourselves */
> ret = fuse_do_truncate(exp, offset + length, false,
> @@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
> inode, int mode,
> offset += size;
> length -= size;
> } while (ret == 0 && length > 0);
> -} else if (!mode) {
> +}
> +#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
> +else if (!mode) {
> /* We can only fallocate at the EOF with a truncate */
> if (offset < blk_len) {
> fuse_reply_err(req, EOPNOTSUPP);
>
Reviewed-by: Philippe Mathieu-Daudé
Re: [PULL 06/23] hw/nvme: namespace parameter for EUI-64
On Tue, 29 Jun 2021 at 19:48, Klaus Jensen wrote:
>
> From: Heinrich Schuchardt
>
> The EUI-64 field is the only identifier for NVMe namespaces in UEFI device
> paths. Add a new namespace property "eui64", that provides the user the
> option to specify the EUI-64.
Hi; Coverity complains about some uses of uninitialized data in this
code (CID 1458835 1459295 1459580). I think the bug was present in
the previous version of this function, but this was the last change
to touch it...
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 7dea64b72e6a..762bb82e3cac 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4426,19 +4426,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
> *n, NvmeRequest *req)
> NvmeIdentify *c = (NvmeIdentify *)>cmd;
> uint32_t nsid = le32_to_cpu(c->nsid);
> uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> -
> -struct data {
> -struct {
> -NvmeIdNsDescr hdr;
> -uint8_t v[NVME_NIDL_UUID];
> -} uuid;
> -struct {
> -NvmeIdNsDescr hdr;
> -uint8_t v;
> -} csi;
> -};
> -
> -struct data *ns_descrs = (struct data *)list;
> +uint8_t *pos = list;
> +struct {
> +NvmeIdNsDescr hdr;
> +uint8_t v[NVME_NIDL_UUID];
> +} QEMU_PACKED uuid;
> +struct {
> +NvmeIdNsDescr hdr;
> +uint64_t v;
> +} QEMU_PACKED eui64;
> +struct {
> +NvmeIdNsDescr hdr;
> +uint8_t v;
> +} QEMU_PACKED csi;
Here we define locals 'uuid', 'eui64', 'csi', without an initializer.
> trace_pci_nvme_identify_ns_descr_list(nsid);
>
> @@ -4452,17 +4452,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
> *n, NvmeRequest *req)
> }
>
> /*
> - * Because the NGUID and EUI64 fields are 0 in the Identify Namespace
> data
> - * structure, a Namespace UUID (nidt = 3h) must be reported in the
> - * Namespace Identification Descriptor. Add the namespace UUID here.
> + * If the EUI-64 field is 0 and the NGUID field is 0, the namespace must
> + * provide a valid Namespace UUID in the Namespace Identification
> Descriptor
> + * data structure. QEMU does not yet support setting NGUID.
> */
> -ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
> -ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
> -memcpy(_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
> +uuid.hdr.nidt = NVME_NIDT_UUID;
> +uuid.hdr.nidl = NVME_NIDL_UUID;
> +memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
Here we fill in some fields of uuid, but we don't touch uuid.hdr.rsvd2[],
which remains thus 2 bytes of uninitialized junk from our stack.
> +memcpy(pos, , sizeof(uuid));
Here we copy all of uuid to a buffer which we're going to hand
to the guest, so we've just given it two bytes of QEMU stack data
that it shouldn't really be able to look at.
> +pos += sizeof(uuid);
>
> -ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
> -ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
> -ns_descrs->csi.v = ns->csi;
> +if (ns->params.eui64) {
> +eui64.hdr.nidt = NVME_NIDT_EUI64;
> +eui64.hdr.nidl = NVME_NIDL_EUI64;
> +eui64.v = cpu_to_be64(ns->params.eui64);
> +memcpy(pos, , sizeof(eui64));
> +pos += sizeof(eui64);
> +}
> +
> +csi.hdr.nidt = NVME_NIDT_CSI;
> +csi.hdr.nidl = NVME_NIDL_CSI;
> +csi.v = ns->csi;
> +memcpy(pos, , sizeof(csi));
> +pos += sizeof(csi);
We do the same thing for the rsvd2[] bytes in csi.hdr and eui64.hdr.
> return nvme_c2h(n, list, sizeof(list), req);
> }
Explicitly zero-initializing uuid, csi, eui64 with "= { }" would
be the most robust fix. If you think it's worth avoiding "zero
init and then overwrite 90% of the fields anyway" then you could
explicitly zero the .hdr.rsvd2 bytes.
thanks
-- PMM
Re: [PATCH for-6.2 v3 05/12] job: @force parameter for job_cancel_sync{, _all}()
On 06.08.21 21:39, Eric Blake wrote:
On Fri, Aug 06, 2021 at 11:38:52AM +0200, Max Reitz wrote:
Callers should be able to specify whether they want job_cancel_sync() to
force-cancel the job or not.
In fact, almost all invocations do not care about consistency of the
result and just want the job to terminate as soon as possible, so they
should pass force=true. The replication block driver is the exception.
This changes some iotest outputs, because quitting qemu while a mirror
job is active will now lead to it being cancelled instead of completed,
which is what we want. (Cancelling a READY mirror job with force=false
may take an indefinite amount of time, which we do not want when
quitting. If users want consistent results, they must have all jobs be
done before they quit qemu.)
Feels somewhat like a bug fix, but I also understand why you'd prefer
to delay this to 6.2 (it is not a fresh regression, but a longstanding
issue).
It is, hence the “Buglink” tag below. However, only all of this series
together really fixes that bug (or at least patches 5+7+9 together),
just taking one wouldn’t help much. And together, it’s just too much
for 6.2 at this point.
Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
Signed-off-by: Max Reitz
---
+++ b/job.c
@@ -982,12 +982,24 @@ static void job_cancel_err(Job *job, Error **errp)
job_cancel(job, false);
}
-int job_cancel_sync(Job *job)
+/**
+ * Same as job_cancel_err(), but force-cancel.
+ */
+static void job_force_cancel_err(Job *job, Error **errp)
{
-return job_finish_sync(job, _cancel_err, NULL);
+job_cancel(job, true);
+}
In isolation, it looks odd that errp is passed but not used. But
looking further, it's because this is a callback that must have a
given signature, so it's okay.
Reviewed-by: Eric Blake
Re: [PATCH for-6.2 v3 01/12] job: Context changes in job_completed_txn_abort()
On 06.08.21 21:16, Eric Blake wrote:
On Fri, Aug 06, 2021 at 11:38:48AM +0200, Max Reitz wrote:
Finalizing the job may cause its AioContext to change. This is noted by
job_exit(), which points at job_txn_apply() to take this fact into
account.
However, job_completed() does not necessarily invoke job_txn_apply()
(through job_completed_txn_success()), but potentially also
job_completed_txn_abort(). The latter stores the context in a local
variable, and so always acquires the same context at its end that it has
released in the beginning -- which may be a different context from the
one that job_exit() releases at its end. If it is different, qemu
aborts ("qemu_mutex_unlock_impl: Operation not permitted").
Is this a bug fix that needs to make it into 6.1?
Well, I only encountered it as part of this series (which I really don’t
think is 6.2 material at this point), and so I don’t know.
Can’t hurt, I suppose, but if we wanted this to be in 6.1, we’d better
have a specific test for it, I think.
Drop the local @outer_ctx variable from job_completed_txn_abort(), and
instead re-acquire the actual job's context at the end of the function,
so job_exit() will release the same.
Signed-off-by: Max Reitz
---
job.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
The commit message makes sense, and does a good job at explaining the
change. I'm still a bit fuzzy on how jobs are supposed to play nice
with contexts,
I can relate :)
but since your patch matches the commit message, I'm
happy to give:
Reviewed-by: Eric Blake
Thanks!
Re: [PATCH] block/export/fuse.c: fix musl build
On 8/9/21 10:50 AM, Fabrice Fontaine wrote:
> Fix the following build failure on musl raised since version 6.0.0 and
> https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
> because musl does not define FALLOC_FL_ZERO_RANGE:
>
> ../block/export/fuse.c: In function 'fuse_fallocate':
> ../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared
> (first use in this function)
> 563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
> | ^~~~
>
> Fixes:
> -
> http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64
>
> Signed-off-by: Fabrice Fontaine
> ---
> block/export/fuse.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index ada9e263eb..07e31129a6 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -635,6 +635,7 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
> inode, int mode,
> offset += size;
> length -= size;
> } while (ret == 0 && length > 0);
> +#ifdef FALLOC_FL_ZERO_RANGE
Please use CONFIG_FALLOCATE_ZERO_RANGE.
> } else if (mode & FALLOC_FL_ZERO_RANGE) {
> if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
> /* No need for zeroes, we are going to write them ourselves */
> @@ -654,6 +655,7 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t
> inode, int mode,
> offset += size;
> length -= size;
> } while (ret == 0 && length > 0);
> +#endif
> } else if (!mode) {
> /* We can only fallocate at the EOF with a truncate */
> if (offset < blk_len) {
>
Maybe safer #ifdef'ry as:
-- >8 --
diff --git a/block/export/fuse.c b/block/export/fuse.c
index ada9e263ebb..fc7b07d2b57 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req,
fuse_ino_t inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
-} else if (mode & FALLOC_FL_ZERO_RANGE) {
+}
+#ifdef CONFIG_FALLOCATE_ZERO_RANGE
+else if (mode & FALLOC_FL_ZERO_RANGE) {
if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
/* No need for zeroes, we are going to write them ourselves */
ret = fuse_do_truncate(exp, offset + length, false,
@@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req,
fuse_ino_t inode, int mode,
offset += size;
length -= size;
} while (ret == 0 && length > 0);
-} else if (!mode) {
+}
+#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
+else if (!mode) {
/* We can only fallocate at the EOF with a truncate */
if (offset < blk_len) {
fuse_reply_err(req, EOPNOTSUPP);
---
[PATCH v9 13/16] qemu-iotests: insert valgrind command line as wrapper for qemu binary
If -gdb and -valgrind are both defined, return an error.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
tests/qemu-iotests/iotests.py | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 85d8c0abbb..74fa56840d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -591,7 +591,11 @@ class VM(qtest.QEMUQtestMachine):
def __init__(self, path_suffix=''):
name = "qemu%s-%d" % (path_suffix, os.getpid())
timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
-super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+if qemu_gdb and qemu_valgrind:
+sys.stderr.write('gdb and valgrind are mutually exclusive\n')
+sys.exit(1)
+wrapper = qemu_gdb if qemu_gdb else qemu_valgrind
+super().__init__(qemu_prog, qemu_opts, wrapper=wrapper,
name=name,
base_temp_dir=test_dir,
socket_scm_helper=socket_scm_helper,
--
2.31.1
[PATCH v9 16/16] docs/devel/testing: add -p option to the debug section of QEMU iotests
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- docs/devel/testing.rst | 4 1 file changed, 4 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 8ebcf85a31..4a0abbf23d 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -249,6 +249,10 @@ a failing test: * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. +* ``-p`` (print) redirects QEMU’s stdout and stderr to the test output, + instead of saving it into a log file in + ``$TEST_DIR/qemu-machine-``. + Test case groups -- 2.31.1
[PATCH v9 12/16] qemu-iotests: allow valgrind to read/delete the generated log file
When using -valgrind on the script tests, it generates a log file
in $TEST_DIR that is either read (if valgrind finds problems) or
otherwise deleted. Provide the same exact behavior when using
-valgrind on the python tests.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
tests/qemu-iotests/iotests.py | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 26c580f9e7..85d8c0abbb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -598,6 +598,17 @@ def __init__(self, path_suffix=''):
sock_dir=sock_dir, qmp_timer=timer)
self._num_drives = 0
+def _post_shutdown(self) -> None:
+super()._post_shutdown()
+if not qemu_valgrind or not self._popen:
+return
+valgrind_filename = f"{test_dir}/{self._popen.pid}.valgrind"
+if self.exitcode() == 99:
+with open(valgrind_filename) as f:
+print(f.read())
+else:
+os.remove(valgrind_filename)
+
def add_object(self, opts):
self._args.append('-object')
self._args.append(opts)
--
2.31.1
[PATCH v9 14/16] docs/devel/testing: add -valgrind option to the debug section of QEMU iotests
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Max Reitz Reviewed-by: Vladimir Sementsov-Ogievskiy --- docs/devel/testing.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 01e1919873..8ebcf85a31 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -240,6 +240,12 @@ a failing test: If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored, regardless of whether it is set or not. +* ``-valgrind`` attaches a valgrind instance to QEMU. If it detects + warnings, it will print and save the log in + ``$TEST_DIR/.valgrind``. + The final command line will be ``valgrind --log-file=$TEST_DIR/ + .valgrind --error-exitcode=99 $QEMU ...`` + * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. -- 2.31.1
[PATCH v9 15/16] qemu-iotests: add option to show qemu binary logs on stdout
Using the flag -p, allow the qemu binary to print to stdout.
Also create the common function _close_qemu_log_file() to
avoid accessing machine.py private fields directly and have
duplicate code.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
python/qemu/machine/machine.py | 9 ++---
tests/qemu-iotests/check | 4 +++-
tests/qemu-iotests/iotests.py | 8
tests/qemu-iotests/testenv.py | 9 +++--
4 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 14c4d17eca..8b935813e9 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -348,6 +348,11 @@ def _post_launch(self) -> None:
if self._qmp_connection:
self._qmp.accept(self._qmp_timer)
+def _close_qemu_log_file(self) -> None:
+if self._qemu_log_file is not None:
+self._qemu_log_file.close()
+self._qemu_log_file = None
+
def _post_shutdown(self) -> None:
"""
Called to cleanup the VM instance after the process has exited.
@@ -360,9 +365,7 @@ def _post_shutdown(self) -> None:
self._qmp.close()
self._qmp_connection = None
-if self._qemu_log_file is not None:
-self._qemu_log_file.close()
-self._qemu_log_file = None
+self._close_qemu_log_file()
self._load_io_log()
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index ebd27946db..da1bfb839e 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,8 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-p', dest='print', action='store_true',
+help='redirects qemu\'s stdout and stderr to the test output')
p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_OPTIONS options \
('localhost:12345' if $GDB_OPTIONS is empty)")
@@ -119,7 +121,7 @@ if __name__ == '__main__':
aiomode=args.aiomode, cachemode=args.cachemode,
imgopts=args.imgopts, misalign=args.misalign,
debug=args.debug, valgrind=args.valgrind,
- gdb=args.gdb)
+ gdb=args.gdb, qprint=args.print)
if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
if not args.tests:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 74fa56840d..2cf5ff965b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -79,6 +79,8 @@
if gdb_qemu_env:
qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
+qemu_print = os.environ.get('PRINT_QEMU', False)
+
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
output_dir = os.environ.get('OUTPUT_DIR', '.')
@@ -613,6 +615,12 @@ def _post_shutdown(self) -> None:
else:
os.remove(valgrind_filename)
+def _pre_launch(self) -> None:
+super()._pre_launch()
+if qemu_print:
+# set QEMU binary output to stdout
+self._close_qemu_log_file()
+
def add_object(self, opts):
self._args.append('-object')
self._args.append(opts)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 8bf154376f..70da0d60c8 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -74,7 +74,7 @@ class TestEnv(ContextManager['TestEnv']):
'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
- 'GDB_OPTIONS']
+ 'GDB_OPTIONS', 'PRINT_QEMU']
def prepare_subprocess(self, args: List[str]) -> Dict[str, str]:
if self.debug:
@@ -181,7 +181,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
misalign: bool = False,
debug: bool = False,
valgrind: bool = False,
- gdb: bool = False) -> None:
+ gdb: bool = False,
+ qprint: bool = False) -> None:
self.imgfmt = imgfmt
self.imgproto = imgproto
self.aiomode = aiomode
@@ -189,6 +190,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
self.misalign = misalign
self.debug = debug
+if qprint:
+self.print_qemu = 'y'
+
if gdb:
self.gdb_options = os.getenv('GDB_OPTIONS', DEF_GDB_OPTIONS)
if not self.gdb_options:
@@ -299,6 +303,7 @@ def print_env(self) -> None:
SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
[PATCH v9 10/16] qemu-iotests: extend the check script to prepare supporting valgrind for python tests
Currently, the check script only parses the option and sets the
VALGRIND_QEMU environmental variable to "y".
Add another local python variable that prepares the command line,
identical to the one provided in the test scripts.
Because the python script does not know in advance the valgrind
PID to assign to the log file name, use the "%p" flag in valgrind
log file name that automatically puts the process PID at runtime.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Max Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/check | 7 ---
tests/qemu-iotests/iotests.py | 11 +++
tests/qemu-iotests/testenv.py | 1 +
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 4365bb8066..ebd27946db 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -39,6 +39,10 @@ def make_argparser() -> argparse.ArgumentParser:
p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_OPTIONS options \
('localhost:12345' if $GDB_OPTIONS is empty)")
+p.add_argument('-valgrind', action='store_true',
+help='use valgrind, sets VALGRIND_QEMU environment '
+'variable')
+
p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -88,9 +92,6 @@ def make_argparser() -> argparse.ArgumentParser:
g_bash.add_argument('-o', dest='imgopts',
help='options to pass to qemu-img create/convert, '
'sets IMGOPTS environment variable')
-g_bash.add_argument('-valgrind', action='store_true',
-help='use valgrind, sets VALGRIND_QEMU environment '
-'variable')
g_sel = p.add_argument_group('test selecting options',
'The following options specify test set '
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e7e3d92d3e..6aa1dc48ba 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -96,6 +96,17 @@
sys.stderr.write('Please run this test via the "check" script\n')
sys.exit(os.EX_USAGE)
+qemu_valgrind = []
+if os.environ.get('VALGRIND_QEMU') == "y" and \
+os.environ.get('NO_VALGRIND') != "y":
+valgrind_logfile = "--log-file=" + test_dir
+# %p allows to put the valgrind process PID, since
+# we don't know it a priori (subprocess.Popen is
+# not yet invoked)
+valgrind_logfile += "/%p.valgrind"
+
+qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
+
socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
luks_default_secret_object = 'secret,id=keysec0,data=' + \
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 8501c6caf5..8bf154376f 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -298,6 +298,7 @@ def print_env(self) -> None:
SOCK_DIR -- {SOCK_DIR}
SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
GDB_OPTIONS -- {GDB_OPTIONS}
+VALGRIND_QEMU -- {VALGRIND_QEMU}
"""
args = collections.defaultdict(str, self.get_env())
--
2.31.1
[PATCH v9 11/16] qemu-iotests: extend QMP socket timeout when using valgrind
As with gdbserver, valgrind delays the test execution, so the default QMP socket timeout and the generic class Timeout in iotests.py timeouts too soon. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- tests/qemu-iotests/iotests.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 6aa1dc48ba..26c580f9e7 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -488,13 +488,13 @@ def __init__(self, seconds, errmsg="Timeout"): self.seconds = seconds self.errmsg = errmsg def __enter__(self): -if qemu_gdb: +if qemu_gdb or qemu_valgrind: return self signal.signal(signal.SIGALRM, self.timeout) signal.setitimer(signal.ITIMER_REAL, self.seconds) return self def __exit__(self, exc_type, value, traceback): -if qemu_gdb: +if qemu_gdb or qemu_valgrind: return False signal.setitimer(signal.ITIMER_REAL, 0) return False @@ -590,7 +590,7 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) -timer = 15.0 if not qemu_gdb else None +timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb, name=name, base_temp_dir=test_dir, -- 2.31.1
[PATCH v9 09/16] docs/devel/testing: add -gdb option to the debugging section of QEMU iotests
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- docs/devel/testing.rst | 11 +++ 1 file changed, 11 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 8359f2ae37..01e1919873 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -229,6 +229,17 @@ Debugging a test case The following options to the ``check`` script can be useful when debugging a failing test: +* ``-gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for a + connection from a gdb client. The options given to ``gdbserver`` (e.g. the + address on which to listen for connections) are taken from the ``$GDB_OPTIONS`` + environment variable. By default (if ``$GDB_OPTIONS`` is empty), it listens on + ``localhost:12345``. + It is possible to connect to it for example with + ``gdb -iex "target remote $addr"``, where ``$addr`` is the address + ``gdbserver`` listens on. + If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored, + regardless of whether it is set or not. + * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. -- 2.31.1
[PATCH v9 08/16] qemu-iotests: add gdbserver option to script tests too
Remove read timer in test script when GDB_OPTIONS are set,
so that the bash tests won't timeout while running gdb.
The only limitation here is that running a script with gdbserver
will make the test output mismatch with the expected
results, making the test fail.
Signed-off-by: Emanuele Giuseppe Esposito
---
tests/qemu-iotests/common.qemu | 7 ++-
tests/qemu-iotests/common.rc | 8 +++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 0fc52d20d7..0f1fecc68e 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -85,7 +85,12 @@ _timed_wait_for()
timeout=yes
QEMU_STATUS[$h]=0
-while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
+read_timeout="-t ${QEMU_COMM_TIMEOUT}"
+if [ -n "${GDB_OPTIONS}" ]; then
+read_timeout=
+fi
+
+while IFS= read ${read_timeout} resp <&${QEMU_OUT[$h]}
do
if [ -n "$capture_events" ]; then
capture=0
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 609d82de89..d8582454de 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -166,8 +166,14 @@ _qemu_wrapper()
if [ -n "${QEMU_NEED_PID}" ]; then
echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
fi
+
+GDB=""
+if [ -n "${GDB_OPTIONS}" ]; then
+GDB="gdbserver ${GDB_OPTIONS}"
+fi
+
VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec
"${VALGRIND_LOGFILE}" \
-"$QEMU_PROG" $QEMU_OPTIONS "$@"
+$GDB "$QEMU_PROG" $QEMU_OPTIONS "$@"
)
RETVAL=$?
_qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
--
2.31.1
[PATCH v9 07/16] qemu_iotests: insert gdbserver command line as wrapper for qemu binary
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- tests/qemu-iotests/iotests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index e176a84620..e7e3d92d3e 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -580,7 +580,8 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) timer = 15.0 if not qemu_gdb else None -super().__init__(qemu_prog, qemu_opts, name=name, +super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb, + name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, sock_dir=sock_dir, qmp_timer=timer) -- 2.31.1
[PATCH v9 04/16] docs/devel/testing: add debug section to the QEMU iotests chapter
Introduce the "Debugging a test case" section, in preparation to the additional flags that will be added in the next patches. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz --- docs/devel/testing.rst | 8 1 file changed, 8 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 8a9cda33a5..8359f2ae37 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -224,6 +224,14 @@ another application on the host may have locked the file, possibly leading to a test failure. If using such devices are explicitly desired, consider adding ``locking=off`` option to disable image locking. +Debugging a test case +--- +The following options to the ``check`` script can be useful when debugging +a failing test: + +* ``-d`` (debug) just increases the logging verbosity, showing + for example the QMP commands and answers. + Test case groups -- 2.31.1
[PATCH v9 06/16] qemu-iotests: delay QMP socket timers
Attaching gdbserver implies that the qmp socket should wait indefinitely for an answer from QEMU. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Max Reitz --- tests/qemu-iotests/iotests.py | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index c86f239d81..e176a84620 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -477,10 +477,14 @@ def __init__(self, seconds, errmsg="Timeout"): self.seconds = seconds self.errmsg = errmsg def __enter__(self): +if qemu_gdb: +return self signal.signal(signal.SIGALRM, self.timeout) signal.setitimer(signal.ITIMER_REAL, self.seconds) return self def __exit__(self, exc_type, value, traceback): +if qemu_gdb: +return False signal.setitimer(signal.ITIMER_REAL, 0) return False def timeout(self, signum, frame): @@ -575,7 +579,7 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) -timer = 15.0 +timer = 15.0 if not qemu_gdb else None super().__init__(qemu_prog, qemu_opts, name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, -- 2.31.1
[PATCH v9 05/16] qemu-iotests: add option to attach gdbserver
Define -gdb flag and GDB_OPTIONS environment variable
to python tests to attach a gdbserver to each qemu instance.
This patch only adds and parses this flag, it does not yet add
the implementation for it.
if -gdb is not provided but $GDB_OPTIONS is set, ignore the
environment variable.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
tests/qemu-iotests/check | 6 +-
tests/qemu-iotests/iotests.py | 5 +
tests/qemu-iotests/testenv.py | 17 +++--
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 2dd529eb75..4365bb8066 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,9 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-gdb', action='store_true',
+ help="start gdbserver with $GDB_OPTIONS options \
+('localhost:12345' if $GDB_OPTIONS is empty)")
p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -114,7 +117,8 @@ if __name__ == '__main__':
env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
aiomode=args.aiomode, cachemode=args.cachemode,
imgopts=args.imgopts, misalign=args.misalign,
- debug=args.debug, valgrind=args.valgrind)
+ debug=args.debug, valgrind=args.valgrind,
+ gdb=args.gdb)
if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
if not args.tests:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6b0db4ce54..c86f239d81 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -74,6 +74,11 @@
qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
+gdb_qemu_env = os.environ.get('GDB_OPTIONS')
+qemu_gdb = []
+if gdb_qemu_env:
+qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
+
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
output_dir = os.environ.get('OUTPUT_DIR', '.')
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 0c3fe75636..8501c6caf5 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -27,6 +27,7 @@
import glob
from typing import List, Dict, Any, Optional, ContextManager
+DEF_GDB_OPTIONS = 'localhost:12345'
def isxfile(path: str) -> bool:
return os.path.isfile(path) and os.access(path, os.X_OK)
@@ -72,7 +73,8 @@ class TestEnv(ContextManager['TestEnv']):
'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO',
'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
- 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
+ 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
+ 'GDB_OPTIONS']
def prepare_subprocess(self, args: List[str]) -> Dict[str, str]:
if self.debug:
@@ -178,7 +180,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
imgopts: Optional[str] = None,
misalign: bool = False,
debug: bool = False,
- valgrind: bool = False) -> None:
+ valgrind: bool = False,
+ gdb: bool = False) -> None:
self.imgfmt = imgfmt
self.imgproto = imgproto
self.aiomode = aiomode
@@ -186,6 +189,15 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode:
str,
self.misalign = misalign
self.debug = debug
+if gdb:
+self.gdb_options = os.getenv('GDB_OPTIONS', DEF_GDB_OPTIONS)
+if not self.gdb_options:
+# cover the case 'export GDB_OPTIONS='
+self.gdb_options = DEF_GDB_OPTIONS
+elif 'GDB_OPTIONS' in os.environ:
+# to not propagate it in prepare_subprocess()
+del os.environ['GDB_OPTIONS']
+
if valgrind:
self.valgrind_qemu = 'y'
@@ -285,6 +297,7 @@ def print_env(self) -> None:
TEST_DIR -- {TEST_DIR}
SOCK_DIR -- {SOCK_DIR}
SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
+GDB_OPTIONS -- {GDB_OPTIONS}
"""
args = collections.defaultdict(str, self.get_env())
--
2.31.1
[PATCH v9 03/16] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow Reviewed-by: Max Reitz Acked-by: John Snow --- python/qemu/machine/qtest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py index 592be263e0..395cc8fbfe 100644 --- a/python/qemu/machine/qtest.py +++ b/python/qemu/machine/qtest.py @@ -112,6 +112,7 @@ class QEMUQtestMachine(QEMUMachine): def __init__(self, binary: str, args: Sequence[str] = (), + wrapper: Sequence[str] = (), name: Optional[str] = None, base_temp_dir: str = "/var/tmp", socket_scm_helper: Optional[str] = None, @@ -123,7 +124,8 @@ def __init__(self, name = "qemu-%d" % os.getpid() if sock_dir is None: sock_dir = base_temp_dir -super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, +super().__init__(binary, args, wrapper=wrapper, name=name, + base_temp_dir=base_temp_dir, socket_scm_helper=socket_scm_helper, sock_dir=sock_dir, qmp_timer=qmp_timer) self._qtest: Optional[QEMUQtestProtocol] = None -- 2.31.1
[PATCH v9 02/16] python: Reduce strictness of pylint's duplicate-code check
From: John Snow Pylint prior to 2.8.3 (We pin at >= 2.8.0) includes function and method signatures as part of its duplicate checking algorithm. This check does not listen to pragmas, so the only way to disable it is to turn it off completely or increase the minimum duplicate lines so that it doesn't trigger for functions with long, multi-line signatures. When we decide to upgrade to pylint 2.8.3 or greater, we will be able to use 'ignore-signatures = true' to the config instead. I'd prefer not to keep us on the very bleeding edge of pylint if I can help it -- 2.8.3 came out only three days ago at time of writing. See: https://github.com/PyCQA/pylint/pull/4474 Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Acked-by: John Snow Reviewed-by: Max Reitz --- python/setup.cfg | 5 + 1 file changed, 5 insertions(+) diff --git a/python/setup.cfg b/python/setup.cfg index 14bab90288..83909c1c97 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -105,6 +105,11 @@ good-names=i, # Ignore imports when computing similarities. ignore-imports=yes +# Minimum lines number of a similarity. +# TODO: Remove after we opt in to Pylint 2.8.3. See commit msg. +min-similarity-lines=6 + + [isort] force_grid_wrap=4 force_sort_within_sections=True -- 2.31.1
[PATCH v9 01/16] python: qemu: add timer parameter for qmp.accept socket
Also add a new _qmp_timer field to the QEMUMachine class. Let's change the default socket timeout to None, so that if a subclass needs to add a timer, it can be done by modifying this private field. At the same time, restore the timer to be 15 seconds in iotests.py, to give an upper bound to the QMP monitor test command execution. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow Acked-by: John Snow Reviewed-by: Max Reitz --- python/qemu/machine/machine.py | 7 +-- python/qemu/machine/qtest.py | 5 +++-- tests/qemu-iotests/iotests.py | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 971ed7e8c6..14c4d17eca 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -97,7 +97,8 @@ def __init__(self, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None, - log_dir: Optional[str] = None): + log_dir: Optional[str] = None, + qmp_timer: Optional[float] = None): ''' Initialize a QEMUMachine @@ -112,6 +113,7 @@ def __init__(self, @param drain_console: (optional) True to drain console socket to buffer @param console_log: (optional) path to console log file @param log_dir: where to create and keep log files +@param qmp_timer: (optional) default QMP socket timeout @note: Qemu process is not started until launch() is used. ''' # pylint: disable=too-many-arguments @@ -121,6 +123,7 @@ def __init__(self, self._binary = binary self._args = list(args) self._wrapper = wrapper +self._qmp_timer = qmp_timer self._name = name or "qemu-%d" % os.getpid() self._base_temp_dir = base_temp_dir @@ -343,7 +346,7 @@ def _pre_launch(self) -> None: def _post_launch(self) -> None: if self._qmp_connection: -self._qmp.accept() +self._qmp.accept(self._qmp_timer) def _post_shutdown(self) -> None: """ diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py index d6d9c6a34a..592be263e0 100644 --- a/python/qemu/machine/qtest.py +++ b/python/qemu/machine/qtest.py @@ -115,7 +115,8 @@ def __init__(self, name: Optional[str] = None, base_temp_dir: str = "/var/tmp", socket_scm_helper: Optional[str] = None, - sock_dir: Optional[str] = None): + sock_dir: Optional[str] = None, + qmp_timer: Optional[float] = None): # pylint: disable=too-many-arguments if name is None: @@ -124,7 +125,7 @@ def __init__(self, sock_dir = base_temp_dir super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, socket_scm_helper=socket_scm_helper, - sock_dir=sock_dir) + sock_dir=sock_dir, qmp_timer=qmp_timer) self._qtest: Optional[QEMUQtestProtocol] = None self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock") diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 89663dac06..6b0db4ce54 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -570,10 +570,11 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) +timer = 15.0 super().__init__(qemu_prog, qemu_opts, name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, - sock_dir=sock_dir) + sock_dir=sock_dir, qmp_timer=timer) self._num_drives = 0 def add_object(self, opts): -- 2.31.1
[PATCH v9 00/16] qemu_iotests: improve debugging options
This series adds the option to attach gdbserver and valgrind to the QEMU binary running in qemu_iotests. It also allows to redirect QEMU binaries output of the python tests to the stdout, instead of a log file. Patches 1-9 introduce the -gdb option to both python and bash tests, 10-14 extend the already existing -valgrind flag to work also on python tests, and patch 15-16 introduces -p to enable logging to stdout. In particular, patches 1,6,8,11 focus on extending the QMP socket timers when using gdb/valgrind, otherwise the python tests will fail due to delays in the QMP responses. Signed-off-by: Emanuele Giuseppe Esposito --- v9: * Replace `! -z` with `-n` in bash scripts (patch 8), and quote $GDB_OPTIONS in the same if condition [Max] * Add r-b from Max to all patches except 8, remove r-b from Vladimir on patch 8 Emanuele Giuseppe Esposito (15): python: qemu: add timer parameter for qmp.accept socket python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine docs/devel/testing: add debug section to the QEMU iotests chapter qemu-iotests: add option to attach gdbserver qemu-iotests: delay QMP socket timers qemu_iotests: insert gdbserver command line as wrapper for qemu binary qemu-iotests: add gdbserver option to script tests too docs/devel/testing: add -gdb option to the debugging section of QEMU iotests qemu-iotests: extend the check script to prepare supporting valgrind for python tests qemu-iotests: extend QMP socket timeout when using valgrind qemu-iotests: allow valgrind to read/delete the generated log file qemu-iotests: insert valgrind command line as wrapper for qemu binary docs/devel/testing: add -valgrind option to the debug section of QEMU iotests qemu-iotests: add option to show qemu binary logs on stdout docs/devel/testing: add -p option to the debug section of QEMU iotests John Snow (1): python: Reduce strictness of pylint's duplicate-code check docs/devel/testing.rst | 29 python/qemu/machine/machine.py | 16 +++ python/qemu/machine/qtest.py | 9 --- python/setup.cfg | 5 tests/qemu-iotests/check | 15 --- tests/qemu-iotests/common.qemu | 7 - tests/qemu-iotests/common.rc | 8 +- tests/qemu-iotests/iotests.py | 49 -- tests/qemu-iotests/testenv.py | 23 ++-- 9 files changed, 143 insertions(+), 18 deletions(-) -- 2.31.1
Re: [PATCH v8 08/16] qemu-iotests: add gdbserver option to script tests too
diff --git a/tests/qemu-iotests/common.qemu
b/tests/qemu-iotests/common.qemu
index 0fc52d20d7..cbca757b49 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -85,7 +85,12 @@ _timed_wait_for()
timeout=yes
QEMU_STATUS[$h]=0
- while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
+ read_timeout="-t ${QEMU_COMM_TIMEOUT}"
+ if [ ! -z ${GDB_OPTIONS} ]; then
Shouldn’t we quote "${GDB_OPTIONS}" so that `test` won’t interpret it as
its own parameters (if something in there starts with `--`, which I
don’t think is the intended usage for $GDB_OPTIONS, but, well...)?
(Also, `! -z` is the same as `-n`, but I suppose choosing between the
two can be a matter of style.)
+
+ GDB=""
+ if [ ! -z ${GDB_OPTIONS} ]; then
Here, too. (Sorry for not noticing in v3 already...)
Sorry, I forgot to reply to this email. I agree, I will put quotes and
change `! -z` in `-n`.
I will send v9 because after all this time this serie has some minor
conflicts with current QEMU version.
Hopefully this will be the last one :)
Thank you,
Emanuele
