Re: [PATCH 1/4] docs: lift block-core.json rST header into parents
Kevin Wolf writes:
> Am 09.09.2020 um 17:28 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf writes:
[...]
>> >> > There are orders that I can't get this way.
>> >>
>> >> You're right, ordering by first include is not completely general.
>> >>
>> >> > For example, if I want to
>> >> > have "Block devices" documented before "Background jobs", there is no
>> >> > way to add includes to actually get this order without having
>> >> > "Background jobs" as a subsection in "Block devices".
>> >>
>> >> "Background jobs" is job.json.
>> >>
>> >> "Block devices" is block.json, which includes block-core.json, which
>> >> includes job.json.
>> >>
>> >> To be able to put "Block devices" first, you'd have to break the chain
>> >> from block.json to job.json. Which might even be an improvement:
>> >>
>> >> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
>> >> 5527 qapi/block-core.json
>> >> 1722 qapi/migration.json
>> >> 1617 qapi/misc.json
>> >> 1180 qapi/ui.json
>> >> 17407 total
>> >>
>> >> Could we split block-core.json into several cohesive parts?
>> >
>> > Possibly. However, while it would be an improvement generally speaking,
>> > how does this change the specific problem? All of the smaller files will
>> > be included by block.json (or whatever file provides the "Block devices"
>> > chapter in the documentation) and at least one of them will still have
>> > to include job.json.
>>
>> Splitting block-core.json can help only if other modules then use less
>> than all the parts. In particular, as long as block.json includes the
>> same stuff, it'll surely still include jobs.json. Could it include
>> less?
>
> Not if the documentation wants to have a single chapter for the block
> layer instead of many small block related top-level chapters.
>
> Otherwise we could include some things directly from qapi-schema.json,
> but obviously, that would still have to be after job.json for some
> parts.
You're right.
Being unable to talk about something before you define it may not be all
bad, though :)
Re: [PATCH v6 25/25] meson: guard the minimal meson version to 0.55.1
Il mer 9 set 2020, 22:11 罗勇刚(Yonggang Luo) ha
scritto:
>
>
> On Thu, Sep 10, 2020 at 4:08 AM Paolo Bonzini wrote:
>
>>
>>
>> Il mer 9 set 2020, 20:43 Yonggang Luo ha scritto:
>>
>>> So we can removal usage of unstable-keyval
>>>
>>
>> Isn't it stable only on 0.56.0?
>>
>> Paolo
>>
> On Windows, there is following warning: WARNING: Module unstable-keyval
> is now stable, please use the keyval module instead.
>
That's because Meson advertises itself as version 0.55.90 until 0.56 is
released. It will fix itself when the next release is out.
Paolo
NOTE: guest cross-compilers enabled: cc
> Using 'PKG_CONFIG_PATH' from environment with value:
> 'C:\\CI-Tools\\msys64\\mingw64\\lib\\pkgconfig;C:\\CI-Tools\\msys64\\mingw64\\share\\pkgconfig'
> Using 'PKG_CONFIG_PATH' from environment with value:
> 'C:\\CI-Tools\\msys64\\mingw64\\lib\\pkgconfig;C:\\CI-Tools\\msys64\\mingw64\\share\\pkgconfig'
> The Meson build system
> Version: 0.55.999
> Source dir: C:/work/xemu/qemu
> Build dir: C:/work/xemu/qemu/build
> Build type: native build
> Project name: qemu
> Project version: 5.1.50
> C compiler for the host machine: cc (gcc 10.2.0 "cc (Rev1, Built by MSYS2
> project) 10.2.0")
> C linker for the host machine: cc ld.bfd 2.35
> Host machine cpu family: x86_64
> Host machine cpu: x86_64
> WARNING: Module unstable-keyval is now stable, please use the keyval
> module instead.
> Program sh found: YES
>
> But when I commit this patch to running CI, osx are failing, so there is
> problem with this patch, sorry for that. this patch need to be discard
>
>>
>>
>>> Signed-off-by: Yonggang Luo
>>> ---
>>> meson.build | 9 +++--
>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 0b1741557d..af34a85bec 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -1,14 +1,11 @@
>>> -project('qemu', ['c'], meson_version: '>=0.55.0',
>>> +project('qemu', ['c'], meson_version: '>=0.55.1',
>>> default_options: ['warning_level=1', 'c_std=gnu99',
>>> 'cpp_std=gnu++11',
>>>'b_colorout=auto'],
>>> version: run_command('head', meson.source_root() /
>>> 'VERSION').stdout().strip())
>>>
>>> not_found = dependency('', required: false)
>>> -if meson.version().version_compare('>=0.56.0')
>>> - keyval = import('keyval')
>>> -else
>>> - keyval = import('unstable-keyval')
>>> -endif
>>> +keyval = import('keyval')
>>> +
>>> ss = import('sourceset')
>>>
>>> sh = find_program('sh')
>>> --
>>> 2.28.0.windows.1
>>>
>>>
>
> --
> 此致
> 礼
> 罗勇刚
> Yours
> sincerely,
> Yonggang Luo
>
Re: [PULL v2] Block layer patches
On 9/9/20 4:55 PM, Peter Maydell wrote: This fails 'make check' on NetBSD and OpenBSD: ./check: line 47: realpath: command not found ./check: line 60: /common.env: No such file or directory check: failed to source common.env (make sure the qemu-iotests are run from tests/qemu-iotests in the build tree) gmake: *** [/home/qemu/qemu-test.vcb7nz/src/tests/Makefile.include:144: check-block] Error 1 BSD has 'readlink -f' (and so does coreutils on Linux), which does the same thing as the Linux-only realpath. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PULL v2] Block layer patches
On Tue, 8 Sep 2020 at 12:53, Kevin Wolf wrote:
>
> The following changes since commit 7c37270b3fbe3d034ba80e488761461676e21eb4:
>
> Merge remote-tracking branch 'remotes/kraxel/tags/ui-20200904-pull-request'
> into staging (2020-09-06 16:23:55 +0100)
>
> are available in the Git repository at:
>
> git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to c984095a47c30e0952d34e77decf9f4c0f8d5a19:
>
> block/nvme: Pair doorbell registers (2020-09-08 13:40:53 +0200)
>
>
> Block layer patches:
>
> - qemu-img create: Fail gracefully when backing file is an empty string
> - Fixes related to filter block nodes ("Deal with filters" series)
> - block/nvme: Various cleanups required to use multiple queues
> - block/nvme: Use NvmeBar structure from "block/nvme.h"
> - file-win32: Fix "locking" option
> - iotests: Allow running from different directory
This fails 'make check' on NetBSD and OpenBSD:
./check: line 47: realpath: command not found
./check: line 60: /common.env: No such file or directory
check: failed to source common.env (make sure the qemu-iotests are run
from tests/qemu-iotests in the build tree)
gmake: *** [/home/qemu/qemu-test.vcb7nz/src/tests/Makefile.include:144:
check-block] Error 1
thanks
-- PMM
Re: [PATCH v6 25/25] meson: guard the minimal meson version to 0.55.1
On Thu, Sep 10, 2020 at 4:08 AM Paolo Bonzini wrote:
>
>
> Il mer 9 set 2020, 20:43 Yonggang Luo ha scritto:
>
>> So we can removal usage of unstable-keyval
>>
>
> Isn't it stable only on 0.56.0?
>
> Paolo
>
On Windows, there is following warning: WARNING: Module unstable-keyval
is now stable, please use the keyval module instead.
NOTE: guest cross-compilers enabled: cc
Using 'PKG_CONFIG_PATH' from environment with value:
'C:\\CI-Tools\\msys64\\mingw64\\lib\\pkgconfig;C:\\CI-Tools\\msys64\\mingw64\\share\\pkgconfig'
Using 'PKG_CONFIG_PATH' from environment with value:
'C:\\CI-Tools\\msys64\\mingw64\\lib\\pkgconfig;C:\\CI-Tools\\msys64\\mingw64\\share\\pkgconfig'
The Meson build system
Version: 0.55.999
Source dir: C:/work/xemu/qemu
Build dir: C:/work/xemu/qemu/build
Build type: native build
Project name: qemu
Project version: 5.1.50
C compiler for the host machine: cc (gcc 10.2.0 "cc (Rev1, Built by MSYS2
project) 10.2.0")
C linker for the host machine: cc ld.bfd 2.35
Host machine cpu family: x86_64
Host machine cpu: x86_64
WARNING: Module unstable-keyval is now stable, please use the keyval module
instead.
Program sh found: YES
But when I commit this patch to running CI, osx are failing, so there is
problem with this patch, sorry for that. this patch need to be discard
>
>
>> Signed-off-by: Yonggang Luo
>> ---
>> meson.build | 9 +++--
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/meson.build b/meson.build
>> index 0b1741557d..af34a85bec 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -1,14 +1,11 @@
>> -project('qemu', ['c'], meson_version: '>=0.55.0',
>> +project('qemu', ['c'], meson_version: '>=0.55.1',
>> default_options: ['warning_level=1', 'c_std=gnu99',
>> 'cpp_std=gnu++11',
>>'b_colorout=auto'],
>> version: run_command('head', meson.source_root() /
>> 'VERSION').stdout().strip())
>>
>> not_found = dependency('', required: false)
>> -if meson.version().version_compare('>=0.56.0')
>> - keyval = import('keyval')
>> -else
>> - keyval = import('unstable-keyval')
>> -endif
>> +keyval = import('keyval')
>> +
>> ss = import('sourceset')
>>
>> sh = find_program('sh')
>> --
>> 2.28.0.windows.1
>>
>>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Re: [PATCH v6 25/25] meson: guard the minimal meson version to 0.55.1
Il mer 9 set 2020, 20:43 Yonggang Luo ha scritto:
> So we can removal usage of unstable-keyval
>
Isn't it stable only on 0.56.0?
Paolo
> Signed-off-by: Yonggang Luo
> ---
> meson.build | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 0b1741557d..af34a85bec 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1,14 +1,11 @@
> -project('qemu', ['c'], meson_version: '>=0.55.0',
> +project('qemu', ['c'], meson_version: '>=0.55.1',
> default_options: ['warning_level=1', 'c_std=gnu99',
> 'cpp_std=gnu++11',
>'b_colorout=auto'],
> version: run_command('head', meson.source_root() /
> 'VERSION').stdout().strip())
>
> not_found = dependency('', required: false)
> -if meson.version().version_compare('>=0.56.0')
> - keyval = import('keyval')
> -else
> - keyval = import('unstable-keyval')
> -endif
> +keyval = import('keyval')
> +
> ss = import('sourceset')
>
> sh = find_program('sh')
> --
> 2.28.0.windows.1
>
>
Re: [PATCH v2] qcow2: Skip copy-on-write when allocating a zero cluster
27.08.2020 17:53, Alberto Garcia wrote:
Since commit c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a when a write
request results in a new allocation QEMU first tries to see if the
rest of the cluster outside the written area contains only zeroes.
In that case, instead of doing a normal copy-on-write operation and
writing explicit zero buffers to disk, the code zeroes the whole
cluster efficiently using pwrite_zeroes() with BDRV_REQ_NO_FALLBACK.
This improves performance very significantly but it only happens when
we are writing to an area that was completely unallocated before. Zero
clusters (QCOW2_CLUSTER_ZERO_*) are treated like normal clusters and
are therefore slower to allocate.
This happens because the code uses bdrv_is_allocated_above() rather
bdrv_block_status_above(). The former is not as accurate for this
purpose but it is faster. However in the case of qcow2 the underlying
call does already report zero clusters just fine so there is no reason
why we cannot use that information.
After testing 4KB writes on an image that only contains zero clusters
this patch results in almost five times more IOPS.
Signed-off-by: Alberto Garcia
---
v2:
- Add new, simpler API: bdrv_is_unallocated_or_zero_above()
include/block/block.h | 2 ++
block/io.c| 24
block/qcow2.c | 37 +
3 files changed, 47 insertions(+), 16 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 6e36154061..477a72b2e9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -496,6 +496,8 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
int64_t bytes,
int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
bool include_base, int64_t offset, int64_t bytes,
int64_t *pnum);
+int bdrv_is_unallocated_or_zero_above(BlockDriverState *bs, int64_t offset,
+ int64_t bytes);
bool bdrv_is_read_only(BlockDriverState *bs);
int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
diff --git a/block/io.c b/block/io.c
index ad3a51ed53..94faa3f9d7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2557,6 +2557,30 @@ int bdrv_block_status(BlockDriverState *bs, int64_t
offset, int64_t bytes,
offset, bytes, pnum, map, file);
}
+/*
+ * Check @bs (and its backing chain) to see if the range defined
+ * by @offset and @bytes is unallocated or known to read as zeroes.
+ * Return 1 if that is the case, 0 otherwise and -errno on error.
+ * This test is meant to be fast rather than accurate so returning 0
+ * does not guarantee non-zero data.
+ */
+int bdrv_is_unallocated_or_zero_above(BlockDriverState *bs, int64_t offset,
+ int64_t bytes)
_above prefix for block-status functions usually assume existing of "base"
parameter, otherwise, it's not clear "above what?"
Also, actually the caller doesn't care about it it allocated or not. It only
wants to know is it read-as-zero or not. So, probably better name is
bdrv_is_zero_fast()
+{
+int ret;
+int64_t pnum = bytes;
+
+ret = bdrv_common_block_status_above(bs, NULL, false, offset,
+ bytes, , NULL, NULL);
+
+if (ret < 0) {
+return ret;
+}
+
+return (pnum == bytes) &&
+((ret & BDRV_BLOCK_ZERO) || (!(ret & BDRV_BLOCK_ALLOCATED)));
Note that some protocol drivers returns unallocated status when it doesn't
read-as-zero, so in general, we can't use this function as is_zero.
I think, that better to keep only (pnum == bytes) && (ret & BDRV_BLOCK_ZERO)
here, and to make it work correctly improve bdrv_co_block_status like this:
diff --git a/block/io.c b/block/io.c
index ad3a51ed53..33b2e91bcd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2408,15 +2408,15 @@ static int coroutine_fn
bdrv_co_block_status(BlockDriverState *bs,
if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
ret |= BDRV_BLOCK_ALLOCATED;
-} else if (want_zero && bs->drv->supports_backing) {
-if (bs->backing) {
+} else if (bs->drv->supports_backing) {
+if (bs->backing && want_zero) {
BlockDriverState *bs2 = bs->backing->bs;
int64_t size2 = bdrv_getlength(bs2);
if (size2 >= 0 && offset >= size2) {
ret |= BDRV_BLOCK_ZERO;
}
-} else {
+} else if (!bs->backing) {
ret |= BDRV_BLOCK_ZERO;
}
}
- we can always add ZERO flag to backing-supporting formats if range is
unallocated and there is no backing file.
+}
+
int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum)
{
diff --git a/block/qcow2.c b/block/qcow2.c
index da56b1a4df..29bea548db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2391,26 +2391,28
[PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp
1. Drop extra error propagation
2. Set errp always on failure. Generic bdrv_open_driver supports driver
functions which can return negative value and forget to set errp.
That's a strange thing.. Let's improve qcow2_do_open to not behave this
way. This allows to simplify code in qcow2_co_invalidate_cache().
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 31dd28d19e..cc4e7dd461 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s,
Error **errp)
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
{
+ERRP_GUARD();
BDRVQcow2State *s = bs->opaque;
unsigned int len, i;
int ret = 0;
@@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
report_unsupported_feature(errp, feature_table,
s->incompatible_features &
~QCOW2_INCOMPAT_MASK);
+error_setg(errp,
+ "qcow2 header contains unknown incompatible_feature bits");
ret = -ENOTSUP;
g_free(feature_table);
goto fail;
@@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs)
static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
Error **errp)
{
+ERRP_GUARD();
BDRVQcow2State *s = bs->opaque;
int flags = s->flags;
QCryptoBlock *crypto = NULL;
QDict *options;
-Error *local_err = NULL;
int ret;
/*
@@ -2731,16 +2734,11 @@ static void coroutine_fn
qcow2_co_invalidate_cache(BlockDriverState *bs,
flags &= ~BDRV_O_INACTIVE;
qemu_co_mutex_lock(>lock);
-ret = qcow2_do_open(bs, options, flags, _err);
+ret = qcow2_do_open(bs, options, flags, errp);
qemu_co_mutex_unlock(>lock);
qobject_unref(options);
-if (local_err) {
-error_propagate_prepend(errp, local_err,
-"Could not reopen qcow2 layer: ");
-bs->drv = NULL;
-return;
-} else if (ret < 0) {
-error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
+if (ret < 0) {
+error_prepend(errp, "Could not reopen qcow2 layer: ");
bs->drv = NULL;
return;
}
--
2.21.3
[PATCH 14/14] block/qed: bdrv_qed_do_open: deal with errp
Set errp always on failure. Generic bdrv_open_driver supports driver
functions which can return negative value and forget to set errp.
That's a strange thing.. Let's improve bdrv_qed_do_open to not behave
this way. This allows to simplify code in
bdrv_qed_co_invalidate_cache().
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qed.c | 24 +++-
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/block/qed.c b/block/qed.c
index b27e7546ca..f45c640513 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -393,6 +393,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState
*bs, QDict *options,
ret = bdrv_pread(bs->file, 0, _header, sizeof(le_header));
if (ret < 0) {
+error_setg(errp, "Failed to read QED header");
return ret;
}
qed_header_le_to_cpu(_header, >header);
@@ -408,25 +409,30 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState
*bs, QDict *options,
return -ENOTSUP;
}
if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
+error_setg(errp, "QED cluster size is invalid");
return -EINVAL;
}
/* Round down file size to the last cluster */
file_size = bdrv_getlength(bs->file->bs);
if (file_size < 0) {
+error_setg(errp, "Failed to get file length");
return file_size;
}
s->file_size = qed_start_of_cluster(s, file_size);
if (!qed_is_table_size_valid(s->header.table_size)) {
+error_setg(errp, "QED table size is invalid");
return -EINVAL;
}
if (!qed_is_image_size_valid(s->header.image_size,
s->header.cluster_size,
s->header.table_size)) {
+error_setg(errp, "QED image size is invalid");
return -EINVAL;
}
if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
+error_setg(errp, "QED table offset is invalid");
return -EINVAL;
}
@@ -438,6 +444,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState
*bs, QDict *options,
/* Header size calculation must not overflow uint32_t */
if (s->header.header_size > UINT32_MAX / s->header.cluster_size) {
+error_setg(errp, "QED header size is too large");
return -EINVAL;
}
@@ -445,6 +452,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState
*bs, QDict *options,
if ((uint64_t)s->header.backing_filename_offset +
s->header.backing_filename_size >
s->header.cluster_size * s->header.header_size) {
+error_setg(errp, "QED backing filename offset is invalid");
return -EINVAL;
}
@@ -453,6 +461,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState
*bs, QDict *options,
bs->auto_backing_file,
sizeof(bs->auto_backing_file));
if (ret < 0) {
+error_setg(errp, "Failed to read backing filename");
return ret;
}
pstrcpy(bs->backing_file, sizeof(bs->backing_file),
@@ -475,6 +484,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState
*bs, QDict *options,
ret = qed_write_header_sync(s);
if (ret) {
+error_setg(errp, "Failed to update header");
return ret;
}
@@ -487,6 +497,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState
*bs, QDict *options,
ret = qed_read_l1_table_sync(s);
if (ret) {
+error_setg(errp, "Failed to read L1 table");
goto out;
}
@@ -503,6 +514,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState
*bs, QDict *options,
ret = qed_check(s, , true);
if (ret) {
+error_setg(errp, "Image corrupted");
goto out;
}
}
@@ -1537,22 +1549,16 @@ static void coroutine_fn
bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
Error **errp)
{
BDRVQEDState *s = bs->opaque;
-Error *local_err = NULL;
int ret;
bdrv_qed_close(bs);
bdrv_qed_init_state(bs);
qemu_co_mutex_lock(>table_lock);
-ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, _err);
+ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, errp);
qemu_co_mutex_unlock(>table_lock);
-if (local_err) {
-error_propagate_prepend(errp, local_err,
-"Could not reopen qed layer: ");
-return;
-} else if (ret < 0) {
-error_setg_errno(errp, -ret, "Could not reopen qed layer");
-return;
+if (ret < 0) {
+error_prepend(errp, "Could not reopen qed layer: ");
}
}
--
2.21.3
[PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation
Don't use error propagation in qcow2_get_specific_info(). For this
refactor qcow2_get_bitmap_info_list, its current interface rather
weird.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.h| 4 ++--
block/qcow2-bitmap.c | 27 +--
block/qcow2.c| 10 +++---
3 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 065ec3df0b..ac6a2d3e2a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -967,8 +967,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size);
bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
-Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
-Error **errp);
+bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
+Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 8c34b2aef7..9b14c0791f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1090,30 +1090,29 @@ static Qcow2BitmapInfoFlagsList
*get_bitmap_info_flags(uint32_t flags)
/*
* qcow2_get_bitmap_info_list()
* Returns a list of QCOW2 bitmap details.
- * In case of no bitmaps, the function returns NULL and
- * the @errp parameter is not set.
- * When bitmap information can not be obtained, the function returns
- * NULL and the @errp parameter is set.
+ * On success return true with bm_list set (probably to NULL, if no bitmaps),
+ * on failure return false with errp set.
*/
-Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
-Error **errp)
+bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
+Qcow2BitmapInfoList **info_list, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
-Qcow2BitmapInfoList *list = NULL;
-Qcow2BitmapInfoList **plist =
if (s->nb_bitmaps == 0) {
-return NULL;
+*info_list = NULL;
+return true;
}
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
-if (bm_list == NULL) {
-return NULL;
+if (!bm_list) {
+return false;
}
+*info_list = NULL;
+
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
@@ -1121,13 +1120,13 @@ Qcow2BitmapInfoList
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
info->name = g_strdup(bm->name);
info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
obj->value = info;
-*plist = obj;
-plist = >next;
+*info_list = obj;
+info_list = >next;
}
bitmap_list_free(bm_list);
-return list;
+return true;
}
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
diff --git a/block/qcow2.c b/block/qcow2.c
index 10175fa399..eb7c82120c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5056,12 +5056,10 @@ static ImageInfoSpecific
*qcow2_get_specific_info(BlockDriverState *bs,
BDRVQcow2State *s = bs->opaque;
ImageInfoSpecific *spec_info;
QCryptoBlockInfo *encrypt_info = NULL;
-Error *local_err = NULL;
if (s->crypto != NULL) {
-encrypt_info = qcrypto_block_get_info(s->crypto, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+encrypt_info = qcrypto_block_get_info(s->crypto, errp);
+if (!encrypt_info) {
return NULL;
}
}
@@ -5078,9 +5076,7 @@ static ImageInfoSpecific
*qcow2_get_specific_info(BlockDriverState *bs,
};
} else if (s->qcow_version == 3) {
Qcow2BitmapInfoList *bitmaps;
-bitmaps = qcow2_get_bitmap_info_list(bs, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!qcow2_get_bitmap_info_list(bs, , errp)) {
qapi_free_ImageInfoSpecific(spec_info);
qapi_free_QCryptoBlockInfo(encrypt_info);
return NULL;
--
2.21.3
[PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
It's better to return status together with setting errp. It makes
possible to avoid error propagation.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.h| 2 +-
block/qcow2-bitmap.c | 13 ++---
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index e7e662533b..49824be5c6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bool release_stored, Error **errp);
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f58923fce3..5eeff1cb1c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1524,9 +1524,10 @@ out:
* readonly to begin with, and whether we opened directly or reopened to that
* state shouldn't matter for the state we get afterward.
*/
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bool release_stored, Error **errp)
{
+ERRP_GUARD();
BdrvDirtyBitmap *bitmap;
BDRVQcow2State *s = bs->opaque;
uint32_t new_nb_bitmaps = s->nb_bitmaps;
@@ -1546,7 +1547,7 @@ void
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
if (bm_list == NULL) {
-return;
+return false;
}
}
@@ -1661,7 +1662,7 @@ success:
}
bitmap_list_free(bm_list);
-return;
+return true;
fail:
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1679,16 +1680,14 @@ fail:
}
bitmap_list_free(bm_list);
+return false;
}
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
{
BdrvDirtyBitmap *bitmap;
-Error *local_err = NULL;
-qcow2_store_persistent_dirty_bitmaps(bs, false, _err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
return -EINVAL;
}
--
2.21.3
[PATCH 07/14] block/blklogwrites: drop error propagation
It's simple to avoid error propagation in blk_log_writes_open(), we
just need to refactor blk_log_writes_find_cur_log_sector() a bit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/blklogwrites.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 7ef046cee9..c7da507b2d 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -96,10 +96,10 @@ static inline bool
blk_log_writes_sector_size_valid(uint32_t sector_size)
sector_size < (1ull << 24);
}
-static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
- uint32_t sector_size,
- uint64_t nr_entries,
- Error **errp)
+static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
+ uint32_t sector_size,
+ uint64_t nr_entries,
+ Error **errp)
{
uint64_t cur_sector = 1;
uint64_t cur_idx = 0;
@@ -112,13 +112,13 @@ static uint64_t
blk_log_writes_find_cur_log_sector(BdrvChild *log,
if (read_ret < 0) {
error_setg_errno(errp, -read_ret,
"Failed to read log entry %"PRIu64, cur_idx);
-return (uint64_t)-1ull;
+return read_ret;
}
if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
le64_to_cpu(cur_entry.flags), cur_idx);
-return (uint64_t)-1ull;
+return -EINVAL;
}
/* Account for the sector of the entry itself */
@@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict
*options, int flags,
{
BDRVBlkLogWritesState *s = bs->opaque;
QemuOpts *opts;
-Error *local_err = NULL;
int ret;
uint64_t log_sector_size;
bool log_append;
@@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
QDict *options, int flags,
s->nr_entries = 0;
if (blk_log_writes_sector_size_valid(log_sector_size)) {
-s->cur_log_sector =
+int64_t cur_log_sector =
blk_log_writes_find_cur_log_sector(s->log_file,
log_sector_size,
-le64_to_cpu(log_sb.nr_entries),
_err);
-if (local_err) {
-ret = -EINVAL;
-error_propagate(errp, local_err);
+le64_to_cpu(log_sb.nr_entries), errp);
+if (cur_log_sector < 0) {
+ret = cur_log_sector;
goto fail_log;
}
+s->cur_log_sector = cur_log_sector;
s->nr_entries = le64_to_cpu(log_sb.nr_entries);
}
} else {
--
2.21.3
[PATCH 06/14] block/mirror: drop extra error propagation in commit_active_start()
Let's check return value of mirror_start_job to check for failure
instead of local_err.
Rename ret to job, as ret is usually integer variable.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/mirror.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index ca250f765d..529ba12b2a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1788,8 +1788,7 @@ BlockJob *commit_active_start(const char *job_id,
BlockDriverState *bs,
bool auto_complete, Error **errp)
{
bool base_read_only;
-Error *local_err = NULL;
-BlockJob *ret;
+BlockJob *job;
base_read_only = bdrv_is_read_only(base);
@@ -1799,19 +1798,18 @@ BlockJob *commit_active_start(const char *job_id,
BlockDriverState *bs,
}
}
-ret = mirror_start_job(
+job = mirror_start_job(
job_id, bs, creation_flags, base, NULL, speed, 0, 0,
MIRROR_LEAVE_BACKING_CHAIN, false,
on_error, on_error, true, cb, opaque,
_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- _err);
-if (local_err) {
-error_propagate(errp, local_err);
+ errp);
+if (!job) {
goto error_restore_flags;
}
-return ret;
+return job;
error_restore_flags:
/* ignore error and errp for bdrv_reopen, because we want to propagate
--
2.21.3
[PATCH 02/14] block: use return status of bdrv_append()
Now bdrv_append returns status and we can drop all the local_err things
around it.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 5 +
block/backup-top.c | 20
block/commit.c | 5 +
block/mirror.c | 6 ++
blockdev.c | 4 +---
tests/test-bdrv-graph-mod.c | 6 +++---
6 files changed, 16 insertions(+), 30 deletions(-)
diff --git a/block.c b/block.c
index 6d35449027..9b624b2535 100644
--- a/block.c
+++ b/block.c
@@ -3156,7 +3156,6 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot = NULL;
-Error *local_err = NULL;
int ret;
/* if snapshot, we create a temporary backing file and open it
@@ -3203,9 +3202,7 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
* order to be able to return one, we have to increase
* bs_snapshot's refcount here */
bdrv_ref(bs_snapshot);
-bdrv_append(bs_snapshot, bs, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (bdrv_append(bs_snapshot, bs, errp) < 0) {
bs_snapshot = NULL;
goto out;
}
diff --git a/block/backup-top.c b/block/backup-top.c
index af2f20f346..de9d5e1634 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -192,7 +192,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState
*source,
BlockCopyState **bcs,
Error **errp)
{
-Error *local_err = NULL;
+ERRP_GUARD();
BDRVBackupTopState *state;
BlockDriverState *top;
bool appended = false;
@@ -225,9 +225,8 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState
*source,
bdrv_drained_begin(source);
bdrv_ref(top);
-bdrv_append(top, source, _err);
-if (local_err) {
-error_prepend(_err, "Cannot append backup-top filter: ");
+if (bdrv_append(top, source, errp) < 0) {
+error_prepend(errp, "Cannot append backup-top filter: ");
goto fail;
}
appended = true;
@@ -237,18 +236,16 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState
*source,
* we want.
*/
state->active = true;
-bdrv_child_refresh_perms(top, top->backing, _err);
-if (local_err) {
-error_prepend(_err,
- "Cannot set permissions for backup-top filter: ");
+if (bdrv_child_refresh_perms(top, top->backing, errp) < 0) {
+error_prepend(errp, "Cannot set permissions for backup-top filter: ");
goto fail;
}
state->cluster_size = cluster_size;
state->bcs = block_copy_state_new(top->backing, state->target,
- cluster_size, write_flags, _err);
-if (local_err) {
-error_prepend(_err, "Cannot create block-copy-state: ");
+ cluster_size, write_flags, errp);
+if (!state->bcs) {
+error_prepend(errp, "Cannot create block-copy-state: ");
goto fail;
}
*bcs = state->bcs;
@@ -266,7 +263,6 @@ fail:
}
bdrv_drained_end(source);
-error_propagate(errp, local_err);
return NULL;
}
diff --git a/block/commit.c b/block/commit.c
index 7732d02dfe..7720d4729b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
CommitBlockJob *s;
BlockDriverState *iter;
BlockDriverState *commit_top_bs = NULL;
-Error *local_err = NULL;
int ret;
assert(top != bs);
@@ -292,10 +291,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
commit_top_bs->total_sectors = top->total_sectors;
-bdrv_append(commit_top_bs, top, _err);
-if (local_err) {
+if (bdrv_append(commit_top_bs, top, errp) < 0) {
commit_top_bs = NULL;
-error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/mirror.c b/block/mirror.c
index e8e8844afc..ca250f765d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1557,7 +1557,6 @@ static BlockJob *mirror_start_job(
BlockDriverState *mirror_top_bs;
bool target_graph_mod;
bool target_is_backing;
-Error *local_err = NULL;
int ret;
if (granularity == 0) {
@@ -1606,12 +1605,11 @@ static BlockJob *mirror_start_job(
* it alive until block_job_create() succeeds even if bs has no parent. */
bdrv_ref(mirror_top_bs);
bdrv_drained_begin(bs);
-bdrv_append(mirror_top_bs, bs, _err);
+ret = bdrv_append(mirror_top_bs, bs, errp);
bdrv_drained_end(bs);
-if (local_err) {
+if (ret < 0) {
bdrv_unref(mirror_top_bs);
-error_propagate(errp, local_err);
return NULL;
}
diff --git a/blockdev.c b/blockdev.c
index 3848a9c8ab..36bef6b188 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1576,9 +1576,7 @@
[PATCH 01/14] block: return status from bdrv_append and friends
The recommended use of qemu error api assumes returning status together
with setting errp and avoid void functions with errp parameter. Let's
improve bdrv_append and some friends to reduce error-propagation
overhead in further patches.
Choose int return status, because bdrv_replace_node() has call to
bdrv_check_update_perm(), which reports int status, which seems correct
to propagate.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/block.h | 12 ++--
block.c | 39 ---
2 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 6e36154061..03b3cee8f8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -336,10 +336,10 @@ int bdrv_create(BlockDriver *drv, const char* filename,
int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
BlockDriverState *bdrv_new(void);
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
- Error **errp);
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
- Error **errp);
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+Error **errp);
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+ Error **errp);
int bdrv_parse_aio(const char *mode, int *flags);
int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
@@ -351,8 +351,8 @@ BdrvChild *bdrv_open_child(const char *filename,
BdrvChildRole child_role,
bool allow_none, Error **errp);
BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp);
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+Error **errp);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp);
BlockDriverState *bdrv_open(const char *filename, const char *reference,
diff --git a/block.c b/block.c
index 2ba76b2c36..6d35449027 100644
--- a/block.c
+++ b/block.c
@@ -2866,14 +2866,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState
*bs)
* Sets the backing file link of a BDS. A new reference is created; callers
* which don't need their own reference any more must call bdrv_unref().
*/
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp)
{
+int ret = 0;
bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
bdrv_inherits_from_recursive(backing_hd, bs);
if (bdrv_is_backing_chain_frozen(bs, backing_bs(bs), errp)) {
-return;
+return -EPERM;
}
if (backing_hd) {
@@ -2891,15 +2892,22 @@ void bdrv_set_backing_hd(BlockDriverState *bs,
BlockDriverState *backing_hd,
bs->backing = bdrv_attach_child(bs, backing_hd, "backing", _of_bds,
bdrv_backing_role(bs), errp);
+if (!bs->backing) {
+ret = -EINVAL;
+goto out;
+}
+
/* If backing_hd was already part of bs's backing chain, and
* inherits_from pointed recursively to bs then let's update it to
* point directly to bs (else it will become NULL). */
-if (bs->backing && update_inherits_from) {
+if (update_inherits_from) {
backing_hd->inherits_from = bs;
}
out:
bdrv_refresh_limits(bs, NULL);
+
+return ret;
}
/*
@@ -4517,8 +4525,8 @@ static bool should_update_child(BdrvChild *c,
BlockDriverState *to)
return ret;
}
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
- Error **errp)
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+ Error **errp)
{
BdrvChild *c, *next;
GSList *list = NULL, *p;
@@ -4540,6 +4548,7 @@ void bdrv_replace_node(BlockDriverState *from,
BlockDriverState *to,
continue;
}
if (c->frozen) {
+ret = -EPERM;
error_setg(errp, "Cannot change '%s' link to '%s'",
c->name, from->node_name);
goto out;
@@ -4575,6 +4584,8 @@ out:
g_slist_free(list);
bdrv_drained_end(from);
bdrv_unref(from);
+
+return ret;
}
/*
@@ -4593,20 +4604,16 @@ out:
* parents of bs_top after bdrv_append() returns. If the caller needs to keep a
* reference of its own, it must call bdrv_ref().
*/
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
- Error **errp)
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+Error **errp)
{
-Error *local_err = NULL;
-
-
[PATCH 08/14] blockjob: return status from block_job_set_speed()
Better to return status together with setting errp. It allows to avoid
error propagation in the caller.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/blockjob.h | 2 +-
blockjob.c | 18 --
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 35faa3aa26..d200f33c10 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -139,7 +139,7 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState
*bs);
* Set a rate-limiting parameter for the job; the actual meaning may
* vary depending on the job type.
*/
-void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
/**
* block_job_query:
diff --git a/blockjob.c b/blockjob.c
index 470facfd47..afddf7a1fb 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -254,28 +254,30 @@ static bool job_timer_pending(Job *job)
return timer_pending(>sleep_timer);
}
-void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
{
int64_t old_speed = job->speed;
-if (job_apply_verb(>job, JOB_VERB_SET_SPEED, errp)) {
-return;
+if (job_apply_verb(>job, JOB_VERB_SET_SPEED, errp) < 0) {
+return false;
}
if (speed < 0) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
"a non-negative value");
-return;
+return false;
}
ratelimit_set_speed(>limit, speed, BLOCK_JOB_SLICE_TIME);
job->speed = speed;
if (speed && speed <= old_speed) {
-return;
+return true;
}
/* kick only if a timer is pending */
job_enter_cond(>job, job_timer_pending);
+
+return true;
}
int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
@@ -448,12 +450,8 @@ void *block_job_create(const char *job_id, const
BlockJobDriver *driver,
/* Only set speed when necessary to avoid NotSupported error */
if (speed != 0) {
-Error *local_err = NULL;
-
-block_job_set_speed(job, speed, _err);
-if (local_err) {
+if (!block_job_set_speed(job, speed, errp)) {
job_early_fail(>job);
-error_propagate(errp, local_err);
return NULL;
}
}
--
2.21.3
[PATCH 04/14] blockdev: fix drive_backup_prepare() missed error
We leak local_err and don't report failure to the caller. It's
definitely wrong, let's fix.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
blockdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 36bef6b188..74259527c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1797,8 +1797,7 @@ static void drive_backup_prepare(BlkActionState *common,
Error **errp)
aio_context_acquire(aio_context);
if (set_backing_hd) {
-bdrv_set_backing_hd(target_bs, source, _err);
-if (local_err) {
+if (bdrv_set_backing_hd(target_bs, source, errp) < 0) {
goto unref;
}
}
--
2.21.3
[PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
It's recommended for bool functions with errp to return true on success
and false on failure. Non-standard interfaces don't help to understand
the code. The change is also needed to reduce error propagation.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.h| 3 ++-
block/qcow2-bitmap.c | 22 +-
block/qcow2.c| 6 ++
3 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index ac6a2d3e2a..e7e662533b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -966,7 +966,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table);
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size);
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+ Error **errp);
bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 9b14c0791f..f58923fce3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -959,30 +959,24 @@ static void set_readonly_helper(gpointer bitmap, gpointer
value)
bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
}
-/* qcow2_load_dirty_bitmaps()
- * Return value is a hint for caller: true means that the Qcow2 header was
- * updated. (false doesn't mean that the header should be updated by the
- * caller, it just means that updating was not needed or the image cannot be
- * written to).
- * On failure the function returns false.
- */
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/* Return true on success, false on failure. */
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
GSList *created_dirty_bitmaps = NULL;
-bool header_updated = false;
bool needs_update = false;
if (s->nb_bitmaps == 0) {
/* No bitmaps - nothing to do */
-return false;
+return true;
}
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
-if (bm_list == NULL) {
+if (!bm_list) {
return false;
}
@@ -1033,7 +1027,9 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
error_setg_errno(errp, -ret, "Can't update bitmap directory");
goto fail;
}
-header_updated = true;
+if (header_updated) {
+*header_updated = true;
+}
}
if (!can_write(bs)) {
@@ -1044,7 +1040,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
g_slist_free(created_dirty_bitmaps);
bitmap_list_free(bm_list);
-return header_updated;
+return true;
fail:
g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
diff --git a/block/qcow2.c b/block/qcow2.c
index eb7c82120c..c2cd9434cc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1297,7 +1297,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
unsigned int len, i;
int ret = 0;
QCowHeader header;
-Error *local_err = NULL;
uint64_t ext_end;
uint64_t l1_vm_state_index;
bool update_header = false;
@@ -1786,9 +1785,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
/* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
-bool header_updated = qcow2_load_dirty_bitmaps(bs, _err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+bool header_updated;
+if (!qcow2_load_dirty_bitmaps(bs, _updated, errp)) {
ret = -EINVAL;
goto fail;
}
--
2.21.3
[PATCH 12/14] block/qcow2: read_cache_sizes: return status value
It's better to return status together with setting errp. It allows to
reduce error propagation.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c2cd9434cc..31dd28d19e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -869,7 +869,7 @@ static void qcow2_attach_aio_context(BlockDriverState *bs,
cache_clean_timer_init(bs, new_context);
}
-static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
+static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
uint64_t *l2_cache_size,
uint64_t *l2_cache_entry_size,
uint64_t *refcount_cache_size, Error **errp)
@@ -907,16 +907,16 @@ static void read_cache_sizes(BlockDriverState *bs,
QemuOpts *opts,
error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE
" and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set "
"at the same time");
-return;
+return false;
} else if (l2_cache_size_set &&
(l2_cache_max_setting > combined_cache_size)) {
error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
QCOW2_OPT_CACHE_SIZE);
-return;
+return false;
} else if (*refcount_cache_size > combined_cache_size) {
error_setg(errp, QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not exceed "
QCOW2_OPT_CACHE_SIZE);
-return;
+return false;
}
if (l2_cache_size_set) {
@@ -955,8 +955,10 @@ static void read_cache_sizes(BlockDriverState *bs,
QemuOpts *opts,
error_setg(errp, "L2 cache entry size must be a power of two "
"between %d and the cluster size (%d)",
1 << MIN_CLUSTER_BITS, s->cluster_size);
-return;
+return false;
}
+
+return true;
}
typedef struct Qcow2ReopenState {
@@ -983,7 +985,6 @@ static int qcow2_update_options_prepare(BlockDriverState
*bs,
int i;
const char *encryptfmt;
QDict *encryptopts = NULL;
-Error *local_err = NULL;
int ret;
qdict_extract_subqdict(options, , "encrypt.");
@@ -996,10 +997,8 @@ static int qcow2_update_options_prepare(BlockDriverState
*bs,
}
/* get L2 table/refcount block cache size from command line options */
-read_cache_sizes(bs, opts, _cache_size, _cache_entry_size,
- _cache_size, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!read_cache_sizes(bs, opts, _cache_size, _cache_entry_size,
+ _cache_size, errp)) {
ret = -EINVAL;
goto fail;
}
--
2.21.3
[PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd
bdrv_set_backing_hd now returns status, let's use it.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 9b624b2535..c5e3a1927e 100644
--- a/block.c
+++ b/block.c
@@ -3011,11 +3011,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict
*parent_options,
/* Hook up the backing file link; drop our reference, bs owns the
* backing_hd reference now */
-bdrv_set_backing_hd(bs, backing_hd, _err);
+ret = bdrv_set_backing_hd(bs, backing_hd, errp);
bdrv_unref(backing_hd);
-if (local_err) {
-error_propagate(errp, local_err);
-ret = -EINVAL;
+if (ret < 0) {
goto free_exit;
}
--
2.21.3
[PATCH 03/14] block: check return value of bdrv_open_child and drop error propagation
This patch is generated by cocci script:
@@
symbol bdrv_open_child, errp, local_err;
expression file;
@@
file = bdrv_open_child(...,
-_err
+errp
);
- if (local_err)
+ if (!file)
{
...
- error_propagate(errp, local_err);
...
}
with command
spatch --sp-file x.cocci --macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80 --use-gitgrep block
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/blkdebug.c | 6 ++
block/blklogwrites.c | 10 --
block/blkreplay.c| 6 ++
block/blkverify.c| 11 ---
block/qcow2.c| 5 ++---
block/quorum.c | 6 ++
6 files changed, 16 insertions(+), 28 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 9c08d8a005..61aaee9879 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -464,7 +464,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict
*options, int flags,
{
BDRVBlkdebugState *s = bs->opaque;
QemuOpts *opts;
-Error *local_err = NULL;
int ret;
uint64_t align;
@@ -494,10 +493,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict
*options, int flags,
bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, _err);
-if (local_err) {
+ false, errp);
+if (!bs->file) {
ret = -EINVAL;
-error_propagate(errp, local_err);
goto out;
}
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 57315f56b4..7ef046cee9 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -157,19 +157,17 @@ static int blk_log_writes_open(BlockDriverState *bs,
QDict *options, int flags,
/* Open the file */
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false,
- _err);
-if (local_err) {
+ errp);
+if (!bs->file) {
ret = -EINVAL;
-error_propagate(errp, local_err);
goto fail;
}
/* Open the log file */
s->log_file = bdrv_open_child(NULL, options, "log", bs, _of_bds,
- BDRV_CHILD_METADATA, false, _err);
-if (local_err) {
+ BDRV_CHILD_METADATA, false, errp);
+if (!s->log_file) {
ret = -EINVAL;
-error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/blkreplay.c b/block/blkreplay.c
index 30a0f5d57a..4a247752fd 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -23,16 +23,14 @@ typedef struct Request {
static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
-Error *local_err = NULL;
int ret;
/* Open the image file */
bs->file = bdrv_open_child(NULL, options, "image", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, _err);
-if (local_err) {
+ false, errp);
+if (!bs->file) {
ret = -EINVAL;
-error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/blkverify.c b/block/blkverify.c
index 4aed53ab59..95ae73e2aa 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -112,7 +112,6 @@ static int blkverify_open(BlockDriverState *bs, QDict
*options, int flags,
{
BDRVBlkverifyState *s = bs->opaque;
QemuOpts *opts;
-Error *local_err = NULL;
int ret;
opts = qemu_opts_create(_opts, NULL, 0, _abort);
@@ -125,20 +124,18 @@ static int blkverify_open(BlockDriverState *bs, QDict
*options, int flags,
bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw",
bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, _err);
-if (local_err) {
+ false, errp);
+if (!bs->file) {
ret = -EINVAL;
-error_propagate(errp, local_err);
goto fail;
}
/* Open the test file */
s->test_file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options,
"test", bs, _of_bds, BDRV_CHILD_DATA,
- false, _err);
-if (local_err) {
+ false, errp);
+if (!s->test_file) {
ret = -EINVAL;
-error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index da56b1a4df..10175fa399 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1613,9 +1613,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict
[PATCH 00/14] block: deal with errp: part I
Hi all! I've start to apply scripts/coccinelle/errp-guard.cocci to block subsystem, but it turned out that in most cases error propagation can be simply avoided. So, here are some improvements to block layer error reporting to avoid error propagation. It's not complete, so its called part I, I don't want to create huge series. Vladimir Sementsov-Ogievskiy (14): block: return status from bdrv_append and friends block: use return status of bdrv_append() block: check return value of bdrv_open_child and drop error propagation blockdev: fix drive_backup_prepare() missed error block: drop extra error propagation for bdrv_set_backing_hd block/mirror: drop extra error propagation in commit_active_start() block/blklogwrites: drop error propagation blockjob: return status from block_job_set_speed() block/qcow2: qcow2_get_specific_info(): drop error propagation block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps block/qcow2: read_cache_sizes: return status value block/qcow2: qcow2_do_open: deal with errp block/qed: bdrv_qed_do_open: deal with errp block/qcow2.h | 9 +++--- include/block/block.h | 12 +++ include/block/blockjob.h| 2 +- block.c | 50 -- block/backup-top.c | 20 +--- block/blkdebug.c| 6 ++-- block/blklogwrites.c| 33 +--- block/blkreplay.c | 6 ++-- block/blkverify.c | 11 +++ block/commit.c | 5 +-- block/mirror.c | 18 +-- block/qcow2-bitmap.c| 62 + block/qcow2.c | 56 ++--- block/qed.c | 24 -- block/quorum.c | 6 ++-- blockdev.c | 7 ++--- blockjob.c | 18 +-- tests/test-bdrv-graph-mod.c | 6 ++-- 18 files changed, 159 insertions(+), 192 deletions(-) -- 2.21.3
Re: [PATCH v6 08/25] tests: disable /char/stdio/* tests in test-char.c on win32
On Thu, Sep 10, 2020 at 2:48 AM Thomas Huth wrote:
> On 09/09/2020 20.42, Yonggang Luo wrote:
> > These tests are blocking test-char to be finished.
> > Disable them by using variable is_win32, so we doesn't
> > need macro to open it. and easy recover those function
> > latter.
> >
> > Signed-off-by: Yonggang Luo
> > ---
> > tests/test-char.c | 26 --
> > 1 file changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/tests/test-char.c b/tests/test-char.c
> > index d35cc839bc..184ddceab8 100644
> > --- a/tests/test-char.c
> > +++ b/tests/test-char.c
> > @@ -77,7 +77,6 @@ static void fe_event(void *opaque, QEMUChrEvent event)
> > }
> > }
> >
> > -#ifdef _WIN32
> > static void char_console_test_subprocess(void)
> > {
> > QemuOpts *opts;
> > @@ -102,7 +101,7 @@ static void char_console_test(void)
> > g_test_trap_assert_passed();
> > g_test_trap_assert_stdout("CONSOLE");
> > }
> > -#endif
> > +
> > static void char_stdio_test_subprocess(void)
> > {
> > Chardev *chr;
> > @@ -1448,7 +1447,11 @@ static SocketAddress unixaddr = {
> >
> > int main(int argc, char **argv)
> > {
> > -bool has_ipv4, has_ipv6;
> > +bool has_ipv4, has_ipv6, is_win32 = false;
> > +
> > +#ifdef _WIN32
> > +is_win32 = true;
> > +#endif
> >
> > qemu_init_main_loop(_abort);
> > socket_init();
> > @@ -1467,12 +1470,15 @@ int main(int argc, char **argv)
> > g_test_add_func("/char/invalid", char_invalid_test);
> > g_test_add_func("/char/ringbuf", char_ringbuf_test);
> > g_test_add_func("/char/mux", char_mux_test);
> > -#ifdef _WIN32
> > -g_test_add_func("/char/console/subprocess",
> char_console_test_subprocess);
> > -g_test_add_func("/char/console", char_console_test);
> > -#endif
> > -g_test_add_func("/char/stdio/subprocess",
> char_stdio_test_subprocess);
> > -g_test_add_func("/char/stdio", char_stdio_test);
> > +if (0) {
> > +g_test_add_func("/char/console/subprocess",
> char_console_test_subprocess);
> > +g_test_add_func("/char/console", char_console_test);
> > +}
>
> Sorry, but this looks pretty much like a work-in-progress debugging
> patch. Please avoid sending such stuff to the mailing list, and if so,
> clearly mark it as an RFC and describe it in the patch description.
>
> It also does not help much if you send your series three times a day to
> the list - nobody has that much reviewing band width. So please take
> some time, finish your patches first, and when you're sure that they are
> really finished, then post a new series to the mailing list.
>
Sorry for that, test-char is hard to fix and I can not fixes in my own, so
I need help from community,
For all other patches I am confident, but for this, I am asking for help,
I'd like to know who is familiar with
char and I'd like to talk with them privately if possible.
>
> Thanks,
> Thomas
>
>
> > +if (!is_win32) {
> > +g_test_add_func("/char/stdio/subprocess",
> char_stdio_test_subprocess);
> > +g_test_add_func("/char/stdio", char_stdio_test);
> > +}
> > #ifndef _WIN32
> > g_test_add_func("/char/pipe", char_pipe_test);
> > #endif
> > @@ -1534,7 +1540,7 @@ int main(int argc, char **argv)
> > g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name,
> \
> >##name, char_socket_client_dupid_test)
> >
> > -if (has_ipv4) {
> > +if (has_ipv4 && !is_win32) {
> > SOCKET_SERVER_TEST(tcp, );
> > SOCKET_CLIENT_TEST(tcp, );
> > g_test_add_data_func("/char/socket/server/two-clients/tcp",
> ,
> >
>
>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Re: [PATCH v6 08/25] tests: disable /char/stdio/* tests in test-char.c on win32
On 09/09/2020 20.42, Yonggang Luo wrote:
> These tests are blocking test-char to be finished.
> Disable them by using variable is_win32, so we doesn't
> need macro to open it. and easy recover those function
> latter.
>
> Signed-off-by: Yonggang Luo
> ---
> tests/test-char.c | 26 --
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index d35cc839bc..184ddceab8 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -77,7 +77,6 @@ static void fe_event(void *opaque, QEMUChrEvent event)
> }
> }
>
> -#ifdef _WIN32
> static void char_console_test_subprocess(void)
> {
> QemuOpts *opts;
> @@ -102,7 +101,7 @@ static void char_console_test(void)
> g_test_trap_assert_passed();
> g_test_trap_assert_stdout("CONSOLE");
> }
> -#endif
> +
> static void char_stdio_test_subprocess(void)
> {
> Chardev *chr;
> @@ -1448,7 +1447,11 @@ static SocketAddress unixaddr = {
>
> int main(int argc, char **argv)
> {
> -bool has_ipv4, has_ipv6;
> +bool has_ipv4, has_ipv6, is_win32 = false;
> +
> +#ifdef _WIN32
> +is_win32 = true;
> +#endif
>
> qemu_init_main_loop(_abort);
> socket_init();
> @@ -1467,12 +1470,15 @@ int main(int argc, char **argv)
> g_test_add_func("/char/invalid", char_invalid_test);
> g_test_add_func("/char/ringbuf", char_ringbuf_test);
> g_test_add_func("/char/mux", char_mux_test);
> -#ifdef _WIN32
> -g_test_add_func("/char/console/subprocess",
> char_console_test_subprocess);
> -g_test_add_func("/char/console", char_console_test);
> -#endif
> -g_test_add_func("/char/stdio/subprocess", char_stdio_test_subprocess);
> -g_test_add_func("/char/stdio", char_stdio_test);
> +if (0) {
> +g_test_add_func("/char/console/subprocess",
> char_console_test_subprocess);
> +g_test_add_func("/char/console", char_console_test);
> +}
Sorry, but this looks pretty much like a work-in-progress debugging
patch. Please avoid sending such stuff to the mailing list, and if so,
clearly mark it as an RFC and describe it in the patch description.
It also does not help much if you send your series three times a day to
the list - nobody has that much reviewing band width. So please take
some time, finish your patches first, and when you're sure that they are
really finished, then post a new series to the mailing list.
Thanks,
Thomas
> +if (!is_win32) {
> +g_test_add_func("/char/stdio/subprocess",
> char_stdio_test_subprocess);
> +g_test_add_func("/char/stdio", char_stdio_test);
> +}
> #ifndef _WIN32
> g_test_add_func("/char/pipe", char_pipe_test);
> #endif
> @@ -1534,7 +1540,7 @@ int main(int argc, char **argv)
> g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
>##name, char_socket_client_dupid_test)
>
> -if (has_ipv4) {
> +if (has_ipv4 && !is_win32) {
> SOCKET_SERVER_TEST(tcp, );
> SOCKET_CLIENT_TEST(tcp, );
> g_test_add_data_func("/char/socket/server/two-clients/tcp", ,
>
[PATCH v6 25/25] meson: guard the minimal meson version to 0.55.1
So we can removal usage of unstable-keyval
Signed-off-by: Yonggang Luo
---
meson.build | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/meson.build b/meson.build
index 0b1741557d..af34a85bec 100644
--- a/meson.build
+++ b/meson.build
@@ -1,14 +1,11 @@
-project('qemu', ['c'], meson_version: '>=0.55.0',
+project('qemu', ['c'], meson_version: '>=0.55.1',
default_options: ['warning_level=1', 'c_std=gnu99', 'cpp_std=gnu++11',
'b_colorout=auto'],
version: run_command('head', meson.source_root() /
'VERSION').stdout().strip())
not_found = dependency('', required: false)
-if meson.version().version_compare('>=0.56.0')
- keyval = import('keyval')
-else
- keyval = import('unstable-keyval')
-endif
+keyval = import('keyval')
+
ss = import('sourceset')
sh = find_program('sh')
--
2.28.0.windows.1
[PATCH v6 24/25] ci: Enable msys2 ci in cirrus
Install msys2 in a proper way refer to
https://github.com/cirruslabs/cirrus-ci-docs/issues/699
The https://wiki.qemu.org/Hosts/W32#Native_builds_with_MSYS2 need to be updated.
There is no need of --cross-prefix, open mingw64.exe instead of msys2.exe then
we don't
need the --cross-prefix, besides we using environment variable settings:
MSYS: winsymlinks:nativestrict
MSYSTEM: MINGW64
CHERE_INVOKING: 1
to opening mingw64 native shell.
We now running tests with make -i check to skip tests errors.
Signed-off-by: Yonggang Luo
Reviewed-by: Daniel P. Berrangé
---
.cirrus.yml | 60 +
1 file changed, 60 insertions(+)
diff --git a/.cirrus.yml b/.cirrus.yml
index a18971aac4..fa6edf1d5c 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -44,3 +44,63 @@ macos_xcode_task:
--enable-werror --cc=clang || { cat config.log; exit 1; }
- gmake -j$(sysctl -n hw.ncpu)
- gmake check
+
+windows_msys2_task:
+ windows_container:
+image: cirrusci/windowsservercore:cmake
+os_version: 2019
+cpu: 8
+memory: 8G
+ env:
+MSYS: winsymlinks:nativestrict
+MSYSTEM: MINGW64
+CHERE_INVOKING: 1
+ printenv_script:
+- C:\tools\msys64\usr\bin\bash.exe -lc 'printenv'
+ install_script:
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz;
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz.sig;
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && pacman -U
--noconfirm msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman -Sy --noconfirm"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --needed --noconfirm -S
bash pacman pacman-mirrors msys2-runtime"
+- taskkill /F /IM gpg-agent.exe
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -Su"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -S --needed
+base-devel
+git
+mingw-w64-x86_64-python
+mingw-w64-x86_64-python-setuptools
+mingw-w64-x86_64-toolchain
+mingw-w64-x86_64-SDL2
+mingw-w64-x86_64-SDL2_image
+mingw-w64-x86_64-gtk3
+mingw-w64-x86_64-glib2
+mingw-w64-x86_64-ninja
+mingw-w64-x86_64-make
+mingw-w64-x86_64-jemalloc
+mingw-w64-x86_64-lzo2
+mingw-w64-x86_64-zstd
+mingw-w64-x86_64-libjpeg-turbo
+mingw-w64-x86_64-pixman
+mingw-w64-x86_64-libgcrypt
+mingw-w64-x86_64-capstone
+mingw-w64-x86_64-libpng
+mingw-w64-x86_64-libssh
+mingw-w64-x86_64-libxml2
+mingw-w64-x86_64-snappy
+mingw-w64-x86_64-libusb
+mingw-w64-x86_64-usbredir
+mingw-w64-x86_64-libtasn1
+mingw-w64-x86_64-libnfs
+mingw-w64-x86_64-nettle
+mingw-w64-x86_64-cyrus-sasl
+mingw-w64-x86_64-curl
+mingw-w64-x86_64-gnutls
+mingw-w64-x86_64-zstd"
+ script:
+- C:\tools\msys64\usr\bin\bash.exe -lc "mkdir build"
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && ../configure
--python=python3 --ninja=ninja"
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make
-j$NUMBER_OF_PROCESSORS"
+ test_script:
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make V=1 check"
+
--
2.28.0.windows.1
[PATCH v6 08/25] tests: disable /char/stdio/* tests in test-char.c on win32
These tests are blocking test-char to be finished.
Disable them by using variable is_win32, so we doesn't
need macro to open it. and easy recover those function
latter.
Signed-off-by: Yonggang Luo
---
tests/test-char.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/tests/test-char.c b/tests/test-char.c
index d35cc839bc..184ddceab8 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -77,7 +77,6 @@ static void fe_event(void *opaque, QEMUChrEvent event)
}
}
-#ifdef _WIN32
static void char_console_test_subprocess(void)
{
QemuOpts *opts;
@@ -102,7 +101,7 @@ static void char_console_test(void)
g_test_trap_assert_passed();
g_test_trap_assert_stdout("CONSOLE");
}
-#endif
+
static void char_stdio_test_subprocess(void)
{
Chardev *chr;
@@ -1448,7 +1447,11 @@ static SocketAddress unixaddr = {
int main(int argc, char **argv)
{
-bool has_ipv4, has_ipv6;
+bool has_ipv4, has_ipv6, is_win32 = false;
+
+#ifdef _WIN32
+is_win32 = true;
+#endif
qemu_init_main_loop(_abort);
socket_init();
@@ -1467,12 +1470,15 @@ int main(int argc, char **argv)
g_test_add_func("/char/invalid", char_invalid_test);
g_test_add_func("/char/ringbuf", char_ringbuf_test);
g_test_add_func("/char/mux", char_mux_test);
-#ifdef _WIN32
-g_test_add_func("/char/console/subprocess", char_console_test_subprocess);
-g_test_add_func("/char/console", char_console_test);
-#endif
-g_test_add_func("/char/stdio/subprocess", char_stdio_test_subprocess);
-g_test_add_func("/char/stdio", char_stdio_test);
+if (0) {
+g_test_add_func("/char/console/subprocess",
char_console_test_subprocess);
+g_test_add_func("/char/console", char_console_test);
+}
+
+if (!is_win32) {
+g_test_add_func("/char/stdio/subprocess", char_stdio_test_subprocess);
+g_test_add_func("/char/stdio", char_stdio_test);
+}
#ifndef _WIN32
g_test_add_func("/char/pipe", char_pipe_test);
#endif
@@ -1534,7 +1540,7 @@ int main(int argc, char **argv)
g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
##name, char_socket_client_dupid_test)
-if (has_ipv4) {
+if (has_ipv4 && !is_win32) {
SOCKET_SERVER_TEST(tcp, );
SOCKET_CLIENT_TEST(tcp, );
g_test_add_data_func("/char/socket/server/two-clients/tcp", ,
--
2.28.0.windows.1
Re: [PATCH v2 14/21] cirrus: Building freebsd in a single short
On Thu, Sep 10, 2020 at 2:16 AM Daniel P. Berrangé
wrote:
> On Wed, Sep 09, 2020 at 01:32:04PM -0400, Ed Maste wrote:
> > On Wed, 9 Sep 2020 at 05:47, Yonggang Luo wrote:
> > >
> > > freebsd 1 hour limit not hit anymore
> > >
> > > Signed-off-by: Yonggang Luo
> >
> > Reviewed-by: Ed Maste
> >
> > > When its running properly, the consumed time are little, but when
> tests running too long,
> > > look at the cpu usage, the cpu usage are nearly zero. does't
> consuming time.
> >
> > So it looks like we have a test getting stuck. After this change is in
> > we could split the test execution out into its own script so to make
> > it more obvious if/when this happens - for example,
> >
> > script:
> > - mkdir build
> > - cd build
> > - ../configure --enable-werror || { cat config.log; exit 1; }
> > - gmake -j8
> > test_script:
> >- gmake V=1 check
> >
> > I can follow up with a proper patch for that (and making the change
> > for macos as well) later.
>
> What would help most is if there's a way to convince meson to print
> the execution time of each test case, instead of merely its name.
> That way we could immediately spot the slow test when it hits
>
> Yeap, that's would be great, but now the tests are running by not by meson
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
> https://www.instagram.com/dberrange :|
>
>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Re: [PATCH v2 14/21] cirrus: Building freebsd in a single short
On Wed, Sep 09, 2020 at 01:32:04PM -0400, Ed Maste wrote:
> On Wed, 9 Sep 2020 at 05:47, Yonggang Luo wrote:
> >
> > freebsd 1 hour limit not hit anymore
> >
> > Signed-off-by: Yonggang Luo
>
> Reviewed-by: Ed Maste
>
> > When its running properly, the consumed time are little, but when tests
> > running too long,
> > look at the cpu usage, the cpu usage are nearly zero. does't consuming
> > time.
>
> So it looks like we have a test getting stuck. After this change is in
> we could split the test execution out into its own script so to make
> it more obvious if/when this happens - for example,
>
> script:
> - mkdir build
> - cd build
> - ../configure --enable-werror || { cat config.log; exit 1; }
> - gmake -j8
> test_script:
>- gmake V=1 check
>
> I can follow up with a proper patch for that (and making the change
> for macos as well) later.
What would help most is if there's a way to convince meson to print
the execution time of each test case, instead of merely its name.
That way we could immediately spot the slow test when it hits
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 05/21] tests: disable /char/stdio/* tests in test-char.c on win32
On Wed, Sep 9, 2020 at 8:52 PM Daniel P. Berrangé wrote: > On Wed, Sep 09, 2020 at 05:46:01PM +0800, Yonggang Luo wrote: > > These tests are blocking test-char to be finished. > > Merge three #ifndef _WIN32 > > > > Signed-off-by: Yonggang Luo > > --- > > tests/test-char.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > Reviewed-by: Daniel P. Berrangé > > > Regards, > Daniel > https://cirrus-ci.com/task/6266195793936384?command=main#L433 test-char still failing randomly. I'd like to disable more to make sure it's not failing ERROR:../tests/test-char.c:103:char_console_test: stdout of child process (/char/console/subprocess) failed to match: CONSOLE stdout was: ERROR test-char - Bail out! ERROR:../tests/test-char.c:103:char_console_test: stdout of child process (/char/console/subprocess) failed to match: CONSOLE make: *** [Makefile.mtest:576: run-test-80] Error 1 C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>if 2 NEQ 0 exit /b 2 > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org-o- > https://www.instagram.com/dberrange :| > > -- 此致 礼 罗勇刚 Yours sincerely, Yonggang Luo
Re: [PATCH v2 14/21] cirrus: Building freebsd in a single short
On Wed, 9 Sep 2020 at 05:47, Yonggang Luo wrote:
>
> freebsd 1 hour limit not hit anymore
>
> Signed-off-by: Yonggang Luo
Reviewed-by: Ed Maste
> When its running properly, the consumed time are little, but when tests
> running too long,
> look at the cpu usage, the cpu usage are nearly zero. does't consuming time.
So it looks like we have a test getting stuck. After this change is in
we could split the test execution out into its own script so to make
it more obvious if/when this happens - for example,
script:
- mkdir build
- cd build
- ../configure --enable-werror || { cat config.log; exit 1; }
- gmake -j8
test_script:
- gmake V=1 check
I can follow up with a proper patch for that (and making the change
for macos as well) later.
Re: [PATCH] qcow2: Return the original error code in qcow2_co_pwrite_zeroes()
On 9/9/20 2:37 PM, Alberto Garcia wrote:
> This function checks the current status of a (sub)cluster in order to
> see if an unaligned 'write zeroes' request can be done efficiently by
> simply updating the L2 metadata and without having to write actual
> zeroes to disk.
>
> If the situation does not allow using the fast path then the function
> returns -ENOTSUP and the caller falls back to writing zeroes.
>
> If can happen however that the aforementioned check returns an actual
> error code so in this case we should pass it to the caller.
>
> Signed-off-by: Alberto Garcia
> ---
> block/qcow2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index da56b1a4df..ca46cbd795 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3916,7 +3916,7 @@ static coroutine_fn int
> qcow2_co_pwrite_zeroes(BlockDriverState *bs,
> type != QCOW2_SUBCLUSTER_ZERO_PLAIN &&
> type != QCOW2_SUBCLUSTER_ZERO_ALLOC)) {
> qemu_co_mutex_unlock(>lock);
> -return -ENOTSUP;
> +return ret < 0 ? ret : -ENOTSUP;
> }
> } else {
> qemu_co_mutex_lock(>lock);
>
Reviewed-by: Philippe Mathieu-Daudé
[PATCH v5 24/24] ci: Enable msys2 ci in cirrus
Install msys2 in a proper way refer to
https://github.com/cirruslabs/cirrus-ci-docs/issues/699
The https://wiki.qemu.org/Hosts/W32#Native_builds_with_MSYS2 need to be updated.
There is no need of --cross-prefix, open mingw64.exe instead of msys2.exe then
we don't
need the --cross-prefix, besides we using environment variable settings:
MSYS: winsymlinks:nativestrict
MSYSTEM: MINGW64
CHERE_INVOKING: 1
to opening mingw64 native shell.
We now running tests with make -i check to skip tests errors.
Signed-off-by: Yonggang Luo
Reviewed-by: Daniel P. Berrangé
---
.cirrus.yml | 58 +
1 file changed, 58 insertions(+)
diff --git a/.cirrus.yml b/.cirrus.yml
index a18971aac4..dfe58642a4 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -44,3 +44,61 @@ macos_xcode_task:
--enable-werror --cc=clang || { cat config.log; exit 1; }
- gmake -j$(sysctl -n hw.ncpu)
- gmake check
+
+windows_msys2_task:
+ windows_container:
+image: cirrusci/windowsservercore:cmake
+os_version: 2019
+cpu: 8
+memory: 8G
+ env:
+MSYS: winsymlinks:nativestrict
+MSYSTEM: MINGW64
+CHERE_INVOKING: 1
+ printenv_script:
+- C:\tools\msys64\usr\bin\bash.exe -lc 'printenv'
+ install_script:
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz;
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz.sig;
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && pacman -U
--noconfirm msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman -Sy --noconfirm"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --needed --noconfirm -S
bash pacman pacman-mirrors msys2-runtime"
+- taskkill /F /IM gpg-agent.exe
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -Su"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -S --needed
+base-devel
+git
+mingw-w64-x86_64-python
+mingw-w64-x86_64-python-setuptools
+mingw-w64-x86_64-toolchain
+mingw-w64-x86_64-SDL2
+mingw-w64-x86_64-SDL2_image
+mingw-w64-x86_64-gtk3
+mingw-w64-x86_64-glib2
+mingw-w64-x86_64-ninja
+mingw-w64-x86_64-make
+mingw-w64-x86_64-jemalloc
+mingw-w64-x86_64-lzo2
+mingw-w64-x86_64-zstd
+mingw-w64-x86_64-libjpeg-turbo
+mingw-w64-x86_64-pixman
+mingw-w64-x86_64-libgcrypt
+mingw-w64-x86_64-capstone
+mingw-w64-x86_64-libpng
+mingw-w64-x86_64-libssh
+mingw-w64-x86_64-libxml2
+mingw-w64-x86_64-snappy
+mingw-w64-x86_64-libusb
+mingw-w64-x86_64-usbredir
+mingw-w64-x86_64-libtasn1
+mingw-w64-x86_64-libnfs
+mingw-w64-x86_64-nettle
+mingw-w64-x86_64-cyrus-sasl
+mingw-w64-x86_64-curl
+mingw-w64-x86_64-gnutls
+mingw-w64-x86_64-zstd"
+ script:
+- C:\tools\msys64\usr\bin\bash.exe -lc "mkdir build"
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && ../configure
--python=python3 --ninja=ninja"
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make
-j$NUMBER_OF_PROCESSORS"
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make check"
--
2.28.0.windows.1
Re: [PATCH v4 24/24] ci: Enable msys2 ci in cirrus
On Thu, Sep 10, 2020 at 12:14:30AM +0800, Yonggang Luo wrote:
> Install msys2 in a proper way refer to
> https://github.com/cirruslabs/cirrus-ci-docs/issues/699
> The https://wiki.qemu.org/Hosts/W32#Native_builds_with_MSYS2 need to be
> updated.
> There is no need of --cross-prefix, open mingw64.exe instead of msys2.exe
> then we don't
> need the --cross-prefix, besides we using environment variable settings:
> MSYS: winsymlinks:nativestrict
> MSYSTEM: MINGW64
> CHERE_INVOKING: 1
> to opening mingw64 native shell.
> We now running tests with make -i check to skip tests errors.
>
> Signed-off-by: Yonggang Luo
> ---
> .cirrus.yml | 59 +
> 1 file changed, 59 insertions(+)
>
> diff --git a/.cirrus.yml b/.cirrus.yml
> index a18971aac4..f819d202db 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -44,3 +44,62 @@ macos_xcode_task:
> --enable-werror --cc=clang || { cat config.log; exit 1; }
> - gmake -j$(sysctl -n hw.ncpu)
> - gmake check
> +
> +windows_msys2_task:
> + windows_container:
> +image: cirrusci/windowsservercore:cmake
> +os_version: 2019
> +cpu: 8
> +memory: 8G
> + env:
> +MSYS: winsymlinks:nativestrict
> +MSYSTEM: MINGW64
> +CHERE_INVOKING: 1
> + printenv_script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc 'printenv'
> + install_script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
> http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz;
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
> http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz.sig;
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && pacman -U
> --noconfirm msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman -Sy --noconfirm"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --needed --noconfirm -S
> bash pacman pacman-mirrors msys2-runtime"
> +- taskkill /F /IM gpg-agent.exe
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -Su"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -S --needed
> +base-devel
> +git
> +mingw-w64-x86_64-python
> +mingw-w64-x86_64-python-setuptools
> +mingw-w64-x86_64-toolchain
> +mingw-w64-x86_64-SDL2
> +mingw-w64-x86_64-SDL2_image
> +mingw-w64-x86_64-gtk3
> +mingw-w64-x86_64-glib2
> +mingw-w64-x86_64-ninja
> +mingw-w64-x86_64-make
> +mingw-w64-x86_64-jemalloc
> +mingw-w64-x86_64-lzo2
> +mingw-w64-x86_64-zstd
> +mingw-w64-x86_64-libjpeg-turbo
> +mingw-w64-x86_64-pixman
> +mingw-w64-x86_64-libgcrypt
> +mingw-w64-x86_64-capstone
> +mingw-w64-x86_64-libpng
> +mingw-w64-x86_64-libssh
> +mingw-w64-x86_64-libxml2
> +mingw-w64-x86_64-snappy
> +mingw-w64-x86_64-libusb
> +mingw-w64-x86_64-usbredir
> +mingw-w64-x86_64-libtasn1
> +mingw-w64-x86_64-libnfs
> +mingw-w64-x86_64-nettle
> +mingw-w64-x86_64-cyrus-sasl
> +mingw-w64-x86_64-curl
> +mingw-w64-x86_64-gnutls
> +mingw-w64-x86_64-zstd"
> + script:
> +- mkdir build
> +- cd build
> +- ../configure --python=python3 --enable-werror --ninja=ninja
> --disable-pie
You shouldn't need --disable-pie anymore, as this is merged:
commit fb648e9cacf4209ddaa8ee67d1a87a9b78a001c6
Author: Daniel P. Berrangé
Date: Mon Aug 24 17:31:09 2020 +0100
configure: default to PIE disabled on Windows platforms
If Windows EXE files are built with -pie/-fpie they will fail to
launch. Historically QEMU defaulted to disabling PIE for Windows,
but this setting was accidentally lost when the configure summary
text was removed in
commit f9332757898a764d85e19d339ec421236e885b68
Author: Paolo Bonzini
Date: Mon Feb 3 13:28:38 2020 +0100
meson: move summary to meson.build
Signed-off-by: Paolo Bonzini
IIUC, the --enable-werror is present by default too thanks to this code
snippet:
if test -z "$werror" ; then
if test -e "$source_path/.git" && \
{ test "$linux" = "yes" || test "$mingw32" = "yes"; }; then
werror="yes"
else
werror="no"
fi
fi
If you remove the --disable-pie && --enable-werror args then you can add
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] qcow2: Return the original error code in qcow2_co_pwrite_zeroes()
On 9/9/20 7:37 AM, Alberto Garcia wrote: This function checks the current status of a (sub)cluster in order to see if an unaligned 'write zeroes' request can be done efficiently by simply updating the L2 metadata and without having to write actual zeroes to disk. If the situation does not allow using the fast path then the function returns -ENOTSUP and the caller falls back to writing zeroes. If can happen however that the aforementioned check returns an actual error code so in this case we should pass it to the caller. Signed-off-by: Alberto Garcia --- block/qcow2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH v4 24/24] ci: Enable msys2 ci in cirrus
Install msys2 in a proper way refer to
https://github.com/cirruslabs/cirrus-ci-docs/issues/699
The https://wiki.qemu.org/Hosts/W32#Native_builds_with_MSYS2 need to be updated.
There is no need of --cross-prefix, open mingw64.exe instead of msys2.exe then
we don't
need the --cross-prefix, besides we using environment variable settings:
MSYS: winsymlinks:nativestrict
MSYSTEM: MINGW64
CHERE_INVOKING: 1
to opening mingw64 native shell.
We now running tests with make -i check to skip tests errors.
Signed-off-by: Yonggang Luo
---
.cirrus.yml | 59 +
1 file changed, 59 insertions(+)
diff --git a/.cirrus.yml b/.cirrus.yml
index a18971aac4..f819d202db 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -44,3 +44,62 @@ macos_xcode_task:
--enable-werror --cc=clang || { cat config.log; exit 1; }
- gmake -j$(sysctl -n hw.ncpu)
- gmake check
+
+windows_msys2_task:
+ windows_container:
+image: cirrusci/windowsservercore:cmake
+os_version: 2019
+cpu: 8
+memory: 8G
+ env:
+MSYS: winsymlinks:nativestrict
+MSYSTEM: MINGW64
+CHERE_INVOKING: 1
+ printenv_script:
+- C:\tools\msys64\usr\bin\bash.exe -lc 'printenv'
+ install_script:
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz;
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz.sig;
+- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && pacman -U
--noconfirm msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman -Sy --noconfirm"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --needed --noconfirm -S
bash pacman pacman-mirrors msys2-runtime"
+- taskkill /F /IM gpg-agent.exe
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -Su"
+- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -S --needed
+base-devel
+git
+mingw-w64-x86_64-python
+mingw-w64-x86_64-python-setuptools
+mingw-w64-x86_64-toolchain
+mingw-w64-x86_64-SDL2
+mingw-w64-x86_64-SDL2_image
+mingw-w64-x86_64-gtk3
+mingw-w64-x86_64-glib2
+mingw-w64-x86_64-ninja
+mingw-w64-x86_64-make
+mingw-w64-x86_64-jemalloc
+mingw-w64-x86_64-lzo2
+mingw-w64-x86_64-zstd
+mingw-w64-x86_64-libjpeg-turbo
+mingw-w64-x86_64-pixman
+mingw-w64-x86_64-libgcrypt
+mingw-w64-x86_64-capstone
+mingw-w64-x86_64-libpng
+mingw-w64-x86_64-libssh
+mingw-w64-x86_64-libxml2
+mingw-w64-x86_64-snappy
+mingw-w64-x86_64-libusb
+mingw-w64-x86_64-usbredir
+mingw-w64-x86_64-libtasn1
+mingw-w64-x86_64-libnfs
+mingw-w64-x86_64-nettle
+mingw-w64-x86_64-cyrus-sasl
+mingw-w64-x86_64-curl
+mingw-w64-x86_64-gnutls
+mingw-w64-x86_64-zstd"
+ script:
+- mkdir build
+- cd build
+- ../configure --python=python3 --enable-werror --ninja=ninja --disable-pie
+- make -j$NUMBER_OF_PROCESSORS
+- make check
--
2.28.0.windows.1
[PATCH v4 23/24] rcu: fixes test-logging.c by call drain_call_rcu before rmdir_full
drain_call_rcu is necessary on win32, because under win32, if you don't close the file before remove it, the remove would be fail. Signed-off-by: Yonggang Luo --- tests/test-logging.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test-logging.c b/tests/test-logging.c index 783fe09a27..8b1522cfed 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -210,6 +210,8 @@ int main(int argc, char **argv) tmp_path, test_logfile_lock); rc = g_test_run(); +qemu_log_close(); +drain_call_rcu(); rmdir_full(tmp_path); return rc; -- 2.28.0.windows.1
[PATCH v4 16/24] cirrus: Building freebsd in a single short
This reverts commit 45f7b7b9f38f5c4d1529a37c93dedfc26a231bba
("cirrus.yml: Split FreeBSD job into two parts").
freebsd 1 hour limit not hit anymore
I think we going to a wrong direction, I think there is some tests a stall the
test runner,
please look at
https://cirrus-ci.com/task/5110577531977728
When its running properly, the consumed time are little, but when tests running
too long,
look at the cpu usage, the cpu usage are nearly zero. doesn't consuming time.
And look at
https://cirrus-ci.com/task/6119341601062912
If the tests running properly, the time consuming are little
We should not hide the error by split them
Signed-off-by: Yonggang Luo
Reviewed-by: Daniel P. Berrangé
---
.cirrus.yml | 37 +
1 file changed, 9 insertions(+), 28 deletions(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index 3dd9fcff7f..a18971aac4 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -1,40 +1,21 @@
env:
CIRRUS_CLONE_DEPTH: 1
-freebsd_1st_task:
+freebsd_12_task:
freebsd_instance:
image_family: freebsd-12-1
-cpu: 4
-memory: 4G
- install_script: ASSUME_ALWAYS_YES=yes pkg bootstrap -f ; pkg install -y
-bash curl cyrus-sasl git glib gmake gnutls gsed
-nettle perl5 pixman pkgconf png usbredir
+cpu: 8
+memory: 8G
+ install_script:
+- ASSUME_ALWAYS_YES=yes pkg bootstrap -f ;
+- pkg install -y bash curl cyrus-sasl git glib gmake gnutls gsed
+ nettle perl5 pixman pkgconf png usbredir
script:
- mkdir build
- cd build
-- ../configure --disable-user --target-list-exclude='alpha-softmmu
-ppc64-softmmu ppc-softmmu riscv32-softmmu riscv64-softmmu s390x-softmmu
-sparc64-softmmu sparc-softmmu x86_64-softmmu i386-softmmu'
---enable-werror || { cat config.log; exit 1; }
+- ../configure --enable-werror || { cat config.log; exit 1; }
- gmake -j$(sysctl -n hw.ncpu)
-- gmake -j$(sysctl -n hw.ncpu) check
-
-freebsd_2nd_task:
- freebsd_instance:
-image_family: freebsd-12-1
-cpu: 4
-memory: 4G
- install_script: ASSUME_ALWAYS_YES=yes pkg bootstrap -f ; pkg install -y
-bash curl cyrus-sasl git glib gmake gnutls gtk3 gsed libepoxy mesa-libs
-nettle perl5 pixman pkgconf png SDL2 usbredir
- script:
-- ./configure --enable-werror --target-list='alpha-softmmu ppc64-softmmu
-ppc-softmmu riscv32-softmmu riscv64-softmmu s390x-softmmu
-sparc64-softmmu sparc-softmmu x86_64-softmmu i386-softmmu
-sparc-bsd-user sparc64-bsd-user x86_64-bsd-user i386-bsd-user'
-|| { cat config.log; exit 1; }
-- gmake -j$(sysctl -n hw.ncpu)
-- gmake -j$(sysctl -n hw.ncpu) check
+- gmake check
macos_task:
osx_instance:
--
2.28.0.windows.1
[PATCH v4 05/24] configure: Fixes ncursesw detection under msys2/mingw and enable curses
The mingw pkg-config are showing following absolute path and contains : as the
separator,
so we must not use : as path separator. and we know the command line parameter
are not likely
contains newline, we could use newline as path command line parameter separator
-D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L
-IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
-DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -pipe
-lncursesw -lgnurx -ltre -lintl -liconv
-DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -lncursesw
-DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -lcursesw
-DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe -lncursesw
-lgnurx -ltre -lintl -liconv
-DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
-DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
-DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx -ltre
-lintl -liconv
-DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
-DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw
Refer to https://unix.stackexchange.com/a/103011/218958
If your file names are guaranteed not to contain newlines, you can use newlines
as the separator. W
hen you expand the variable, first turn off globbing with set -f and set the
list of field splitting characters
IFS to contain only a newline.
msys2/mingw lacks the POSIX-required langinfo.h.
gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe -lncursesw
-lgnurx -ltre -lintl -liconv
test.c:4:10: fatal error: langinfo.h: No such file or directory
4 | #include
| ^~~~
compilation terminated.
So we using g_get_codeset instead of nl_langinfo(CODESET)
Signed-off-by: Yonggang Luo
Reviewed-by: Gerd Hoffmann
---
configure | 25 +++--
ui/curses.c | 10 +-
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/configure b/configure
index f4f8bc3756..b21843fdb9 100755
--- a/configure
+++ b/configure
@@ -3653,35 +3653,40 @@ if test "$iconv" = "no" ; then
fi
if test "$curses" != "no" ; then
if test "$mingw32" = "yes" ; then
-curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
-curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
+curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null)
+ $($pkg_config --cflags ncursesw 2>/dev/null)"
+curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null)
+ $($pkg_config --libs ncursesw 2>/dev/null)
+ -lpdcurses"
else
-curses_inc_list="$($pkg_config --cflags ncursesw
2>/dev/null):-I/usr/include/ncursesw:"
-curses_lib_list="$($pkg_config --libs ncursesw
2>/dev/null):-lncursesw:-lcursesw"
+curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null)
+ -I/usr/include/ncursesw:"
+curses_lib_list="$($pkg_config --libs ncursesw 2>/dev/null)
+ -lncursesw
+ -lcursesw"
fi
curses_found=no
cat > $TMPC << EOF
#include
#include
#include
-#include
int main(void) {
- const char *codeset;
wchar_t wch = L'w';
setlocale(LC_ALL, "");
resize_term(0, 0);
addwstr(L"wide chars\n");
addnwstr(, 1);
add_wch(WACS_DEGREE);
- codeset = nl_langinfo(CODESET);
- return codeset != 0;
+ return 0;
}
EOF
- IFS=:
+ IFS='
+' # turn off variable value expansion except for
splitting at newlines
for curses_inc in $curses_inc_list; do
# Make sure we get the wide character prototypes
curses_inc="-DNCURSES_WIDECHAR $curses_inc"
-IFS=:
+IFS='
+' # turn off variable value expansion except for
splitting at newlines
for curses_lib in $curses_lib_list; do
unset IFS
if compile_prog "$curses_inc" "$curses_lib" ; then
diff --git a/ui/curses.c b/ui/curses.c
index a59b23a9cf..12bc682cf9 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -30,7 +30,6 @@
#endif
#include
#include
-#include
#include
#include "qapi/error.h"
@@ -526,6 +525,7 @@ static void font_setup(void)
iconv_t nativecharset_to_ucs2;
iconv_t font_conv;
int i;
+g_autofree gchar *local_codeset = g_get_codeset();
/*
* Control characters are normally non-printable, but VGA does have
@@ -566,14 +566,14 @@ static void font_setup(void)
0x25bc
};
-ucs2_to_nativecharset = iconv_open(nl_langinfo(CODESET), "UCS-2");
+ucs2_to_nativecharset = iconv_open(local_codeset, "UCS-2");
if (ucs2_to_nativecharset == (iconv_t) -1) {
fprintf(stderr, "Could not convert font glyphs from UCS-2: '%s'\n",
strerror(errno));
exit(1);
}
-nativecharset_to_ucs2 = iconv_open("UCS-2", nl_langinfo(CODESET));
+nativecharset_to_ucs2 = iconv_open("UCS-2", local_codeset);
if (nativecharset_to_ucs2 == (iconv_t) -1) {
iconv_close(ucs2_to_nativecharset);
fprintf(stderr, "Could not convert font glyphs to
[PATCH v4 15/24] vmstate: Fixes test-vmstate.c on msys2/mingw
The vmstate are valid on win32, just need generate tmp path properly
Signed-off-by: Yonggang Luo
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Reviewed-by: Daniel P. Berrangé
---
tests/test-vmstate.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index f8de709a0b..fc38e93d29 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -34,7 +34,6 @@
#include "qemu/module.h"
#include "io/channel-file.h"
-static char temp_file[] = "/tmp/vmst.test.XX";
static int temp_fd;
@@ -1487,6 +1486,8 @@ static void test_tmp_struct(void)
int main(int argc, char **argv)
{
+g_autofree char *temp_file = g_strdup_printf(
+"%s/vmst.test.XX", g_get_tmp_dir());
temp_fd = mkstemp(temp_file);
module_call_init(MODULE_INIT_QOM);
--
2.28.0.windows.1
[PATCH v4 04/24] ci: fixes msys2 build by upgrading capstone to 4.0.2
The currently random version capstone have the following compiling issue:
CC /c/work/xemu/qemu/build/slirp/src/arp_table.o
make[1]: *** No rule to make target
“/c/work/xemu/qemu/build/capstone/capstone.lib”。 Stop.
Subproject commit 1d230532840a37ac032c6ab80128238fc930c6c1 are the tagged
version 4.0.2
when upgrading to this version, the folder structure of include are changed to
qemu\capstone\include
│ platform.h
│
├─capstone
│ arm.h
│ arm64.h
│ capstone.h
│ evm.h
│ m680x.h
│ m68k.h
│ mips.h
│ platform.h
│ ppc.h
│ sparc.h
│ systemz.h
│ tms320c64x.h
│ x86.h
│ xcore.h
│
└─windowsce
intrin.h
stdint.h
in capstone. so we need add extra include path
-I${source_path}/capstone/include/capstone
for directly #include , and the exist include path should preserve,
because
in capstone code there something like #include "capstone/capstone.h"
If only using
capstone_cflags="-I${source_path}/capstone/include/capstone"
Then will cause the following compiling error:
CC cs.o
cs.c:17:10: fatal error: 'capstone/capstone.h' file not found
#include
^
1 error generated.
Signed-off-by: Yonggang Luo
---
capstone | 2 +-
configure | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/capstone b/capstone
index 22ead3e0bf..1d23053284 16
--- a/capstone
+++ b/capstone
@@ -1 +1 @@
-Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
+Subproject commit 1d230532840a37ac032c6ab80128238fc930c6c1
diff --git a/configure b/configure
index 4231d56bcc..f4f8bc3756 100755
--- a/configure
+++ b/configure
@@ -5156,7 +5156,7 @@ case "$capstone" in
LIBCAPSTONE=libcapstone.a
fi
capstone_libs="-Lcapstone -lcapstone"
-capstone_cflags="-I${source_path}/capstone/include"
+capstone_cflags="-I${source_path}/capstone/include
-I${source_path}/capstone/include/capstone"
;;
system)
--
2.28.0.windows.1
[PATCH v4 02/24] rcu: Implement drain_call_rcu
From: Maxim Levitsky
This will allow is to preserve the semantics of hmp_device_del,
that the device is deleted immediatly which was changed by previos
patch that delayed this to RCU callback
Signed-off-by: Maxim Levitsky
Suggested-by: Stefan Hajnoczi
Reviewed-by: Stefan Hajnoczi
---
include/qemu/rcu.h | 1 +
util/rcu.c | 55 ++
2 files changed, 56 insertions(+)
diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 570aa603eb..0e375ebe13 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -133,6 +133,7 @@ struct rcu_head {
};
extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
+extern void drain_call_rcu(void);
/* The operands of the minus operator must have the same type,
* which must be the one that we specify in the cast.
diff --git a/util/rcu.c b/util/rcu.c
index 60a37f72c3..c4fefa9333 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -293,6 +293,61 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct
rcu_head *node))
qemu_event_set(_call_ready_event);
}
+
+struct rcu_drain {
+struct rcu_head rcu;
+QemuEvent drain_complete_event;
+};
+
+static void drain_rcu_callback(struct rcu_head *node)
+{
+struct rcu_drain *event = (struct rcu_drain *)node;
+qemu_event_set(>drain_complete_event);
+}
+
+/*
+ * This function ensures that all pending RCU callbacks
+ * on the current thread are done executing
+
+ * drops big qemu lock during the wait to allow RCU thread
+ * to process the callbacks
+ *
+ */
+
+void drain_call_rcu(void)
+{
+struct rcu_drain rcu_drain;
+bool locked = qemu_mutex_iothread_locked();
+
+memset(_drain, 0, sizeof(struct rcu_drain));
+qemu_event_init(_drain.drain_complete_event, false);
+
+if (locked) {
+qemu_mutex_unlock_iothread();
+}
+
+
+/*
+ * RCU callbacks are invoked in the same order as in which they
+ * are registered, thus we can be sure that when 'drain_rcu_callback'
+ * is called, all RCU callbacks that were registered on this thread
+ * prior to calling this function are completed.
+ *
+ * Note that since we have only one global queue of the RCU callbacks,
+ * we also end up waiting for most of RCU callbacks that were registered
+ * on the other threads, but this is a side effect that shoudn't be
+ * assumed.
+ */
+
+call_rcu1(_drain.rcu, drain_rcu_callback);
+qemu_event_wait(_drain.drain_complete_event);
+
+if (locked) {
+qemu_mutex_lock_iothread();
+}
+
+}
+
void rcu_register_thread(void)
{
assert(rcu_reader.ctr == 0);
--
2.28.0.windows.1
[PATCH v4 20/24] tests: Fixes test-io-channel-file by mask only owner file state mask bits
This is the error on msys2/mingw
Running test test-io-channel-file
**
ERROR:../tests/test-io-channel-file.c:59:test_io_channel_file_helper: assertion
failed (TEST_MASK & ~mask == st.st_mode & 0777): (384 == 438)
ERROR test-io-channel-file - Bail out!
ERROR:../tests/test-io-channel-file.c:59:test_io_channel_file_helper: assertion
failed (TEST_MASK & ~mask == st.st_mode & 0777): (384 == 438)
Signed-off-by: Yonggang Luo
---
tests/test-io-channel-file.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/tests/test-io-channel-file.c b/tests/test-io-channel-file.c
index bac2b07562..1b0e8d7c1b 100644
--- a/tests/test-io-channel-file.c
+++ b/tests/test-io-channel-file.c
@@ -28,6 +28,12 @@
#define TEST_FILE "tests/test-io-channel-file.txt"
#define TEST_MASK 0600
+#ifdef _WIN32
+#define TEST_MASK_EXPECT 0700
+#else
+#define TEST_MASK_EXPECT 0777
+#endif
+
static void test_io_channel_file_helper(int flags)
{
QIOChannel *src, *dst;
@@ -56,7 +62,9 @@ static void test_io_channel_file_helper(int flags)
umask(mask);
ret = stat(TEST_FILE, );
g_assert_cmpint(ret, >, -1);
-g_assert_cmpuint(TEST_MASK & ~mask, ==, st.st_mode & 0777);
+/* On Windows the stat() function in the C library checks only
+ the FAT-style READONLY attribute and does not look at the ACL at all. */
+g_assert_cmpuint(TEST_MASK & ~mask, ==, st.st_mode & TEST_MASK_EXPECT);
unlink(TEST_FILE);
object_unref(OBJECT(src));
--
2.28.0.windows.1
[PATCH v4 01/24] file-win32: Fix "locking" option
From: Kevin Wolf
The intended behaviour was that locking=off/auto work and have no
effect (to remain compatible with file-posix), whereas locking=on would
return an error. Unfortunately, the code forgot to remove "locking" from
the options QDict, so any attempt to use the option would fail.
Replace the option parsing code for "locking" with something that is
part of the raw_runtime_opts QemuOptsList (so it is properly removed
from the QDict) and looks more like file-posix.
Signed-off-by: Kevin Wolf
Message-Id: <20200907092739.9988-1-kw...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Max Reitz
Signed-off-by: Kevin Wolf
---
block/file-win32.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/block/file-win32.c b/block/file-win32.c
index ab69bd811a..e2900c3a51 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -299,6 +299,11 @@ static QemuOptsList raw_runtime_opts = {
.type = QEMU_OPT_STRING,
.help = "host AIO implementation (threads, native)",
},
+{
+.name = "locking",
+.type = QEMU_OPT_STRING,
+.help = "file locking mode (on/off/auto, default: auto)",
+},
{ /* end of list */ }
},
};
@@ -333,6 +338,7 @@ static int raw_open(BlockDriverState *bs, QDict *options,
int flags,
Error *local_err = NULL;
const char *filename;
bool use_aio;
+OnOffAuto locking;
int ret;
s->type = FTYPE_FILE;
@@ -343,10 +349,24 @@ static int raw_open(BlockDriverState *bs, QDict *options,
int flags,
goto fail;
}
-if (qdict_get_try_bool(options, "locking", false)) {
+locking = qapi_enum_parse(_lookup,
+ qemu_opt_get(opts, "locking"),
+ ON_OFF_AUTO_AUTO, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail;
+}
+switch (locking) {
+case ON_OFF_AUTO_ON:
error_setg(errp, "locking=on is not supported on Windows");
ret = -EINVAL;
goto fail;
+case ON_OFF_AUTO_OFF:
+case ON_OFF_AUTO_AUTO:
+break;
+default:
+g_assert_not_reached();
}
filename = qemu_opt_get(opts, "filename");
--
2.28.0.windows.1
[PATCH v4 06/24] win32: Simplify gmtime_r detection direct base on _POSIX_THREAD_SAFE_FUNCTIONS.
First, this reduce the size of configure, configure are tending to removal in
future,
and this didn't introduce any new feature or remove any exist feature.
Second, the current localtime_r detection are conflict with ncursesw detection
in
mingw, when ncursesw detected, it will provide the following compile flags
pkg-config --cflags ncursesw
-D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L
-IC:/CI-Tools/msys64/mingw64/include/ncursesw
And the compile flag _POSIX_C_SOURCE will always cause
_POSIX_THREAD_SAFE_FUNCTIONS to
be defined, in new version of mingw, that's will cause localtime_r to be
defined.
But the configure script didn't provide _POSIX_C_SOURCE macro, and that's will
result
localtime_r not detected because localtime_r are defined in forceinline manner.
And finally cause conflict between QEMU defined localtime_r
struct tm *localtime_r(const time_t *timep, struct tm *result);
with mingw defined localtime_r
```
#if defined(_POSIX_C_SOURCE) && !defined(_POSIX_THREAD_SAFE_FUNCTIONS)
#define _POSIX_THREAD_SAFE_FUNCTIONS 200112L
#endif
#ifdef _POSIX_THREAD_SAFE_FUNCTIONS
__forceinline struct tm *__CRTDECL localtime_r(const time_t *_Time, struct tm
*_Tm) {
return localtime_s(_Tm, _Time) ? NULL : _Tm;
}
__forceinline struct tm *__CRTDECL gmtime_r(const time_t *_Time, struct tm
*_Tm) {
return gmtime_s(_Tm, _Time) ? NULL : _Tm;
}
__forceinline char *__CRTDECL ctime_r(const time_t *_Time, char *_Str) {
return ctime_s(_Str, 0x7fff, _Time) ? NULL : _Str;
}
__forceinline char *__CRTDECL asctime_r(const struct tm *_Tm, char * _Str) {
return asctime_s(_Str, 0x7fff, _Tm) ? NULL : _Str;
}
#endif
```
So I suggest remove this configure script, and restrict msys2/mingw version to
easy to maintain.
And use _POSIX_THREAD_SAFE_FUNCTIONS to guard the localtime_r and counterpart
functions
Signed-off-by: Yonggang Luo
---
configure | 34 --
include/sysemu/os-win32.h | 4 ++--
util/oslib-win32.c| 2 +-
3 files changed, 3 insertions(+), 37 deletions(-)
diff --git a/configure b/configure
index b21843fdb9..af86ba1db3 100755
--- a/configure
+++ b/configure
@@ -2495,37 +2495,6 @@ if test "$vhost_net" = ""; then
test "$vhost_kernel" = "yes" && vhost_net=yes
fi
-##
-# MinGW / Mingw-w64 localtime_r/gmtime_r check
-
-if test "$mingw32" = "yes"; then
-# Some versions of MinGW / Mingw-w64 lack localtime_r
-# and gmtime_r entirely.
-#
-# Some versions of Mingw-w64 define a macro for
-# localtime_r/gmtime_r.
-#
-# Some versions of Mingw-w64 will define functions
-# for localtime_r/gmtime_r, but only if you have
-# _POSIX_THREAD_SAFE_FUNCTIONS defined. For fun
-# though, unistd.h and pthread.h both define
-# that for you.
-#
-# So this #undef localtime_r and #include
-# are not in fact redundant.
-cat > $TMPC << EOF
-#include
-#include
-#undef localtime_r
-int main(void) { localtime_r(NULL, NULL); return 0; }
-EOF
-if compile_prog "" "" ; then
-localtime_r="yes"
-else
-localtime_r="no"
-fi
-fi
-
##
# pkg-config probe
@@ -7087,9 +7056,6 @@ if [ "$bsd" = "yes" ] ; then
echo "CONFIG_BSD=y" >> $config_host_mak
fi
-if test "$localtime_r" = "yes" ; then
- echo "CONFIG_LOCALTIME_R=y" >> $config_host_mak
-fi
if test "$qom_cast_debug" = "yes" ; then
echo "CONFIG_QOM_CAST_DEBUG=y" >> $config_host_mak
fi
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index d8978e28c0..3ac8a53bac 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -48,12 +48,12 @@
#define siglongjmp(env, val) longjmp(env, val)
/* Missing POSIX functions. Don't use MinGW-w64 macros. */
-#ifndef CONFIG_LOCALTIME_R
+#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
#undef gmtime_r
struct tm *gmtime_r(const time_t *timep, struct tm *result);
#undef localtime_r
struct tm *localtime_r(const time_t *timep, struct tm *result);
-#endif /* CONFIG_LOCALTIME_R */
+#endif
static inline void os_setup_signal_handling(void) {}
static inline void os_daemonize(void) {}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index c654dafd93..f2fa9a3549 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -106,7 +106,7 @@ void qemu_anon_ram_free(void *ptr, size_t size)
}
}
-#ifndef CONFIG_LOCALTIME_R
+#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
/* FIXME: add proper locking */
struct tm *gmtime_r(const time_t *timep, struct tm *result)
{
--
2.28.0.windows.1
[PATCH v4 00/24] W32, W64 msys2/mingw patches
It first introduce msys2 CI on cirrus by fixes nfs, capstone, curses and disable partial test-char tests. And then fixes all unit tests failure on msys2/mingw This fixes the reviews suggested in the mailling list Kevin Wolf (1): file-win32: Fix "locking" option Maxim Levitsky (1): rcu: Implement drain_call_rcu Yonggang Luo (22): block: Fixes nfs compiling error on msys2/mingw ci: fixes msys2 build by upgrading capstone to 4.0.2 configure: Fixes ncursesw detection under msys2/mingw and enable curses win32: Simplify gmtime_r detection direct base on _POSIX_THREAD_SAFE_FUNCTIONS. curses: Fixes curses compiling errors. tests: disable /char/stdio/* tests in test-char.c on win32 tests: Trying fixes test-replication.c on msys2/mingw. tests: test-replication disable /replication/secondary/* on msys2/mingw. osdep: osdep: file locking functions are not available on Win32 meson: Use -b to ignore CR vs. CR-LF issues on Windows meson: disable crypto tests are empty under win32 meson: remove empty else and duplicated gio deps vmstate: Fixes test-vmstate.c on msys2/mingw cirrus: Building freebsd in a single short tests: Convert g_free to g_autofree macro in test-logging.c tests: Fixes test-io-channel-socket.c tests under msys2/mingw tests: fixes aio-win32 about aio_remove_fd_handler, get it consistence with aio-posix.c tests: Fixes test-io-channel-file by mask only owner file state mask bits tests: fix test-util-sockets.c tests: Fixes test-qdev-global-props.c rcu: fixes test-logging.c by call drain_call_rcu before rmdir_full ci: Enable msys2 ci in cirrus .cirrus.yml| 96 -- block/file-win32.c | 22 +++- block/nfs.c| 26 + capstone | 2 +- configure | 61 ++--- include/qemu/osdep.h | 2 +- include/qemu/rcu.h | 1 + include/sysemu/os-win32.h | 4 +- meson.build| 6 --- tests/meson.build | 3 +- tests/qapi-schema/meson.build | 2 +- tests/test-char.c | 8 +-- tests/test-io-channel-file.c | 10 +++- tests/test-io-channel-socket.c | 2 + tests/test-logging.c | 5 +- tests/test-qdev-global-props.c | 6 +-- tests/test-replication.c | 22 ++-- tests/test-util-sockets.c | 6 ++- tests/test-vmstate.c | 3 +- ui/curses.c| 14 ++--- util/aio-win32.c | 11 +++- util/oslib-win32.c | 2 +- util/rcu.c | 55 +++ 23 files changed, 248 insertions(+), 121 deletions(-) -- 2.28.0.windows.1
Re: [PATCH v2 04/21] curses: Fixes curses compiling errors.
On Wed, Sep 9, 2020 at 9:26 PM Peter Maydell wrote: > On Wed, 9 Sep 2020 at 10:46, Yonggang Luo wrote: > > > > This is the compiling error: > > ../ui/curses.c: In function 'curses_refresh': > > ../ui/curses.c:256:5: error: 'next_maybe_keycode' may be used > uninitialized in this function [-Werror=maybe-uninitialized] > > 256 | curses2foo(_curses2keycode, _curseskey2keycode, chr, > maybe_keycode) > > | ^~ > > ../ui/curses.c:302:32: note: 'next_maybe_keycode' was declared here > > 302 | enum maybe_keycode next_maybe_keycode; > > |^~ > > ../ui/curses.c:256:5: error: 'maybe_keycode' may be used uninitialized > in this function [-Werror=maybe-uninitialized] > > 256 | curses2foo(_curses2keycode, _curseskey2keycode, chr, > maybe_keycode) > > | ^~ > > ../ui/curses.c:265:24: note: 'maybe_keycode' was declared here > > 265 | enum maybe_keycode maybe_keycode; > > |^ > > cc1.exe: all warnings being treated as errors > > > > gcc version 10.2.0 (Rev1, Built by MSYS2 project) > > > > Signed-off-by: Yonggang Luo > > Reviewed-by: Peter Maydell > > Reviewed-by: Gerd Hoffmann > > I never gave this a reviewed-by tag -- can you be more careful > with your tag handling, please? > Sorry, I see you replied the patch, and misunderstand as a review by > > thanks > -- PMM > -- 此致 礼 罗勇刚 Yours sincerely, Yonggang Luo
Re: [PATCH 1/4] docs: lift block-core.json rST header into parents
Am 09.09.2020 um 17:28 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
> >> Kevin Wolf writes:
> >>
> >> > Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf writes:
> >> >>
> >> >> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
> >> >> >> Hi Stefan,
> >> >> >>
> >> >> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
> >> >> >> > block-core.json is included from several places. It has no way of
> >> >> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx
> >> >> >> > reports
> >> >> >> > errors when it encounters an h2 header where it expects an h1
> >> >> >> > header.
> >> >> >> > This issue prevents the next patch from generating documentation
> >> >> >> > for
> >> >> >> > qemu-storage-daemon QMP commands.
> >> >> >> >
> >> >> >> > Move the header into parents so that the correct header level can
> >> >> >> > be
> >> >> >> > used. Note that transaction.json is not updated since it doesn't
> >> >> >> > seem to
> >> >> >> > need a header.
> >> >> >> >
> >> >> >> > Signed-off-by: Stefan Hajnoczi
> >> >> >> > ---
> >> >> >> > docs/interop/firmware.json | 4
> >> >> >> > qapi/block-core.json | 4
> >> >> >> > qapi/block.json| 1 +
> >> >> >> > 3 files changed, 5 insertions(+), 4 deletions(-)
> >> >> >> >
> >> >> >> > diff --git a/docs/interop/firmware.json
> >> >> >> > b/docs/interop/firmware.json
> >> >> >> > index 989f10b626..48af327f98 100644
> >> >> >> > --- a/docs/interop/firmware.json
> >> >> >> > +++ b/docs/interop/firmware.json
> >> >> >> > @@ -15,6 +15,10 @@
> >> >> >> > ##
> >> >> >> >
> >> >> >> > { 'include' : 'machine.json' }
> >> >> >> > +
> >> >> >> > +##
> >> >> >> > +# == Block devices
> >> >> >> > +##
> >> >> >> > { 'include' : 'block-core.json' }
> >> >> >> >
> >> >> >> > ##
> >> >> >>
> >> >> >> I think "docs/interop/firmware.json" deserves the same treatment as
> >> >> >> "transaction.json".
> >> >> >>
> >> >> >> It's been a long time since I last looked at a rendered view of
> >> >> >> "docs/interop/firmware.json", but it only includes "block-core.json"
> >> >> >> so
> >> >> >> it can refer to some block-related types (@BlockdevDriver seems like
> >> >> >> the
> >> >> >> main, or only, one).
> >> >> >>
> >> >> >> I wouldn't expect the rendered view of "firmware.json" to have a
> >> >> >> section
> >> >> >> header saying "Block devices".
> >> >> >>
> >> >> >> I think it should be fine to drop this hunk (and my CC along with it
> >> >> >> ;))
> >> >> >
> >> >> > I think this is actually a more general problem with the way we
> >> >> > generate
> >> >> > the documentation. For example, the "Background jobs" documentation
> >> >> > ends
> >> >> > up under "Block Devices" just because qapi-schema.json includes
> >> >> > block-core.json before job.json and block-core.json includes job.json
> >> >> > to
> >> >> > be able to access some types.
> >> >>
> >> >> The doc generator is stupid and greedy (which also makes it
> >> >> predictable): a module's documentation is emitted where it is first
> >> >> included.
> >> >>
> >> >> For full control of the order, have the main module include all
> >> >> sub-modules in the order you want.
> >> >
> >> > This works as long as the order that we want is consistent with the
> >> > requirement that every file must be included by qapi-schea.json before
> >> > it is included by any other file (essentially making the additional
> >> > includes in other files useless).
> >>
> >> These other includes are not useless: they are essential for generating
> >> self-contained headers.
> >>
> >> When MOD.json includes SUBMOD.json, then the generated qapi-FOO-MOD.h
> >> include qapi-FOO-SUBMOD.h. When every module pulls in the modules it
> >> requires, so do the generated headers. When a module doesn't, its
> >> generated headers won't compile unless you manually include the missing
> >> generated headers it requires.
> >
> > Hm, right. So we're using includes for two different purposes, but just
> > from looking at the include line, you can't know which one it is.
>
> Correct. The use for controlling doc order is a bit of a hack.
>
> >> > Is this the order that we want?
> >> >
> >> > If so, we could support following the rule consistently by making double
> >> > include of a file an error.
> >>
> >> Breaks our simple & stupid way to generate self-contained headers.
> >>
> >> >> Alternatively, add just enough includes to get the order you want.
> >> >
> >> > There are orders that I can't get this way.
> >>
> >> You're right, ordering by first include is not completely general.
> >>
> >> > For example, if I want to
> >> > have "Block devices" documented before "Background jobs", there is no
> >> > way to add includes to actually get this order without having
> >> > "Background jobs" as a subsection in "Block devices".
> >>
> >>
Re: [PATCH 1/4] docs: lift block-core.json rST header into parents
Kevin Wolf writes:
> Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf writes:
>> >>
>> >> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
>> >> >> Hi Stefan,
>> >> >>
>> >> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
>> >> >> > block-core.json is included from several places. It has no way of
>> >> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx
>> >> >> > reports
>> >> >> > errors when it encounters an h2 header where it expects an h1 header.
>> >> >> > This issue prevents the next patch from generating documentation for
>> >> >> > qemu-storage-daemon QMP commands.
>> >> >> >
>> >> >> > Move the header into parents so that the correct header level can be
>> >> >> > used. Note that transaction.json is not updated since it doesn't
>> >> >> > seem to
>> >> >> > need a header.
>> >> >> >
>> >> >> > Signed-off-by: Stefan Hajnoczi
>> >> >> > ---
>> >> >> > docs/interop/firmware.json | 4
>> >> >> > qapi/block-core.json | 4
>> >> >> > qapi/block.json| 1 +
>> >> >> > 3 files changed, 5 insertions(+), 4 deletions(-)
>> >> >> >
>> >> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
>> >> >> > index 989f10b626..48af327f98 100644
>> >> >> > --- a/docs/interop/firmware.json
>> >> >> > +++ b/docs/interop/firmware.json
>> >> >> > @@ -15,6 +15,10 @@
>> >> >> > ##
>> >> >> >
>> >> >> > { 'include' : 'machine.json' }
>> >> >> > +
>> >> >> > +##
>> >> >> > +# == Block devices
>> >> >> > +##
>> >> >> > { 'include' : 'block-core.json' }
>> >> >> >
>> >> >> > ##
>> >> >>
>> >> >> I think "docs/interop/firmware.json" deserves the same treatment as
>> >> >> "transaction.json".
>> >> >>
>> >> >> It's been a long time since I last looked at a rendered view of
>> >> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
>> >> >> it can refer to some block-related types (@BlockdevDriver seems like
>> >> >> the
>> >> >> main, or only, one).
>> >> >>
>> >> >> I wouldn't expect the rendered view of "firmware.json" to have a
>> >> >> section
>> >> >> header saying "Block devices".
>> >> >>
>> >> >> I think it should be fine to drop this hunk (and my CC along with it
>> >> >> ;))
>> >> >
>> >> > I think this is actually a more general problem with the way we generate
>> >> > the documentation. For example, the "Background jobs" documentation ends
>> >> > up under "Block Devices" just because qapi-schema.json includes
>> >> > block-core.json before job.json and block-core.json includes job.json to
>> >> > be able to access some types.
>> >>
>> >> The doc generator is stupid and greedy (which also makes it
>> >> predictable): a module's documentation is emitted where it is first
>> >> included.
>> >>
>> >> For full control of the order, have the main module include all
>> >> sub-modules in the order you want.
>> >
>> > This works as long as the order that we want is consistent with the
>> > requirement that every file must be included by qapi-schea.json before
>> > it is included by any other file (essentially making the additional
>> > includes in other files useless).
>>
>> These other includes are not useless: they are essential for generating
>> self-contained headers.
>>
>> When MOD.json includes SUBMOD.json, then the generated qapi-FOO-MOD.h
>> include qapi-FOO-SUBMOD.h. When every module pulls in the modules it
>> requires, so do the generated headers. When a module doesn't, its
>> generated headers won't compile unless you manually include the missing
>> generated headers it requires.
>
> Hm, right. So we're using includes for two different purposes, but just
> from looking at the include line, you can't know which one it is.
Correct. The use for controlling doc order is a bit of a hack.
>> > Is this the order that we want?
>> >
>> > If so, we could support following the rule consistently by making double
>> > include of a file an error.
>>
>> Breaks our simple & stupid way to generate self-contained headers.
>>
>> >> Alternatively, add just enough includes to get the order you want.
>> >
>> > There are orders that I can't get this way.
>>
>> You're right, ordering by first include is not completely general.
>>
>> > For example, if I want to
>> > have "Block devices" documented before "Background jobs", there is no
>> > way to add includes to actually get this order without having
>> > "Background jobs" as a subsection in "Block devices".
>>
>> "Background jobs" is job.json.
>>
>> "Block devices" is block.json, which includes block-core.json, which
>> includes job.json.
>>
>> To be able to put "Block devices" first, you'd have to break the chain
>> from block.json to job.json. Which might even be an improvement:
>>
>> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
>> 5527 qapi/block-core.json
>> 1722 qapi/migration.json
>>
Re: [PATCH v2 02/21] ci: fixes msys2 build by upgrading capstone to 4.0.2
On Wed, Sep 9, 2020 at 8:27 PM Daniel P. Berrangé
wrote:
> On Wed, Sep 09, 2020 at 05:45:58PM +0800, Yonggang Luo wrote:
> > The currently random version capstone have the following compiling issue:
> > CC /c/work/xemu/qemu/build/slirp/src/arp_table.o
> > make[1]: *** No rule to make target
> “/c/work/xemu/qemu/build/capstone/capstone.lib”。 Stop.
> >
> > Subproject commit 1d230532840a37ac032c6ab80128238fc930c6c1 are the
> tagged version 4.0.2
> > when upgrading to this version, the folder structure of include are
> changed to
> > qemu\capstone\include
> > │ platform.h
> > │
> > ├─capstone
> > │ arm.h
> > │ arm64.h
> > │ capstone.h
> > │ evm.h
> > │ m680x.h
> > │ m68k.h
> > │ mips.h
> > │ platform.h
> > │ ppc.h
> > │ sparc.h
> > │ systemz.h
> > │ tms320c64x.h
> > │ x86.h
> > │ xcore.h
> > │
> > └─windowsce
> > intrin.h
> > stdint.h
> >
> > in capstone. so we need add extra include path
> -I${source_path}/capstone/include/capstone
> > for directly #include , and the exist include path should
> preserve, because
> > in capstone code there something like #include "capstone/capstone.h"
> >
> > Signed-off-by: Yonggang Luo
> > ---
> > capstone | 2 +-
> > configure | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/capstone b/capstone
> > index 22ead3e0bf..1d23053284 16
> > --- a/capstone
> > +++ b/capstone
> > @@ -1 +1 @@
> > -Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
> > +Subproject commit 1d230532840a37ac032c6ab80128238fc930c6c1
> > diff --git a/configure b/configure
> > index 4231d56bcc..f4f8bc3756 100755
> > --- a/configure
> > +++ b/configure
> > @@ -5156,7 +5156,7 @@ case "$capstone" in
> >LIBCAPSTONE=libcapstone.a
> > fi
> > capstone_libs="-Lcapstone -lcapstone"
> > -capstone_cflags="-I${source_path}/capstone/include"
> > +capstone_cflags="-I${source_path}/capstone/include
> -I${source_path}/capstone/include/capstone"
>
> IIUC, the original -I arg can be removed - we just need the new one.
>
That's not correct, doing that will cause compiling failure
Please take a look at
https://cirrus-ci.com/task/6709042959613952?command=main#L384
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
> https://www.instagram.com/dberrange :|
>
>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Re: [PATCH v7 00/13] monitor: Optionally run handlers in coroutines
Patchew URL: https://patchew.org/QEMU/20200909151149.490589-1-kw...@redhat.com/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200909151149.490589-1-kw...@redhat.com/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[PATCH v7 13/13] block: Convert 'block_resize' to coroutine
block_resize performs some I/O that could potentially take quite some
time, so use it as an example for the new 'coroutine': true annotation
in the QAPI schema.
bdrv_truncate() requires that we're already in the right AioContext for
the BlockDriverState if called in coroutine context. So instead of just
taking the AioContext lock, move the QMP handler coroutine to the
context.
Call blk_unref() only after switching back because blk_unref() may only
be called in the main thread.
Signed-off-by: Kevin Wolf
---
qapi/block-core.json | 3 ++-
blockdev.c | 13 ++---
hmp-commands.hx | 1 +
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0345f6f2d2..d3e49c9419 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1302,7 +1302,8 @@
{ 'command': 'block_resize',
'data': { '*device': 'str',
'*node-name': 'str',
-'size': 'int' } }
+'size': 'int' },
+ 'coroutine': true }
##
# @NewImageMode:
diff --git a/blockdev.c b/blockdev.c
index 7f2561081e..064989fc2d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2439,14 +2439,14 @@ BlockDirtyBitmapSha256
*qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
return ret;
}
-void qmp_block_resize(bool has_device, const char *device,
- bool has_node_name, const char *node_name,
- int64_t size, Error **errp)
+void coroutine_fn qmp_block_resize(bool has_device, const char *device,
+ bool has_node_name, const char *node_name,
+ int64_t size, Error **errp)
{
Error *local_err = NULL;
BlockBackend *blk = NULL;
BlockDriverState *bs;
-AioContext *aio_context;
+AioContext *old_ctx;
bs = bdrv_lookup_bs(has_device ? device : NULL,
has_node_name ? node_name : NULL,
@@ -2456,8 +2456,7 @@ void qmp_block_resize(bool has_device, const char *device,
return;
}
-aio_context = bdrv_get_aio_context(bs);
-aio_context_acquire(aio_context);
+old_ctx = bdrv_co_move_to_aio_context(bs);
if (size < 0) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
@@ -2479,8 +2478,8 @@ void qmp_block_resize(bool has_device, const char *device,
bdrv_drained_end(bs);
out:
+aio_co_reschedule_self(old_ctx);
blk_unref(blk);
-aio_context_release(aio_context);
}
void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 60f395c276..ac360b73f6 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -76,6 +76,7 @@ ERST
.params = "device size",
.help = "resize a block image",
.cmd= hmp_block_resize,
+.coroutine = true,
},
SRST
--
2.25.4
[PATCH v7 09/13] qmp: Move dispatcher to a coroutine
This moves the QMP dispatcher to a coroutine and runs all QMP command
handlers that declare 'coroutine': true in coroutine context so they
can avoid blocking the main loop while doing I/O or waiting for other
events.
For commands that are not declared safe to run in a coroutine, the
dispatcher drops out of coroutine context by calling the QMP command
handler from a bottom half.
Signed-off-by: Kevin Wolf
Reviewed-by: Markus Armbruster
---
include/qapi/qmp/dispatch.h | 1 +
monitor/monitor-internal.h | 6 +-
monitor/monitor.c | 55 +---
monitor/qmp.c | 122 +++-
qapi/qmp-dispatch.c | 61 --
qapi/qmp-registry.c | 3 +
util/aio-posix.c| 8 ++-
7 files changed, 210 insertions(+), 46 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 9fd2b720a7..af8d96c570 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -31,6 +31,7 @@ typedef enum QmpCommandOptions
typedef struct QmpCommand
{
const char *name;
+/* Runs in coroutine context if QCO_COROUTINE is set */
QmpCommandFunc *fn;
QmpCommandOptions options;
QTAILQ_ENTRY(QmpCommand) node;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index b39e03b744..b55d6df07f 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
extern IOThread *mon_iothread;
-extern QEMUBH *qmp_dispatcher_bh;
+extern Coroutine *qmp_dispatcher_co;
+extern bool qmp_dispatcher_co_shutdown;
+extern bool qmp_dispatcher_co_busy;
extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
extern QemuMutex monitor_lock;
extern MonitorList mon_list;
@@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void);
void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
void monitor_data_destroy_qmp(MonitorQMP *mon);
-void monitor_qmp_bh_dispatcher(void *data);
+void coroutine_fn monitor_qmp_dispatcher_co(void *data);
int get_monitor_def(int64_t *pval, const char *name);
void help_cmd(Monitor *mon, const char *name);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 629aa073ee..ac2722bf91 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -55,8 +55,32 @@ typedef struct {
/* Shared monitor I/O thread */
IOThread *mon_iothread;
-/* Bottom half to dispatch the requests received from I/O thread */
-QEMUBH *qmp_dispatcher_bh;
+/* Coroutine to dispatch the requests received from I/O thread */
+Coroutine *qmp_dispatcher_co;
+
+/* Set to true when the dispatcher coroutine should terminate */
+bool qmp_dispatcher_co_shutdown;
+
+/*
+ * qmp_dispatcher_co_busy is used for synchronisation between the
+ * monitor thread and the main thread to ensure that the dispatcher
+ * coroutine never gets scheduled a second time when it's already
+ * scheduled (scheduling the same coroutine twice is forbidden).
+ *
+ * It is true if the coroutine is active and processing requests.
+ * Additional requests may then be pushed onto mon->qmp_requests,
+ * and @qmp_dispatcher_co_shutdown may be set without further ado.
+ * @qmp_dispatcher_co_busy must not be woken up in this case.
+ *
+ * If false, you also have to set @qmp_dispatcher_co_busy to true and
+ * wake up @qmp_dispatcher_co after pushing the new requests.
+ *
+ * The coroutine will automatically change this variable back to false
+ * before it yields. Nobody else may set the variable to false.
+ *
+ * Access must be atomic for thread safety.
+ */
+bool qmp_dispatcher_co_busy;
/*
* Protects mon_list, monitor_qapi_event_state, coroutine_mon,
@@ -623,9 +647,24 @@ void monitor_cleanup(void)
}
qemu_mutex_unlock(_lock);
-/* QEMUBHs needs to be deleted before destroying the I/O thread */
-qemu_bh_delete(qmp_dispatcher_bh);
-qmp_dispatcher_bh = NULL;
+/*
+ * The dispatcher needs to stop before destroying the I/O thread.
+ *
+ * We need to poll both qemu_aio_context and iohandler_ctx to make
+ * sure that the dispatcher coroutine keeps making progress and
+ * eventually terminates. qemu_aio_context is automatically
+ * polled by calling AIO_WAIT_WHILE on it, but we must poll
+ * iohandler_ctx manually.
+ */
+qmp_dispatcher_co_shutdown = true;
+if (!atomic_xchg(_dispatcher_co_busy, true)) {
+aio_co_wake(qmp_dispatcher_co);
+}
+
+AIO_WAIT_WHILE(qemu_get_aio_context(),
+ (aio_poll(iohandler_get_aio_context(), false),
+atomic_mb_read(_dispatcher_co_busy)));
+
if (mon_iothread) {
iothread_destroy(mon_iothread);
mon_iothread = NULL;
@@ -649,9 +688,9 @@ void monitor_init_globals_core(void)
* have commands assuming that context. It would be nice to get
* rid of those assumptions.
*/
-qmp_dispatcher_bh =
[PATCH v7 12/13] block: Add bdrv_co_move_to_aio_context()
Add a function to move the current coroutine to the AioContext of a
given BlockDriverState.
Signed-off-by: Kevin Wolf
---
include/block/block.h | 6 ++
block.c | 10 ++
2 files changed, 16 insertions(+)
diff --git a/include/block/block.h b/include/block/block.h
index 981ab5b314..80ab322f11 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -626,6 +626,12 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const
char *tag);
*/
AioContext *bdrv_get_aio_context(BlockDriverState *bs);
+/**
+ * Move the current coroutine to the AioContext of @bs and return the old
+ * AioContext of the coroutine.
+ */
+AioContext *coroutine_fn bdrv_co_move_to_aio_context(BlockDriverState *bs);
+
/**
* Transfer control to @co in the aio context of @bs
*/
diff --git a/block.c b/block.c
index 9538af4884..81403e00d1 100644
--- a/block.c
+++ b/block.c
@@ -6372,6 +6372,16 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs)
return bs ? bs->aio_context : qemu_get_aio_context();
}
+AioContext *coroutine_fn bdrv_co_move_to_aio_context(BlockDriverState *bs)
+{
+Coroutine *self = qemu_coroutine_self();
+AioContext *old_ctx = qemu_coroutine_get_aio_context(self);
+AioContext *new_ctx = bdrv_get_aio_context(bs);
+
+aio_co_reschedule_self(new_ctx);
+return old_ctx;
+}
+
void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)
{
aio_co_enter(bdrv_get_aio_context(bs), co);
--
2.25.4
[PATCH v7 05/13] qmp: Assert that no other monitor is active
monitor_qmp_dispatch() is never supposed to be called in the context of
another monitor, so assert that monitor_cur() is NULL instead of saving
and restoring it.
Signed-off-by: Kevin Wolf
---
monitor/qmp.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/monitor/qmp.c b/monitor/qmp.c
index bb2d9d0cc7..8469970c69 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -140,8 +140,11 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject
*req)
QDict *error;
old_mon = monitor_set_cur(>common);
+assert(old_mon == NULL);
+
rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
-monitor_set_cur(old_mon);
+
+monitor_set_cur(NULL);
if (mon->commands == _cap_negotiation_commands) {
error = qdict_get_qdict(rsp, "error");
--
2.25.4
[PATCH v7 10/13] hmp: Add support for coroutine command handlers
Often, QMP command handlers are not only called to handle QMP commands,
but also from a corresponding HMP command handler. In order to give them
a consistent environment, optionally run HMP command handlers in a
coroutine, too.
The implementation is a lot simpler than in QMP because for HMP, we
still block the VM while the coroutine is running.
Signed-off-by: Kevin Wolf
---
monitor/monitor-internal.h | 1 +
monitor/hmp.c | 37 -
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index b55d6df07f..ad2e64be13 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -74,6 +74,7 @@ typedef struct HMPCommand {
const char *help;
const char *flags; /* p=preconfig */
void (*cmd)(Monitor *mon, const QDict *qdict);
+bool coroutine;
/*
* @sub_table is a list of 2nd level of commands. If it does not exist,
* cmd should be used. If it exists, sub_table[?].cmd should be
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 4b66ca1cd6..b858b0dbde 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1056,12 +1056,26 @@ fail:
return NULL;
}
+typedef struct HandleHmpCommandCo {
+Monitor *mon;
+const HMPCommand *cmd;
+QDict *qdict;
+bool done;
+} HandleHmpCommandCo;
+
+static void handle_hmp_command_co(void *opaque)
+{
+HandleHmpCommandCo *data = opaque;
+data->cmd->cmd(data->mon, data->qdict);
+monitor_set_cur(qemu_coroutine_self(), NULL);
+data->done = true;
+}
+
void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
{
QDict *qdict;
const HMPCommand *cmd;
const char *cmd_start = cmdline;
-Monitor *old_mon;
trace_handle_hmp_command(mon, cmdline);
@@ -1080,10 +1094,23 @@ void handle_hmp_command(MonitorHMP *mon, const char
*cmdline)
return;
}
-/* old_mon is non-NULL when called from qmp_human_monitor_command() */
-old_mon = monitor_set_cur(qemu_coroutine_self(), >common);
-cmd->cmd(>common, qdict);
-monitor_set_cur(qemu_coroutine_self(), old_mon);
+if (!cmd->coroutine) {
+/* old_mon is non-NULL when called from qmp_human_monitor_command() */
+Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(),
>common);
+cmd->cmd(>common, qdict);
+monitor_set_cur(qemu_coroutine_self(), old_mon);
+} else {
+HandleHmpCommandCo data = {
+.mon = >common,
+.cmd = cmd,
+.qdict = qdict,
+.done = false,
+};
+Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, );
+monitor_set_cur(co, >common);
+aio_co_enter(qemu_get_aio_context(), co);
+AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
+}
qobject_unref(qdict);
}
--
2.25.4
[PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
The correct way to set the current monitor for a coroutine handler will
be different than for a blocking handler, so monitor_set_cur() needs to
be called in qmp_dispatch().
Signed-off-by: Kevin Wolf
---
include/qapi/qmp/dispatch.h | 3 ++-
monitor/qmp.c | 8 +---
qapi/qmp-dispatch.c | 8 +++-
qga/main.c | 2 +-
stubs/monitor-core.c| 5 +
tests/test-qmp-cmds.c | 6 +++---
6 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 5a9cf82472..0c2f467028 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -14,6 +14,7 @@
#ifndef QAPI_QMP_DISPATCH_H
#define QAPI_QMP_DISPATCH_H
+#include "monitor/monitor.h"
#include "qemu/queue.h"
typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
@@ -49,7 +50,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
bool qmp_has_success_response(const QmpCommand *cmd);
QDict *qmp_error_response(Error *err);
QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
-bool allow_oob);
+bool allow_oob, Monitor *cur_mon);
bool qmp_is_oob(const QDict *dict);
typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 8469970c69..922fdb5541 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -135,16 +135,10 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict
*rsp)
static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
{
-Monitor *old_mon;
QDict *rsp;
QDict *error;
-old_mon = monitor_set_cur(>common);
-assert(old_mon == NULL);
-
-rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
-
-monitor_set_cur(NULL);
+rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), >common);
if (mon->commands == _cap_negotiation_commands) {
error = qdict_get_qdict(rsp, "error");
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 79347e0864..2fdbc0fba4 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -89,7 +89,7 @@ bool qmp_is_oob(const QDict *dict)
}
QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
-bool allow_oob)
+bool allow_oob, Monitor *cur_mon)
{
Error *err = NULL;
bool oob;
@@ -152,7 +152,13 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject
*request,
args = qdict_get_qdict(dict, "arguments");
qobject_ref(args);
}
+
+assert(monitor_cur() == NULL);
+monitor_set_cur(cur_mon);
+
cmd->fn(args, , );
+
+monitor_set_cur(NULL);
qobject_unref(args);
if (err) {
/* or assert(!ret) after reviewing all handlers: */
diff --git a/qga/main.c b/qga/main.c
index 3febf3b0fd..241779a1d6 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -578,7 +578,7 @@ static void process_event(void *opaque, QObject *obj, Error
*err)
}
g_debug("processing command");
-rsp = qmp_dispatch(_commands, obj, false);
+rsp = qmp_dispatch(_commands, obj, false, NULL);
end:
ret = send_response(s, rsp);
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index 0cd2d864b2..dc1748bf13 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -8,6 +8,11 @@ Monitor *monitor_cur(void)
return NULL;
}
+Monitor *monitor_set_cur(Monitor *mon)
+{
+return NULL;
+}
+
void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
{
}
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index d12ff47e26..5f1b245e19 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -152,7 +152,7 @@ static QObject *do_qmp_dispatch(bool allow_oob, const char
*template, ...)
req = qdict_from_vjsonf_nofail(template, ap);
va_end(ap);
-resp = qmp_dispatch(_commands, QOBJECT(req), allow_oob);
+resp = qmp_dispatch(_commands, QOBJECT(req), allow_oob, NULL);
g_assert(resp);
ret = qdict_get(resp, "return");
g_assert(ret);
@@ -175,7 +175,7 @@ static void do_qmp_dispatch_error(bool allow_oob,
ErrorClass cls,
req = qdict_from_vjsonf_nofail(template, ap);
va_end(ap);
-resp = qmp_dispatch(_commands, QOBJECT(req), allow_oob);
+resp = qmp_dispatch(_commands, QOBJECT(req), allow_oob, NULL);
g_assert(resp);
error = qdict_get_qdict(resp, "error");
g_assert(error);
@@ -231,7 +231,7 @@ static void test_dispatch_cmd_success_response(void)
QDict *resp;
qdict_put_str(req, "execute", "cmd-success-response");
-resp = qmp_dispatch(_commands, QOBJECT(req), false);
+resp = qmp_dispatch(_commands, QOBJECT(req), false, NULL);
g_assert_null(resp);
qobject_unref(req);
}
--
2.25.4
[PATCH v7 11/13] util/async: Add aio_co_reschedule_self()
Add a function that can be used to move the currently running coroutine
to a different AioContext (and therefore potentially a different
thread).
Signed-off-by: Kevin Wolf
---
include/block/aio.h | 10 ++
util/async.c| 30 ++
2 files changed, 40 insertions(+)
diff --git a/include/block/aio.h b/include/block/aio.h
index b2f703fa3f..c37617b404 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -17,6 +17,7 @@
#ifdef CONFIG_LINUX_IO_URING
#include
#endif
+#include "qemu/coroutine.h"
#include "qemu/queue.h"
#include "qemu/event_notifier.h"
#include "qemu/thread.h"
@@ -654,6 +655,15 @@ static inline bool aio_node_check(AioContext *ctx, bool
is_external)
*/
void aio_co_schedule(AioContext *ctx, struct Coroutine *co);
+/**
+ * aio_co_reschedule_self:
+ * @new_ctx: the new context
+ *
+ * Move the currently running coroutine to new_ctx. If the coroutine is already
+ * running in new_ctx, do nothing.
+ */
+void coroutine_fn aio_co_reschedule_self(AioContext *new_ctx);
+
/**
* aio_co_wake:
* @co: the coroutine
diff --git a/util/async.c b/util/async.c
index 4266745dee..a609e18693 100644
--- a/util/async.c
+++ b/util/async.c
@@ -569,6 +569,36 @@ void aio_co_schedule(AioContext *ctx, Coroutine *co)
aio_context_unref(ctx);
}
+typedef struct AioCoRescheduleSelf {
+Coroutine *co;
+AioContext *new_ctx;
+} AioCoRescheduleSelf;
+
+static void aio_co_reschedule_self_bh(void *opaque)
+{
+AioCoRescheduleSelf *data = opaque;
+aio_co_schedule(data->new_ctx, data->co);
+}
+
+void coroutine_fn aio_co_reschedule_self(AioContext *new_ctx)
+{
+AioContext *old_ctx = qemu_get_current_aio_context();
+
+if (old_ctx != new_ctx) {
+AioCoRescheduleSelf data = {
+.co = qemu_coroutine_self(),
+.new_ctx = new_ctx,
+};
+/*
+ * We can't directly schedule the coroutine in the target context
+ * because this would be racy: The other thread could try to enter the
+ * coroutine before it has yielded in this one.
+ */
+aio_bh_schedule_oneshot(old_ctx, aio_co_reschedule_self_bh, );
+qemu_coroutine_yield();
+}
+}
+
void aio_co_wake(struct Coroutine *co)
{
AioContext *ctx;
--
2.25.4
[PATCH v7 08/13] qapi: Add a 'coroutine' flag for commands
This patch adds a new 'coroutine' flag to QMP command definitions that
tells the QMP dispatcher that the command handler is safe to be run in a
coroutine.
The documentation of the new flag pretends that this flag is already
used as intended, which it isn't yet after this patch. We'll implement
this in another patch in this series.
Signed-off-by: Kevin Wolf
Reviewed-by: Marc-André Lureau
Reviewed-by: Markus Armbruster
---
docs/devel/qapi-code-gen.txt| 12
include/qapi/qmp/dispatch.h | 1 +
tests/test-qmp-cmds.c | 4
scripts/qapi/commands.py| 10 +++---
scripts/qapi/doc.py | 2 +-
scripts/qapi/expr.py| 10 --
scripts/qapi/introspect.py | 2 +-
scripts/qapi/schema.py | 12
tests/qapi-schema/test-qapi.py | 7 ---
tests/qapi-schema/meson.build | 1 +
tests/qapi-schema/oob-coroutine.err | 2 ++
tests/qapi-schema/oob-coroutine.json| 2 ++
tests/qapi-schema/oob-coroutine.out | 0
tests/qapi-schema/qapi-schema-test.json | 1 +
tests/qapi-schema/qapi-schema-test.out | 2 ++
15 files changed, 54 insertions(+), 14 deletions(-)
create mode 100644 tests/qapi-schema/oob-coroutine.err
create mode 100644 tests/qapi-schema/oob-coroutine.json
create mode 100644 tests/qapi-schema/oob-coroutine.out
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index f3e7ced212..36daa9b5f8 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -472,6 +472,7 @@ Syntax:
'*gen': false,
'*allow-oob': true,
'*allow-preconfig': true,
+'*coroutine': true,
'*if': COND,
'*features': FEATURES }
@@ -596,6 +597,17 @@ before the machine is built. It defaults to false. For
example:
QMP is available before the machine is built only when QEMU was
started with --preconfig.
+Member 'coroutine' tells the QMP dispatcher whether the command handler
+is safe to be run in a coroutine. It defaults to false. If it is true,
+the command handler is called from coroutine context and may yield while
+waiting for an external event (such as I/O completion) in order to avoid
+blocking the guest and other background operations.
+
+It is an error to specify both 'coroutine': true and 'allow-oob': true
+for a command. We don't currently have a use case for both together and
+without a use case, it's not entirely clear what the semantics should
+be.
+
The optional 'if' member specifies a conditional. See "Configuring
the schema" below for more on this.
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 0c2f467028..9fd2b720a7 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -25,6 +25,7 @@ typedef enum QmpCommandOptions
QCO_NO_SUCCESS_RESP = (1U << 0),
QCO_ALLOW_OOB = (1U << 1),
QCO_ALLOW_PRECONFIG = (1U << 2),
+QCO_COROUTINE = (1U << 3),
} QmpCommandOptions;
typedef struct QmpCommand
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 5f1b245e19..d3413bfef0 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -36,6 +36,10 @@ void qmp_cmd_success_response(Error **errp)
{
}
+void qmp_coroutine_cmd(Error **errp)
+{
+}
+
Empty2 *qmp_user_def_cmd0(Error **errp)
{
return g_new0(Empty2, 1);
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 3cf9e1110b..6e6fc94a14 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -176,7 +176,8 @@ out:
return ret
-def gen_register_command(name, success_response, allow_oob, allow_preconfig):
+def gen_register_command(name, success_response, allow_oob, allow_preconfig,
+ coroutine):
options = []
if not success_response:
@@ -185,6 +186,8 @@ def gen_register_command(name, success_response, allow_oob,
allow_preconfig):
options += ['QCO_ALLOW_OOB']
if allow_preconfig:
options += ['QCO_ALLOW_PRECONFIG']
+if coroutine:
+options += ['QCO_COROUTINE']
if not options:
options = ['QCO_NO_OPTIONS']
@@ -267,7 +270,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
- allow_oob, allow_preconfig):
+ allow_oob, allow_preconfig, coroutine):
if not gen:
return
# FIXME: If T is a user-defined type, the user is responsible
@@ -285,7 +288,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
self._genh.add(gen_marshal_decl(name))
self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
self._regy.add(gen_register_command(name,
[PATCH v7 07/13] monitor: Make current monitor a per-coroutine property
This way, a monitor command handler will still be able to access the
current monitor, but when it yields, all other code code will correctly
get NULL from monitor_cur().
This uses a hash table to map the coroutine pointer to the current
monitor of that coroutine. Outside of coroutine context, we associate
the current monitor with the leader coroutine of the current thread.
Approaches to implement some form of coroutine local storage directly in
the coroutine core code have been considered and discarded because they
didn't end up being much more generic than the hash table and their
performance impact on coroutines not using coroutine local storage was
unclear. As the block layer uses a coroutine per I/O request, this is a
fast path and we have to be careful. It's safest to just stay out of
this path with code only used by the monitor.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
include/monitor/monitor.h | 2 +-
monitor/hmp.c | 4 ++--
monitor/monitor.c | 34 +++---
qapi/qmp-dispatch.c | 4 ++--
stubs/monitor-core.c | 2 +-
5 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 7b0ad1de12..026f8a31b2 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions;
extern QemuOptsList qemu_mon_opts;
Monitor *monitor_cur(void);
-Monitor *monitor_set_cur(Monitor *mon);
+Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
bool monitor_cur_is_qmp(void);
void monitor_init_globals(void);
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 896c670183..4b66ca1cd6 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1081,9 +1081,9 @@ void handle_hmp_command(MonitorHMP *mon, const char
*cmdline)
}
/* old_mon is non-NULL when called from qmp_human_monitor_command() */
-old_mon = monitor_set_cur(>common);
+old_mon = monitor_set_cur(qemu_coroutine_self(), >common);
cmd->cmd(>common, qdict);
-monitor_set_cur(old_mon);
+monitor_set_cur(qemu_coroutine_self(), old_mon);
qobject_unref(qdict);
}
diff --git a/monitor/monitor.c b/monitor/monitor.c
index be3839a7aa..629aa073ee 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -58,29 +58,48 @@ IOThread *mon_iothread;
/* Bottom half to dispatch the requests received from I/O thread */
QEMUBH *qmp_dispatcher_bh;
-/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */
+/*
+ * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
+ * monitor_destroyed.
+ */
QemuMutex monitor_lock;
static GHashTable *monitor_qapi_event_state;
+static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
MonitorList mon_list;
int mon_refcount;
static bool monitor_destroyed;
-static __thread Monitor *cur_monitor;
-
Monitor *monitor_cur(void)
{
-return cur_monitor;
+Monitor *mon;
+
+qemu_mutex_lock(_lock);
+mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
+qemu_mutex_unlock(_lock);
+
+return mon;
}
/**
* Sets a new current monitor and returns the old one.
+ *
+ * If a non-NULL monitor is set for a coroutine, another call resetting it to
+ * NULL is required before the coroutine terminates, otherwise a stale entry
+ * would remain in the hash table.
*/
-Monitor *monitor_set_cur(Monitor *mon)
+Monitor *monitor_set_cur(Coroutine *co, Monitor *mon)
{
-Monitor *old_monitor = cur_monitor;
+Monitor *old_monitor = monitor_cur();
+
+qemu_mutex_lock(_lock);
+if (mon) {
+g_hash_table_replace(coroutine_mon, co, mon);
+} else {
+g_hash_table_remove(coroutine_mon, co);
+}
+qemu_mutex_unlock(_lock);
-cur_monitor = mon;
return old_monitor;
}
@@ -623,6 +642,7 @@ void monitor_init_globals_core(void)
{
monitor_qapi_event_init();
qemu_mutex_init(_lock);
+coroutine_mon = g_hash_table_new(NULL, NULL);
/*
* The dispatcher BH must run in the main loop thread, since we
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 2fdbc0fba4..5677ba92ca 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -154,11 +154,11 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject
*request,
}
assert(monitor_cur() == NULL);
-monitor_set_cur(cur_mon);
+monitor_set_cur(qemu_coroutine_self(), cur_mon);
cmd->fn(args, , );
-monitor_set_cur(NULL);
+monitor_set_cur(qemu_coroutine_self(), NULL);
qobject_unref(args);
if (err) {
/* or assert(!ret) after reviewing all handlers: */
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index dc1748bf13..d058a2a00d 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -8,7 +8,7 @@ Monitor *monitor_cur(void)
return NULL;
}
-Monitor *monitor_set_cur(Monitor *mon)
+Monitor *monitor_set_cur(Coroutine *co, Monitor *mon)
{
return NULL;
}
--
2.25.4
[PATCH v7 04/13] hmp: Update current monitor only in handle_hmp_command()
The current monitor is updated relatively early in the command handling
code even though only the command handler actually needs it.
The current monitor will become coroutine-local later, so we can only
update it when we know in which coroutine the command will be exectued.
Move it to handle_hmp_command() where this information will be
available.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
monitor/hmp.c | 10 +-
monitor/misc.c | 5 -
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 561e32d02f..896c670183 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1061,6 +1061,7 @@ void handle_hmp_command(MonitorHMP *mon, const char
*cmdline)
QDict *qdict;
const HMPCommand *cmd;
const char *cmd_start = cmdline;
+Monitor *old_mon;
trace_handle_hmp_command(mon, cmdline);
@@ -1079,7 +1080,11 @@ void handle_hmp_command(MonitorHMP *mon, const char
*cmdline)
return;
}
+/* old_mon is non-NULL when called from qmp_human_monitor_command() */
+old_mon = monitor_set_cur(>common);
cmd->cmd(>common, qdict);
+monitor_set_cur(old_mon);
+
qobject_unref(qdict);
}
@@ -1301,11 +1306,8 @@ cleanup:
static void monitor_read(void *opaque, const uint8_t *buf, int size)
{
MonitorHMP *mon = container_of(opaque, MonitorHMP, common);
-Monitor *old_mon;
int i;
-old_mon = monitor_set_cur(>common);
-
if (mon->rs) {
for (i = 0; i < size; i++) {
readline_handle_byte(mon->rs, buf[i]);
@@ -1317,8 +1319,6 @@ static void monitor_read(void *opaque, const uint8_t
*buf, int size)
handle_hmp_command(mon, (char *)buf);
}
}
-
-monitor_set_cur(old_mon);
}
static void monitor_event(void *opaque, QEMUChrEvent event)
diff --git a/monitor/misc.c b/monitor/misc.c
index a5eb3025f8..ba1063024c 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -120,17 +120,13 @@ char *qmp_human_monitor_command(const char *command_line,
bool has_cpu_index,
int64_t cpu_index, Error **errp)
{
char *output = NULL;
-Monitor *old_mon;
MonitorHMP hmp = {};
monitor_data_init(, false, true, false);
-old_mon = monitor_set_cur();
-
if (has_cpu_index) {
int ret = monitor_set_cpu(, cpu_index);
if (ret < 0) {
-monitor_set_cur(old_mon);
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
"a CPU number");
goto out;
@@ -138,7 +134,6 @@ char *qmp_human_monitor_command(const char *command_line,
bool has_cpu_index,
}
handle_hmp_command(, command_line);
-monitor_set_cur(old_mon);
qemu_mutex_lock(_lock);
if (qstring_get_length(hmp.common.outbuf) > 0) {
--
2.25.4
[PATCH v7 03/13] monitor: Use getter/setter functions for cur_mon
cur_mon really needs to be coroutine-local as soon as we move monitor
command handlers to coroutines and let them yield. As a first step, just
remove all direct accesses to cur_mon so that we can implement this in
the getter function later.
Signed-off-by: Kevin Wolf
---
include/monitor/monitor.h | 3 ++-
audio/wavcapture.c | 8
dump/dump.c| 2 +-
hw/scsi/vhost-scsi.c | 2 +-
hw/virtio/vhost-vsock.c| 2 +-
migration/fd.c | 4 ++--
monitor/hmp.c | 11 +--
monitor/misc.c | 13 +++--
monitor/monitor.c | 24 +++-
monitor/qmp-cmds-control.c | 2 ++
monitor/qmp-cmds.c | 2 +-
monitor/qmp.c | 7 ++-
net/socket.c | 2 +-
net/tap.c | 6 +++---
softmmu/cpus.c | 2 +-
stubs/monitor-core.c | 5 -
tests/test-util-sockets.c | 12 +---
trace/control.c| 2 +-
util/qemu-error.c | 6 +++---
util/qemu-print.c | 3 ++-
util/qemu-sockets.c| 1 +
21 files changed, 72 insertions(+), 47 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a64502fbb2..7b0ad1de12 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -5,7 +5,6 @@
#include "qapi/qapi-types-misc.h"
#include "qemu/readline.h"
-extern __thread Monitor *cur_mon;
typedef struct MonitorHMP MonitorHMP;
typedef struct MonitorOptions MonitorOptions;
@@ -13,6 +12,8 @@ typedef struct MonitorOptions MonitorOptions;
extern QemuOptsList qemu_mon_opts;
+Monitor *monitor_cur(void);
+Monitor *monitor_set_cur(Monitor *mon);
bool monitor_cur_is_qmp(void);
void monitor_init_globals(void);
diff --git a/audio/wavcapture.c b/audio/wavcapture.c
index 17e87ed6f4..c60286e162 100644
--- a/audio/wavcapture.c
+++ b/audio/wavcapture.c
@@ -1,5 +1,5 @@
#include "qemu/osdep.h"
-#include "monitor/monitor.h"
+#include "qemu/qemu-print.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "audio.h"
@@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
WAVState *wav = opaque;
char *path = wav->path;
-monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
-wav->freq, wav->bits, wav->nchannels,
-path ? path : "", wav->bytes);
+qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
+wav->freq, wav->bits, wav->nchannels,
+path ? path : "", wav->bytes);
}
static struct capture_ops wav_capture_ops = {
diff --git a/dump/dump.c b/dump/dump.c
index 383bc7876b..232027e92c 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1986,7 +1986,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
#if !defined(WIN32)
if (strstart(file, "fd:", )) {
-fd = monitor_get_fd(cur_mon, p, errp);
+fd = monitor_get_fd(monitor_cur(), p, errp);
if (fd == -1) {
return;
}
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index a83ffeefc8..4d70fa036b 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -177,7 +177,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error
**errp)
}
if (vs->conf.vhostfd) {
-vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, errp);
+vhostfd = monitor_fd_param(monitor_cur(), vs->conf.vhostfd, errp);
if (vhostfd == -1) {
error_prepend(errp, "vhost-scsi: unable to parse vhostfd: ");
return;
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index c8f0699b4f..f9db4beb47 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -143,7 +143,7 @@ static void vhost_vsock_device_realize(DeviceState *dev,
Error **errp)
}
if (vsock->conf.vhostfd) {
-vhostfd = monitor_fd_param(cur_mon, vsock->conf.vhostfd, errp);
+vhostfd = monitor_fd_param(monitor_cur(), vsock->conf.vhostfd, errp);
if (vhostfd == -1) {
error_prepend(errp, "vhost-vsock: unable to parse vhostfd: ");
return;
diff --git a/migration/fd.c b/migration/fd.c
index 0a29ecdebf..6f2f50475f 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -26,7 +26,7 @@
void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error
**errp)
{
QIOChannel *ioc;
-int fd = monitor_get_fd(cur_mon, fdname, errp);
+int fd = monitor_get_fd(monitor_cur(), fdname, errp);
if (fd == -1) {
return;
}
@@ -55,7 +55,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
void fd_start_incoming_migration(const char *fdname, Error **errp)
{
QIOChannel *ioc;
-int fd = monitor_fd_param(cur_mon, fdname, errp);
+int fd = monitor_fd_param(monitor_cur(), fdname, errp);
if (fd == -1) {
return;
}
diff --git a/monitor/hmp.c b/monitor/hmp.c
index d598dd02bb..561e32d02f 100644
--- a/monitor/hmp.c
+++
[PATCH v7 01/13] monitor: Add Monitor parameter to monitor_set_cpu()
Most callers actually don't have to rely on cur_mon, but already know
for which monitor they call monitor_set_cpu().
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
include/monitor/monitor.h | 2 +-
monitor/hmp-cmds.c| 2 +-
monitor/misc.c| 10 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1018d754a6..0dcaefd4f9 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -33,7 +33,7 @@ int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
GCC_FMT_ATTR(2, 0);
int monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
void monitor_flush(Monitor *mon);
-int monitor_set_cpu(int cpu_index);
+int monitor_set_cpu(Monitor *mon, int cpu_index);
int monitor_get_cpu_index(void);
void monitor_read_command(MonitorHMP *mon, int show_prompt);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 7711726fd2..f608b5b27b 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -996,7 +996,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict)
/* XXX: drop the monitor_set_cpu() usage when all HMP commands that
use it are converted to the QAPI */
cpu_index = qdict_get_int(qdict, "index");
-if (monitor_set_cpu(cpu_index) < 0) {
+if (monitor_set_cpu(mon, cpu_index) < 0) {
monitor_printf(mon, "invalid CPU index\n");
}
}
diff --git a/monitor/misc.c b/monitor/misc.c
index e847b58a8c..b4f779b8d3 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -129,7 +129,7 @@ char *qmp_human_monitor_command(const char *command_line,
bool has_cpu_index,
cur_mon =
if (has_cpu_index) {
-int ret = monitor_set_cpu(cpu_index);
+int ret = monitor_set_cpu(, cpu_index);
if (ret < 0) {
cur_mon = old_mon;
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
@@ -255,7 +255,7 @@ static void monitor_init_qmp_commands(void)
}
/* Set the current CPU defined by the user. Callers must hold BQL. */
-int monitor_set_cpu(int cpu_index)
+int monitor_set_cpu(Monitor *mon, int cpu_index)
{
CPUState *cpu;
@@ -263,8 +263,8 @@ int monitor_set_cpu(int cpu_index)
if (cpu == NULL) {
return -1;
}
-g_free(cur_mon->mon_cpu_path);
-cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
+g_free(mon->mon_cpu_path);
+mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
return 0;
}
@@ -285,7 +285,7 @@ static CPUState *mon_get_cpu_sync(bool synchronize)
if (!first_cpu) {
return NULL;
}
-monitor_set_cpu(first_cpu->cpu_index);
+monitor_set_cpu(cur_mon, first_cpu->cpu_index);
cpu = first_cpu;
}
assert(cpu != NULL);
--
2.25.4
[PATCH v7 00/13] monitor: Optionally run handlers in coroutines
Some QMP command handlers can block the main loop for a relatively long time, for example because they perform some I/O. This is quite nasty. Allowing such handlers to run in a coroutine where they can yield (and therefore release the BQL) while waiting for an event such as I/O completion solves the problem. This series adds the infrastructure to allow this and switches block_resize to run in a coroutine as a first example. This is an alternative solution to Marc-André's "monitor: add asynchronous command type" series. v7: - Added patch 2 to add a Monitor parameter to monitor_get_cpu_index(), too [Markus] - Let monitor_set_cur() return the old monitor [Markus] - Fixed comment about linking stub objects in test-util-sockets [Markus] - More detailed commit message for per-coroutine current monitor and document that monitor_set_cur(NULL) must be called eventually [Markus] - Resolve some merge conflicts on rebase v6: - Fixed cur_mon behaviour: It should always point to the Monitor while we're in the handler coroutine, but be NULL while the handler coroutines has yielded. [Markus] - Give HMP handlers the coroutine option, too, because they will call QMP handlers, and life is easier when we know whether we are in coroutine context or not. - Fixed block_resize for block devices in iothreads and for HMP - Resolved some merge conflict with QAPI generator and monitor refactorings that were merged in the meantime v5: - Improved comments and documentation [Markus] v4: - Forbid 'oob': true, 'coroutine': true [Markus] - Removed Python type hints [Markus] - Introduced separate bool qmp_dispatcher_co_shutdown to make it clearer how a shutdown request is signalled to the dispatcher [Markus] - Allow using aio_poll() with iohandler_ctx and use that instead of aio_bh_poll() [Markus] - Removed coroutine_fn from qmp_block_resize() again because at least one caller (HMP) calls it outside of coroutine context [Markus] - Tried to make the synchronisation between dispatcher and the monitor thread clearer, and fixed a race condition - Improved documentation and comments v3: - Fix race between monitor thread and dispatcher that could schedule the dispatcher coroutine twice if a second requests comes in before the dispatcher can wake up [Patchew] v2: - Fix typo in a commit message [Eric] - Use hyphen instead of underscore for the test command [Eric] - Mark qmp_block_resize() as coroutine_fn [Stefan] Kevin Wolf (13): monitor: Add Monitor parameter to monitor_set_cpu() monitor: Add Monitor parameter to monitor_get_cpu_index() monitor: Use getter/setter functions for cur_mon hmp: Update current monitor only in handle_hmp_command() qmp: Assert that no other monitor is active qmp: Call monitor_set_cur() only in qmp_dispatch() monitor: Make current monitor a per-coroutine property qapi: Add a 'coroutine' flag for commands qmp: Move dispatcher to a coroutine hmp: Add support for coroutine command handlers util/async: Add aio_co_reschedule_self() block: Add bdrv_co_move_to_aio_context() block: Convert 'block_resize' to coroutine qapi/block-core.json| 3 +- docs/devel/qapi-code-gen.txt| 12 +++ include/block/aio.h | 10 ++ include/block/block.h | 6 ++ include/monitor/monitor.h | 7 +- include/qapi/qmp/dispatch.h | 5 +- monitor/monitor-internal.h | 7 +- audio/wavcapture.c | 8 +- block.c | 10 ++ blockdev.c | 13 ++- dump/dump.c | 2 +- hw/core/machine-hmp-cmds.c | 2 +- hw/scsi/vhost-scsi.c| 2 +- hw/virtio/vhost-vsock.c | 2 +- migration/fd.c | 4 +- monitor/hmp-cmds.c | 4 +- monitor/hmp.c | 44 ++-- monitor/misc.c | 38 --- monitor/monitor.c | 101 -- monitor/qmp-cmds-control.c | 2 + monitor/qmp-cmds.c | 2 +- monitor/qmp.c | 130 +--- net/socket.c| 2 +- net/tap.c | 6 +- qapi/qmp-dispatch.c | 61 ++- qapi/qmp-registry.c | 3 + qga/main.c | 2 +- softmmu/cpus.c | 2 +- stubs/monitor-core.c| 10 +- tests/test-qmp-cmds.c | 10 +- tests/test-util-sockets.c | 12 +-- trace/control.c | 2 +- util/aio-posix.c| 8 +- util/async.c| 30 ++ util/qemu-error.c | 6 +- util/qemu-print.c | 3 +- util/qemu-sockets.c
[PATCH v7 02/13] monitor: Add Monitor parameter to monitor_get_cpu_index()
Most callers actually don't have to rely on cur_mon, but already know
for which monitor they call monitor_get_cpu_index().
Signed-off-by: Kevin Wolf
---
include/monitor/monitor.h | 2 +-
hw/core/machine-hmp-cmds.c | 2 +-
monitor/hmp-cmds.c | 2 +-
monitor/misc.c | 20 ++--
softmmu/cpus.c | 2 +-
5 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 0dcaefd4f9..a64502fbb2 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -34,7 +34,7 @@ int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
int monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
void monitor_flush(Monitor *mon);
int monitor_set_cpu(Monitor *mon, int cpu_index);
-int monitor_get_cpu_index(void);
+int monitor_get_cpu_index(Monitor *mon);
void monitor_read_command(MonitorHMP *mon, int show_prompt);
int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
index 3c47c5..da7d2e27b6 100644
--- a/hw/core/machine-hmp-cmds.c
+++ b/hw/core/machine-hmp-cmds.c
@@ -34,7 +34,7 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
for (cpu = cpu_list; cpu; cpu = cpu->next) {
int active = ' ';
-if (cpu->value->cpu_index == monitor_get_cpu_index()) {
+if (cpu->value->cpu_index == monitor_get_cpu_index(mon)) {
active = '*';
}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index f608b5b27b..1e5e611ed9 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1007,7 +1007,7 @@ void hmp_memsave(Monitor *mon, const QDict *qdict)
const char *filename = qdict_get_str(qdict, "filename");
uint64_t addr = qdict_get_int(qdict, "val");
Error *err = NULL;
-int cpu_index = monitor_get_cpu_index();
+int cpu_index = monitor_get_cpu_index(mon);
if (cpu_index < 0) {
monitor_printf(mon, "No CPU available\n");
diff --git a/monitor/misc.c b/monitor/misc.c
index b4f779b8d3..972726061c 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -269,23 +269,23 @@ int monitor_set_cpu(Monitor *mon, int cpu_index)
}
/* Callers must hold BQL. */
-static CPUState *mon_get_cpu_sync(bool synchronize)
+static CPUState *mon_get_cpu_sync(Monitor *mon, bool synchronize)
{
CPUState *cpu = NULL;
-if (cur_mon->mon_cpu_path) {
-cpu = (CPUState *) object_resolve_path_type(cur_mon->mon_cpu_path,
+if (mon->mon_cpu_path) {
+cpu = (CPUState *) object_resolve_path_type(mon->mon_cpu_path,
TYPE_CPU, NULL);
if (!cpu) {
-g_free(cur_mon->mon_cpu_path);
-cur_mon->mon_cpu_path = NULL;
+g_free(mon->mon_cpu_path);
+mon->mon_cpu_path = NULL;
}
}
-if (!cur_mon->mon_cpu_path) {
+if (!mon->mon_cpu_path) {
if (!first_cpu) {
return NULL;
}
-monitor_set_cpu(cur_mon, first_cpu->cpu_index);
+monitor_set_cpu(mon, first_cpu->cpu_index);
cpu = first_cpu;
}
assert(cpu != NULL);
@@ -297,7 +297,7 @@ static CPUState *mon_get_cpu_sync(bool synchronize)
CPUState *mon_get_cpu(void)
{
-return mon_get_cpu_sync(true);
+return mon_get_cpu_sync(cur_mon, true);
}
CPUArchState *mon_get_cpu_env(void)
@@ -307,9 +307,9 @@ CPUArchState *mon_get_cpu_env(void)
return cs ? cs->env_ptr : NULL;
}
-int monitor_get_cpu_index(void)
+int monitor_get_cpu_index(Monitor *mon)
{
-CPUState *cs = mon_get_cpu_sync(false);
+CPUState *cs = mon_get_cpu_sync(mon, false);
return cs ? cs->cpu_index : UNASSIGNED_CPU_INDEX;
}
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index e3b98065c9..ae643447e9 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -2224,7 +2224,7 @@ exit:
void qmp_inject_nmi(Error **errp)
{
-nmi_monitor_handle(monitor_get_cpu_index(), errp);
+nmi_monitor_handle(monitor_get_cpu_index(cur_mon), errp);
}
void dump_drift_info(void)
--
2.25.4
[PATCH v6 1/4] util/vfio-helpers: Improve reporting unsupported IOMMU type
Change the confuse "VFIO IOMMU check failed" error message by
the explicit "VFIO IOMMU Type1 is not supported" once.
Example on POWER:
$ qemu-system-ppc64 -drive
if=none,id=nvme0,file=nvme://0001:01:00.0/1,format=raw
qemu-system-ppc64: -drive
if=none,id=nvme0,file=nvme://0001:01:00.0/1,format=raw: VFIO IOMMU Type1 is not
supported
Suggested-by: Alex Williamson
Reviewed-by: Fam Zheng
Signed-off-by: Philippe Mathieu-Daudé
---
util/vfio-helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 583bdfb36fc..55b4107ce69 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -261,7 +261,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char
*device,
}
if (!ioctl(s->container, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU)) {
-error_setg_errno(errp, errno, "VFIO IOMMU check failed");
+error_setg_errno(errp, errno, "VFIO IOMMU Type1 is not supported");
ret = -EINVAL;
goto fail_container;
}
--
2.26.2
[PATCH v6 3/4] util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()
qemu_vfio_pci_init_irq() allows us to initialize any type of IRQ,
but only one. Introduce qemu_vfio_pci_init_msix_irqs() which is
specific to MSIX IRQ type, and allow us to use multiple IRQs
(thus passing multiple eventfd notifiers).
Reviewed-by: Fam Zheng
Signed-off-by: Philippe Mathieu-Daudé
---
include/qemu/vfio-helpers.h | 6 +++-
util/vfio-helpers.c | 65 -
2 files changed, 69 insertions(+), 2 deletions(-)
diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 1f057c2b9e4..092b7b243f9 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -1,11 +1,13 @@
/*
* QEMU VFIO helpers
*
- * Copyright 2016 - 2018 Red Hat, Inc.
+ * Copyright 2016 - 2020 Red Hat, Inc.
*
* Authors:
* Fam Zheng
+ * Philippe Mathieu-Daudé
*
+ * SPDX-License-Identifier: GPL-2.0-or-later
* This work is licensed under the terms of the GNU GPL, version 2 or later.
* See the COPYING file in the top-level directory.
*/
@@ -28,5 +30,7 @@ void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index,
void *bar,
uint64_t offset, uint64_t size);
int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
int irq_type, Error **errp);
+int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *e,
+ unsigned *irq_count, Error **errp);
#endif
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 4cc5697dcb2..3b1b81caf8b 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -1,11 +1,13 @@
/*
* VFIO utility
*
- * Copyright 2016 - 2018 Red Hat, Inc.
+ * Copyright 2016 - 2020 Red Hat, Inc.
*
* Authors:
* Fam Zheng
+ * Philippe Mathieu-Daudé
*
+ * SPDX-License-Identifier: GPL-2.0-or-later
* This work is licensed under the terms of the GNU GPL, version 2 or later.
* See the COPYING file in the top-level directory.
*/
@@ -216,6 +218,67 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier
*e,
return 0;
}
+/**
+ * Initialize device MSIX IRQs and register event notifiers.
+ * @irq_count: pointer to number of MSIX IRQs to initialize
+ * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX IRQ)
+
+ * If the number of IRQs requested exceeds the available on the device,
+ * store the number of available IRQs in @irq_count and return -EOVERFLOW.
+ */
+int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *notifier,
+ unsigned *irq_count, Error **errp)
+{
+int r;
+size_t irq_set_size;
+struct vfio_irq_set *irq_set;
+struct vfio_irq_info irq_info = {
+.argsz = sizeof(irq_info),
+.index = VFIO_PCI_MSIX_IRQ_INDEX
+};
+
+if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, _info)) {
+error_setg_errno(errp, errno, "Failed to get device interrupt info");
+return -errno;
+}
+if (irq_info.count < *irq_count) {
+error_setg(errp, "Not enough device interrupts available");
+*irq_count = irq_info.count;
+return -EOVERFLOW;
+}
+if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
+error_setg(errp, "Device interrupt doesn't support eventfd");
+return -EINVAL;
+}
+
+irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t);
+irq_set = g_malloc0(irq_set_size);
+
+/* Get to a known IRQ state */
+*irq_set = (struct vfio_irq_set) {
+.argsz = irq_set_size,
+.flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
+.index = irq_info.index,
+.start = 0,
+.count = *irq_count,
+};
+
+for (unsigned i = 0; i < *irq_count; i++) {
+((int32_t *)_set->data)[i] = event_notifier_get_fd([i]);
+}
+r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
+g_free(irq_set);
+if (r <= 0) {
+error_setg_errno(errp, errno, "Failed to setup device interrupts");
+return -errno;
+} else if (r < *irq_count) {
+error_setg(errp, "Not enough device interrupts available");
+*irq_count = r;
+return -EOVERFLOW;
+}
+return 0;
+}
+
static int qemu_vfio_pci_read_config(QEMUVFIOState *s, void *buf,
int size, int ofs)
{
--
2.26.2
[PATCH v6 4/4] block/nvme: Use qemu_vfio_pci_init_msix_irqs() to initialize our IRQ
Instead of initializing one MSIX IRQ with the generic
qemu_vfio_pci_init_irq() function, use the MSIX specific one which
will allow us to use multiple IRQs. For now we provide an array of
a single IRQ.
Reviewed-by: Fam Zheng
Signed-off-by: Philippe Mathieu-Daudé
---
block/nvme.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index f4f27b6da7d..e6dac027f72 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -694,6 +694,7 @@ static int nvme_init(BlockDriverState *bs, const char
*device, int namespace,
uint64_t timeout_ms;
uint64_t deadline, now;
Error *local_err = NULL;
+unsigned irq_count = MSIX_IRQ_COUNT;
qemu_co_mutex_init(>dma_map_lock);
qemu_co_queue_init(>dma_flush_queue);
@@ -779,9 +780,13 @@ static int nvme_init(BlockDriverState *bs, const char
*device, int namespace,
}
}
-ret = qemu_vfio_pci_init_irq(s->vfio, s->irq_notifier,
- VFIO_PCI_MSIX_IRQ_INDEX, errp);
+ret = qemu_vfio_pci_init_msix_irqs(s->vfio, s->irq_notifier,
+ _count, errp);
if (ret) {
+if (ret == -EOVERFLOW) {
+error_append_hint(errp, "%u IRQs requested but only %u
available\n",
+ MSIX_IRQ_COUNT, irq_count);
+}
goto out;
}
aio_set_event_notifier(bdrv_get_aio_context(bs),
--
2.26.2
[PATCH v6 2/4] util/vfio-helpers: Report error when IOMMU page size is not supported
This driver uses the host page size to align its memory regions,
but this size is not always compatible with the IOMMU. Add a
check if the size matches, and bails out with listing the sizes
the IOMMU supports.
Example on Aarch64:
$ qemu-system-aarch64 -M virt -drive
if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw
qemu-system-aarch64: -drive
if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw: Unsupported IOMMU page
size: 4 KiB
Available page size:
64 KiB
512 MiB
Suggested-by: Alex Williamson
Reviewed-by: Fam Zheng
Signed-off-by: Philippe Mathieu-Daudé
---
util/vfio-helpers.c | 20
1 file changed, 20 insertions(+)
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 55b4107ce69..4cc5697dcb2 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -11,6 +11,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include
#include
#include "qapi/error.h"
@@ -316,6 +317,25 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char
*device,
ret = -errno;
goto fail;
}
+if (!(iommu_info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
+error_setg(errp, "Failed to get IOMMU page size info");
+ret = -EINVAL;
+goto fail;
+}
+if (!extract64(iommu_info.iova_pgsizes,
+ ctz64(qemu_real_host_page_size), 1)) {
+g_autofree char *host_page_size =
size_to_str(qemu_real_host_page_size);
+error_setg(errp, "Unsupported IOMMU page size: %s", host_page_size);
+error_append_hint(errp, "Available page size:\n");
+for (int i = 0; i < 64; i++) {
+if (extract64(iommu_info.iova_pgsizes, i, 1)) {
+g_autofree char *iova_pgsizes = size_to_str(1UL << i);
+error_append_hint(errp, " %s\n", iova_pgsizes);
+}
+}
+ret = -EINVAL;
+goto fail;
+}
s->device = ioctl(s->group, VFIO_GROUP_GET_DEVICE_FD, device);
--
2.26.2
[PATCH v6 0/4] util/vfio-helpers: Add support for multiple IRQs
This series intends to setup the VFIO helper to allow binding notifiers on different IRQs. For the NVMe use case, we only care about MSIX interrupts. To not disrupt other users, introduce the qemu_vfio_pci_init_msix_irqs function to initialize multiple MSIX IRQs and attach eventfd to them. Since RFC v5: - addressed Fam review comment (return EINVAL) - addressed Fam R-b tags - no more RFC :) Since RFC v4: - addressed Alex review comment: check ioctl(VFIO_DEVICE_SET_IRQS) return value Since RFC v3: - addressed Alex and Stefan review comments Since RFC v2: - new patch to report vfio-helpers is not supported on AA64/POWER (NVMe block driver series will follow). Based-on: <20200908115322.325832-1-kw...@redhat.com> (Block layer pending pull request) Philippe Mathieu-Daudé (4): util/vfio-helpers: Improve reporting unsupported IOMMU type util/vfio-helpers: Report error when IOMMU page size is not supported util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs() block/nvme: Use qemu_vfio_pci_init_msix_irqs() to initialize our IRQ include/qemu/vfio-helpers.h | 6 ++- block/nvme.c| 9 +++- util/vfio-helpers.c | 87 - 3 files changed, 97 insertions(+), 5 deletions(-) -- 2.26.2
Re: [RFC PATCH v5 2/4] util/vfio-helpers: Report error when IOMMU page size is not supported
On 9/9/20 10:38 AM, Fam Zheng wrote:
> On 2020-09-08 20:03, Philippe Mathieu-Daudé wrote:
>> This driver uses the host page size to align its memory regions,
>> but this size is not always compatible with the IOMMU. Add a
>> check if the size matches, and bails out with listing the sizes
>> the IOMMU supports.
>>
>> Example on Aarch64:
>>
>> $ qemu-system-aarch64 -M virt -drive
>> if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw
>> qemu-system-aarch64: -drive
>> if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw: Unsupported IOMMU
>> page size: 4 KiB
>> Available page size:
>> 64 KiB
>> 512 MiB
>>
>> Suggested-by: Alex Williamson
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> util/vfio-helpers.c | 20
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index 55b4107ce69..6d9ec7d365c 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c
>> @@ -11,6 +11,7 @@
>> */
>>
>> #include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>> #include
>> #include
>> #include "qapi/error.h"
>> @@ -316,6 +317,25 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const
>> char *device,
>> ret = -errno;
>> goto fail;
>> }
>> +if (!(iommu_info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
>> +error_setg(errp, "Failed to get IOMMU page size info");
>> +ret = -errno;
>
> We don't have errno here, do we?
Oops thanks, I'll replace by "ret = -EINVAL;".
>
>> +goto fail;
>> +}
>> +if (!extract64(iommu_info.iova_pgsizes,
>> + ctz64(qemu_real_host_page_size), 1)) {
>> +g_autofree char *host_page_size =
>> size_to_str(qemu_real_host_page_size);
>> +error_setg(errp, "Unsupported IOMMU page size: %s", host_page_size);
>> +error_append_hint(errp, "Available page size:\n");
>> +for (int i = 0; i < 64; i++) {
>> +if (extract64(iommu_info.iova_pgsizes, i, 1)) {
>> +g_autofree char *iova_pgsizes = size_to_str(1UL << i);
>> +error_append_hint(errp, " %s\n", iova_pgsizes);
>
> Interesting... Since it's printing page size which is fairly low level,
> why not just plain (hex) numbers?
Because this error is reported to the user (not the developer)
and depends of the host/iommu.
If you don't mind I'll keep it as it (as the code is written).
Thank you for reviewing the series!
Phil.
>
> Fam
>
>> +}
>> +}
>> +ret = -EINVAL;
>> +goto fail;
>> +}
>>
>> s->device = ioctl(s->group, VFIO_GROUP_GET_DEVICE_FD, device);
>>
>> --
>> 2.26.2
>>
>>
>
Re: [RFC PATCH 00/12] hw: Forbid DMA write accesses to MMIO regions
On Wed, Sep 9, 2020 at 2:23 PM Peter Maydell wrote: > On Wed, 9 Sep 2020 at 10:12, Stefan Hajnoczi wrote: > > On Thu, Sep 03, 2020 at 01:08:19PM +0200, Philippe Mathieu-Daudé wrote: > > > The main patch is: > > > "exec/memattrs: Introduce MemTxAttrs::direct_access field". > > > This way we can restrict accesses to ROM/RAM by setting the > > > 'direct_access' field. Illegal accesses return MEMTX_BUS_ERROR. > > > > QEMU needs to simulate the behavior of real hardware. What is the > > behavior of real hardware? > > It varies, depending on the hardware. The most common thing > is probably "happens to work by luck", which is OK for hardware > but doesn't help us much since our software implementation is > naturally more serialized than hardware is and since we don't > want to allow guests to make QEMU crash or misbehave. The memory API bounce buffer mechanism is evidence that some board(s) need or needed it. At a minimum we need to find out the reason for the bounce buffer mechanism to avoid breaking guests. Stefan
Re: [PATCH v2 04/21] curses: Fixes curses compiling errors.
On Wed, 9 Sep 2020 at 10:46, Yonggang Luo wrote: > > This is the compiling error: > ../ui/curses.c: In function 'curses_refresh': > ../ui/curses.c:256:5: error: 'next_maybe_keycode' may be used uninitialized > in this function [-Werror=maybe-uninitialized] > 256 | curses2foo(_curses2keycode, _curseskey2keycode, chr, > maybe_keycode) > | ^~ > ../ui/curses.c:302:32: note: 'next_maybe_keycode' was declared here > 302 | enum maybe_keycode next_maybe_keycode; > |^~ > ../ui/curses.c:256:5: error: 'maybe_keycode' may be used uninitialized in > this function [-Werror=maybe-uninitialized] > 256 | curses2foo(_curses2keycode, _curseskey2keycode, chr, > maybe_keycode) > | ^~ > ../ui/curses.c:265:24: note: 'maybe_keycode' was declared here > 265 | enum maybe_keycode maybe_keycode; > |^ > cc1.exe: all warnings being treated as errors > > gcc version 10.2.0 (Rev1, Built by MSYS2 project) > > Signed-off-by: Yonggang Luo > Reviewed-by: Peter Maydell > Reviewed-by: Gerd Hoffmann I never gave this a reviewed-by tag -- can you be more careful with your tag handling, please? thanks -- PMM
Re: [PATCH v2 03/21] configure: Fixes ncursesw detection under msys2/mingw and enable curses
On Wed, Sep 09, 2020 at 08:55:15PM +0800, 罗勇刚(Yonggang Luo) wrote:
> On Wed, Sep 9, 2020 at 8:51 PM Daniel P. Berrangé
> wrote:
>
> > On Wed, Sep 09, 2020 at 05:45:59PM +0800, Yonggang Luo wrote:
> > > The mingw pkg-config are showing following absolute path and contains :
> > as the separator,
> > > so we must handling : properly.
> > >
> > > -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L
> > -IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
> > > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> > -pipe -lncursesw -lgnurx -ltre -lintl -liconv
> > > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> > -lncursesw
> > > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> > -lcursesw
> > > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe
> > -lncursesw -lgnurx -ltre -lintl -liconv
> > > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
> > > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
> > > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx
> > -ltre -lintl -liconv
> > > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
> > > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw
> > >
> > > msys2/mingw lacks the POSIX-required langinfo.h.
> > >
> > > gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe
> > -lncursesw -lgnurx -ltre -lintl -liconv
> > > test.c:4:10: fatal error: langinfo.h: No such file or directory
> > > 4 | #include
> > > | ^~~~
> > > compilation terminated.
> > >
> > > So we using g_get_codeset instead of nl_langinfo(CODESET)
> > >
> > > Signed-off-by: Yonggang Luo
> > > Reviewed-by: Gerd Hoffmann
> > > ---
> > > configure | 9 +++--
> > > ui/curses.c | 10 +-
> > > 2 files changed, 8 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/configure b/configure
> > > index f4f8bc3756..2e6d54e15b 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -3653,8 +3653,8 @@ if test "$iconv" = "no" ; then
> > > fi
> > > if test "$curses" != "no" ; then
> > >if test "$mingw32" = "yes" ; then
> > > -curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
> > > -curses_lib_list="$($pkg_config --libs ncurses
> > 2>/dev/null):-lpdcurses"
> > > +curses_inc_list="$($pkg_config --cflags ncursesw
> > 2>/dev/null):-I/${MSYSTEM,,}/include/ncursesw:"
> > > +curses_lib_list="$($pkg_config --libs ncursesw
> > 2>/dev/null):-lncursesw"
> >
> > The original code would try ncurses via pkg-config and if that failed,
> > would
> > falback to pdcurses.
> >
> > The new code tries ncursesw via pkg-config and then tries ncursesw again
> > via manually specified args, and doesn't try ncurses or pdcurses at all.
> >
> Gotcha, Indeed $pkg_config --cflags ncurses can find curses on mingw32,
> the problem is onw mingw32 the include path
> have :, so we can not use : as the path sepaerator, for cross-paltform
> reason, which is best for path separator?
I guess it was using ":" because " " might be valid in the file path.
How about using "#" or "%" instead as those should be more unlikely to
clash.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH 00/12] hw: Forbid DMA write accesses to MMIO regions
On Wed, 9 Sep 2020 at 10:12, Stefan Hajnoczi wrote: > > On Thu, Sep 03, 2020 at 01:08:19PM +0200, Philippe Mathieu-Daudé wrote: > > The main patch is: > > "exec/memattrs: Introduce MemTxAttrs::direct_access field". > > This way we can restrict accesses to ROM/RAM by setting the > > 'direct_access' field. Illegal accesses return MEMTX_BUS_ERROR. > > QEMU needs to simulate the behavior of real hardware. What is the > behavior of real hardware? It varies, depending on the hardware. The most common thing is probably "happens to work by luck", which is OK for hardware but doesn't help us much since our software implementation is naturally more serialized than hardware is and since we don't want to allow guests to make QEMU crash or misbehave. thanks -- PMM
Re: [PATCH 1/4] docs: lift block-core.json rST header into parents
Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
> >> Kevin Wolf writes:
> >>
> >> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
> >> >> Hi Stefan,
> >> >>
> >> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
> >> >> > block-core.json is included from several places. It has no way of
> >> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> >> >> > errors when it encounters an h2 header where it expects an h1 header.
> >> >> > This issue prevents the next patch from generating documentation for
> >> >> > qemu-storage-daemon QMP commands.
> >> >> >
> >> >> > Move the header into parents so that the correct header level can be
> >> >> > used. Note that transaction.json is not updated since it doesn't seem
> >> >> > to
> >> >> > need a header.
> >> >> >
> >> >> > Signed-off-by: Stefan Hajnoczi
> >> >> > ---
> >> >> > docs/interop/firmware.json | 4
> >> >> > qapi/block-core.json | 4
> >> >> > qapi/block.json| 1 +
> >> >> > 3 files changed, 5 insertions(+), 4 deletions(-)
> >> >> >
> >> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> >> >> > index 989f10b626..48af327f98 100644
> >> >> > --- a/docs/interop/firmware.json
> >> >> > +++ b/docs/interop/firmware.json
> >> >> > @@ -15,6 +15,10 @@
> >> >> > ##
> >> >> >
> >> >> > { 'include' : 'machine.json' }
> >> >> > +
> >> >> > +##
> >> >> > +# == Block devices
> >> >> > +##
> >> >> > { 'include' : 'block-core.json' }
> >> >> >
> >> >> > ##
> >> >>
> >> >> I think "docs/interop/firmware.json" deserves the same treatment as
> >> >> "transaction.json".
> >> >>
> >> >> It's been a long time since I last looked at a rendered view of
> >> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
> >> >> it can refer to some block-related types (@BlockdevDriver seems like the
> >> >> main, or only, one).
> >> >>
> >> >> I wouldn't expect the rendered view of "firmware.json" to have a section
> >> >> header saying "Block devices".
> >> >>
> >> >> I think it should be fine to drop this hunk (and my CC along with it ;))
> >> >
> >> > I think this is actually a more general problem with the way we generate
> >> > the documentation. For example, the "Background jobs" documentation ends
> >> > up under "Block Devices" just because qapi-schema.json includes
> >> > block-core.json before job.json and block-core.json includes job.json to
> >> > be able to access some types.
> >>
> >> The doc generator is stupid and greedy (which also makes it
> >> predictable): a module's documentation is emitted where it is first
> >> included.
> >>
> >> For full control of the order, have the main module include all
> >> sub-modules in the order you want.
> >
> > This works as long as the order that we want is consistent with the
> > requirement that every file must be included by qapi-schea.json before
> > it is included by any other file (essentially making the additional
> > includes in other files useless).
>
> These other includes are not useless: they are essential for generating
> self-contained headers.
>
> When MOD.json includes SUBMOD.json, then the generated qapi-FOO-MOD.h
> include qapi-FOO-SUBMOD.h. When every module pulls in the modules it
> requires, so do the generated headers. When a module doesn't, its
> generated headers won't compile unless you manually include the missing
> generated headers it requires.
Hm, right. So we're using includes for two different purposes, but just
from looking at the include line, you can't know which one it is.
> > Is this the order that we want?
> >
> > If so, we could support following the rule consistently by making double
> > include of a file an error.
>
> Breaks our simple & stupid way to generate self-contained headers.
>
> >> Alternatively, add just enough includes to get the order you want.
> >
> > There are orders that I can't get this way.
>
> You're right, ordering by first include is not completely general.
>
> > For example, if I want to
> > have "Block devices" documented before "Background jobs", there is no
> > way to add includes to actually get this order without having
> > "Background jobs" as a subsection in "Block devices".
>
> "Background jobs" is job.json.
>
> "Block devices" is block.json, which includes block-core.json, which
> includes job.json.
>
> To be able to put "Block devices" first, you'd have to break the chain
> from block.json to job.json. Which might even be an improvement:
>
> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
> 5527 qapi/block-core.json
> 1722 qapi/migration.json
> 1617 qapi/misc.json
> 1180 qapi/ui.json
> 17407 total
>
> Could we split block-core.json into several cohesive parts?
Possibly. However, while it would be an improvement generally speaking,
how does this change the specific problem? All of
Re: [PATCH v2 21/21] tests: Fixes test-qdev-global-props.c
On Wed, Sep 09, 2020 at 05:46:17PM +0800, Yonggang Luo wrote: > On win32 the line ending are \r\n, so we skip the \n in function > test_dynamic_globalprop > > Signed-off-by: Yonggang Luo > --- > tests/test-qdev-global-props.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 14/21] cirrus: Building freebsd in a single short
On 9/9/20 12:08 PM, Yonggang Luo wrote:
> freebsd 1 hour limit not hit anymore
>
> I think we going to a wrong direction, I think there is some tests a stall
> the test runner,
> please look at
> https://cirrus-ci.com/task/5110577531977728
> When its running properly, the consumed time are little, but when tests
> running too long,
> look at the cpu usage, the cpu usage are nearly zero. does't consuming time.
>
> And look at
> https://cirrus-ci.com/task/6119341601062912
>
> If the tests running properly, the time consuming are little
> We should not hide the error by split them
>
Can you add:
This reverts commit 45f7b7b9f38f5c4d1529a37c93dedfc26a231bba
("cirrus.yml: Split FreeBSD job into two parts").
(no need to repost right know, let other review your patches.)
> Signed-off-by: Yonggang Luo
> ---
> .cirrus.yml | 35 ---
> 1 file changed, 8 insertions(+), 27 deletions(-)
>
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 49335e68c9..b0004273bb 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -1,38 +1,19 @@
> env:
>CIRRUS_CLONE_DEPTH: 1
>
> -freebsd_1st_task:
> +freebsd_12_task:
>freebsd_instance:
> image_family: freebsd-12-1
> -cpu: 4
> -memory: 4G
> - install_script: ASSUME_ALWAYS_YES=yes pkg bootstrap -f ; pkg install -y
> -bash curl cyrus-sasl git glib gmake gnutls gsed
> -nettle perl5 pixman pkgconf png usbredir
> +cpu: 8
> +memory: 8G
> + install_script:
> +- ASSUME_ALWAYS_YES=yes pkg bootstrap -f ;
> +- pkg install -y bash curl cyrus-sasl git glib gmake gnutls gsed
> + nettle perl5 pixman pkgconf png usbredir
>script:
> - mkdir build
> - cd build
> -- ../configure --disable-user --target-list-exclude='alpha-softmmu
> -ppc64-softmmu ppc-softmmu riscv32-softmmu riscv64-softmmu
> s390x-softmmu
> -sparc64-softmmu sparc-softmmu x86_64-softmmu i386-softmmu'
> ---enable-werror || { cat config.log; exit 1; }
> -- gmake -j$(sysctl -n hw.ncpu)
> -- gmake -j$(sysctl -n hw.ncpu) check
> -
> -freebsd_2nd_task:
> - freebsd_instance:
> -image_family: freebsd-12-1
> -cpu: 4
> -memory: 4G
> - install_script: ASSUME_ALWAYS_YES=yes pkg bootstrap -f ; pkg install -y
> -bash curl cyrus-sasl git glib gmake gnutls gtk3 gsed libepoxy mesa-libs
> -nettle perl5 pixman pkgconf png SDL2 usbredir
> - script:
> -- ./configure --enable-werror --target-list='alpha-softmmu ppc64-softmmu
> -ppc-softmmu riscv32-softmmu riscv64-softmmu s390x-softmmu
> -sparc64-softmmu sparc-softmmu x86_64-softmmu i386-softmmu
> -sparc-bsd-user sparc64-bsd-user x86_64-bsd-user i386-bsd-user'
> -|| { cat config.log; exit 1; }
> +- ../configure --enable-werror || { cat config.log; exit 1; }
> - gmake -j$(sysctl -n hw.ncpu)
> - gmake -j$(sysctl -n hw.ncpu) check
>
>
Re: [PATCH v2 20/21] tests: fix test-util-sockets.c
On Wed, Sep 09, 2020 at 05:46:16PM +0800, Yonggang Luo wrote: > Fixes following errors: > Running test test-util-sockets > ERROR test-util-sockets - missing test plan > > # Start of name tests > ** > ERROR:../tests/test-util-sockets.c:93:test_socket_fd_pass_name_good: > assertion failed (fd != -1): (-1 != -1) > Bail out! > ERROR:../tests/test-util-sockets.c:93:test_socket_fd_pass_name_good: > assertion failed (fd != -1): (-1 != -1) > > First should call to qemu_init_main_loop before socket_init, > then on win32 doesn't support for SOCKET_ADDRESS_TYPE_FD socket type > > Signed-off-by: Yonggang Luo > --- > tests/test-util-sockets.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 19/21] tests: Fixes test-io-channel-file by mask only owner file state mask bits
On Wed, Sep 09, 2020 at 05:46:15PM +0800, Yonggang Luo wrote: > This is the error on msys2/mingw > Running test test-io-channel-file > ** > ERROR:../tests/test-io-channel-file.c:59:test_io_channel_file_helper: > assertion failed (TEST_MASK & ~mask == st.st_mode & 0777): (384 == 438) > ERROR test-io-channel-file - Bail out! > ERROR:../tests/test-io-channel-file.c:59:test_io_channel_file_helper: > assertion failed (TEST_MASK & ~mask == st.st_mode & 0777): (384 == 438) > > Signed-off-by: Yonggang Luo > --- > tests/test-io-channel-file.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tests/test-io-channel-file.c b/tests/test-io-channel-file.c > index bac2b07562..75aea6450a 100644 > --- a/tests/test-io-channel-file.c > +++ b/tests/test-io-channel-file.c > @@ -56,7 +56,9 @@ static void test_io_channel_file_helper(int flags) > umask(mask); > ret = stat(TEST_FILE, ); > g_assert_cmpint(ret, >, -1); > -g_assert_cmpuint(TEST_MASK & ~mask, ==, st.st_mode & 0777); > +/* On Windows the stat() function in the C library checks only > + the FAT-style READONLY attribute and does not look at the ACL at all. */ > +g_assert_cmpuint(TEST_MASK & ~mask, ==, st.st_mode & 0700); I think we will want the stronger check on non-Win32, so better to ifdef this to use 0700 only on Win32. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 17/21] tests: Fixes test-io-channel-socket.c tests under msys2/mingw
On Wed, Sep 09, 2020 at 05:46:13PM +0800, Yonggang Luo wrote: > Currently test-io-channel-socket doesn't init with > qemu_init_main_loop > and that's cause the qemu_aio_context not inited, > and the following is the stack when null pointer accessed: > > qemu_fd_register (c:\work\xemu\qemu\util\main-loop.c:336) > qemu_try_set_nonblock (c:\work\xemu\qemu\util\oslib-win32.c:224) > qemu_set_nonblock (c:\work\xemu\qemu\util\oslib-win32.c:230) > socket_can_bind_connect (c:\work\xemu\qemu\tests\socket-helpers.c:93) > socket_check_protocol_support (c:\work\xemu\qemu\tests\socket-helpers.c:141) > main (c:\work\xemu\qemu\tests\test-io-channel-socket.c:568) > __tmainCRTStartup (@__tmainCRTStartup:142) > mainCRTStartup (@1400014f6..140001539:3) > BaseThreadInitThunk (@BaseThreadInitThunk:9) > RtlUserThreadStart (@RtlUserThreadStart:12) > > Signed-off-by: Yonggang Luo > --- > tests/test-io-channel-socket.c | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 15/21] tests: Convert g_free to g_autofree macro in test-logging.c
On Wed, Sep 09, 2020 at 05:46:11PM +0800, Yonggang Luo wrote: > g_autofree are prefer than g_free when possible. > > Signed-off-by: Yonggang Luo > Reviewed-by: Philippe Mathieu-Daudé > --- > tests/test-logging.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 14/21] cirrus: Building freebsd in a single short
On Wed, Sep 09, 2020 at 06:08:42PM +0800, Yonggang Luo wrote: > freebsd 1 hour limit not hit anymore > > I think we going to a wrong direction, I think there is some tests a stall > the test runner, > please look at > https://cirrus-ci.com/task/5110577531977728 > When its running properly, the consumed time are little, but when tests > running too long, > look at the cpu usage, the cpu usage are nearly zero. does't consuming time. > > And look at > https://cirrus-ci.com/task/6119341601062912 > > If the tests running properly, the time consuming are little > We should not hide the error by split them > > Signed-off-by: Yonggang Luo > --- > .cirrus.yml | 35 --- > 1 file changed, 8 insertions(+), 27 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 13/21] vmstate: Fixes test-vmstate.c on msys2/mingw
On Wed, Sep 09, 2020 at 05:46:09PM +0800, Yonggang Luo wrote:
> The vmstate are valid on win32, just need generate tmp path properly
>
> Signed-off-by: Yonggang Luo
> Reviewed-by: Philippe Mathieu-Daudé
> Reviewed-by: Thomas Huth
> ---
> tests/test-vmstate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index f8de709a0b..51fe8e8ec5 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -34,7 +34,6 @@
> #include "qemu/module.h"
> #include "io/channel-file.h"
>
> -static char temp_file[] = "/tmp/vmst.test.XX";
> static int temp_fd;
>
>
> @@ -1487,6 +1486,7 @@ static void test_tmp_struct(void)
>
> int main(int argc, char **argv)
> {
> +g_autofree char *temp_file = g_strdup_printf("%s/vmst.test.XX",
> g_get_tmp_dir());
Could do with a line break before the arg. I'm surprised
'checkpatch.pl' didn't complain about this.
With the line break
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 11/21] meson: disable crypto tests are empty under win32
On Wed, Sep 09, 2020 at 05:46:07PM +0800, Yonggang Luo wrote: > Disable following tests on msys2/mingw > 'test-crypto-tlscredsx509': ['crypto-tls-x509-helpers.c', > 'pkix_asn1_tab.c', >tasn1, crypto], > 'test-crypto-tlssession': ['crypto-tls-x509-helpers.c', > 'pkix_asn1_tab.c', 'crypto-tls-psk-helpers.c', > tasn1, crypto], > 'test-io-channel-tls': ['io-channel-helpers.c', > 'crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c', > tasn1, io, crypto]} > These tests are failure with: > ERROR test-crypto-tlscredsx509 - missing test plan > ERROR test-crypto-tlssession - missing test plan > ERROR test-io-channel-tls - missing test plan > > Because on win32 those test case are all disabled. > > Signed-off-by: Yonggang Luo > --- > tests/meson.build | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 12/21] meson: remove empty else and duplicated gio deps
On Wed, Sep 09, 2020 at 05:46:08PM +0800, Yonggang Luo wrote: > Signed-off-by: Yonggang Luo > --- > meson.build | 6 -- > 1 file changed, 6 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 10/21] meson: Use -b to ignore CR vs. CR-LF issues on Windows
On Wed, Sep 09, 2020 at 05:46:06PM +0800, Yonggang Luo wrote: > On windows, a difference in line endings causes testsuite failures > complaining that every single line in files such as > 'tests/qapi-schemadoc-good.texi' is wrong. Fix it by adding -b to diff. > > Signed-off-by: Yonggang Luo > Reviewed-by: Eric Blake > --- > tests/qapi-schema/meson.build | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 07/21] tests: Trying fixes test-replication.c on msys2/mingw.
On Wed, Sep 9, 2020 at 8:56 PM Daniel P. Berrangé
wrote:
> On Wed, Sep 09, 2020 at 05:46:03PM +0800, Yonggang Luo wrote:
> > On Windows there is no path like /tmp/s_local_disk.XX
> > Use g_get_tmp_dir instead of /tmp.
> >
> > Signed-off-by: Yonggang Luo
> > ---
> > tests/test-replication.c | 18 ++
> > 1 file changed, 14 insertions(+), 4 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé
>
>
> > @@ -571,6 +571,11 @@ static void setup_sigabrt_handler(void)
> > int main(int argc, char **argv)
> > {
> > int ret;
> > +const char *tmpdir = g_get_tmp_dir();
> > +p_local_disk = g_strdup_printf("%s/p_local_disk.XX", tmpdir);
> > +s_local_disk = g_strdup_printf("%s/s_local_disk.XX", tmpdir);
> > +s_active_disk = g_strdup_printf("%s/s_active_disk.XX", tmpdir);
> > +s_hidden_disk = g_strdup_printf("%s/s_hidden_disk.XX", tmpdir);
>
> I presume msys is taking care of translating "/" into "\" so that
> we don't need to use g_build_filename to use the native directory
> separator straightaway ?
>
Not only msys2, but also win32 api can recoginize "/", so we doesn't need
to care about it
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
> https://www.instagram.com/dberrange :|
>
>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Re: [PATCH v2 09/21] osdep: These function are only available on Non-Win32 system.
Reword the subject to say "osdep: file locking functions are not available on Win32" On Wed, Sep 09, 2020 at 05:46:05PM +0800, Yonggang Luo wrote: > int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); > int qemu_unlock_fd(int fd, int64_t start, int64_t len); > int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); > bool qemu_has_ofd_lock(void); > > Signed-off-by: Yonggang Luo > --- > include/qemu/osdep.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index 412962d91a..e80fddd1e8 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -502,11 +502,11 @@ int qemu_close(int fd); > int qemu_unlink(const char *name); > #ifndef _WIN32 > int qemu_dup(int fd); > -#endif > int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); > int qemu_unlock_fd(int fd, int64_t start, int64_t len); > int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); > bool qemu_has_ofd_lock(void); > +#endif Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 08/21] tests: test-replication disable /replication/secondary/* on msys2/mingw.
On Wed, Sep 09, 2020 at 05:46:04PM +0800, Yonggang Luo wrote:
> They cause failure on msys2/mingw, that's because file-win32.c not implement
> .bdrv_reopen_prepare/commit/abort yet.
>
> > $ ./tests/test-replication.exe
> > # random seed: R02S3f4d1c01af2b0a046990e0235c481faf
> > 1..13
> > # Start of replication tests
> > # Start of primary tests
> > ok 1 /replication/primary/read
> > ok 2 /replication/primary/write
> > ok 3 /replication/primary/start
> > ok 4 /replication/primary/stop
> > ok 5 /replication/primary/do_checkpoint
> > ok 6 /replication/primary/get_error_all
> > # End of primary tests
> > # Start of secondary tests
> > ok 7 /replication/secondary/read
> > ok 8 /replication/secondary/write
> > Unexpected error in bdrv_reopen_prepare() at ../block.c:4191:
> > Block format 'file' used by node '#block4287' does not support reopening
> > files
>
> Can you try to find out what reopen this is?
>
> I assume it's for switching between read-write and read-only. In this
> case an implementation of .bdrv_reopen_prepare/commit/abort that can do
> this switch is required.
>
> This is more serious development work, so I can't propose a quick fix.
>
> Alternatively, we could just declare replication unsupported on Windows
> and let configure make sure that CONFIG_REPLICATION is never set for it.
>
> luoyonggang: That might be missing functionality in
> block/file-win32.c.
> * davidgiluk yawns and looks up
> luoyonggang: The block/file-posix.c block driver supports
> .bdrv_reopen_*()
> while block/file-win32.c does not. It's probably because no one has tried to
> implement it.
> OK, I got the direction,
> Just need implement .bdrv_reopen_*() functions in file-win32.c
We don't need to add IRC transscripts into the commit message. It
is sufficient to note that .bdrv_reopen* are not implemented on
in block/file-win32.c, which you already did at the start.
>
> Signed-off-by: Yonggang Luo
> ---
> tests/test-replication.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/tests/test-replication.c b/tests/test-replication.c
> index e7cbd6b144..b067240add 100644
> --- a/tests/test-replication.c
> +++ b/tests/test-replication.c
> @@ -392,6 +392,7 @@ static void test_secondary_write(void)
> teardown_secondary();
> }
>
> +#ifndef _WIN32
> static void test_secondary_start(void)
> {
> BlockBackend *top_blk, *local_blk;
> @@ -546,6 +547,7 @@ static void test_secondary_get_error_all(void)
>
> teardown_secondary();
> }
> +#endif
>
> static void sigabrt_handler(int signo)
> {
> @@ -597,6 +599,7 @@ int main(int argc, char **argv)
> /* Secondary */
> g_test_add_func("/replication/secondary/read", test_secondary_read);
> g_test_add_func("/replication/secondary/write", test_secondary_write);
> +#ifndef _WIN32
> g_test_add_func("/replication/secondary/start", test_secondary_start);
> g_test_add_func("/replication/secondary/stop", test_secondary_stop);
> g_test_add_func("/replication/secondary/continuous_replication",
> @@ -605,6 +608,7 @@ int main(int argc, char **argv)
> test_secondary_do_checkpoint);
> g_test_add_func("/replication/secondary/get_error_all",
> test_secondary_get_error_all);
> +#endif
>
> ret = g_test_run();
With the commit msg trimmed
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 03/21] configure: Fixes ncursesw detection under msys2/mingw and enable curses
On Wed, Sep 9, 2020 at 8:51 PM Daniel P. Berrangé
wrote:
> On Wed, Sep 09, 2020 at 05:45:59PM +0800, Yonggang Luo wrote:
> > The mingw pkg-config are showing following absolute path and contains :
> as the separator,
> > so we must handling : properly.
> >
> > -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L
> -IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
> > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> -pipe -lncursesw -lgnurx -ltre -lintl -liconv
> > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> -lncursesw
> > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> -lcursesw
> > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe
> -lncursesw -lgnurx -ltre -lintl -liconv
> > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
> > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
> > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx
> -ltre -lintl -liconv
> > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
> > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw
> >
> > msys2/mingw lacks the POSIX-required langinfo.h.
> >
> > gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe
> -lncursesw -lgnurx -ltre -lintl -liconv
> > test.c:4:10: fatal error: langinfo.h: No such file or directory
> > 4 | #include
> > | ^~~~
> > compilation terminated.
> >
> > So we using g_get_codeset instead of nl_langinfo(CODESET)
> >
> > Signed-off-by: Yonggang Luo
> > Reviewed-by: Gerd Hoffmann
> > ---
> > configure | 9 +++--
> > ui/curses.c | 10 +-
> > 2 files changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/configure b/configure
> > index f4f8bc3756..2e6d54e15b 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3653,8 +3653,8 @@ if test "$iconv" = "no" ; then
> > fi
> > if test "$curses" != "no" ; then
> >if test "$mingw32" = "yes" ; then
> > -curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
> > -curses_lib_list="$($pkg_config --libs ncurses
> 2>/dev/null):-lpdcurses"
> > +curses_inc_list="$($pkg_config --cflags ncursesw
> 2>/dev/null):-I/${MSYSTEM,,}/include/ncursesw:"
> > +curses_lib_list="$($pkg_config --libs ncursesw
> 2>/dev/null):-lncursesw"
>
> The original code would try ncurses via pkg-config and if that failed,
> would
> falback to pdcurses.
>
> The new code tries ncursesw via pkg-config and then tries ncursesw again
> via manually specified args, and doesn't try ncurses or pdcurses at all.
>
Gotcha, Indeed $pkg_config --cflags ncurses can find curses on mingw32,
the problem is onw mingw32 the include path
have :, so we can not use : as the path sepaerator, for cross-paltform
reason, which is best for path separator?
>
> This fixes your build as you've installed ncursesw, but breaks anyone
> who was using ncurses or pdcurses previously. At least on Fedora only
> pdcurses is available for mingw, so I don't think we should be dropping
> this. It looks like we just need to check all three of ncursesw, ncurses
> and pdcurses.
>
> Copying Samuel who introduced this logic originally in
> commit 8ddc5bf9e5de51c2a4842c01dd3a97f5591776fd
>
> >else
> > curses_inc_list="$($pkg_config --cflags ncursesw
> 2>/dev/null):-I/usr/include/ncursesw:"
> > curses_lib_list="$($pkg_config --libs ncursesw
> 2>/dev/null):-lncursesw:-lcursesw"
> > @@ -3664,17 +3664,14 @@ if test "$curses" != "no" ; then
> > #include
> > #include
> > #include
> > -#include
> > int main(void) {
> > - const char *codeset;
> >wchar_t wch = L'w';
> >setlocale(LC_ALL, "");
> >resize_term(0, 0);
> >addwstr(L"wide chars\n");
> >addnwstr(, 1);
> >add_wch(WACS_DEGREE);
> > - codeset = nl_langinfo(CODESET);
> > - return codeset != 0;
> > + return 0;
> > }
> > EOF
> >IFS=:
> > diff --git a/ui/curses.c b/ui/curses.c
> > index a59b23a9cf..12bc682cf9 100644
> > --- a/ui/curses.c
> > +++ b/ui/curses.c
> > @@ -30,7 +30,6 @@
> > #endif
> > #include
> > #include
> > -#include
> > #include
> >
> > #include "qapi/error.h"
> > @@ -526,6 +525,7 @@ static void font_setup(void)
> > iconv_t nativecharset_to_ucs2;
> > iconv_t font_conv;
> > int i;
> > +g_autofree gchar *local_codeset = g_get_codeset();
> >
> > /*
> > * Control characters are normally non-printable, but VGA does have
> > @@ -566,14 +566,14 @@ static void font_setup(void)
> >0x25bc
> > };
> >
> > -ucs2_to_nativecharset = iconv_open(nl_langinfo(CODESET), "UCS-2");
> > +ucs2_to_nativecharset = iconv_open(local_codeset, "UCS-2");
> > if (ucs2_to_nativecharset == (iconv_t) -1) {
> > fprintf(stderr, "Could not convert font glyphs from UCS-2:
> '%s'\n",
> > strerror(errno));
> > exit(1);
> > }
> >
> > -nativecharset_to_ucs2 = iconv_open("UCS-2", nl_langinfo(CODESET));
> > +
Re: [PATCH v2 07/21] tests: Trying fixes test-replication.c on msys2/mingw.
On Wed, Sep 09, 2020 at 05:46:03PM +0800, Yonggang Luo wrote:
> On Windows there is no path like /tmp/s_local_disk.XX
> Use g_get_tmp_dir instead of /tmp.
>
> Signed-off-by: Yonggang Luo
> ---
> tests/test-replication.c | 18 ++
> 1 file changed, 14 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé
> @@ -571,6 +571,11 @@ static void setup_sigabrt_handler(void)
> int main(int argc, char **argv)
> {
> int ret;
> +const char *tmpdir = g_get_tmp_dir();
> +p_local_disk = g_strdup_printf("%s/p_local_disk.XX", tmpdir);
> +s_local_disk = g_strdup_printf("%s/s_local_disk.XX", tmpdir);
> +s_active_disk = g_strdup_printf("%s/s_active_disk.XX", tmpdir);
> +s_hidden_disk = g_strdup_printf("%s/s_hidden_disk.XX", tmpdir);
I presume msys is taking care of translating "/" into "\" so that
we don't need to use g_build_filename to use the native directory
separator straightaway ?
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 06/21] ci: Enable msys2 ci in cirrus
On Wed, Sep 09, 2020 at 05:46:02PM +0800, Yonggang Luo wrote:
> Install msys2 in a proper way refer to
> https://github.com/cirruslabs/cirrus-ci-docs/issues/699
> The https://wiki.qemu.org/Hosts/W32#Native_builds_with_MSYS2 need to be
> updated.
> There is no need of --cross-prefix, open mingw64.exe instead of msys2.exe
> then we don't
> need the --cross-prefix, besides we using environment variable settings:
> MSYS: winsymlinks:nativestrict
> MSYSTEM: MINGW64
> CHERE_INVOKING: 1
> to opening mingw64 native shell.
> We now running tests with make -i check to skip tests errors.
>
> Signed-off-by: Yonggang Luo
> ---
> .cirrus.yml | 24 +
> scripts/ci/windows/msys2-build.sh | 28
> scripts/ci/windows/msys2-install.sh | 33 +
> 3 files changed, 85 insertions(+)
> create mode 100644 scripts/ci/windows/msys2-build.sh
> create mode 100644 scripts/ci/windows/msys2-install.sh
>
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 3dd9fcff7f..49335e68c9 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -63,3 +63,27 @@ macos_xcode_task:
> --enable-werror --cc=clang || { cat config.log; exit 1; }
> - gmake -j$(sysctl -n hw.ncpu)
> - gmake check
> +
> +windows_msys2_task:
> + windows_container:
> +image: cirrusci/windowsservercore:cmake
> +os_version: 2019
> +cpu: 8
> +memory: 8G
> + env:
> +MSYS: winsymlinks:nativestrict
> +MSYSTEM: MINGW64
> +CHERE_INVOKING: 1
> + printenv_script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc 'printenv'
> + install_script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
> http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz;
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O
> http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz.sig;
> +- C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && pacman -U
> --noconfirm msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman -Sy --noconfirm"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --needed --noconfirm -S
> bash pacman pacman-mirrors msys2-runtime"
> +- taskkill /F /IM gpg-agent.exe
> +- C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -Su"
> +- C:\tools\msys64\usr\bin\bash.exe -lc "sh
> scripts/ci/windows/msys2-install.sh"
> + script:
> +- C:\tools\msys64\usr\bin\bash.exe -lc "sh
> scripts/ci/windows/msys2-build.sh"
> diff --git a/scripts/ci/windows/msys2-build.sh
> b/scripts/ci/windows/msys2-build.sh
> new file mode 100644
> index 00..d9d046b5b0
> --- /dev/null
> +++ b/scripts/ci/windows/msys2-build.sh
> @@ -0,0 +1,28 @@
> +mkdir build
> +cd build
> +../configure \
> +--python=python3 \
> +--ninja=ninja \
> +--enable-stack-protector \
> +--enable-guest-agent \
> +--disable-pie \
> +--enable-gnutls --enable-nettle \
> +--enable-sdl --enable-sdl-image --enable-gtk --disable-vte --enable-curses
> --enable-iconv \
> +--enable-vnc --enable-vnc-sasl --enable-vnc-jpeg --enable-vnc-png \
> +--enable-slirp=git \
> +--disable-brlapi --enable-curl \
> +--enable-fdt \
> +--disable-kvm --enable-hax --enable-whpx \
> +--enable-libnfs --enable-libusb --enable-live-block-migration
> --enable-usb-redir \
> +--enable-lzo --enable-snappy --enable-bzip2 --enable-zstd \
> +--enable-membarrier --enable-coroutine-pool \
> +--enable-libssh --enable-libxml2 \
> +--enable-jemalloc --enable-avx2 \
> +--enable-replication \
> +--enable-tools \
> +--enable-bochs --enable-cloop --enable-dmg --enable-qcow1 --enable-vdi
> --enable-vvfat --enable-qed --enable-parallels \
> +--enable-sheepdog \
> +--enable-capstone=git
> +
> +make -j$NUMBER_OF_PROCESSORS
> +make -i -j$NUMBER_OF_PROCESSORS check
This still needs the changes discussed in v1 to remove all the
configure args and move the commands into the main cirrus.yml
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 05/21] tests: disable /char/stdio/* tests in test-char.c on win32
On Wed, Sep 09, 2020 at 05:46:01PM +0800, Yonggang Luo wrote: > These tests are blocking test-char to be finished. > Merge three #ifndef _WIN32 > > Signed-off-by: Yonggang Luo > --- > tests/test-char.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 04/21] curses: Fixes curses compiling errors.
On Wed, Sep 09, 2020 at 05:46:00PM +0800, Yonggang Luo wrote: > This is the compiling error: > ../ui/curses.c: In function 'curses_refresh': > ../ui/curses.c:256:5: error: 'next_maybe_keycode' may be used uninitialized > in this function [-Werror=maybe-uninitialized] > 256 | curses2foo(_curses2keycode, _curseskey2keycode, chr, > maybe_keycode) > | ^~ > ../ui/curses.c:302:32: note: 'next_maybe_keycode' was declared here > 302 | enum maybe_keycode next_maybe_keycode; > |^~ > ../ui/curses.c:256:5: error: 'maybe_keycode' may be used uninitialized in > this function [-Werror=maybe-uninitialized] > 256 | curses2foo(_curses2keycode, _curseskey2keycode, chr, > maybe_keycode) > | ^~ > ../ui/curses.c:265:24: note: 'maybe_keycode' was declared here > 265 | enum maybe_keycode maybe_keycode; > |^ > cc1.exe: all warnings being treated as errors > > gcc version 10.2.0 (Rev1, Built by MSYS2 project) > > Signed-off-by: Yonggang Luo > Reviewed-by: Peter Maydell > Reviewed-by: Gerd Hoffmann > --- > ui/curses.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2] qcow2: Skip copy-on-write when allocating a zero cluster
ping On Thu 27 Aug 2020 04:53:50 PM CEST, Alberto Garcia wrote: > Since commit c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a when a write > request results in a new allocation QEMU first tries to see if the > rest of the cluster outside the written area contains only zeroes. > > In that case, instead of doing a normal copy-on-write operation and > writing explicit zero buffers to disk, the code zeroes the whole > cluster efficiently using pwrite_zeroes() with BDRV_REQ_NO_FALLBACK. > > This improves performance very significantly but it only happens when > we are writing to an area that was completely unallocated before. Zero > clusters (QCOW2_CLUSTER_ZERO_*) are treated like normal clusters and > are therefore slower to allocate. > > This happens because the code uses bdrv_is_allocated_above() rather > bdrv_block_status_above(). The former is not as accurate for this > purpose but it is faster. However in the case of qcow2 the underlying > call does already report zero clusters just fine so there is no reason > why we cannot use that information. > > After testing 4KB writes on an image that only contains zero clusters > this patch results in almost five times more IOPS. Berto
Re: [PATCH] MAINTAINERS: add Stefan Hajnoczi as block/nvme.c maintainer
Hi Stefan, +Alex On 9/7/20 1:16 PM, Stefan Hajnoczi wrote: > Development of the userspace NVMe block driver picked up again recently. > After talking with Fam I am stepping up as block/nvme.c maintainer. > Patches will be merged through my 'block' tree. > > Cc: Kevin Wolf > Cc: Klaus Jensen > Cc: Fam Zheng > Signed-off-by: Stefan Hajnoczi > --- > MAINTAINERS | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index b233da2a73..a143941551 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2895,10 +2895,12 @@ S: Supported > F: block/null.c > > NVMe Block Driver > -M: Fam Zheng > +M: Stefan Hajnoczi > +R: Fam Zheng > L: qemu-block@nongnu.org > S: Supported > F: block/nvme* > +T: git https://github.com/stefanha/qemu.git block As this driver is the only consumer of the 'VFIO helpers', maybe we can: - maintains them in the same bucket - add another entry (eventually with R: tag for Alex) The 'VFIO helpers' files are: - util/vfio-helpers.c - include/qemu/vfio-helpers.h What do you think? Regards, Phil.
Re: [PATCH v2 03/21] configure: Fixes ncursesw detection under msys2/mingw and enable curses
On Wed, Sep 09, 2020 at 05:45:59PM +0800, Yonggang Luo wrote:
> The mingw pkg-config are showing following absolute path and contains : as
> the separator,
> so we must handling : properly.
>
> -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L
> -IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -pipe
> -lncursesw -lgnurx -ltre -lintl -liconv
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> -lncursesw
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -lcursesw
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe -lncursesw
> -lgnurx -ltre -lintl -liconv
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx -ltre
> -lintl -liconv
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw
>
> msys2/mingw lacks the POSIX-required langinfo.h.
>
> gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe -lncursesw
> -lgnurx -ltre -lintl -liconv
> test.c:4:10: fatal error: langinfo.h: No such file or directory
> 4 | #include
> | ^~~~
> compilation terminated.
>
> So we using g_get_codeset instead of nl_langinfo(CODESET)
>
> Signed-off-by: Yonggang Luo
> Reviewed-by: Gerd Hoffmann
> ---
> configure | 9 +++--
> ui/curses.c | 10 +-
> 2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/configure b/configure
> index f4f8bc3756..2e6d54e15b 100755
> --- a/configure
> +++ b/configure
> @@ -3653,8 +3653,8 @@ if test "$iconv" = "no" ; then
> fi
> if test "$curses" != "no" ; then
>if test "$mingw32" = "yes" ; then
> -curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
> -curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
> +curses_inc_list="$($pkg_config --cflags ncursesw
> 2>/dev/null):-I/${MSYSTEM,,}/include/ncursesw:"
> +curses_lib_list="$($pkg_config --libs ncursesw 2>/dev/null):-lncursesw"
The original code would try ncurses via pkg-config and if that failed, would
falback to pdcurses.
The new code tries ncursesw via pkg-config and then tries ncursesw again
via manually specified args, and doesn't try ncurses or pdcurses at all.
This fixes your build as you've installed ncursesw, but breaks anyone
who was using ncurses or pdcurses previously. At least on Fedora only
pdcurses is available for mingw, so I don't think we should be dropping
this. It looks like we just need to check all three of ncursesw, ncurses
and pdcurses.
Copying Samuel who introduced this logic originally in
commit 8ddc5bf9e5de51c2a4842c01dd3a97f5591776fd
>else
> curses_inc_list="$($pkg_config --cflags ncursesw
> 2>/dev/null):-I/usr/include/ncursesw:"
> curses_lib_list="$($pkg_config --libs ncursesw
> 2>/dev/null):-lncursesw:-lcursesw"
> @@ -3664,17 +3664,14 @@ if test "$curses" != "no" ; then
> #include
> #include
> #include
> -#include
> int main(void) {
> - const char *codeset;
>wchar_t wch = L'w';
>setlocale(LC_ALL, "");
>resize_term(0, 0);
>addwstr(L"wide chars\n");
>addnwstr(, 1);
>add_wch(WACS_DEGREE);
> - codeset = nl_langinfo(CODESET);
> - return codeset != 0;
> + return 0;
> }
> EOF
>IFS=:
> diff --git a/ui/curses.c b/ui/curses.c
> index a59b23a9cf..12bc682cf9 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -30,7 +30,6 @@
> #endif
> #include
> #include
> -#include
> #include
>
> #include "qapi/error.h"
> @@ -526,6 +525,7 @@ static void font_setup(void)
> iconv_t nativecharset_to_ucs2;
> iconv_t font_conv;
> int i;
> +g_autofree gchar *local_codeset = g_get_codeset();
>
> /*
> * Control characters are normally non-printable, but VGA does have
> @@ -566,14 +566,14 @@ static void font_setup(void)
>0x25bc
> };
>
> -ucs2_to_nativecharset = iconv_open(nl_langinfo(CODESET), "UCS-2");
> +ucs2_to_nativecharset = iconv_open(local_codeset, "UCS-2");
> if (ucs2_to_nativecharset == (iconv_t) -1) {
> fprintf(stderr, "Could not convert font glyphs from UCS-2: '%s'\n",
> strerror(errno));
> exit(1);
> }
>
> -nativecharset_to_ucs2 = iconv_open("UCS-2", nl_langinfo(CODESET));
> +nativecharset_to_ucs2 = iconv_open("UCS-2", local_codeset);
> if (nativecharset_to_ucs2 == (iconv_t) -1) {
> iconv_close(ucs2_to_nativecharset);
> fprintf(stderr, "Could not convert font glyphs to UCS-2: '%s'\n",
> @@ -581,7 +581,7 @@ static void font_setup(void)
> exit(1);
> }
>
> -font_conv = iconv_open(nl_langinfo(CODESET), font_charset);
> +font_conv = iconv_open(local_codeset, font_charset);
> if (font_conv == (iconv_t) -1) {
>
[PATCH] qcow2: Return the original error code in qcow2_co_pwrite_zeroes()
This function checks the current status of a (sub)cluster in order to
see if an unaligned 'write zeroes' request can be done efficiently by
simply updating the L2 metadata and without having to write actual
zeroes to disk.
If the situation does not allow using the fast path then the function
returns -ENOTSUP and the caller falls back to writing zeroes.
If can happen however that the aforementioned check returns an actual
error code so in this case we should pass it to the caller.
Signed-off-by: Alberto Garcia
---
block/qcow2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index da56b1a4df..ca46cbd795 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3916,7 +3916,7 @@ static coroutine_fn int
qcow2_co_pwrite_zeroes(BlockDriverState *bs,
type != QCOW2_SUBCLUSTER_ZERO_PLAIN &&
type != QCOW2_SUBCLUSTER_ZERO_ALLOC)) {
qemu_co_mutex_unlock(>lock);
-return -ENOTSUP;
+return ret < 0 ? ret : -ENOTSUP;
}
} else {
qemu_co_mutex_lock(>lock);
--
2.20.1
[PATCH] qemu-img: Support bitmap --merge into backing image
If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a
bitmap from top into base, qemu-img was failing with:
qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to
get shared "write" lock
Is another process using the image [base.qcow2]?
The easiest fix is to not open the entire backing chain of the source
image, so that we aren't worrying about competing BDS visiting the
backing image as both a read-only backing of the source and the
writeable destination.
Fixes: http://bugzilla.redhat.com/1877209
Reported-by: Eyal Shenitzky
Signed-off-by: Eric Blake
---
qemu-img.c | 3 +-
tests/qemu-iotests/291 | 12
tests/qemu-iotests/291.out | 56 ++
3 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/qemu-img.c b/qemu-img.c
index eb2fc1f86243..b15098a2f9b3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4755,7 +4755,8 @@ static int img_bitmap(int argc, char **argv)
}
bs = blk_bs(blk);
if (src_filename) {
-src = img_open(false, src_filename, src_fmt, 0, false, false, false);
+src = img_open(false, src_filename, src_fmt, BDRV_O_NO_BACKING,
+ false, false, false);
if (!src) {
goto out;
}
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
index 1e0bb76959bb..4f837b205655 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/291
@@ -91,6 +91,15 @@ $QEMU_IMG bitmap --remove --image-opts \
driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp
_img_info --format-specific
+echo
+echo "=== Merge from top layer into backing image ==="
+echo
+
+$QEMU_IMG rebase -u -F qcow2 -b "$TEST_IMG.base" "$TEST_IMG"
+$QEMU_IMG bitmap --add --merge b2 -b "$TEST_IMG" -F $IMGFMT \
+ -f $IMGFMT "$TEST_IMG.base" b3
+_img_info --format-specific --backing-chain
+
echo
echo "=== Check bitmap contents ==="
echo
@@ -107,6 +116,9 @@ $QEMU_IMG map --output=json --image-opts \
nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG"
$QEMU_IMG map --output=json --image-opts \
"$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b3 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+"$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b3" | _filter_qemu_img_map
# success, all done
echo '*** done'
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index ee89a728855f..3990f7aacc7b 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -68,6 +68,59 @@ Format specific information:
corrupt: false
extended l2: false
+=== Merge from top layer into backing image ===
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+Format specific information:
+compat: 1.1
+compression type: zlib
+lazy refcounts: false
+bitmaps:
+[0]:
+flags:
+name: b1
+granularity: 524288
+[1]:
+flags:
+[0]: auto
+name: b2
+granularity: 65536
+[2]:
+flags:
+name: b0
+granularity: 65536
+refcount bits: 16
+corrupt: false
+extended l2: false
+
+image: TEST_DIR/t.IMGFMT.base
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+cluster_size: 65536
+Format specific information:
+compat: 1.1
+compression type: zlib
+lazy refcounts: false
+bitmaps:
+[0]:
+flags:
+[0]: auto
+name: b0
+granularity: 65536
+[1]:
+flags:
+[0]: auto
+name: b3
+granularity: 65536
+refcount bits: 16
+corrupt: false
+extended l2: false
+
=== Check bitmap contents ===
[{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true,
"offset": OFFSET},
@@ -79,4 +132,7 @@ Format specific information:
[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true,
"offset": OFFSET},
{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data":
false},
{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data":
true, "offset": OFFSET}]
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true,
"offset": OFFSET},
+{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data":
false},
+{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data":
true, "offset": OFFSET}]
*** done
--
2.28.0
