Re: [PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces
"Dr. David Alan Gilbert" writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> New option parameters unstable-input and unstable-output set policy >> for unstable interfaces just like deprecated-input and >> deprecated-output set policy for deprecated interfaces (see commit >> 6dd75472d5 "qemu-options: New -compat to set policy for deprecated >> interfaces"). This is intended for testing users of the management >> interfaces. It is experimental. > > So is there no way to mark the option as unstable? The closest we have to marking command line options as unstable is putting them under "Debug/Expert options" in -help. For option *parameters*, we sometimes use the x- convention. QAPIfication will give us better tools.
Re: [PATCH 5/9] qapi: Generalize struct member policy checking
Philippe Mathieu-Daudé writes:
> On 10/25/21 07:25, Markus Armbruster wrote:
>> The generated visitor functions call visit_deprecated_accept() and
>> visit_deprecated() when visiting a struct member with special feature
>> flag 'deprecated'. This makes the feature flag visible to the actual
>> visitors. I want to make feature flag 'unstable' visible there as
>> well, so I can add policy for it.
>>
>> To let me make it visible, replace these functions by
>> visit_policy_reject() and visit_policy_skip(), which take the member's
>> special features as an argument. Note that the new functions have the
>> opposite sense, i.e. the return value flips.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> include/qapi/visitor-impl.h | 6 --
>> include/qapi/visitor.h| 17 +
>> qapi/qapi-forward-visitor.c | 16 +---
>> qapi/qapi-visit-core.c| 22 --
>> qapi/qobject-input-visitor.c | 15 ++-
>> qapi/qobject-output-visitor.c | 9 ++---
>> qapi/trace-events | 4 ++--
>> scripts/qapi/visit.py | 14 +++---
>> 8 files changed, 63 insertions(+), 40 deletions(-)
>
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index 71b24a4429..fda485614b 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -662,16 +662,21 @@ static void qobject_input_optional(Visitor *v, const
>> char *name, bool *present)
>> *present = true;
>> }
>>
>> -static bool qobject_input_deprecated_accept(Visitor *v, const char *name,
>> -Error **errp)
>> +static bool qobject_input_policy_reject(Visitor *v, const char *name,
>> +unsigned special_features,
>> +Error **errp)
>> {
>> +if (!(special_features && 1u << QAPI_DEPRECATED)) {
>
> Unreachable =) Proof than extract() is safer :P
Good eyes, thank you!
I actually like extract & desposit macros when the width is greater than
one. Then, the longhand C code is illegible anyway, and having to
remember what the macros mean is no worse.
For width 1 it feels like a wash. Universal use of the macros could
build familiarity and thus tip the balance.
I count more than a thousand instances of '& (1 <<'.
I wasn't even aware the macros existed in QEMU[*].
>
>> +return false;
>> +}
[*] I may well have seen them before, but my memory is limited and
lossy.
Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
John Snow writes: > On Tue, Oct 26, 2021 at 3:56 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster >> wrote: >> > >> >> Add special feature 'unstable' everywhere the name starts with 'x-', >> >> except for InputBarrierProperties member x-origin and >> >> MemoryBackendProperties member x-use-canonical-path-for-ramblock-id, >> >> because these two are actually stable. >> >> >> >> Signed-off-by: Markus Armbruster >> >> --- >> >> qapi/block-core.json | 123 +++ >> >> qapi/migration.json | 35 +--- >> >> qapi/misc.json | 6 ++- >> >> qapi/qom.json| 11 ++-- >> >> 4 files changed, 130 insertions(+), 45 deletions(-) >> >> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> >> index 6d3217abb6..ce2c1352cb 100644 >> >> --- a/qapi/block-core.json >> >> +++ b/qapi/block-core.json >> >> @@ -1438,6 +1438,9 @@ >> >> # >> >> # @x-perf: Performance options. (Since 6.0) >> >> # >> >> +# Features: >> >> +# @unstable: Member @x-perf is experimental. >> >> +# >> >> >> > >> > It'd be a lot cooler if we could annotate the unstable member directly >> > instead of confusing it with the syntax that might describe the entire >> > struct/union/command/etc, but ... eh, it's just a doc field, so I'm not >> > gonna press on this. I don't have the energy to get into a doc formatting >> > standard discussion right now, so: sure, why not? >> >> By design, we have a single doc comment block for the entire definition. >> >> When Kevin invented feature flags (merge commit 4747524f9f2), he added >> them just to struct types. He added "feature sections" for documenting >> features. It mirrors the "argument sections" for documenting members. >> Makes sense. >> >> Aside: he neglected to update qapi-code-gen.rst section "Definition >> documentation", and I failed to catch it. I'll make up for it. >> >> Peter and I then added feature flags to the remaining definitions >> (commit 23394b4c39 and 013b4efc9b). Feature sections make sense there, >> too. >> >> I then added them to struct members (commit 84ab008687). Instead of >> doing something fancy for documenting feature flags of members, I simply >> used the existing feature sections. This conflates member features with >> struct features. >> >> > Yeah, that's the part I don't like. If this isn't the first instance of > breaking the seal, though, this is the wrong time for me to comment on it. > Don't worry about it for this series. Okay. >> What could "something fancy" be? All we have for members is "argument >> sections", which are basically a name plus descriptive text. We'd have >> to add structure to that, say nest feature sections into argument >> sections. I have no appetite for that right now. >> >> > (Tangent below, unrelated to acceptance of this series) > > Yeah, I don't have an appetite for it at the moment either. I'll have to > read Marc-Andre's recent sphinx patches and see if I want to do more work > -- I had a bigger prototype a few months back I didn't bring all the way > home, but I am still thinking about reworking our QAPIDoc format. It's ... > well. I don't really want to, but I am not sure how else to bring some of > the features I want home, and I think I need some more time to think > carefully through exactly what I want to do and why. > > I wouldn't mind chatting about it with you sometime soon -- there's a few > things to balance here, such as: > > (1) Reworking the doc format would be an obnoxious amount of churn, ... Yes. But that need not be the end of the argument, it may be the beginning of one. > (2) ...but the Sphinx internals are really not meant to be used the way > we're using them right now, ... Happy to trust you on this one. > (3) ...but I also don't want to write our QAPI docstrings in a way that > *targets* Sphinx. Not that I think we'll be dropping it any time soon, but > it still feels like the wrong idea to tie them so closely together. Maybe. Depends on what we get for it. > I'm hoping there's an opportunity to make the parsing nicer with minimal > changes to the parsing and format, though. It just might require enforcing > a *pinch* more structure. I could see how I feel about per-field > annotations at that point. The doc comment language is the result of a series of pragmatic decisions that got us from semi-accidental comment conventions to where we are now. Each of the decisions made sense at the time. The result is messy to describe and to parse. It's limiting and hard to grow further. >I consider them interesting for things like the > Python SDK where I may want to enable/disable warnings for using deprecated > stuff at the client-level. (e.g., let's say you're using Python SDK 6.2 to > talk to a 6.1 client. Nothing stops you from doing this, but some commands > will simply be rejected by QEMU as it won't know what you're talking about. > Using
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
Daniel P. Berrangé writes: > On Tue, Oct 26, 2021 at 05:15:10PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé writes: >> >> > On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote: [...] >> >> Management applications are better off with a feature flag than with a >> >> naming convention we sometimes ignore. >> > >> > We will sometimes ignore/forget the feature flag too though, so I'm >> > not convinced there's much difference there. >> >> -compat unstable-input=reject,unstable-output=hide should help you stay >> on the straight & narrow :) > > That's from the pov of the mgmt app. I meant from the POV of QEMU > maintainers forgetting to add "unstable" flag, just as they might > forget to add a "x-" prefix. Got it. My point was that feature flag "unstable" is an unequivocal signal for "this thing is unstable", while a name starting with "x-" isn't: there are exceptions. The converse is a wash: we can forget to mark something unstable no matter how the mark works.
Re: [PATCH v2 1/3] file-posix: add `aio-max-batch` option
Stefano Garzarella writes:
> Commit d7ddd0a161 ("linux-aio: limit the batch size using
> `aio-max-batch` parameter") added a way to limit the batch size
> of Linux AIO backend for the entire AIO context.
>
> The same AIO context can be shared by multiple devices, so
> latency-sensitive devices may want to limit the batch size even
> more to avoid increasing latency.
>
> For this reason we add the `aio-max-batch` option to the file
> backend, which will be used by the next commits to limit the size of
> batches including requests generated by this device.
>
> Suggested-by: Kevin Wolf
> Reviewed-by: Kevin Wolf
> Signed-off-by: Stefano Garzarella
> ---
>
> Notes:
> v2:
> - @aio-max-batch documentation rewrite [Stefan, Kevin]
>
> qapi/block-core.json | 7 +++
> block/file-posix.c | 9 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6d3217abb6..fef76b0ea2 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2896,6 +2896,12 @@
> # for this device (default: none, forward the commands via
> SG_IO;
> # since 2.11)
> # @aio: AIO backend (default: threads) (since: 2.8)
> +# @aio-max-batch: maximum number of requests to batch together into a single
> +# submission in the AIO backend. The smallest value between
> +# this and the aio-max-batch value of the IOThread object is
> +# chosen.
> +# 0 means that the AIO backend will handle it automatically.
> +# (default: 0, since 6.2)
"(default 0) (since 6.2)" seems to be more common.
> # @locking: whether to enable file locking. If set to 'auto', only enable
> # when Open File Descriptor (OFD) locking API is available
> # (default: auto, since 2.10)
> @@ -2924,6 +2930,7 @@
> '*pr-manager': 'str',
> '*locking': 'OnOffAuto',
> '*aio': 'BlockdevAioOptions',
> +'*aio-max-batch': 'int',
> '*drop-cache': {'type': 'bool',
> 'if': 'CONFIG_LINUX'},
> '*x-check-cache-dropped': 'bool' },
Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI
On Tue, Oct 26, 2021 at 2:36 PM John Snow wrote: > > > On Tue, Oct 26, 2021 at 6:57 AM Hanna Reitz wrote: > >> On 19.10.21 16:49, John Snow wrote: >> > We need at least a tiny little shim here to join test file discovery >> > with test invocation. This logic could conceivably be hosted somewhere >> > in python/, but I felt it was strictly the least-rude thing to keep the >> > test logic here in iotests/, even if this small function isn't itself an >> > iotest. >> > >> > Note that we don't actually even need the executable bit here, we'll be >> > relying on the ability to run this module as a script using Python CLI >> > arguments. No chance it gets misunderstood as an actual iotest that way. >> > >> > (It's named, not in tests/, doesn't have the execute bit, and doesn't >> > have an execution shebang.) >> > >> > Signed-off-by: John Snow >> > --- >> > tests/qemu-iotests/linters.py | 27 +++ >> > 1 file changed, 27 insertions(+) >> >> Reviewed-by: Hanna Reitz >> >> > Thanks! I'll endeavor to try and clean up the list of exempted files to > continue cleaning up this mess, but it's not a top priority right now. I'll > put it on the backburner after I finish typing the QAPI generator. A lot of > the weird compatibility goop will go away over time as I consolidate more > of the venv logic. > > (I think this series is good to go, now? I think it could be applied in > any order vs my other series. If you want, if/when you give the go-ahead > for the other series, I could just stage them both myself and make sure > they work well together and save you the headache.) > Update: I pre-emptively staged both series (the iotests one first, followed by the AQMP one) to jsnow/python and verified that all of the python tests pass for each commit between: [14] python-add-iotest-linters-to # python: Add iotest linters to test suite ... [22] python-iotests-replace-qmp # python, iotests: replace qmp with aqmp and I'm running the CI on all of that now at https://gitlab.com/jsnow/qemu/-/pipelines/396002744 (I just wanted to double-check they didn't conflict with each other in any unanticipated ways. Let me know if I should send the PR or if that'll just create hassle for you.) --js
Re: [PATCH v2 0/2] pylint: fix new errors and warnings in qemu-iotests
On Mon, Oct 11, 2021 at 5:58 AM Emanuele Giuseppe Esposito < eespo...@redhat.com> wrote: > > > On 11/10/2021 11:29, Hanna Reitz wrote: > > On 08.10.21 08:28, Emanuele Giuseppe Esposito wrote: > >> There are some warnings and errors that we either miss or > >> are new in pylint. Anyways, test 297 of qemu-iotests fails > >> because of that, so we need to fix it. > >> > >> All these fixes involve just indentation or additional spaces > >> added. > >> > >> Signed-off-by: Emanuele Giuseppe Esposito > >> --- > >> v2: > >> * temporarly enable and then disable "bad whitespace" error in > >> image-fleecing > >> * better indentation for a fix in test 129 in patch one > > > > So the changes look good to me, but I can’t get my pylint to generate a > > bad-whitespace warning no matter how hard I try. (When you asked on IRC > > whether others see pylint warnings, I thought you meant the > > consider-using-f-string warnings that John disabled in > > 3765315d4c84f9c0799744f43a314169baaccc05.) > > > > I have pylint 2.11.1, which should be the newest version. So I tried to > > look around what might be the cause and found this: > > https://pylint.pycqa.org/en/latest/whatsnew/2.6.html – it seems that as > > of pylint 2.6, bad-whitespace warnings are no longer emitted. If that’s > > the reason why I can’t see the warning, then I think we should take only > > patch 1 (because it just makes sense), but skip patch 2. > > > > Yes you are right. I had 2.4.4, and now that I upgraded to 2.11.1 I > don't see bad-whitespace errors anymore (actually pylint does not seem > to complain at all). So I agree we can just take patch 1, as formatting > is wrong anyways. > > FWIW, the minimum version of pylint supported by the python/ tests is 2.8.0, for other reasons -- see commit b4d37d81. I no longer test or check for compatibility with older versions of pylint on any of our codebase. I've also authored a series to add iotest 297 to the Python CI directly, see https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg04329.html -- this gives a sense of "canonically supported linter versions" to that test. Currently, that support matrix is: Python 3.6 to Python 3.10 mypy >= 0.770 (Known to work with current latest, 0.910) pylint >= 2.8.0 (Known to work with current latest, 2.11.1) The "check-python-pipenv" test is the one Python CI test that will actually gate a build -- that test runs Python 3.6 with the oldest possible linter versions we support to ensure we don't accidentally start using new features. "check-python-tox" tests all python versions, 3.6 through 3.10, with bleeding edge packages. That CI test is "allowed to fail with a warning" and serves to alert me to new incompatible changes in updated Python dependencies. --js
Re: [PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces
* Markus Armbruster (arm...@redhat.com) wrote:
> New option parameters unstable-input and unstable-output set policy
> for unstable interfaces just like deprecated-input and
> deprecated-output set policy for deprecated interfaces (see commit
> 6dd75472d5 "qemu-options: New -compat to set policy for deprecated
> interfaces"). This is intended for testing users of the management
> interfaces. It is experimental.
So is there no way to mark the option as unstable?
Dave
> For now, this covers only syntactic aspects of QMP, i.e. stuff tagged
> with feature 'unstable'. We may want to extend it to cover semantic
> aspects, or the command line.
>
> Note that there is no good way for management application to detect
> presence of these new option parameters: they are not visible output
> of query-qmp-schema or query-command-line-options. Tolerable, because
> it's meant for testing. If running with -compat fails, skip the test.
>
> Signed-off-by: Markus Armbruster
> ---
> qapi/compat.json | 6 +-
> include/qapi/util.h | 1 +
> qapi/qmp-dispatch.c | 6 ++
> qapi/qobject-output-visitor.c | 8 ++--
> qemu-options.hx | 20 +++-
> scripts/qapi/events.py| 10 ++
> scripts/qapi/schema.py| 10 ++
> 7 files changed, 49 insertions(+), 12 deletions(-)
>
> diff --git a/qapi/compat.json b/qapi/compat.json
> index 74a8493d3d..9bc9804abb 100644
> --- a/qapi/compat.json
> +++ b/qapi/compat.json
> @@ -47,9 +47,13 @@
> #
> # @deprecated-input: how to handle deprecated input (default 'accept')
> # @deprecated-output: how to handle deprecated output (default 'accept')
> +# @unstable-input: how to handle unstable input (default 'accept')
> +# @unstable-output: how to handle unstable output (default 'accept')
> #
> # Since: 6.0
> ##
> { 'struct': 'CompatPolicy',
>'data': { '*deprecated-input': 'CompatPolicyInput',
> -'*deprecated-output': 'CompatPolicyOutput' } }
> +'*deprecated-output': 'CompatPolicyOutput',
> +'*unstable-input': 'CompatPolicyInput',
> +'*unstable-output': 'CompatPolicyOutput' } }
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 0cc98db9f9..81a2b13a33 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -13,6 +13,7 @@
>
> typedef enum {
> QAPI_DEPRECATED,
> +QAPI_UNSTABLE,
> } QapiSpecialFeature;
>
> typedef struct QEnumLookup {
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index e29ade134c..c5c6e521a2 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -59,6 +59,12 @@ bool compat_policy_input_ok(unsigned special_features,
> error_class, kind, name, errp)) {
> return false;
> }
> +if ((special_features & (1u << QAPI_UNSTABLE))
> +&& !compat_policy_input_ok1("Unstable",
> +policy->unstable_input,
> +error_class, kind, name, errp)) {
> +return false;
> +}
> return true;
> }
>
> diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
> index b5c6564cbb..74770edd73 100644
> --- a/qapi/qobject-output-visitor.c
> +++ b/qapi/qobject-output-visitor.c
> @@ -212,8 +212,12 @@ static bool qobject_output_type_null(Visitor *v, const
> char *name,
> static bool qobject_output_policy_skip(Visitor *v, const char *name,
> unsigned special_features)
> {
> -return !(special_features && 1u << QAPI_DEPRECATED)
> -|| v->compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE;
> +CompatPolicy *pol = >compat_policy;
> +
> +return ((special_features & 1u << QAPI_DEPRECATED)
> +&& pol->deprecated_output == COMPAT_POLICY_OUTPUT_HIDE)
> +|| ((special_features & 1u << QAPI_UNSTABLE)
> +&& pol->unstable_output == COMPAT_POLICY_OUTPUT_HIDE);
> }
>
> /* Finish building, and return the root object.
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 5f375bbfa6..f051536b63 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3641,7 +3641,9 @@ DEFHEADING(Debug/Expert options:)
>
> DEF("compat", HAS_ARG, QEMU_OPTION_compat,
> "-compat
> [deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n"
> -"Policy for handling deprecated management interfaces\n",
> +"Policy for handling deprecated management interfaces\n"
> +"-compat
> [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
> +"Policy for handling unstable management interfaces\n",
> QEMU_ARCH_ALL)
> SRST
> ``-compat
> [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
> @@ -3659,6 +3661,22 @@ SRST
> Suppress deprecated command results and events
>
> Limitation: covers only syntactic aspects of QMP.
> +
>
Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI
On Tue, Oct 26, 2021 at 6:57 AM Hanna Reitz wrote: > On 19.10.21 16:49, John Snow wrote: > > We need at least a tiny little shim here to join test file discovery > > with test invocation. This logic could conceivably be hosted somewhere > > in python/, but I felt it was strictly the least-rude thing to keep the > > test logic here in iotests/, even if this small function isn't itself an > > iotest. > > > > Note that we don't actually even need the executable bit here, we'll be > > relying on the ability to run this module as a script using Python CLI > > arguments. No chance it gets misunderstood as an actual iotest that way. > > > > (It's named, not in tests/, doesn't have the execute bit, and doesn't > > have an execution shebang.) > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/linters.py | 27 +++ > > 1 file changed, 27 insertions(+) > > Reviewed-by: Hanna Reitz > > Thanks! I'll endeavor to try and clean up the list of exempted files to continue cleaning up this mess, but it's not a top priority right now. I'll put it on the backburner after I finish typing the QAPI generator. A lot of the weird compatibility goop will go away over time as I consolidate more of the venv logic. (I think this series is good to go, now? I think it could be applied in any order vs my other series. If you want, if/when you give the go-ahead for the other series, I could just stage them both myself and make sure they work well together and save you the headache.) --js
Re: [PATCH v2 11/15] iotests: split linters.py out from 297
On Tue, Oct 26, 2021 at 6:51 AM Hanna Reitz wrote: > On 19.10.21 16:49, John Snow wrote: > > Now, 297 is just the iotests-specific incantations and linters.py is as > > minimal as I can think to make it. The only remaining element in here > > that ought to be configuration and not code is the list of skip files, > > but they're still numerous enough that repeating them for mypy and > > pylint configurations both would be ... a hassle. > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/297| 72 + > > tests/qemu-iotests/linters.py | 76 +++ > > 2 files changed, 87 insertions(+), 61 deletions(-) > > create mode 100644 tests/qemu-iotests/linters.py > > Reviewed-by: Hanna Reitz > > Thanks! > I wonder about `check_linter()`, though. By not moving it to > linters.py, we can’t use it in its entry point, and so the Python test > infrastructure will have a strong dependency on these linters. Though > then again, it probably already does, and I suppose that’s one of the > points hindering us from running this from make check? > > That one is left behind because it uses iotests API to skip a test. Environment setup that guarantees we won't *need* to skip the test is handled by the virtual environment setup magic in qemu/python/Makefile. > Hanna > More info than you require: There's maybe about four ways you could run the python tests that might make sense depending on who you are and what you're trying to accomplish; they're documented in "make help" and again in qemu/python/README.rst; (1) make check-dev -- makes a venv with whatever python you happen to have, installs the latest packages, runs the tests (2) make check-pipenv -- requires python 3.6 specifically, installs the *oldest* packages, runs the tests (3) make check-tox -- requires python 3.6 through 3.10, installs the newest packages, runs the tests per each python version (4) make check -- perform no setup at all, just run the tests in the current environment. (Used directly by all three prior invocations) In general, I assume that human beings that aren't working on Python stuff actively will be using (1) at their interactive console, if they decide to run any of these at all. It imposes the least pre-requirements while still endeavoring to be a target that will "just work". Options (2) and (3) are what power the CI tests 'check-python-pipenv' and 'check-python-tox', respectively; with(2) being the one that actually gates the CI. Option (4) is only really a convenience for bypassing the venv-setup stuff if you want to construct your own (or not use one at all). So the tests just assume that the tools they have available will work. It's kind of a 'power user' target. The way the CI works at the moment is that it uses a "fedora:latest" base image and installs python interpreters 3.6 through 3.10 inclusive, pipenv, and tox. From there, it has enough juice to run the makefile targets which take care of all of the actual linting dependencies and their different versions to get a wider matrix on the version testing to ensure we're still keeping compatibility with the 3.6 platform while also checking for new problems that show up on the bleeding edge. iotests 297 right now doesn't do any python environment setup at all, so we can't guarantee that the linters are present, so we decide to allow the test to be skipped. My big hesitation of adding this test directly into 'make check' is that I will need to do some environment probing to make sure that the 'pylint' version isn't too old -- or otherwise set up a venv in the build directory that can be used to run tests. I know we already set one up for the acceptance tests, so maybe there's an opportunity to re-use that venv, but I need to make sure that the dependencies between the two sets of tests are aligned. I don't know if they agree, currently. I think I probably want to minimize the number of different virtual python environments we create during the build, so I will look into what problems might exist in re-purposing the acceptance test venv. If that can get squared away easily, there's likely no real big barrier to adding more tests that rely on a python venv to get cooking during the normal build/check process, including iotest 297. --js
Re: [PATCH 00/15] hw/nvme: SR-IOV with Virtualization Enhancements
On Oct 7 18:23, Lukasz Maniak wrote: > Hi, > > This series of patches is an attempt to add support for the following > sections of NVMe specification revision 1.4: > > 8.5 Virtualization Enhancements (Optional) > 8.5.1 VQ Resource Definition > 8.5.2 VI Resource Definition > 8.5.3 Secondary Controller States and Resource Configuration > 8.5.4 Single Root I/O Virtualization and Sharing (SR-IOV) > > The NVMe controller's Single Root I/O Virtualization and Sharing > implementation is based on patches introducing SR-IOV support for PCI > Express proposed by Knut Omang: > https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg05155.html > > However, based on what I was able to find historically, Knut's patches > have not yet been pulled into QEMU due to no example of a working device > up to this point: > https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg02722.html > > In terms of design, the Physical Function controller and the Virtual > Function controllers are almost independent, with few exceptions: > PF handles flexible resource allocation for all its children (VFs have > read-only access to this data), and reset (PF explicitly calls it on VFs). > Since the MMIO access is serialized, no extra precautions are required > to handle concurrent resets, as well as the secondary controller state > access doesn't need to be atomic. > > A controller with full SR-IOV support must be capable of handling the > Namespace Management command. As there is a pending review with this > functionality, this patch list is not duplicating efforts. > Yet, NS management patches are not required to test the SR-IOV support. > > We tested the patches on Ubuntu 20.04.3 LTS with kernel 5.4.0. We have > hit various issues with NVMe CLI (list and virt-mgmt commands) between > releases from version 1.09 to master, thus we chose this golden NVMe CLI > hash for testing: a50a0c1. > > The implementation is not 100% finished and certainly not bug free, > since we are already aware of some issues e.g. interaction with > namespaces related to AER, or unexpected (?) kernel behavior in more > complex reset scenarios. However, our SR-IOV implementation is already > able to support typical SR-IOV use cases, so we believe the patches are > ready to share with the community. > > Hope you find some time to review the work we did, and share your > thoughts. > > Kind regards, > Lukasz Hi Lukasz, If possible, can you share a brief guide on testing this? I keep hitting an assert qemu-system-x86_64: ../hw/pci/pci.c:1215: pci_register_bar: Assertion `!pci_is_vf(pci_dev)' failed. when I try to modify sriov_numvfs. This should be fixed anyway, but I might be doing something wrong in the first place. signature.asc Description: PGP signature
[PATCH v5 6/8] iotests/300: avoid abnormal shutdown race condition
Wait for the destination VM to close itself instead of racing to shut it
down first, which produces different error log messages from AQMP
depending on precisely when we tried to shut it down.
(For example: We may try to issue 'quit' immediately prior to the target
VM closing its QMP socket, which will cause an ECONNRESET error to be
logged. Waiting for the VM to exit itself avoids the race on shutdown
behavior.)
Reported-by: Hanna Reitz
Signed-off-by: John Snow
---
tests/qemu-iotests/300 | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 10f9f2a8da6..dbd28384ec3 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -24,8 +24,6 @@ import random
import re
from typing import Dict, List, Optional
-from qemu.machine import machine
-
import iotests
@@ -461,12 +459,11 @@ class
TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
f"'{self.src_node_name}': Name is longer than 255 bytes",
log)
-# Expect abnormal shutdown of the destination VM because of
-# the failed migration
-try:
-self.vm_b.shutdown()
-except machine.AbnormalShutdown:
-pass
+# Destination VM will terminate w/ error of its own accord
+# due to the failed migration.
+self.vm_b.wait()
+rc = self.vm_b.exitcode()
+assert rc is not None and rc > 0
def test_aliased_bitmap_name_too_long(self) -> None:
# Longer than the maximum for bitmap names
--
2.31.1
Re: [PATCH v2 08/15] iotests/297: Change run_linter() to raise an exception on failure
On Tue, Oct 26, 2021 at 6:10 AM Hanna Reitz wrote: > On 19.10.21 16:49, John Snow wrote: > > Instead of using a process return code as the python function return > > value (or just not returning anything at all), allow run_linter() to > > raise an exception instead. > > > > The responsibility for printing output on error shifts from the function > > itself to the caller, who will know best how to present/format that > > information. (Also, "suppress_output" is now a lot more accurate of a > > parameter name.) > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/297 | 24 ++-- > > 1 file changed, 14 insertions(+), 10 deletions(-) > > Thanks! :) > > Reviewed-by: Hanna Reitz > > No problem. Thanks for taking the time to review it! --js
[PATCH v5 7/8] python/aqmp: Create sync QMP wrapper for iotests
This is a wrapper around the async QMPClient that mimics the old,
synchronous QEMUMonitorProtocol class. It is designed to be
interchangeable with the old implementation.
It does not, however, attempt to mimic Exception compatibility.
Signed-off-by: John Snow
Acked-by: Hanna Reitz
---
python/qemu/aqmp/legacy.py | 138 +
1 file changed, 138 insertions(+)
create mode 100644 python/qemu/aqmp/legacy.py
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
new file mode 100644
index 000..9e7b9fb80b9
--- /dev/null
+++ b/python/qemu/aqmp/legacy.py
@@ -0,0 +1,138 @@
+"""
+Sync QMP Wrapper
+
+This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+"""
+
+import asyncio
+from typing import (
+Awaitable,
+List,
+Optional,
+TypeVar,
+Union,
+)
+
+import qemu.qmp
+from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
+
+from .qmp_client import QMPClient
+
+
+# pylint: disable=missing-docstring
+
+
+class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+def __init__(self, address: SocketAddrT,
+ server: bool = False,
+ nickname: Optional[str] = None):
+
+# pylint: disable=super-init-not-called
+self._aqmp = QMPClient(nickname)
+self._aloop = asyncio.get_event_loop()
+self._address = address
+self._timeout: Optional[float] = None
+
+_T = TypeVar('_T')
+
+def _sync(
+self, future: Awaitable[_T], timeout: Optional[float] = None
+) -> _T:
+return self._aloop.run_until_complete(
+asyncio.wait_for(future, timeout=timeout)
+)
+
+def _get_greeting(self) -> Optional[QMPMessage]:
+if self._aqmp.greeting is not None:
+# pylint: disable=protected-access
+return self._aqmp.greeting._asdict()
+return None
+
+# __enter__ and __exit__ need no changes
+# parse_address needs no changes
+
+def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+self._aqmp.await_greeting = negotiate
+self._aqmp.negotiate = negotiate
+
+self._sync(
+self._aqmp.connect(self._address)
+)
+return self._get_greeting()
+
+def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+self._aqmp.await_greeting = True
+self._aqmp.negotiate = True
+
+self._sync(
+self._aqmp.accept(self._address),
+timeout
+)
+
+ret = self._get_greeting()
+assert ret is not None
+return ret
+
+def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+return dict(
+self._sync(
+# pylint: disable=protected-access
+
+# _raw() isn't a public API, because turning off
+# automatic ID assignment is discouraged. For
+# compatibility with iotests *only*, do it anyway.
+self._aqmp._raw(qmp_cmd, assign_id=False),
+self._timeout
+)
+)
+
+# Default impl of cmd() delegates to cmd_obj
+
+def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+return self._sync(
+self._aqmp.execute(cmd, kwds),
+self._timeout
+)
+
+def pull_event(self,
+ wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+if not wait:
+# wait is False/0: "do not wait, do not except."
+if self._aqmp.events.empty():
+return None
+
+# If wait is 'True', wait forever. If wait is False/0, the events
+# queue must not be empty; but it still needs some real amount
+# of time to complete.
+timeout = None
+if wait and isinstance(wait, float):
+timeout = wait
+
+return dict(
+self._sync(
+self._aqmp.events.get(),
+timeout
+)
+)
+
+def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
+events = [dict(x) for x in self._aqmp.events.clear()]
+if events:
+return events
+
+event = self.pull_event(wait)
+return [event] if event is not None else []
+
+def clear_events(self) -> None:
+self._aqmp.events.clear()
+
+def close(self) -> None:
+self._sync(
+self._aqmp.disconnect()
+)
+
+def settimeout(self, timeout: Optional[float]) -> None:
+self._timeout = timeout
+
+def send_fd_scm(self, fd: int) -> None:
+self._aqmp.send_fd_scm(fd)
--
2.31.1
[PATCH v5 5/8] iotests: Conditionally silence certain AQMP errors
AQMP likes to be very chatty about errors it encounters. In general,
this is good because it allows us to get good diagnostic information for
otherwise complex async failures.
For example, during a failed QMP connection attempt, we might see:
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session: EOFError
This might be nice in iotests output, because failure scenarios
involving the new QMP library will be spelled out plainly in the output
diffs.
For tests that are intentionally causing this scenario though, filtering
that log output could be a hassle. For now, add a context manager that
simply lets us toggle this output off during a critical region.
(Additionally, a forthcoming patch allows the use of either legacy or
async QMP to be toggled with an environment variable. In this
circumstance, we can't amend the iotest output to just always expect the
error message, either. Just suppress it for now. More rigorous log
filtering can be investigated later if/when it is deemed safe to
permanently replace the legacy QMP library.)
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 20 +++-
tests/qemu-iotests/tests/mirror-top-perms | 12
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e5fff6ddcfc..e2f9d873ada 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,7 +30,7 @@
import subprocess
import sys
import time
-from typing import (Any, Callable, Dict, Iterable,
+from typing import (Any, Callable, Dict, Iterable, Iterator,
List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
import unittest
@@ -114,6 +114,24 @@
sample_img_dir = os.environ['SAMPLE_IMG_DIR']
+@contextmanager
+def change_log_level(
+logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]:
+"""
+Utility function for temporarily changing the log level of a logger.
+
+This can be used to silence errors that are expected or uninteresting.
+"""
+_logger = logging.getLogger(logger_name)
+current_level = _logger.level
+_logger.setLevel(level)
+
+try:
+yield
+finally:
+_logger.setLevel(current_level)
+
+
def unarchive_sample_image(sample, fname):
sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index a2d5c269d7a..0a51a613f39 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -26,7 +26,7 @@ from qemu.machine import machine
from qemu.qmp import QMPConnectError
import iotests
-from iotests import qemu_img
+from iotests import change_log_level, qemu_img
image_size = 1 * 1024 * 1024
@@ -100,9 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
try:
-self.vm_b.launch()
-print('ERROR: VM B launched successfully, this should not have '
- 'happened')
+# Silence AQMP errors temporarily.
+# TODO: Remove this and just allow the errors to be logged when
+# AQMP fully replaces QMP.
+with change_log_level('qemu.aqmp'):
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, '
+ 'this should not have happened')
except (QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
--
2.31.1
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 10/26/21 10:26 AM, Thomas Huth wrote: Would it maybe make sense to tweak check_patch.pl to forbid __thread in certain folders only, e.g. block/ and util/ (i.e. where we know that there might be code that the iothreads are using)? This sounds plausible; hw/ too, perhaps. r~
[PATCH v5 2/8] python/machine: Handle QMP errors on close more meticulously
To use the AQMP backend, Machine just needs to be a little more diligent
about what happens when closing a QMP connection. The operation is no
longer a freebie in the async world; it may return errors encountered in
the async bottom half on incoming message receipt, etc.
(AQMP's disconnect, ultimately, serves as the quiescence point where all
async contexts are gathered together, and any final errors reported at
that point.)
Because async QMP continues to check for messages asynchronously, it's
almost certainly likely that the loop will have exited due to EOF after
issuing the last 'quit' command. That error will ultimately be bubbled
up when attempting to close the QMP connection. The manager class here
then is free to discard it -- if it was expected.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
python/qemu/machine/machine.py | 48 +-
1 file changed, 42 insertions(+), 6 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 0bd40bc2f76..a0cf69786b4 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
# Comprehensive reset for the failed launch case:
self._early_cleanup()
-if self._qmp_connection:
-self._qmp.close()
-self._qmp_connection = None
+try:
+self._close_qmp_connection()
+except Exception as err: # pylint: disable=broad-except
+LOG.warning(
+"Exception closing QMP connection: %s",
+str(err) if str(err) else type(err).__name__
+)
+finally:
+assert self._qmp_connection is None
self._close_qemu_log_file()
@@ -420,6 +426,31 @@ def _launch(self) -> None:
close_fds=False)
self._post_launch()
+def _close_qmp_connection(self) -> None:
+"""
+Close the underlying QMP connection, if any.
+
+Dutifully report errors that occurred while closing, but assume
+that any error encountered indicates an abnormal termination
+process and not a failure to close.
+"""
+if self._qmp_connection is None:
+return
+
+try:
+self._qmp.close()
+except EOFError:
+# EOF can occur as an Exception here when using the Async
+# QMP backend. It indicates that the server closed the
+# stream. If we successfully issued 'quit' at any point,
+# then this was expected. If the remote went away without
+# our permission, it's worth reporting that as an abnormal
+# shutdown case.
+if not (self._user_killed or self._quit_issued):
+raise
+finally:
+self._qmp_connection = None
+
def _early_cleanup(self) -> None:
"""
Perform any cleanup that needs to happen before the VM exits.
@@ -460,9 +491,14 @@ def _soft_shutdown(self, timeout: Optional[int]) -> None:
self._early_cleanup()
if self._qmp_connection:
-if not self._quit_issued:
-# Might raise ConnectionReset
-self.qmp('quit')
+try:
+if not self._quit_issued:
+# May raise ExecInterruptedError or StateError if the
+# connection dies or has *already* died.
+self.qmp('quit')
+finally:
+# Regardless, we want to quiesce the connection.
+self._close_qmp_connection()
# May raise subprocess.TimeoutExpired
self._subp.wait(timeout=timeout)
--
2.31.1
Re: [PATCH v2 10/15] iotests/297: split test into sub-cases
On Tue, Oct 26, 2021 at 6:29 AM Hanna Reitz wrote: > On 19.10.21 16:49, John Snow wrote: > > Take iotest 297's main() test function and split it into two sub-cases > > that can be skipped individually. We can also drop custom environment > > setup from the pylint test as it isn't needed. > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/297 | 63 ++ > > 1 file changed, 39 insertions(+), 24 deletions(-) > > Reviewed-by: Hanna Reitz > > (while heavily scratching myself to ease the itch to turn this into a > unit test now) > > (I strongly considered it, but didn't want to add yet-more-rewrites into this series. If the ultimate fate is to sunset this particular iotest, I didn't see a big benefit to doing it.)
[PATCH v5 4/8] iotests: Accommodate async QMP Exception classes
(But continue to support the old ones for now, too.)
There are very few cases of any user of QEMUMachine or a subclass
thereof relying on a QMP Exception type. If you'd like to check for
yourself, you want to grep for all of the derivatives of QMPError,
excluding 'AQMPError' and its derivatives. That'd be these:
- QMPError
- QMPConnectError
- QMPCapabilitiesError
- QMPTimeoutError
- QMPProtocolError
- QMPResponseError
- QMPBadPortError
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
scripts/simplebench/bench_block_job.py| 3 ++-
tests/qemu-iotests/tests/mirror-top-perms | 5 +++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 4f03c121697..a403c35b08f 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -28,6 +28,7 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
from qemu.qmp import QMPConnectError
+from qemu.aqmp import ConnectError
def bench_block_job(cmd, cmd_args, qemu_args):
@@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.launch()
except OSError as e:
return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, socket.timeout):
+except (QMPConnectError, ConnectError, socket.timeout):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index 3d475aa3a54..a2d5c269d7a 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,8 +21,9 @@
import os
-from qemu import qmp
+from qemu.aqmp import ConnectError
from qemu.machine import machine
+from qemu.qmp import QMPConnectError
import iotests
from iotests import qemu_img
@@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.launch()
print('ERROR: VM B launched successfully, this should not have '
'happened')
-except qmp.QMPConnectError:
+except (QMPConnectError, ConnectError):
assert 'Is another process using the image' in self.vm_b.get_log()
result = self.vm.qmp('block-job-cancel',
--
2.31.1
[PATCH v5 3/8] python/aqmp: Remove scary message
The scary message interferes with the iotests output. Coincidentally, if
iotests works by removing this, then it's good evidence that we don't
really need to scare people away from using it.
Signed-off-by: John Snow
Acked-by: Hanna Reitz
---
python/qemu/aqmp/__init__.py | 12
1 file changed, 12 deletions(-)
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index d1b0e4dc3d3..880d5b6fa7f 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,7 +22,6 @@
# the COPYING file in the top-level directory.
import logging
-import warnings
from .error import AQMPError
from .events import EventListener
@@ -31,17 +30,6 @@
from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
-_WMSG = """
-
-The Asynchronous QMP library is currently in development and its API
-should be considered highly fluid and subject to change. It should
-not be used by any other scripts checked into the QEMU tree.
-
-Proceed with caution!
-"""
-
-warnings.warn(_WMSG, FutureWarning)
-
# Suppress logging unless an application engages it.
logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
--
2.31.1
[PATCH v5 1/8] python/machine: remove has_quit argument
If we spy on the QMP commands instead, we don't need callers to remember
to pass it. Seems like a fair trade-off.
The one slightly weird bit is overloading this instance variable for
wait(), where we use it to mean "don't issue the qmp 'quit'
command". This means that wait() will "fail" if the QEMU process does
not terminate of its own accord.
In most cases, we probably did already actually issue quit -- some
iotests do this -- but in some others, we may be waiting for QEMU to
terminate for some other reason, such as a test wherein we tell the
guest (directly) to shut down.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
---
python/qemu/machine/machine.py | 34 +++---
tests/qemu-iotests/040 | 7 +--
tests/qemu-iotests/218 | 2 +-
tests/qemu-iotests/255 | 2 +-
4 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 056d340e355..0bd40bc2f76 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -170,6 +170,7 @@ def __init__(self,
self._console_socket: Optional[socket.socket] = None
self._remove_files: List[str] = []
self._user_killed = False
+self._quit_issued = False
def __enter__(self: _T) -> _T:
return self
@@ -368,6 +369,7 @@ def _post_shutdown(self) -> None:
command = ''
LOG.warning(msg, -int(exitcode), command)
+self._quit_issued = False
self._user_killed = False
self._launched = False
@@ -443,15 +445,13 @@ def _hard_shutdown(self) -> None:
self._subp.kill()
self._subp.wait(timeout=60)
-def _soft_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _soft_shutdown(self, timeout: Optional[int]) -> None:
"""
Perform early cleanup, attempt to gracefully shut down the VM, and wait
for it to terminate.
:param timeout: Timeout in seconds for graceful shutdown.
A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
:raise ConnectionReset: On QMP communication errors
:raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
@@ -460,21 +460,19 @@ def _soft_shutdown(self, timeout: Optional[int],
self._early_cleanup()
if self._qmp_connection:
-if not has_quit:
+if not self._quit_issued:
# Might raise ConnectionReset
-self._qmp.cmd('quit')
+self.qmp('quit')
# May raise subprocess.TimeoutExpired
self._subp.wait(timeout=timeout)
-def _do_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _do_shutdown(self, timeout: Optional[int]) -> None:
"""
Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
:param timeout: Timeout in seconds for graceful shutdown.
A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
:raise AbnormalShutdown: When the VM could not be shut down gracefully.
The inner exception will likely be ConnectionReset or
@@ -482,13 +480,13 @@ def _do_shutdown(self, timeout: Optional[int],
may result in its own exceptions, likely subprocess.TimeoutExpired.
"""
try:
-self._soft_shutdown(timeout, has_quit)
+self._soft_shutdown(timeout)
except Exception as exc:
self._hard_shutdown()
raise AbnormalShutdown("Could not perform graceful shutdown") \
from exc
-def shutdown(self, has_quit: bool = False,
+def shutdown(self,
hard: bool = False,
timeout: Optional[int] = 30) -> None:
"""
@@ -498,7 +496,6 @@ def shutdown(self, has_quit: bool = False,
If the VM has not yet been launched, or shutdown(), wait(), or kill()
have already been called, this method does nothing.
-:param has_quit: When true, do not attempt to issue 'quit' QMP command.
:param hard: When true, do not attempt graceful shutdown, and
suppress the SIGKILL warning log message.
:param timeout: Optional timeout in seconds for graceful shutdown.
@@ -512,7 +509,7 @@ def shutdown(self, has_quit: bool = False,
self._user_killed = True
self._hard_shutdown()
else:
-self._do_shutdown(timeout, has_quit)
+self._do_shutdown(timeout)
finally:
self._post_shutdown()
@@ -529,7 +526,8 @@ def wait(self, timeout: Optional[int] = 30) -> None:
:param timeout: Optional timeout in seconds. Default 30 seconds.
[PATCH v5 0/8] Switch iotests to using Async QMP
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper CI: https://gitlab.com/jsnow/qemu/-/pipelines/395925703 Hiya, This series continues where the last two AQMP series left off and adds a synchronous 'legacy' wrapper around the new AQMP interface, then drops it straight into iotests to prove that AQMP is functional and totally cool and fine. The disruption and churn to iotests is pretty minimal. In the event that a regression happens and I am not physically proximate to inflict damage upon, one may set the QEMU_PYTHON_LEGACY_QMP variable to any non-empty string as it pleases you to engage the QMP machinery you are used to. V5: 006: Fixed a typing error. (Hanna) V4: 006: Added to address a race condition in iotest 300. (Thanks for the repro, Hanna) V3: Near as we can tell, our immediate ancestral forebear. V2: A distant dream, half-remembered. V1: Apocrypha. John Snow (8): python/machine: remove has_quit argument python/machine: Handle QMP errors on close more meticulously python/aqmp: Remove scary message iotests: Accommodate async QMP Exception classes iotests: Conditionally silence certain AQMP errors iotests/300: avoid abnormal shutdown race condition python/aqmp: Create sync QMP wrapper for iotests python, iotests: replace qmp with aqmp python/qemu/aqmp/__init__.py | 12 -- python/qemu/aqmp/legacy.py| 138 ++ python/qemu/machine/machine.py| 85 + scripts/simplebench/bench_block_job.py| 3 +- tests/qemu-iotests/040| 7 +- tests/qemu-iotests/218| 2 +- tests/qemu-iotests/255| 2 +- tests/qemu-iotests/300| 13 +- tests/qemu-iotests/iotests.py | 20 +++- tests/qemu-iotests/tests/mirror-top-perms | 17 ++- 10 files changed, 243 insertions(+), 56 deletions(-) create mode 100644 python/qemu/aqmp/legacy.py -- 2.31.1
[PATCH v5 8/8] python, iotests: replace qmp with aqmp
Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync
wrapper from qemu.aqmp instead.
Add an escape hatch in the form of the environment variable
QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using
the old implementation, proving that both implementations work
concurrently.
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a0cf69786b4..a487c397459 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -41,7 +41,6 @@
)
from qemu.qmp import ( # pylint: disable=import-error
-QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
SocketAddrT,
@@ -50,6 +49,12 @@
from . import console_socket
+if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
+from qemu.qmp import QEMUMonitorProtocol
+else:
+from qemu.aqmp.legacy import QEMUMonitorProtocol
+
+
LOG = logging.getLogger(__name__)
--
2.31.1
Re: [PATCH 01/15] pcie: Set default and supported MaxReadReq to 512
On Tue, 2021-10-26 at 16:36 +0200, Lukasz Maniak wrote:
> On Thu, Oct 07, 2021 at 06:12:41PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Oct 07, 2021 at 06:23:52PM +0200, Lukasz Maniak wrote:
> > > From: Knut Omang
> > >
> > > Make the default PCI Express Capability for PCIe devices set
> > > MaxReadReq to 512.
> >
> > code says 256
> >
> > > Tyipcal modern devices people would want to
> >
> >
> > typo
> >
> > > emulate or simulate would want this. The previous value would
> > > cause warnings from the root port driver on some kernels.
> >
> >
> > which specifically?
> >
> > >
> > > Signed-off-by: Knut Omang
> >
> > we can't make changes like this unconditionally, this will
> > break migration across versions.
> > Pls tie this to a machine version.
> >
> > Thanks!
> > > ---
> > > hw/pci/pcie.c | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 6e95d82903..c1a12f3744 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -62,8 +62,9 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t
> > > type, uint8_t
> > > version)
> > > * Functions conforming to the ECN, PCI Express Base
> > > * Specification, Revision 1.1., or subsequent PCI Express Base
> > > * Specification revisions.
> > > + * + set max payload size to 256, which seems to be a common value
> > > */
> > > - pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER);
> > > + pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER | (0x1 &
> > > PCI_EXP_DEVCAP_PAYLOAD));
> > >
> > > pci_set_long(exp_cap + PCI_EXP_LNKCAP,
> > > (port << PCI_EXP_LNKCAP_PN_SHIFT) |
> > > @@ -179,6 +180,8 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
> > > pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
> > > PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
> > >
> > > + pci_set_word(exp_cap + PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_READRQ_256B);
> > > +
> > > pci_set_word(dev->wmask + pos + PCI_EXP_DEVCTL2,
> > > PCI_EXP_DEVCTL2_EETLPPB);
> > >
> > > if (dev->cap_present & QEMU_PCIE_EXTCAP_INIT) {
> > > --
> > > 2.25.1
> >
>
> Hi Michael,
>
> The reason Knut keeps rebasing this fix along with SR-IOV patch is not
> clear for us.
Sorry for the slow response - I seem to have messed up my mail filters so this
thread slipped past my attention.
> Since we have tested the NVMe device without this fix and did not notice
> any issues mentioned by Knut on kernel 5.4.0, we decided to drop it for
> v2.
I agree, let's just drop it.
It was likely in the 3.x kernels I had to relate to back then,
likely discovered in Oracle Linux given that I did not specifically point to a
kernel
range already back then.
> However, I have posted your comments to this patch on Knut's github so
> they can be addressed in case Knut decides to resubmit it later though.
Thanks for that ping, Lukasz, and great to see the patch finally being used in a
functional device!
Knut
> Thanks,
> Lukasz
Re: [PATCH 13/15] pcie: Add helpers to the SR/IOV API
On Thu, 2021-10-07 at 18:24 +0200, Lukasz Maniak wrote:
> From: Łukasz Gieryk
>
> Two convenience functions for retrieving:
> - the total number of VFs,
> - the PCIDevice object of the N-th VF.
>
> Signed-off-by: Łukasz Gieryk
> ---
> hw/pci/pcie_sriov.c | 14 ++
> include/hw/pci/pcie_sriov.h | 8
> 2 files changed, 22 insertions(+)
>
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index cac2aee061..5a8e92d5ab 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -292,8 +292,22 @@ uint16_t pcie_sriov_vf_number(PCIDevice *dev)
> return dev->exp.sriov_vf.vf_number;
> }
>
> +uint16_t pcie_sriov_vf_number_total(PCIDevice *dev)
> +{
> + assert(!pci_is_vf(dev));
> + return dev->exp.sriov_pf.num_vfs;
> +}
>
> PCIDevice *pcie_sriov_get_pf(PCIDevice *dev)
> {
> return dev->exp.sriov_vf.pf;
> }
> +
> +PCIDevice *pcie_sriov_get_vf_at_index(PCIDevice *dev, int n)
> +{
> + assert(!pci_is_vf(dev));
> + if (n < dev->exp.sriov_pf.num_vfs) {
> + return dev->exp.sriov_pf.vf[n];
> + }
> + return NULL;
> +}
> diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
> index 9ab48b79c0..d1f39b7223 100644
> --- a/include/hw/pci/pcie_sriov.h
> +++ b/include/hw/pci/pcie_sriov.h
> @@ -65,9 +65,17 @@ void pcie_sriov_pf_disable_vfs(PCIDevice *dev);
> /* Get logical VF number of a VF - only valid for VFs */
> uint16_t pcie_sriov_vf_number(PCIDevice *dev);
>
> +/* Get the total number of VFs - only valid for PF */
> +uint16_t pcie_sriov_vf_number_total(PCIDevice *dev);
> +
> /* Get the physical function that owns this VF.
> * Returns NULL if dev is not a virtual function
> */
> PCIDevice *pcie_sriov_get_pf(PCIDevice *dev);
>
> +/* Get the n-th VF of this physical function - only valid for PF.
> + * Returns NULL if index is invalid
> + */
> +PCIDevice *pcie_sriov_get_vf_at_index(PCIDevice *dev, int n);
> +
> #endif /* QEMU_PCIE_SRIOV_H */
These look like natural improvements to me, thanks!
Reviewed-by: Knut Omang
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 26/10/2021 19.10, Richard Henderson wrote: On 10/26/21 9:34 AM, Stefan Hajnoczi wrote: On Tue, Oct 26, 2021 at 08:10:16AM -0700, Richard Henderson wrote: On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: If "safe" TLS variables are opt-in then we'll likely have obscure bugs when code changes to access a TLS variable that was previously never accessed from a coroutine. There is no compiler error and no way to detect this. When it happens debugging it is painful. Co-routines are never used in user-only builds. If developers have the choice of using __thread then bugs can slip through. Huh? How. No, really. Are you concerned about performance, the awkwardness of calling getters/setters, or something else for qemu-user? Awkwardness first, performance second. I'll also note that coroutines never run on vcpu threads, only io threads. So I'll resist any use of these interfaces in TCG as well. Would it maybe make sense to tweak check_patch.pl to forbid __thread in certain folders only, e.g. block/ and util/ (i.e. where we know that there might be code that the iothreads are using)? Thomas
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 10/26/21 9:34 AM, Stefan Hajnoczi wrote: On Tue, Oct 26, 2021 at 08:10:16AM -0700, Richard Henderson wrote: On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: If "safe" TLS variables are opt-in then we'll likely have obscure bugs when code changes to access a TLS variable that was previously never accessed from a coroutine. There is no compiler error and no way to detect this. When it happens debugging it is painful. Co-routines are never used in user-only builds. If developers have the choice of using __thread then bugs can slip through. Huh? How. No, really. Are you concerned about performance, the awkwardness of calling getters/setters, or something else for qemu-user? Awkwardness first, performance second. I'll also note that coroutines never run on vcpu threads, only io threads. So I'll resist any use of these interfaces in TCG as well. r~
Re: [PATCH v4 6/8] iotests/300: avoid abnormal shutdown race condition
On Mon, Oct 25, 2021 at 9:20 AM Hanna Reitz wrote:
> On 13.10.21 23:57, John Snow wrote:
> > Wait for the destination VM to close itself instead of racing to shut it
> > down first, which produces different error log messages from AQMP
> > depending on precisely when we tried to shut it down.
> >
> > (For example: We may try to issue 'quit' immediately prior to the target
> > VM closing its QMP socket, which will cause an ECONNRESET error to be
> > logged. Waiting for the VM to exit itself avoids the race on shutdown
> > behavior.)
> >
> > Reported-by: Hanna Reitz
> > Signed-off-by: John Snow
> > ---
> > tests/qemu-iotests/300 | 12
> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> > index 10f9f2a8da6..bbea7248005 100755
> > --- a/tests/qemu-iotests/300
> > +++ b/tests/qemu-iotests/300
> > @@ -24,8 +24,6 @@ import random
> > import re
> > from typing import Dict, List, Optional
> >
> > -from qemu.machine import machine
> > -
> > import iotests
> >
> >
> > @@ -461,12 +459,10 @@ class
> TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
> > f"'{self.src_node_name}': Name is longer than
> 255 bytes",
> > log)
> >
> > -# Expect abnormal shutdown of the destination VM because of
> > -# the failed migration
> > -try:
> > -self.vm_b.shutdown()
> > -except machine.AbnormalShutdown:
> > -pass
> > +# Destination VM will terminate w/ error of its own accord
> > +# due to the failed migration.
> > +self.vm_b.wait()
> > +assert self.vm_b.exitcode() > 0
>
> Trying to test, I can see that this fails iotest 297, because
> `.exitcode()` is `Optional[int]`...
>
> (I can’t believe how long it took me to figure this out – the message
> “300:465: Unsupported operand types for < ("int" and "None")” made me
> believe that it was 300 that was failing, because `exitcode()` was
> returning `None` for some inconceivable reason. I couldn’t understand
> why my usual test setup failed on every run, but I couldn’t get 300 to
> fail manually... Until I noticed that the message came below the “297”
> line, not the “300” line...)
>
>
Oops. Is there anything we can do to improve the visual clarity there?
> Hanna
>
>
Embarrassing. I scrutinized the other series I sent out, but forgot to
apply the same tests to this one. :(
Fixed, sorry for the noise.
--js
Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
On Tue, Oct 26, 2021 at 3:56 AM Markus Armbruster wrote: > John Snow writes: > > > On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster > wrote: > > > >> Add special feature 'unstable' everywhere the name starts with 'x-', > >> except for InputBarrierProperties member x-origin and > >> MemoryBackendProperties member x-use-canonical-path-for-ramblock-id, > >> because these two are actually stable. > >> > >> Signed-off-by: Markus Armbruster > >> --- > >> qapi/block-core.json | 123 +++ > >> qapi/migration.json | 35 +--- > >> qapi/misc.json | 6 ++- > >> qapi/qom.json| 11 ++-- > >> 4 files changed, 130 insertions(+), 45 deletions(-) > >> > >> diff --git a/qapi/block-core.json b/qapi/block-core.json > >> index 6d3217abb6..ce2c1352cb 100644 > >> --- a/qapi/block-core.json > >> +++ b/qapi/block-core.json > >> @@ -1438,6 +1438,9 @@ > >> # > >> # @x-perf: Performance options. (Since 6.0) > >> # > >> +# Features: > >> +# @unstable: Member @x-perf is experimental. > >> +# > >> > > > > It'd be a lot cooler if we could annotate the unstable member directly > > instead of confusing it with the syntax that might describe the entire > > struct/union/command/etc, but ... eh, it's just a doc field, so I'm not > > gonna press on this. I don't have the energy to get into a doc formatting > > standard discussion right now, so: sure, why not? > > By design, we have a single doc comment block for the entire definition. > > When Kevin invented feature flags (merge commit 4747524f9f2), he added > them just to struct types. He added "feature sections" for documenting > features. It mirrors the "argument sections" for documenting members. > Makes sense. > > Aside: he neglected to update qapi-code-gen.rst section "Definition > documentation", and I failed to catch it. I'll make up for it. > > Peter and I then added feature flags to the remaining definitions > (commit 23394b4c39 and 013b4efc9b). Feature sections make sense there, > too. > > I then added them to struct members (commit 84ab008687). Instead of > doing something fancy for documenting feature flags of members, I simply > used the existing feature sections. This conflates member features with > struct features. > > Yeah, that's the part I don't like. If this isn't the first instance of breaking the seal, though, this is the wrong time for me to comment on it. Don't worry about it for this series. > What could "something fancy" be? All we have for members is "argument > sections", which are basically a name plus descriptive text. We'd have > to add structure to that, say nest feature sections into argument > sections. I have no appetite for that right now. > > (Tangent below, unrelated to acceptance of this series) Yeah, I don't have an appetite for it at the moment either. I'll have to read Marc-Andre's recent sphinx patches and see if I want to do more work -- I had a bigger prototype a few months back I didn't bring all the way home, but I am still thinking about reworking our QAPIDoc format. It's ... well. I don't really want to, but I am not sure how else to bring some of the features I want home, and I think I need some more time to think carefully through exactly what I want to do and why. I wouldn't mind chatting about it with you sometime soon -- there's a few things to balance here, such as: (1) Reworking the doc format would be an obnoxious amount of churn, ... (2) ...but the Sphinx internals are really not meant to be used the way we're using them right now, ... (3) ...but I also don't want to write our QAPI docstrings in a way that *targets* Sphinx. Not that I think we'll be dropping it any time soon, but it still feels like the wrong idea to tie them so closely together. I'm hoping there's an opportunity to make the parsing nicer with minimal changes to the parsing and format, though. It just might require enforcing a *pinch* more structure. I could see how I feel about per-field annotations at that point. I consider them interesting for things like the Python SDK where I may want to enable/disable warnings for using deprecated stuff at the client-level. (e.g., let's say you're using Python SDK 6.2 to talk to a 6.1 client. Nothing stops you from doing this, but some commands will simply be rejected by QEMU as it won't know what you're talking about. Using deprecated fields or commands to talk to an older client will produce no visible warning from QEMU either, as it wasn't deprecated at that point in time. Still, the client may wish to know that they're asking for future trouble -- so it's a thought that client-side warnings have some play here.) E don't worry about it for now, I guess I'll plow forward a bit with the Sphinx stuff I'm thinking of at some point Soon:tm: and worry about where the immovable objects are when I get there. This is foot-based landmine-detection, and it works 100% of the time. > > > > >> # Note: @on-source-error and
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On Tue, Oct 26, 2021 at 08:10:16AM -0700, Richard Henderson wrote: > On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: > > If "safe" TLS variables are opt-in then we'll likely have obscure bugs > > when code changes to access a TLS variable that was previously never > > accessed from a coroutine. There is no compiler error and no way to > > detect this. When it happens debugging it is painful. > > Co-routines are never used in user-only builds. If developers have the choice of using __thread then bugs can slip through. Your assembly get_addr() approach reduces the performance overhead of TLS getters/setters. Are you concerned about performance, the awkwardness of calling getters/setters, or something else for qemu-user? Stefan signature.asc Description: PGP signature
Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables
On Tue, Oct 26, 2021 at 08:32:11AM -0700, Richard Henderson wrote:
> On 10/26/21 6:30 AM, Stefan Hajnoczi wrote:
> > On Mon, Oct 25, 2021 at 10:19:04AM -0700, Richard Henderson wrote:
> > > On 10/25/21 7:07 AM, Stefan Hajnoczi wrote:
> > > > Compiler optimizations can cache TLS values across coroutine yield
> > > > points, resulting in stale values from the previous thread when a
> > > > coroutine is re-entered by a new thread.
> > > ...
> > > >include/qemu/tls.h | 142
> > > > +
> > >
> > > Better as qemu/coroutine-tls.h, since it is needed for no other purpose.
> > >
> > > > +#define QEMU_DEFINE_TLS(type, var) \
> > > > +__thread type qemu_tls_##var; \
> > > > +type get_##var(void) { return qemu_tls_##var; } \
> > > > +void set_##var(type v) { qemu_tls_##var = v; }
> > >
> > > You might as well make the variable static, since it may only be
> > > referenced
> > > by these two functions.
> >
> > Oops, that's a bug. It should be declared extern. QEMU_DEFINE_TLS() is
> > meant for use in header files.
>
> No, QEMU_DECLARE_TLS was for use in header files, and it of course does not
> globally declare the tls variable at all. Only the definition here requires
> the tls variable.
You are right, thanks for pointing this out.
>
> > Nice. That makes me wonder if it's possible to write a portable version:
> >
> >static inline TYPE get_##VAR(void) \
> >{ volatile TYPE *p = _tls_##VAR; return *p; }
> >
> > But the trouble is we need _tls_##VAR to be "volatile" and I don't
> > think there is a way to express that?
>
> No, there's not.
>
> Anyway, with those four hosts we get coverage of almost all users. I'll
> note that both arm32 and s390x use the constant pool in computing these tls
> addresses, so they'll need to keep using your functional version. So we'll
> still have testing of that path as well.
Okay, thanks!
Stefan
signature.asc
Description: PGP signature
Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables
On Tue, Oct 26, 2021 at 04:10:16PM +0200, Kevin Wolf wrote: > Am 26.10.2021 um 15:41 hat Stefan Hajnoczi geschrieben: > > Actually, nevermind what I said about the callback scenario. I don't > > think that is a problem because the compiler cannot assume the __thread > > variable remains unchanged across the callback. Therefore it cannot > > safely cache the value. > > And additionally, I think callbacks are never coroutine_fn (especially > not if they come from an external library), so they must not yield > anyway. There's a tiny chance that the third-party library was called from coroutine context and the callback invokes a non-coroutine_fn that uses qemu_in_coroutine() to dynamically decide it's possible to yield. But it seems very unlikely. > > So I think only the header file scenario is a problem. > > The mere existence of a __thread variable in the header file isn't a > problem either, but if QEMU accesses it, we would have to implement > wrappers similar to what you're proposing for QEMU's thread local > variables. There could be static inline functions that access it in the header file. If QEMU calls those functions then the compiler can optimize that. Stefan signature.asc Description: PGP signature
[PATCH v2 3/3] linux-aio: add `dev_max_batch` parameter to laio_io_unplug()
Between the submission of a request and the unplug, other devices
with larger limits may have been queued new requests without flushing
the batch.
Using the new `dev_max_batch` parameter, laio_io_unplug() can check
if the batch exceeds the device limit to flush the current batch.
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Kevin Wolf
Signed-off-by: Stefano Garzarella
---
include/block/raw-aio.h | 3 ++-
block/file-posix.c | 2 +-
block/linux-aio.c | 8 +---
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index ebd042fa27..21fc10c4c9 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -56,7 +56,8 @@ int coroutine_fn laio_co_submit(BlockDriverState *bs,
LinuxAioState *s, int fd,
void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s);
+void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
+uint64_t dev_max_batch);
#endif
/* io_uring.c - Linux io_uring implementation */
#ifdef CONFIG_LINUX_IO_URING
diff --git a/block/file-posix.c b/block/file-posix.c
index baf4197e88..966eab8d0b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2125,7 +2125,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
#ifdef CONFIG_LINUX_AIO
if (s->use_linux_aio) {
LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
-laio_io_unplug(bs, aio);
+laio_io_unplug(bs, aio, s->aio_max_batch);
}
#endif
#ifdef CONFIG_LINUX_IO_URING
diff --git a/block/linux-aio.c b/block/linux-aio.c
index 88b44fee72..f53ae72e21 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -356,11 +356,13 @@ void laio_io_plug(BlockDriverState *bs, LinuxAioState *s)
s->io_q.plugged++;
}
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s)
+void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
+uint64_t dev_max_batch)
{
assert(s->io_q.plugged);
-if (--s->io_q.plugged == 0 &&
-!s->io_q.blocked && !QSIMPLEQ_EMPTY(>io_q.pending)) {
+if (s->io_q.in_queue >= laio_max_batch(s, dev_max_batch) ||
+(--s->io_q.plugged == 0 &&
+ !s->io_q.blocked && !QSIMPLEQ_EMPTY(>io_q.pending))) {
ioq_submit(s);
}
}
--
2.31.1
[PATCH v2 1/3] file-posix: add `aio-max-batch` option
Commit d7ddd0a161 ("linux-aio: limit the batch size using
`aio-max-batch` parameter") added a way to limit the batch size
of Linux AIO backend for the entire AIO context.
The same AIO context can be shared by multiple devices, so
latency-sensitive devices may want to limit the batch size even
more to avoid increasing latency.
For this reason we add the `aio-max-batch` option to the file
backend, which will be used by the next commits to limit the size of
batches including requests generated by this device.
Suggested-by: Kevin Wolf
Reviewed-by: Kevin Wolf
Signed-off-by: Stefano Garzarella
---
Notes:
v2:
- @aio-max-batch documentation rewrite [Stefan, Kevin]
qapi/block-core.json | 7 +++
block/file-posix.c | 9 +
2 files changed, 16 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6d3217abb6..fef76b0ea2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2896,6 +2896,12 @@
# for this device (default: none, forward the commands via SG_IO;
# since 2.11)
# @aio: AIO backend (default: threads) (since: 2.8)
+# @aio-max-batch: maximum number of requests to batch together into a single
+# submission in the AIO backend. The smallest value between
+# this and the aio-max-batch value of the IOThread object is
+# chosen.
+# 0 means that the AIO backend will handle it automatically.
+# (default: 0, since 6.2)
# @locking: whether to enable file locking. If set to 'auto', only enable
# when Open File Descriptor (OFD) locking API is available
# (default: auto, since 2.10)
@@ -2924,6 +2930,7 @@
'*pr-manager': 'str',
'*locking': 'OnOffAuto',
'*aio': 'BlockdevAioOptions',
+'*aio-max-batch': 'int',
'*drop-cache': {'type': 'bool',
'if': 'CONFIG_LINUX'},
'*x-check-cache-dropped': 'bool' },
diff --git a/block/file-posix.c b/block/file-posix.c
index 53be0bdc1b..d655fd0c45 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -150,6 +150,8 @@ typedef struct BDRVRawState {
uint64_t locked_perm;
uint64_t locked_shared_perm;
+uint64_t aio_max_batch;
+
int perm_change_fd;
int perm_change_flags;
BDRVReopenState *reopen_state;
@@ -530,6 +532,11 @@ static QemuOptsList raw_runtime_opts = {
.type = QEMU_OPT_STRING,
.help = "host AIO implementation (threads, native, io_uring)",
},
+{
+.name = "aio-max-batch",
+.type = QEMU_OPT_NUMBER,
+.help = "AIO max batch size (0 = auto handled by AIO backend,
default: 0)",
+},
{
.name = "locking",
.type = QEMU_OPT_STRING,
@@ -609,6 +616,8 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
s->use_linux_io_uring = (aio == BLOCKDEV_AIO_OPTIONS_IO_URING);
#endif
+s->aio_max_batch = qemu_opt_get_number(opts, "aio-max-batch", 0);
+
locking = qapi_enum_parse(_lookup,
qemu_opt_get(opts, "locking"),
ON_OFF_AUTO_AUTO, _err);
--
2.31.1
[PATCH v2 2/3] linux-aio: add `dev_max_batch` parameter to laio_co_submit()
This new parameter can be used by block devices to limit the
Linux AIO batch size more than the limit set by the AIO context.
file-posix backend supports this, passing its `aio-max-batch` option
previously added.
Add an helper function to calculate the maximum batch size.
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Kevin Wolf
Signed-off-by: Stefano Garzarella
---
include/block/raw-aio.h | 3 ++-
block/file-posix.c | 3 ++-
block/linux-aio.c | 30 ++
3 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index 251b10d273..ebd042fa27 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -51,7 +51,8 @@ typedef struct LinuxAioState LinuxAioState;
LinuxAioState *laio_init(Error **errp);
void laio_cleanup(LinuxAioState *s);
int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
-uint64_t offset, QEMUIOVector *qiov, int type);
+uint64_t offset, QEMUIOVector *qiov, int type,
+uint64_t dev_max_batch);
void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);
diff --git a/block/file-posix.c b/block/file-posix.c
index d655fd0c45..baf4197e88 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2066,7 +2066,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs,
uint64_t offset,
} else if (s->use_linux_aio) {
LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
assert(qiov->size == bytes);
-return laio_co_submit(bs, aio, s->fd, offset, qiov, type);
+return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
+ s->aio_max_batch);
#endif
}
diff --git a/block/linux-aio.c b/block/linux-aio.c
index 0dab507b71..88b44fee72 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -334,6 +334,23 @@ static void ioq_submit(LinuxAioState *s)
}
}
+static uint64_t laio_max_batch(LinuxAioState *s, uint64_t dev_max_batch)
+{
+uint64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH;
+
+/*
+ * AIO context can be shared between multiple block devices, so
+ * `dev_max_batch` allows reducing the batch size for latency-sensitive
+ * devices.
+ */
+max_batch = MIN_NON_ZERO(dev_max_batch, max_batch);
+
+/* limit the batch with the number of available events */
+max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight, max_batch);
+
+return max_batch;
+}
+
void laio_io_plug(BlockDriverState *bs, LinuxAioState *s)
{
s->io_q.plugged++;
@@ -349,15 +366,11 @@ void laio_io_unplug(BlockDriverState *bs, LinuxAioState
*s)
}
static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset,
- int type)
+ int type, uint64_t dev_max_batch)
{
LinuxAioState *s = laiocb->ctx;
struct iocb *iocbs = >iocb;
QEMUIOVector *qiov = laiocb->qiov;
-int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH;
-
-/* limit the batch with the number of available events */
-max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight, max_batch);
switch (type) {
case QEMU_AIO_WRITE:
@@ -378,7 +391,7 @@ static int laio_do_submit(int fd, struct qemu_laiocb
*laiocb, off_t offset,
s->io_q.in_queue++;
if (!s->io_q.blocked &&
(!s->io_q.plugged ||
- s->io_q.in_queue >= max_batch)) {
+ s->io_q.in_queue >= laio_max_batch(s, dev_max_batch))) {
ioq_submit(s);
}
@@ -386,7 +399,8 @@ static int laio_do_submit(int fd, struct qemu_laiocb
*laiocb, off_t offset,
}
int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
-uint64_t offset, QEMUIOVector *qiov, int type)
+uint64_t offset, QEMUIOVector *qiov, int type,
+uint64_t dev_max_batch)
{
int ret;
struct qemu_laiocb laiocb = {
@@ -398,7 +412,7 @@ int coroutine_fn laio_co_submit(BlockDriverState *bs,
LinuxAioState *s, int fd,
.qiov = qiov,
};
-ret = laio_do_submit(fd, , offset, type);
+ret = laio_do_submit(fd, , offset, type, dev_max_batch);
if (ret < 0) {
return ret;
}
--
2.31.1
[PATCH v2 0/3] linux-aio: allow block devices to limit aio-max-batch
v1:
https://lore.kernel.org/qemu-devel/20210923143100.182979-1-sgarz...@redhat.com
v2:
- @aio-max-batch documentation rewrite [Stefan, Kevin]
- added Stefan and Kevin R-b tags
Commit d7ddd0a161 ("linux-aio: limit the batch size using
`aio-max-batch` parameter") added a way to limit the batch size
of Linux AIO backend for the entire AIO context.
The same AIO context can be shared by multiple devices, so
latency-sensitive devices may want to limit the batch size even
more to avoid increasing latency.
This series add the `aio-max-batch` option to the file backend,
and use it in laio_co_submit() and laio_io_unplug() to limit the
Linux AIO batch size more than the limit set by the AIO context.
Stefano Garzarella (3):
file-posix: add `aio-max-batch` option
linux-aio: add `dev_max_batch` parameter to laio_co_submit()
linux-aio: add `dev_max_batch` parameter to laio_io_unplug()
qapi/block-core.json| 7 +++
include/block/raw-aio.h | 6 --
block/file-posix.c | 14 --
block/linux-aio.c | 38 +++---
4 files changed, 50 insertions(+), 15 deletions(-)
--
2.31.1
Re: [PATCH 7/9] qapi: Generalize enum member policy checking
On Tue, Oct 26, 2021 at 5:43 AM Markus Armbruster wrote:
> John Snow writes:
>
> > On Mon, Oct 25, 2021 at 1:26 AM Markus Armbruster
> wrote:
> >
> >> The code to check enumeration value policy can see special feature
> >> flag 'deprecated' in QEnumLookup member flags[value]. I want to make
> >> feature flag 'unstable' visible there as well, so I can add policy for
> >> it.
> >>
> >> Instead of extending flags[], replace it by @special_features (a
> >> bitset of QapiSpecialFeature), because that's how special features get
> >> passed around elsewhere.
> >>
> >> Signed-off-by: Markus Armbruster
> >> ---
> >> include/qapi/util.h| 5 +
> >> qapi/qapi-visit-core.c | 3 ++-
> >> scripts/qapi/types.py | 22 --
> >> 3 files changed, 15 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/include/qapi/util.h b/include/qapi/util.h
> >> index 7a8d5c7d72..0cc98db9f9 100644
> >> --- a/include/qapi/util.h
> >> +++ b/include/qapi/util.h
> >> @@ -15,12 +15,9 @@ typedef enum {
> >> QAPI_DEPRECATED,
> >> } QapiSpecialFeature;
> >>
> >> -/* QEnumLookup flags */
> >> -#define QAPI_ENUM_DEPRECATED 1
> >> -
> >> typedef struct QEnumLookup {
> >> const char *const *array;
> >> -const unsigned char *const flags;
> >> +const unsigned char *const special_features;
> >> const int size;
> >> } QEnumLookup;
> >>
> >> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> >> index b4a81f1757..5572d90efb 100644
> >> --- a/qapi/qapi-visit-core.c
> >> +++ b/qapi/qapi-visit-core.c
> >> @@ -407,7 +407,8 @@ static bool input_type_enum(Visitor *v, const char
> >> *name, int *obj,
> >> return false;
> >> }
> >>
> >> -if (lookup->flags && (lookup->flags[value] &
> QAPI_ENUM_DEPRECATED)) {
> >> +if (lookup->special_features
> >> +&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
> >> switch (v->compat_policy.deprecated_input) {
> >> case COMPAT_POLICY_INPUT_ACCEPT:
> >> break;
> >> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> >> index ab2441adc9..3013329c24 100644
> >> --- a/scripts/qapi/types.py
> >> +++ b/scripts/qapi/types.py
> >> @@ -16,7 +16,7 @@
> >> from typing import List, Optional
> >>
> >> from .common import c_enum_const, c_name, mcgen
> >> -from .gen import QAPISchemaModularCVisitor, ifcontext
> >> +from .gen import QAPISchemaModularCVisitor, gen_special_features,
> >> ifcontext
> >> from .schema import (
> >> QAPISchema,
> >> QAPISchemaEnumMember,
> >> @@ -39,7 +39,7 @@ def gen_enum_lookup(name: str,
> >> members: List[QAPISchemaEnumMember],
> >> prefix: Optional[str] = None) -> str:
> >> max_index = c_enum_const(name, '_MAX', prefix)
> >> -flags = ''
> >> +feats = ''
> >> ret = mcgen('''
> >>
> >> const QEnumLookup %(c_name)s_lookup = {
> >> @@ -54,19 +54,21 @@ def gen_enum_lookup(name: str,
> >> ''',
> >> index=index, name=memb.name)
> >> ret += memb.ifcond.gen_endif()
> >> -if 'deprecated' in (f.name for f in memb.features):
> >> -flags += mcgen('''
> >> -[%(index)s] = QAPI_ENUM_DEPRECATED,
> >> -''',
> >> - index=index)
> >>
> >> -if flags:
> >> +special_features = gen_special_features(memb.features)
> >> +if special_features != '0':
> >>
> >
> > Though, I have to admit the common reoccurrence of this pattern suggests
> to
> > me that gen_special_features really ought to be returning something
> false-y
> > in these cases. Something about testing for the empty case with something
> > that represents, but isn't empty, gives me a brief pause.
> >
> > Not willing to wage war over it.
>
> I habitually start stupid, and when stupid confuses or annoys me later,
> I smarten it up some.
>
> Let's see how this instance of "stupid" ages, okay?
>
>
"I see what you're saying, but meh" is a relatable feeling ;)
>
> >> +feats += mcgen('''
> >> +[%(index)s] = %(special_features)s,
> >> +''',
> >> + index=index,
> special_features=special_features)
> >> +
> >> +if feats:
> >> ret += mcgen('''
> >> },
> >> -.flags = (const unsigned char[%(max_index)s]) {
> >> +.special_features = (const unsigned char[%(max_index)s]) {
> >> ''',
> >> max_index=max_index)
> >> -ret += flags
> >> +ret += feats
> >>
> >> ret += mcgen('''
> >> },
> >> --
> >> 2.31.1
> >>
> >>
> > Python bits: Acked-by: John Snow
>
> Thanks!
>
>
Re: [PATCH 5/9] qapi: Generalize struct member policy checking
On 10/25/21 07:25, Markus Armbruster wrote:
> The generated visitor functions call visit_deprecated_accept() and
> visit_deprecated() when visiting a struct member with special feature
> flag 'deprecated'. This makes the feature flag visible to the actual
> visitors. I want to make feature flag 'unstable' visible there as
> well, so I can add policy for it.
>
> To let me make it visible, replace these functions by
> visit_policy_reject() and visit_policy_skip(), which take the member's
> special features as an argument. Note that the new functions have the
> opposite sense, i.e. the return value flips.
>
> Signed-off-by: Markus Armbruster
> ---
> include/qapi/visitor-impl.h | 6 --
> include/qapi/visitor.h| 17 +
> qapi/qapi-forward-visitor.c | 16 +---
> qapi/qapi-visit-core.c| 22 --
> qapi/qobject-input-visitor.c | 15 ++-
> qapi/qobject-output-visitor.c | 9 ++---
> qapi/trace-events | 4 ++--
> scripts/qapi/visit.py | 14 +++---
> 8 files changed, 63 insertions(+), 40 deletions(-)
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 71b24a4429..fda485614b 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -662,16 +662,21 @@ static void qobject_input_optional(Visitor *v, const
> char *name, bool *present)
> *present = true;
> }
>
> -static bool qobject_input_deprecated_accept(Visitor *v, const char *name,
> -Error **errp)
> +static bool qobject_input_policy_reject(Visitor *v, const char *name,
> +unsigned special_features,
> +Error **errp)
> {
> +if (!(special_features && 1u << QAPI_DEPRECATED)) {
Unreachable =) Proof than extract() is safer :P
> +return false;
> +}
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
On Tue, Oct 26, 2021 at 05:15:10PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé writes: > > > On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote: > >> Kevin Wolf writes: > >> > >> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: > >> >> By convention, names starting with "x-" are experimental. The parts > >> >> of external interfaces so named may be withdrawn or changed > >> >> incompatibly in future releases. > >> >> > >> >> Drawback: promoting something from experimental to stable involves a > >> >> name change. Client code needs to be updated. > >> >> > >> >> Moreover, the convention is not universally observed: > >> >> > >> >> * QOM type "input-barrier" has properties "x-origin", "y-origin". > >> >> Looks accidental, but it's ABI since 4.2. > >> >> > >> >> * QOM types "memory-backend-file", "memory-backend-memfd", > >> >> "memory-backend-ram", and "memory-backend-epc" have a property > >> >> "x-use-canonical-path-for-ramblock-id" that is documented to be > >> >> stable despite its name. > >> >> > >> >> We could document these exceptions, but documentation helps only > >> >> humans. We want to recognize "unstable" in code, like "deprecated". > >> >> > >> >> Replace the convention by a new special feature flag "unstable". It > >> >> will be recognized by the QAPI generator, like the existing feature > >> >> flag "deprecated", and unlike regular feature flags. > >> >> > >> >> This commit updates documentation and prepares tests. The next commit > >> >> updates the QAPI schema. The remaining patches update the QAPI > >> >> generator and wire up -compat policy checking. > >> >> > >> >> Signed-off-by: Markus Armbruster > >> > > >> > Obviously, replacing the old convention gets rid of the old drawbacks, > >> > but adds a new one: While using x- makes it very obvious for a human > >> > user that this is an unstable feature, a feature flag in the schema will > >> > almost certainly go unnoticed in manual use. > >> > >> I thought about this, but neglected to put it in writing. My bad. > >> > >> Manual use of unstable interfaces is mostly fine. Human users can adapt > >> to changing interfaces. HMP works that way. > >> > >> Management applications are better off with a feature flag than with a > >> naming convention we sometimes ignore. > > > > We will sometimes ignore/forget the feature flag too though, so I'm > > not convinced there's much difference there. > > -compat unstable-input=reject,unstable-output=hide should help you stay > on the straight & narrow :) That's from the pov of the mgmt app. I meant from the POV of QEMU maintainers forgetting to add "unstable" flag, just as they might forget to add a "x-" prefix. 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 1/2] tls: add macros for coroutine-safe TLS variables
On 10/26/21 6:30 AM, Stefan Hajnoczi wrote:
On Mon, Oct 25, 2021 at 10:19:04AM -0700, Richard Henderson wrote:
On 10/25/21 7:07 AM, Stefan Hajnoczi wrote:
Compiler optimizations can cache TLS values across coroutine yield
points, resulting in stale values from the previous thread when a
coroutine is re-entered by a new thread.
...
include/qemu/tls.h | 142 +
Better as qemu/coroutine-tls.h, since it is needed for no other purpose.
+#define QEMU_DEFINE_TLS(type, var) \
+__thread type qemu_tls_##var; \
+type get_##var(void) { return qemu_tls_##var; } \
+void set_##var(type v) { qemu_tls_##var = v; }
You might as well make the variable static, since it may only be referenced
by these two functions.
Oops, that's a bug. It should be declared extern. QEMU_DEFINE_TLS() is
meant for use in header files.
No, QEMU_DECLARE_TLS was for use in header files, and it of course does not globally
declare the tls variable at all. Only the definition here requires the tls variable.
Nice. That makes me wonder if it's possible to write a portable version:
static inline TYPE get_##VAR(void) \
{ volatile TYPE *p = _tls_##VAR; return *p; }
But the trouble is we need _tls_##VAR to be "volatile" and I don't
think there is a way to express that?
No, there's not.
Anyway, with those four hosts we get coverage of almost all users. I'll note that both
arm32 and s390x use the constant pool in computing these tls addresses, so they'll need to
keep using your functional version. So we'll still have testing of that path as well.
r~
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
Daniel P. Berrangé writes: > On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote: >> Kevin Wolf writes: >> >> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: >> >> By convention, names starting with "x-" are experimental. The parts >> >> of external interfaces so named may be withdrawn or changed >> >> incompatibly in future releases. >> >> >> >> Drawback: promoting something from experimental to stable involves a >> >> name change. Client code needs to be updated. >> >> >> >> Moreover, the convention is not universally observed: >> >> >> >> * QOM type "input-barrier" has properties "x-origin", "y-origin". >> >> Looks accidental, but it's ABI since 4.2. >> >> >> >> * QOM types "memory-backend-file", "memory-backend-memfd", >> >> "memory-backend-ram", and "memory-backend-epc" have a property >> >> "x-use-canonical-path-for-ramblock-id" that is documented to be >> >> stable despite its name. >> >> >> >> We could document these exceptions, but documentation helps only >> >> humans. We want to recognize "unstable" in code, like "deprecated". >> >> >> >> Replace the convention by a new special feature flag "unstable". It >> >> will be recognized by the QAPI generator, like the existing feature >> >> flag "deprecated", and unlike regular feature flags. >> >> >> >> This commit updates documentation and prepares tests. The next commit >> >> updates the QAPI schema. The remaining patches update the QAPI >> >> generator and wire up -compat policy checking. >> >> >> >> Signed-off-by: Markus Armbruster >> > >> > Obviously, replacing the old convention gets rid of the old drawbacks, >> > but adds a new one: While using x- makes it very obvious for a human >> > user that this is an unstable feature, a feature flag in the schema will >> > almost certainly go unnoticed in manual use. >> >> I thought about this, but neglected to put it in writing. My bad. >> >> Manual use of unstable interfaces is mostly fine. Human users can adapt >> to changing interfaces. HMP works that way. >> >> Management applications are better off with a feature flag than with a >> naming convention we sometimes ignore. > > We will sometimes ignore/forget the feature flag too though, so I'm > not convinced there's much difference there. -compat unstable-input=reject,unstable-output=hide should help you stay on the straight & narrow :) >> If we want to keep "unstable" obvious to the humans who write such >> programs, we can continue to require "x-", in addition to the feature >> flag. We pay for it with renames, and the risk of forgetting to rename >> in time (which is what got us the awkward stable >> "x-use-canonical-path-for-ramblock-id"). Tradeoff. I chose not to, but >> if y'all think we should... > > IMHO the renames arguably make life easier for mgmt apps, as they > only need to check for the name without the x- prefix, and they > know they won't be accidentally using the fature from an older > QEMU where it was unstable because the older QEMU had a different > name they won't be checking for. > > We can just as easily forget to remove the "unstable" feature > flag, as forget to rename. > > The plus point about the feature flag is that it is aligned with > the "deprecated" feature flag, and potentially aligned with a > future "insecure" feature flag. Yup.
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On 10/26/21 6:22 AM, Stefan Hajnoczi wrote: If "safe" TLS variables are opt-in then we'll likely have obscure bugs when code changes to access a TLS variable that was previously never accessed from a coroutine. There is no compiler error and no way to detect this. When it happens debugging it is painful. Co-routines are never used in user-only builds. r~
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote: > Kevin Wolf writes: > > > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: > >> By convention, names starting with "x-" are experimental. The parts > >> of external interfaces so named may be withdrawn or changed > >> incompatibly in future releases. > >> > >> Drawback: promoting something from experimental to stable involves a > >> name change. Client code needs to be updated. > >> > >> Moreover, the convention is not universally observed: > >> > >> * QOM type "input-barrier" has properties "x-origin", "y-origin". > >> Looks accidental, but it's ABI since 4.2. > >> > >> * QOM types "memory-backend-file", "memory-backend-memfd", > >> "memory-backend-ram", and "memory-backend-epc" have a property > >> "x-use-canonical-path-for-ramblock-id" that is documented to be > >> stable despite its name. > >> > >> We could document these exceptions, but documentation helps only > >> humans. We want to recognize "unstable" in code, like "deprecated". > >> > >> Replace the convention by a new special feature flag "unstable". It > >> will be recognized by the QAPI generator, like the existing feature > >> flag "deprecated", and unlike regular feature flags. > >> > >> This commit updates documentation and prepares tests. The next commit > >> updates the QAPI schema. The remaining patches update the QAPI > >> generator and wire up -compat policy checking. > >> > >> Signed-off-by: Markus Armbruster > > > > Obviously, replacing the old convention gets rid of the old drawbacks, > > but adds a new one: While using x- makes it very obvious for a human > > user that this is an unstable feature, a feature flag in the schema will > > almost certainly go unnoticed in manual use. > > I thought about this, but neglected to put it in writing. My bad. > > Manual use of unstable interfaces is mostly fine. Human users can adapt > to changing interfaces. HMP works that way. > > Management applications are better off with a feature flag than with a > naming convention we sometimes ignore. We will sometimes ignore/forget the feature flag too though, so I'm not convinced there's much difference there. > If we want to keep "unstable" obvious to the humans who write such > programs, we can continue to require "x-", in addition to the feature > flag. We pay for it with renames, and the risk of forgetting to rename > in time (which is what got us the awkward stable > "x-use-canonical-path-for-ramblock-id"). Tradeoff. I chose not to, but > if y'all think we should... IMHO the renames arguably make life easier for mgmt apps, as they only need to check for the name without the x- prefix, and they know they won't be accidentally using the fature from an older QEMU where it was unstable because the older QEMU had a different name they won't be checking for. We can just as easily forget to remove the "unstable" feature flag, as forget to rename. The plus point about the feature flag is that it is aligned with the "deprecated" feature flag, and potentially aligned with a future "insecure" feature flag. 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 v5 0/6] block/rbd: migrate to coroutines and add write zeroes support
Am 25.10.21 um 14:58 schrieb Kevin Wolf:
> Am 25.10.2021 um 13:39 hat Peter Lieven geschrieben:
>> Am 16.09.21 um 14:34 schrieb Peter Lieven:
>>> Am 09.07.21 um 12:21 schrieb Kevin Wolf:
Am 08.07.2021 um 20:23 hat Peter Lieven geschrieben:
> Am 08.07.2021 um 14:18 schrieb Kevin Wolf :
>> Am 07.07.2021 um 20:13 hat Peter Lieven geschrieben:
Am 06.07.2021 um 17:25 schrieb Kevin Wolf :
Am 06.07.2021 um 16:55 hat Peter Lieven geschrieben:
> I will have a decent look after my vacation.
Sounds good, thanks. Enjoy your vacation!
>>> As I had to fire up my laptop to look into another issue anyway, I
>>> have sent two patches for updating MAINTAINERS and to fix the int vs.
>>> bool mix for task->complete.
>> I think you need to reevaluate your definition of vacation. ;-)
> Lets talk about this when the kids are grown up. Sometimes sending
> patches can be quite relaxing :-)
Heh, fair enough. :-)
>> But thanks anyway.
>>
>>> As Paolos fix (5f50be9b5) is relatively new and there are maybe other
>>> non obvious problems when removing the BH indirection and we are close
>>> to soft freeze I would leave the BH removal change for 6.2.
>> Sure, code cleanups aren't urgent.
> Isn’t the indirection also a slight performance drop?
Yeah, I guess technically it is, though I doubt it's measurable.
>>>
>>> As promised I was trying to remove the indirection through the BH after
>>> Qemu 6.1 release.
>>>
>>> However, if I remove the BH I run into the following assertion while
>>> running some fio tests:
>>>
>>>
>>> qemu-system-x86_64: ../block/block-backend.c:1197: blk_wait_while_drained:
>>> Assertion `blk->in_flight > 0' failed.
>>>
>>>
>>> Any idea?
>>>
>>>
>>> This is what I changed:
>>>
>>>
>>> diff --git a/block/rbd.c b/block/rbd.c
>>> index 3cb24f9981..bc1dbc20f7 100644
>>> --- a/block/rbd.c
>>> +++ b/block/rbd.c
>>> @@ -1063,13 +1063,6 @@ static int qemu_rbd_resize(BlockDriverState *bs,
>>> uint64_t size)
>>> return 0;
>>> }
>>>
>>> -static void qemu_rbd_finish_bh(void *opaque)
>>> -{
>>> - RBDTask *task = opaque;
>>> - task->complete = true;
>>> - aio_co_wake(task->co);
>>> -}
>>> -
>>> /*
>>> * This is the completion callback function for all rbd aio calls
>>> * started from qemu_rbd_start_co().
>>> @@ -1083,8 +1076,8 @@ static void qemu_rbd_completion_cb(rbd_completion_t
>>> c, RBDTask *task)
>>> {
>>> task->ret = rbd_aio_get_return_value(c);
>>> rbd_aio_release(c);
>>> - aio_bh_schedule_oneshot(bdrv_get_aio_context(task->bs),
>>> - qemu_rbd_finish_bh, task);
>>> + task->complete = true;
>>> + aio_co_wake(task->co);
>>> }
>> Kevin, Paolo, any idea?
> Not really, I don't see the connection between both places.
>
> Do you have a stack trace for the crash?
The crash seems not to be limited to that assertion. I have also seen:
qemu-system-x86_64: ../block/block-backend.c:1497: blk_aio_write_entry:
Assertion `!qiov || qiov->size == acb->bytes' failed.
Altough harder to trigger I catch this backtrace in gdb:
qemu-system-x86_64: ../block/block-backend.c:1497: blk_aio_write_entry:
Assertion `!qiov || qiov->size == acb->bytes' failed.
[Wechseln zu Thread 0x77fa8f40 (LWP 17852)]
Thread 1 "qemu-system-x86" hit Breakpoint 1, __GI_abort () at abort.c:49
49 abort.c: Datei oder Verzeichnis nicht gefunden.
(gdb) bt
#0 0x742567e0 in __GI_abort () at abort.c:49
#1 0x7424648a in __assert_fail_base (fmt=0x743cd750 "%s%s%s:%u:
%s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x55e638e0
"!qiov || qiov->size == acb->bytes", file=file@entry=0x55e634b2
"../block/block-backend.c", line=line@entry=1497,
function=function@entry=0x55e63b20 <__PRETTY_FUNCTION__.32450>
"blk_aio_write_entry") at assert.c:92
#2 0x74246502 in __GI___assert_fail
(assertion=assertion@entry=0x55e638e0 "!qiov || qiov->size == acb->bytes",
file=file@entry=0x55e634b2 "../block/block-backend.c",
line=line@entry=1497, function=function@entry=0x55e63b20
<__PRETTY_FUNCTION__.32450> "blk_aio_write_entry") at assert.c:101
#3 0x55becc78 in blk_aio_write_entry (opaque=0x56b534f0) at
../block/block-backend.c:1497
#4 0x55cf0e4c in coroutine_trampoline (i0=,
i1=) at ../util/coroutine-ucontext.c:173
#5 0x7426e7b0 in __start_context () at /lib/x86_64-linux-gnu/libc.so.6
#6 0x7fffd5a0 in ()
#7 0x in ()
(gdb) thread apply all bt full
Thread 27 (Thread 0x7fffb89ff700 (LWP 17884)):
#0 0x74614ad3 in futex_wait_cancelable (private=,
expected=0, futex_word=0x571d9dec) at
../sysdeps/unix/sysv/linux/futex-internal.h:88
__ret = -512
oldtype = 0
err =
spin = 0
buffer = {__routine = 0x74614770 <__condvar_cleanup_waiting>, __arg
= 0x7fffb89eb1d0, __canceltype = 0, __prev = 0x0}
cbuffer = {wseq =
Re: [PATCH 01/15] pcie: Set default and supported MaxReadReq to 512
On Thu, Oct 07, 2021 at 06:12:41PM -0400, Michael S. Tsirkin wrote:
> On Thu, Oct 07, 2021 at 06:23:52PM +0200, Lukasz Maniak wrote:
> > From: Knut Omang
> >
> > Make the default PCI Express Capability for PCIe devices set
> > MaxReadReq to 512.
>
> code says 256
>
> > Tyipcal modern devices people would want to
>
>
> typo
>
> > emulate or simulate would want this. The previous value would
> > cause warnings from the root port driver on some kernels.
>
>
> which specifically?
>
> >
> > Signed-off-by: Knut Omang
>
> we can't make changes like this unconditionally, this will
> break migration across versions.
> Pls tie this to a machine version.
>
> Thanks!
> > ---
> > hw/pci/pcie.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 6e95d82903..c1a12f3744 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -62,8 +62,9 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t
> > type, uint8_t version)
> > * Functions conforming to the ECN, PCI Express Base
> > * Specification, Revision 1.1., or subsequent PCI Express Base
> > * Specification revisions.
> > + * + set max payload size to 256, which seems to be a common value
> > */
> > -pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER);
> > +pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER | (0x1 &
> > PCI_EXP_DEVCAP_PAYLOAD));
> >
> > pci_set_long(exp_cap + PCI_EXP_LNKCAP,
> > (port << PCI_EXP_LNKCAP_PN_SHIFT) |
> > @@ -179,6 +180,8 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
> > pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
> > PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
> >
> > +pci_set_word(exp_cap + PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_READRQ_256B);
> > +
> > pci_set_word(dev->wmask + pos + PCI_EXP_DEVCTL2,
> > PCI_EXP_DEVCTL2_EETLPPB);
> >
> > if (dev->cap_present & QEMU_PCIE_EXTCAP_INIT) {
> > --
> > 2.25.1
>
Hi Michael,
The reason Knut keeps rebasing this fix along with SR-IOV patch is not
clear for us.
Since we have tested the NVMe device without this fix and did not notice
any issues mentioned by Knut on kernel 5.4.0, we decided to drop it for
v2.
However, I have posted your comments to this patch on Knut's github so
they can be addressed in case Knut decides to resubmit it later though.
Thanks,
Lukasz
Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables
Am 26.10.2021 um 15:41 hat Stefan Hajnoczi geschrieben: > Actually, nevermind what I said about the callback scenario. I don't > think that is a problem because the compiler cannot assume the __thread > variable remains unchanged across the callback. Therefore it cannot > safely cache the value. And additionally, I think callbacks are never coroutine_fn (especially not if they come from an external library), so they must not yield anyway. > So I think only the header file scenario is a problem. The mere existence of a __thread variable in the header file isn't a problem either, but if QEMU accesses it, we would have to implement wrappers similar to what you're proposing for QEMU's thread local variables. Kevin signature.asc Description: PGP signature
Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables
Actually, nevermind what I said about the callback scenario. I don't think that is a problem because the compiler cannot assume the __thread variable remains unchanged across the callback. Therefore it cannot safely cache the value. So I think only the header file scenario is a problem. Stefan signature.asc Description: PGP signature
Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables
On Mon, Oct 25, 2021 at 03:14:36PM +0100, Daniel P. Berrangé wrote: > On Mon, Oct 25, 2021 at 03:07:15PM +0100, Stefan Hajnoczi wrote: > > Compiler optimizations can cache TLS values across coroutine yield > > points, resulting in stale values from the previous thread when a > > coroutine is re-entered by a new thread. > > > > Serge Guelton developed an __attribute__((noinline)) wrapper and tested > > it with clang and gcc. I formatted his idea according to QEMU's coding > > style and wrote documentation. > > > > These macros must be used instead of __thread from now on to prevent > > coroutine TLS bugs. > > Does this apply to all __thread usage in the QEMU process that can > be used from coroutine context, or just certain __thread usage ? > > Mostly I'm wondering if this is going to have implications on external > libraries we use. eg if block layer is using librbd.so APIs, is librbd.sp > safe to use __thread directly in any way it desires ? There is a theoretical problem but I'm not sure if it will occur in practice: If a __thread variable is in an extern function then there's little risk of the value being cached. The function executes and returns back to QEMU, never yielding. The only case I can think of is when the library accesses a __thread variable, invokes a callback into QEMU, and that callback yields. If the coroutine is re-entered from another thread and returns back to the library, then we have a problem. This seems like a very rare case though. It gets trickier if the library has the __thread variable in a header file, because then the compiler may optimize it into a QEMU coroutine function and cache its value. Stefan signature.asc Description: PGP signature
Re: [RFC 1/2] tls: add macros for coroutine-safe TLS variables
On Mon, Oct 25, 2021 at 10:19:04AM -0700, Richard Henderson wrote:
> On 10/25/21 7:07 AM, Stefan Hajnoczi wrote:
> > Compiler optimizations can cache TLS values across coroutine yield
> > points, resulting in stale values from the previous thread when a
> > coroutine is re-entered by a new thread.
> ...
> > include/qemu/tls.h | 142 +
>
> Better as qemu/coroutine-tls.h, since it is needed for no other purpose.
>
> > +#define QEMU_DEFINE_TLS(type, var) \
> > +__thread type qemu_tls_##var; \
> > +type get_##var(void) { return qemu_tls_##var; } \
> > +void set_##var(type v) { qemu_tls_##var = v; }
>
> You might as well make the variable static, since it may only be referenced
> by these two functions.
Oops, that's a bug. It should be declared extern. QEMU_DEFINE_TLS() is
meant for use in header files.
>
> > +#define QEMU_DEFINE_STATIC_TLS(type, var) \
> > +static __thread type qemu_tls_##var; \
> > +static __attribute__((noinline)) type get_##var(void); \
> > +static type get_##var(void) { return qemu_tls_##var; } \
> > +static __attribute__((noinline)) void set_##var(type v); \
> > +static void set_##var(type v) { qemu_tls_##var = v; }
>
> You don't need separate function declarations; you can fold them together.
>
> If would be nice to inline this when possible,
>
> #if defined(__aarch64__)
> #define QEMU_COROUTINE_TLS_ADDR(RET, VAR) \
> asm volatile("mrs %0, tpidr_el0\n\t"\
> "add %0, %0, #:tprel_hi12:"#VAR", lsl #12\n\t" \
> "add %0, %0, #:tprel_lo12_nc:"#VAR \
> : "=r"(RET))
> #elif defined(__powerpc64__)
> #define QEMU_COROUTINE_TLS_ADDR(RET, VAR) \
> asm volatile("addis %0,13,"#VAR"@tprel@ha\n\t" \
> "add %0,%0,"#VAR"@tprel@l" \
> : "=r"(RET))
> #elif defined(__riscv)
> #define QEMU_COROUTINE_TLS_ADDR(RET, VAR) \
> asm volatile("lui %0,%%tprel_hi("#VAR")\n\t" \
> "add %0,%0,%%tprel_add("#VAR")\n\t" \
> "addi %0,%0,%%tprel_lo("#VAR")"\
> : "=r"(RET))
> #elif defined(__x86_64__)
> #define QEMU_COROUTINE_TLS_ADDR(RET, VAR) \
> asm volatile("mov %%fs:"#VAR"@tpoff, %0" : "=r"(RET))
> #endif
>
> #ifdef QEMU_COROUTINE_TLS_ADDR
> #define QEMU_COROUTINE_TLS_DECLARE(TYPE, VAR) \
> extern __thread TYPE co_tls_##VAR; \
> static inline TYPE get_##VAR(void) \
> { TYPE *p; QEMU_COROUTINE_TLS_ADDR(p, co_tls_##VAR); return *p; } \
> static inline void set_##VAR(TYPE v)\
> { TYPE *p; QEMU_COROUTINE_TLS_ADDR(p, co_tls_##VAR); *p = v; }
> #else
> etc
> #endif
Nice. That makes me wonder if it's possible to write a portable version:
static inline TYPE get_##VAR(void) \
{ volatile TYPE *p = _tls_##VAR; return *p; }
But the trouble is we need _tls_##VAR to be "volatile" and I don't
think there is a way to express that?
Stefan
signature.asc
Description: PGP signature
Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables
On Mon, Oct 25, 2021 at 05:27:29PM -0600, Warner Losh wrote: > On Mon, Oct 25, 2021 at 10:18 AM Richard Henderson < > richard.hender...@linaro.org> wrote: > > > On 10/25/21 7:07 AM, Stefan Hajnoczi wrote: > > > This is a preview of how we can solve the coroutines TLS problem. > > Coroutines > > > re-entered from another thread sometimes see stale TLS values. This > > happens > > > because compilers may cache values across yield points, so a value from > > the > > > previous thread will be used when the coroutine is re-entered in another > > > thread. > > > > I'm not thrilled by this, but I guess it does work. > > > > It could be worthwhile to add some inline asm instead for specific hosts > > -- one > > instruction instead of an out-of-line call. > > > > > > > Serge Guelton developed this technique, see the first patch for details. > > I'm > > > submitting it for discussion before I go ahead with a full conversion of > > the > > > source tree. > > > > > > Todo: > > > - Convert all uses of __thread > > > - Extend checkpatch.pl to reject code that uses __thread > > > > Absolutely not. *Perhaps* one or two tls variables which are accessible > > by coroutines, > > but there are plenty that have absolutely no relation. Especially > > everything related to > > user-only execution. > > > > I had the same worry. I'd also worry that the hoops that are jumped through > for > coroutines would somehow conflict with the low-level user-only execution > environment. I mean, it should be fine, but I know I'd be cranky if I traced > obscure regressions to being forced to use this construct... I have the opposite worry: If "safe" TLS variables are opt-in then we'll likely have obscure bugs when code changes to access a TLS variable that was previously never accessed from a coroutine. There is no compiler error and no way to detect this. When it happens debugging it is painful. That's why I think all __thread variables should be converted. Stefan signature.asc Description: PGP signature
Re: [RFC PATCH: v4 1/2] add mi device in qemu
This patch addresses most of the review comments raised by Klaus.
Mainly, I have ensured that the emulated mi device in qemu posts
the response rather than waiting for the guest-os(mi utility) to ask
for the response. For the same purpose, I have added a new device called
nvme-mi-slave which acts as an i2c slave and to which the emulated
mi device posts the response. The guest-os(mi utility) reads response
from this slave. The nvme-mi-slave has to be used in tandem with nvme-mi device.
In addition to the above change, we will be providing the mi utility
on the guest as a standalone rather than as a plugin to the
nvme-cli application.
We will be glad to hear any suggestions, corrections in the approach
we have used.
Please find the patch below:
diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
index 3cf4004..8768ca1 100644
--- a/hw/nvme/meson.build
+++ b/hw/nvme/meson.build
@@ -1 +1 @@
-softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c',
'dif.c', 'ns.c', 'subsys.c'))
+softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c',
'dif.c', 'ns.c', 'subsys.c', 'nvme-mi.c', 'nvme-mi-slave.c'))
diff --git a/hw/nvme/nvme-mi-slave.c b/hw/nvme/nvme-mi-slave.c
new file mode 100644
index 000..e6ada07
--- /dev/null
+++ b/hw/nvme/nvme-mi-slave.c
@@ -0,0 +1,93 @@
+/*
+ * QEMU NVMe-MI Controller
+ *
+ * Copyright (c) 2021, Samsung Electronics co Ltd.
+ *
+ * Written by Padmakar Kalghatgi
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * This module acts as a host slave, to which the QEMU-MI module
+ * will post the response to.
+ *
+ * Need to use as following to enable this device
+ * -device nvme-mi-i2c-slave, addr=
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-core.h"
+#include "hw/block/block.h"
+#include "nvme-mi-slave.h"
+
+static uint8_t nvme_mi_slave_i2c_recv(I2CSlave *s)
+{
+Nvmemislave *mislave = (Nvmemislave *)s;
+
+if (mislave->syncflag == true) {
+return -1;
+}
+return mislave->recvbuffer[mislave->recvlen++];
+}
+
+static int nvme_mi_slave_i2c_send(I2CSlave *s, uint8_t data)
+{
+Nvmemislave *mislave = (Nvmemislave *)s;
+mislave->syncflag = true;
+
+switch (mislave->pktpos) {
+case NVME_MI_BYTE_LENGTH_POS:
+mislave->pktlen = data;
+break;
+case NVME_MI_EOM_POS:
+mislave->eom = (data >> 6) & 1;
+break;
+}
+mislave->recvbuffer[mislave->sendlen++] = data;
+mislave->pktpos++;
+if (mislave->pktpos == mislave->pktlen + 3) {
+mislave->pktlen = 0;
+mislave->pktpos = 0;
+
+if (mislave->eom == 1) {
+mislave->sendlen = 0;
+mislave->recvlen = 0;
+mislave->eom = 0;
+mislave->syncflag = false;
+}
+}
+return 0;
+}
+
+static void nvme_mi_slave_realize(DeviceState *dev, Error **errp)
+{
+Nvmemislave *mislave = (Nvmemislave *)dev;
+mislave->sendlen = 0;
+mislave->recvlen = 0;
+mislave->eom = 0;
+mislave->syncflag = false;
+}
+
+static void nvme_mi_slave_class_init(ObjectClass *oc, void *data)
+{
+I2CSlaveClass *k = I2C_SLAVE_CLASS(oc);
+DeviceClass *dc = DEVICE_CLASS(oc);
+
+dc->realize = nvme_mi_slave_realize;
+k->recv = nvme_mi_slave_i2c_recv;
+k->send = nvme_mi_slave_i2c_send;
+}
+
+static const TypeInfo nvme_mi_slave = {
+.name = TYPE_NVME_MI_SLAVE,
+.parent = TYPE_I2C_SLAVE,
+.instance_size = sizeof(Nvmemislave),
+.class_init = nvme_mi_slave_class_init,
+};
+
+static void register_types(void)
+{
+type_register_static(_mi_slave);
+}
+
+type_init(register_types);
diff --git a/hw/nvme/nvme-mi-slave.h b/hw/nvme/nvme-mi-slave.h
new file mode 100644
index 000..971411a
--- /dev/null
+++ b/hw/nvme/nvme-mi-slave.h
@@ -0,0 +1,28 @@
+#ifndef NVMEMISLAVEH
+#define NVMEMISLAVEH
+
+#include "hw/i2c/i2c.h"
+#define TYPE_NVME_MI_SLAVE "nvme-mi-i2c-slave"
+
+#define MAX_NVME_MI_BUF_SIZE 5000
+
+enum Nvmemislavepktpos
+{
+ NVME_MI_ADDR_POS = 0,
+ NVME_MI_BYTE_LENGTH_POS = 2,
+ NVME_MI_EOM_POS = 7
+};
+
+typedef struct Nvmemislave
+{
+I2CSlave parent_obj;
+uint32_t sendlen;
+uint32_t recvlen;
+uint32_t pktpos;
+uint32_t pktlen;
+uint8_t eom;
+bool syncflag;
+u_char recvbuffer[MAX_NVME_MI_BUF_SIZE];
+} Nvmemislave;
+
+#endif
\ No newline at end of file
diff --git a/hw/nvme/nvme-mi.c b/hw/nvme/nvme-mi.c
new file mode 100644
index 000..ad98bd8
--- /dev/null
+++ b/hw/nvme/nvme-mi.c
@@ -0,0 +1,684 @@
+/*
+ * QEMU NVMe-MI Controller
+ *
+ * Copyright (c) 2021, Samsung Electronics co Ltd.
+ *
+ * Written by Padmakar Kalghatgi
+ *Arun Kumar Agasar
+ *Saurav Kumar
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+/**
+ * Reference Specs: http://www.nvmexpress.org,
+ *
+ *
+ * Usage
+ * -
+ * The nvme-mi device has
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
Am 26.10.2021 um 11:37 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: > >> By convention, names starting with "x-" are experimental. The parts > >> of external interfaces so named may be withdrawn or changed > >> incompatibly in future releases. > >> > >> Drawback: promoting something from experimental to stable involves a > >> name change. Client code needs to be updated. > >> > >> Moreover, the convention is not universally observed: > >> > >> * QOM type "input-barrier" has properties "x-origin", "y-origin". > >> Looks accidental, but it's ABI since 4.2. > >> > >> * QOM types "memory-backend-file", "memory-backend-memfd", > >> "memory-backend-ram", and "memory-backend-epc" have a property > >> "x-use-canonical-path-for-ramblock-id" that is documented to be > >> stable despite its name. > >> > >> We could document these exceptions, but documentation helps only > >> humans. We want to recognize "unstable" in code, like "deprecated". > >> > >> Replace the convention by a new special feature flag "unstable". It > >> will be recognized by the QAPI generator, like the existing feature > >> flag "deprecated", and unlike regular feature flags. > >> > >> This commit updates documentation and prepares tests. The next commit > >> updates the QAPI schema. The remaining patches update the QAPI > >> generator and wire up -compat policy checking. > >> > >> Signed-off-by: Markus Armbruster > > > > Obviously, replacing the old convention gets rid of the old drawbacks, > > but adds a new one: While using x- makes it very obvious for a human > > user that this is an unstable feature, a feature flag in the schema will > > almost certainly go unnoticed in manual use. > > I thought about this, but neglected to put it in writing. My bad. > > Manual use of unstable interfaces is mostly fine. Human users can adapt > to changing interfaces. HMP works that way. > > Management applications are better off with a feature flag than with a > naming convention we sometimes ignore. > > The most potential for trouble is in between: programs that aren't > full-fledged management applications. > > If we want to keep "unstable" obvious to the humans who write such > programs, we can continue to require "x-", in addition to the feature > flag. We pay for it with renames, and the risk of forgetting to rename > in time (which is what got us the awkward stable > "x-use-canonical-path-for-ramblock-id"). Tradeoff. I chose not to, but > if y'all think we should... Just to clarify, I'm not implying that we should keep it. I'm merely pointing out that there is a tradeoff that requires us to make a choice. The decision for one of the options should be explicit rather than just happening as a side effect. Documenting that it was a conscious decision is probably best done by adding the reasoning for it to the commit message. > What we can't do, at least not easily, is to use *only* the "x-" > convention: it is not reliable. We'd have to add a way to say 'this is > stable even though the name starts with "x-"'. No question. Kevin
Re: [PATCH v2 11/15] iotests: split linters.py out from 297
On 19.10.21 16:49, John Snow wrote: Now, 297 is just the iotests-specific incantations and linters.py is as minimal as I can think to make it. The only remaining element in here that ought to be configuration and not code is the list of skip files, but they're still numerous enough that repeating them for mypy and pylint configurations both would be ... a hassle. Signed-off-by: John Snow --- tests/qemu-iotests/297| 72 + tests/qemu-iotests/linters.py | 76 +++ 2 files changed, 87 insertions(+), 61 deletions(-) create mode 100644 tests/qemu-iotests/linters.py Reviewed-by: Hanna Reitz I wonder about `check_linter()`, though. By not moving it to linters.py, we can’t use it in its entry point, and so the Python test infrastructure will have a strong dependency on these linters. Though then again, it probably already does, and I suppose that’s one of the points hindering us from running this from make check? Hanna
Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
On 10/26/21 11:46, Markus Armbruster wrote:
> Philippe Mathieu-Daudé writes:
>
>> On 10/25/21 07:25, Markus Armbruster wrote:
>>> The code to check policy for handling deprecated input is triplicated.
>>> Factor it out into compat_policy_input_ok() before I mess with it in
>>> the next commit.
>>>
>>> Signed-off-by: Markus Armbruster
>>> ---
>>> include/qapi/compat-policy.h | 7 +
>>> qapi/qapi-visit-core.c | 18 +
>>> qapi/qmp-dispatch.c | 51 +++-
>>> qapi/qobject-input-visitor.c | 19 +++---
>>> 4 files changed, 55 insertions(+), 40 deletions(-)
>>
>>> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>>> index 8cca18c891..e29ade134c 100644
>>> --- a/qapi/qmp-dispatch.c
>>> +++ b/qapi/qmp-dispatch.c
>>> @@ -28,6 +28,40 @@
>>>
>>> CompatPolicy compat_policy;
>>>
>>> +static bool compat_policy_input_ok1(const char *adjective,
>>> +CompatPolicyInput policy,
>>> +ErrorClass error_class,
>>> +const char *kind, const char *name,
>>> +Error **errp)
>>> +{
>>> +switch (policy) {
>>> +case COMPAT_POLICY_INPUT_ACCEPT:
>>> +return true;
>>> +case COMPAT_POLICY_INPUT_REJECT:
>>> +error_set(errp, error_class, "%s %s %s disabled by policy",
>>> + adjective, kind, name);
>>> +return false;
>>> +case COMPAT_POLICY_INPUT_CRASH:
>>> +default:
>>> +abort();
>>
>> g_assert_not_reached() provides a nicer user experience.
>
> I find it hard to care for making the experience of a crash that should
> never ever happen nicer :)
Well COMPAT_POLICY_INPUT_CRASH can happen... What about:
case COMPAT_POLICY_INPUT_CRASH:
error_printf("%s %s %s disabled by policy",
adjective, kind, name);
abort();
default:
g_assert_not_reached();
Re: [PATCH] block-backend: Silence clang -m32 compiler warning
On 10/26/21 11:07, Hanna Reitz wrote:
> Similarly to e7e588d432d31ecebc26358e47201dd108db964c, there is a
> warning in block/block-backend.c that qiov->size <= INT64_MAX is always
> true on machines where size_t is narrower than a uint64_t. In said
> commit, we silenced this warning by casting to uint64_t.
>
> The commit introducing this warning here
> (a93d81c84afa717b0a1a6947524d8d1fbfd6bbf5) anticipated it and so tried
> to address it the same way. However, it only did so in one of two
> places where this comparison occurs, and so we still need to fix up the
> other one.
>
> Fixes: a93d81c84afa717b0a1a6947524d8d1fbfd6bbf5
>("block-backend: convert blk_aio_ functions to int64_t bytes
>paramter")
> Signed-off-by: Hanna Reitz
> ---
> block/block-backend.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI
On 19.10.21 16:49, John Snow wrote: We need at least a tiny little shim here to join test file discovery with test invocation. This logic could conceivably be hosted somewhere in python/, but I felt it was strictly the least-rude thing to keep the test logic here in iotests/, even if this small function isn't itself an iotest. Note that we don't actually even need the executable bit here, we'll be relying on the ability to run this module as a script using Python CLI arguments. No chance it gets misunderstood as an actual iotest that way. (It's named, not in tests/, doesn't have the execute bit, and doesn't have an execution shebang.) Signed-off-by: John Snow --- tests/qemu-iotests/linters.py | 27 +++ 1 file changed, 27 insertions(+) Reviewed-by: Hanna Reitz
Re: [PATCH v2 10/15] iotests/297: split test into sub-cases
On 19.10.21 16:49, John Snow wrote: Take iotest 297's main() test function and split it into two sub-cases that can be skipped individually. We can also drop custom environment setup from the pylint test as it isn't needed. Signed-off-by: John Snow --- tests/qemu-iotests/297 | 63 ++ 1 file changed, 39 insertions(+), 24 deletions(-) Reviewed-by: Hanna Reitz (while heavily scratching myself to ease the itch to turn this into a unit test now)
Re: [PATCH v2 09/15] iotests/297: update tool availability checks
On 19.10.21 16:49, John Snow wrote: As mentioned in 'iotests/297: Don't rely on distro-specific linter binaries', these checks are overly strict. Update them to be in-line with how we actually invoke the linters themselves. Signed-off-by: John Snow --- tests/qemu-iotests/297 | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH v2 08/15] iotests/297: Change run_linter() to raise an exception on failure
On 19.10.21 16:49, John Snow wrote: Instead of using a process return code as the python function return value (or just not returning anything at all), allow run_linter() to raise an exception instead. The responsibility for printing output on error shifts from the function itself to the caller, who will know best how to present/format that information. (Also, "suppress_output" is now a lot more accurate of a parameter name.) Signed-off-by: John Snow --- tests/qemu-iotests/297 | 24 ++-- 1 file changed, 14 insertions(+), 10 deletions(-) Thanks! :) Reviewed-by: Hanna Reitz
Re: [PATCH v2 07/15] iotests/297: refactor run_[mypy|pylint] as generic execution shim
On 19.10.21 16:49, John Snow wrote: There's virtually nothing special here anymore; we can combine these into a single, rather generic function. Signed-off-by: John Snow --- tests/qemu-iotests/297 | 42 ++ 1 file changed, 22 insertions(+), 20 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
Philippe Mathieu-Daudé writes:
> On 10/25/21 07:25, Markus Armbruster wrote:
>> The code to check policy for handling deprecated input is triplicated.
>> Factor it out into compat_policy_input_ok() before I mess with it in
>> the next commit.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> include/qapi/compat-policy.h | 7 +
>> qapi/qapi-visit-core.c | 18 +
>> qapi/qmp-dispatch.c | 51 +++-
>> qapi/qobject-input-visitor.c | 19 +++---
>> 4 files changed, 55 insertions(+), 40 deletions(-)
>
>> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>> index 8cca18c891..e29ade134c 100644
>> --- a/qapi/qmp-dispatch.c
>> +++ b/qapi/qmp-dispatch.c
>> @@ -28,6 +28,40 @@
>>
>> CompatPolicy compat_policy;
>>
>> +static bool compat_policy_input_ok1(const char *adjective,
>> +CompatPolicyInput policy,
>> +ErrorClass error_class,
>> +const char *kind, const char *name,
>> +Error **errp)
>> +{
>> +switch (policy) {
>> +case COMPAT_POLICY_INPUT_ACCEPT:
>> +return true;
>> +case COMPAT_POLICY_INPUT_REJECT:
>> +error_set(errp, error_class, "%s %s %s disabled by policy",
>> + adjective, kind, name);
>> +return false;
>> +case COMPAT_POLICY_INPUT_CRASH:
>> +default:
>> +abort();
>
> g_assert_not_reached() provides a nicer user experience.
I find it hard to care for making the experience of a crash that should
never ever happen nicer :)
>> +}
>> +}
>> +
>> +bool compat_policy_input_ok(unsigned special_features,
>> +const CompatPolicy *policy,
>> +ErrorClass error_class,
>> +const char *kind, const char *name,
>> +Error **errp)
>> +{
>> +if ((special_features & 1u << QAPI_DEPRECATED)
>
> Matter of taste, I find code using extract() easier to review:
>
> extract64(special_features, QAPI_DEPRECATED, 1)
I agree for width > 1.
>> +&& !compat_policy_input_ok1("Deprecated",
>> +policy->deprecated_input,
>> +error_class, kind, name, errp)) {
>> +return false;
>> +}
>> +return true;
>> +}
>
> Reviewed-by: Philippe Mathieu-Daudé
Thanks!
Re: [PATCH 7/9] qapi: Generalize enum member policy checking
John Snow writes:
> On Mon, Oct 25, 2021 at 1:26 AM Markus Armbruster wrote:
>
>> The code to check enumeration value policy can see special feature
>> flag 'deprecated' in QEnumLookup member flags[value]. I want to make
>> feature flag 'unstable' visible there as well, so I can add policy for
>> it.
>>
>> Instead of extending flags[], replace it by @special_features (a
>> bitset of QapiSpecialFeature), because that's how special features get
>> passed around elsewhere.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> include/qapi/util.h| 5 +
>> qapi/qapi-visit-core.c | 3 ++-
>> scripts/qapi/types.py | 22 --
>> 3 files changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>> index 7a8d5c7d72..0cc98db9f9 100644
>> --- a/include/qapi/util.h
>> +++ b/include/qapi/util.h
>> @@ -15,12 +15,9 @@ typedef enum {
>> QAPI_DEPRECATED,
>> } QapiSpecialFeature;
>>
>> -/* QEnumLookup flags */
>> -#define QAPI_ENUM_DEPRECATED 1
>> -
>> typedef struct QEnumLookup {
>> const char *const *array;
>> -const unsigned char *const flags;
>> +const unsigned char *const special_features;
>> const int size;
>> } QEnumLookup;
>>
>> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
>> index b4a81f1757..5572d90efb 100644
>> --- a/qapi/qapi-visit-core.c
>> +++ b/qapi/qapi-visit-core.c
>> @@ -407,7 +407,8 @@ static bool input_type_enum(Visitor *v, const char
>> *name, int *obj,
>> return false;
>> }
>>
>> -if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
>> +if (lookup->special_features
>> +&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
>> switch (v->compat_policy.deprecated_input) {
>> case COMPAT_POLICY_INPUT_ACCEPT:
>> break;
>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>> index ab2441adc9..3013329c24 100644
>> --- a/scripts/qapi/types.py
>> +++ b/scripts/qapi/types.py
>> @@ -16,7 +16,7 @@
>> from typing import List, Optional
>>
>> from .common import c_enum_const, c_name, mcgen
>> -from .gen import QAPISchemaModularCVisitor, ifcontext
>> +from .gen import QAPISchemaModularCVisitor, gen_special_features,
>> ifcontext
>> from .schema import (
>> QAPISchema,
>> QAPISchemaEnumMember,
>> @@ -39,7 +39,7 @@ def gen_enum_lookup(name: str,
>> members: List[QAPISchemaEnumMember],
>> prefix: Optional[str] = None) -> str:
>> max_index = c_enum_const(name, '_MAX', prefix)
>> -flags = ''
>> +feats = ''
>> ret = mcgen('''
>>
>> const QEnumLookup %(c_name)s_lookup = {
>> @@ -54,19 +54,21 @@ def gen_enum_lookup(name: str,
>> ''',
>> index=index, name=memb.name)
>> ret += memb.ifcond.gen_endif()
>> -if 'deprecated' in (f.name for f in memb.features):
>> -flags += mcgen('''
>> -[%(index)s] = QAPI_ENUM_DEPRECATED,
>> -''',
>> - index=index)
>>
>> -if flags:
>> +special_features = gen_special_features(memb.features)
>> +if special_features != '0':
>>
>
> Though, I have to admit the common reoccurrence of this pattern suggests to
> me that gen_special_features really ought to be returning something false-y
> in these cases. Something about testing for the empty case with something
> that represents, but isn't empty, gives me a brief pause.
>
> Not willing to wage war over it.
I habitually start stupid, and when stupid confuses or annoys me later,
I smarten it up some.
Let's see how this instance of "stupid" ages, okay?
>> +feats += mcgen('''
>> +[%(index)s] = %(special_features)s,
>> +''',
>> + index=index, special_features=special_features)
>> +
>> +if feats:
>> ret += mcgen('''
>> },
>> -.flags = (const unsigned char[%(max_index)s]) {
>> +.special_features = (const unsigned char[%(max_index)s]) {
>> ''',
>> max_index=max_index)
>> -ret += flags
>> +ret += feats
>>
>> ret += mcgen('''
>> },
>> --
>> 2.31.1
>>
>>
> Python bits: Acked-by: John Snow
Thanks!
Re: [PATCH 6/9] qapi: Generalize command policy checking
Philippe Mathieu-Daudé writes: > On 10/25/21 07:25, Markus Armbruster wrote: >> The code to check command policy can see special feature flag >> 'deprecated' as command flag QCO_DEPRECATED. I want to make feature >> flag 'unstable' visible there as well, so I can add policy for it. >> >> To let me make it visible, add member @special_features (a bitset of >> QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it >> through qmp_register_command(). Then replace "QCO_DEPRECATED in >> @flags" by QAPI_DEPRECATED in @special_features", and drop >> QCO_DEPRECATED. >> >> Signed-off-by: Markus Armbruster >> --- >> include/qapi/qmp/dispatch.h | 5 +++-- >> monitor/misc.c | 6 -- >> qapi/qmp-dispatch.c | 2 +- >> qapi/qmp-registry.c | 4 +++- >> storage-daemon/qemu-storage-daemon.c | 3 ++- >> scripts/qapi/commands.py | 9 - >> 6 files changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h >> index 0ce88200b9..1e4240fd0d 100644 >> --- a/include/qapi/qmp/dispatch.h >> +++ b/include/qapi/qmp/dispatch.h >> @@ -25,7 +25,6 @@ typedef enum QmpCommandOptions >> QCO_ALLOW_OOB = (1U << 1), >> QCO_ALLOW_PRECONFIG = (1U << 2), >> QCO_COROUTINE = (1U << 3), >> -QCO_DEPRECATED= (1U << 4), >> } QmpCommandOptions; >> >> typedef struct QmpCommand >> @@ -34,6 +33,7 @@ typedef struct QmpCommand >> /* Runs in coroutine context if QCO_COROUTINE is set */ >> QmpCommandFunc *fn; >> QmpCommandOptions options; >> +unsigned special_features; >> QTAILQ_ENTRY(QmpCommand) node; >> bool enabled; >> const char *disable_reason; >> @@ -42,7 +42,8 @@ typedef struct QmpCommand >> typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList; >> >> void qmp_register_command(QmpCommandList *cmds, const char *name, >> - QmpCommandFunc *fn, QmpCommandOptions options); >> + QmpCommandFunc *fn, QmpCommandOptions options, >> + unsigned special_features); > > What about: > > typedef unsigned QapiFeatureMask; > > ? I think the name @special_features makes the connection to QapiSpecialFeature clear enough. > Otherwise: > Reviewed-by: Philippe Mathieu-Daudé Thanks!
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
Kevin Wolf writes: > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: >> By convention, names starting with "x-" are experimental. The parts >> of external interfaces so named may be withdrawn or changed >> incompatibly in future releases. >> >> Drawback: promoting something from experimental to stable involves a >> name change. Client code needs to be updated. >> >> Moreover, the convention is not universally observed: >> >> * QOM type "input-barrier" has properties "x-origin", "y-origin". >> Looks accidental, but it's ABI since 4.2. >> >> * QOM types "memory-backend-file", "memory-backend-memfd", >> "memory-backend-ram", and "memory-backend-epc" have a property >> "x-use-canonical-path-for-ramblock-id" that is documented to be >> stable despite its name. >> >> We could document these exceptions, but documentation helps only >> humans. We want to recognize "unstable" in code, like "deprecated". >> >> Replace the convention by a new special feature flag "unstable". It >> will be recognized by the QAPI generator, like the existing feature >> flag "deprecated", and unlike regular feature flags. >> >> This commit updates documentation and prepares tests. The next commit >> updates the QAPI schema. The remaining patches update the QAPI >> generator and wire up -compat policy checking. >> >> Signed-off-by: Markus Armbruster > > Obviously, replacing the old convention gets rid of the old drawbacks, > but adds a new one: While using x- makes it very obvious for a human > user that this is an unstable feature, a feature flag in the schema will > almost certainly go unnoticed in manual use. I thought about this, but neglected to put it in writing. My bad. Manual use of unstable interfaces is mostly fine. Human users can adapt to changing interfaces. HMP works that way. Management applications are better off with a feature flag than with a naming convention we sometimes ignore. The most potential for trouble is in between: programs that aren't full-fledged management applications. If we want to keep "unstable" obvious to the humans who write such programs, we can continue to require "x-", in addition to the feature flag. We pay for it with renames, and the risk of forgetting to rename in time (which is what got us the awkward stable "x-use-canonical-path-for-ramblock-id"). Tradeoff. I chose not to, but if y'all think we should... What we can't do, at least not easily, is to use *only* the "x-" convention: it is not reliable. We'd have to add a way to say 'this is stable even though the name starts with "x-"'.
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
On Tue, Oct 26, 2021 at 10:22:15AM +0100, Dr. David Alan Gilbert wrote: > * Kevin Wolf (kw...@redhat.com) wrote: > > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: > > > By convention, names starting with "x-" are experimental. The parts > > > of external interfaces so named may be withdrawn or changed > > > incompatibly in future releases. > > > > > > Drawback: promoting something from experimental to stable involves a > > > name change. Client code needs to be updated. > > > > > > Moreover, the convention is not universally observed: > > > > > > * QOM type "input-barrier" has properties "x-origin", "y-origin". > > > Looks accidental, but it's ABI since 4.2. > > > > > > * QOM types "memory-backend-file", "memory-backend-memfd", > > > "memory-backend-ram", and "memory-backend-epc" have a property > > > "x-use-canonical-path-for-ramblock-id" that is documented to be > > > stable despite its name. > > > > > > We could document these exceptions, but documentation helps only > > > humans. We want to recognize "unstable" in code, like "deprecated". > > > > > > Replace the convention by a new special feature flag "unstable". It > > > will be recognized by the QAPI generator, like the existing feature > > > flag "deprecated", and unlike regular feature flags. > > > > > > This commit updates documentation and prepares tests. The next commit > > > updates the QAPI schema. The remaining patches update the QAPI > > > generator and wire up -compat policy checking. > > > > > > Signed-off-by: Markus Armbruster > > > > Obviously, replacing the old convention gets rid of the old drawbacks, > > but adds a new one: While using x- makes it very obvious for a human > > user that this is an unstable feature, a feature flag in the schema will > > almost certainly go unnoticed in manual use. > > Agreed, I'd keep the x- as well. > > Having said that, the x- represents a few different things (that we > don't currently distinguish): > - experimental > - for internal use > - for debugging/human use All of those usage scenarios have the same implication though: Command/data format is liable to change in incompatible ways, or be deleted, with no prior warning. I don't think we need to distinguish the use cases - some commands may belong to two or three of those use cases. All that matters is that they're considered "unstable" from an API compat POV. 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 1/9] qapi: New special feature flag "unstable"
* Kevin Wolf (kw...@redhat.com) wrote: > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: > > By convention, names starting with "x-" are experimental. The parts > > of external interfaces so named may be withdrawn or changed > > incompatibly in future releases. > > > > Drawback: promoting something from experimental to stable involves a > > name change. Client code needs to be updated. > > > > Moreover, the convention is not universally observed: > > > > * QOM type "input-barrier" has properties "x-origin", "y-origin". > > Looks accidental, but it's ABI since 4.2. > > > > * QOM types "memory-backend-file", "memory-backend-memfd", > > "memory-backend-ram", and "memory-backend-epc" have a property > > "x-use-canonical-path-for-ramblock-id" that is documented to be > > stable despite its name. > > > > We could document these exceptions, but documentation helps only > > humans. We want to recognize "unstable" in code, like "deprecated". > > > > Replace the convention by a new special feature flag "unstable". It > > will be recognized by the QAPI generator, like the existing feature > > flag "deprecated", and unlike regular feature flags. > > > > This commit updates documentation and prepares tests. The next commit > > updates the QAPI schema. The remaining patches update the QAPI > > generator and wire up -compat policy checking. > > > > Signed-off-by: Markus Armbruster > > Obviously, replacing the old convention gets rid of the old drawbacks, > but adds a new one: While using x- makes it very obvious for a human > user that this is an unstable feature, a feature flag in the schema will > almost certainly go unnoticed in manual use. Agreed, I'd keep the x- as well. Having said that, the x- represents a few different things (that we don't currently distinguish): - experimental - for internal use - for debugging/human use Dave > Kevin > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2 1/4] qemu-img: implement compare --stat
26.10.2021 11:47, Hanna Reitz wrote:
On 26.10.21 09:53, Vladimir Sementsov-Ogievskiy wrote:
25.10.2021 19:40, Hanna Reitz wrote:
On 21.10.21 12:12, Vladimir Sementsov-Ogievskiy wrote:
With new option qemu-img compare will not stop at first mismatch, but
instead calculate statistics: how many clusters with different data,
how many clusters with equal data, how many clusters were unallocated
but become data and so on.
We compare images chunk by chunk. Chunk size depends on what
block_status returns for both images. It may return less than cluster
(remember about qcow2 subclusters), it may return more than cluster (if
several consecutive clusters share same status). Finally images may
have different cluster sizes. This all leads to ambiguity in how to
finally compare the data.
What we can say for sure is that, when we compare two qcow2 images with
same cluster size, we should compare clusters with data separately.
Otherwise, if we for example compare 10 consecutive clusters of data
where only one byte differs we'll report 10 different clusters.
Expected result in this case is 1 different cluster and 9 equal ones.
So, to serve this case and just to have some defined rule let's do the
following:
1. Select some block-size for compare procedure. In this commit it must
be specified by user, next commit will add some automatic logic and
make --block-size optional.
2. Go chunk-by-chunk using block_status as we do now with only one
differency:
If block_status() returns DATA region that intersects block-size
aligned boundary, crop this region at this boundary.
This way it's still possible to compare less than cluster and report
subcluster-level accuracy, but we newer compare more than one cluster
of data.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
docs/tools/qemu-img.rst | 18 +++-
qemu-img.c | 206 +---
qemu-img-cmds.hx | 4 +-
3 files changed, 212 insertions(+), 16 deletions(-)
Looks good to me overall! Just some technical comments below.
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index d58980aef8..21164253d4 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -159,6 +159,18 @@ Parameters to compare subcommand:
Strict mode - fail on different image size or sector allocation
+.. option:: --stat
+
+ Instead of exit on first mismatch compare the whole images and print
+ statistics on amount of different pairs of clusters, based on their
+ block-status and are they equal or not.
I’d phrase this as:
Instead of exiting on the first mismatch, compare the whole images and print
statistics on how much they differ in terms of block status (i.e. are blocks
allocated or not, do they contain data, are they marked as containing only
zeroes) and block content (a block of data that contains only zero still has
the same content as a marked-zero block).
For me the rest starting from "and block content" sounds unclear, seems doesn't
add any information to previous (i.e. are blocks allocated ...)
By “block content” I meant what you said by “equal or not”, i.e. what is
returned when reading from the block.
Now that I think about it again, I believe we should go with your original
“equal or not”, though, because that reflects what qemu-img --stat prints, like
so perhaps:
Instead of exiting on the first mismatch, compare the whole images and print
statistics on the amount of different pairs of blocks, based on their block
status and whether they are equal or not.
OK
I’d still like to add something like what I had in parentheses, though, because
as a user, I’d find the “block status” and “equal or not” terms to be a bit
handwave-y. I don’t think “block status” is a common term in our
documentation, so I wanted to add some examples; and I wanted to show by
example that “equal” blocks don’t need to have the same block status.
Actually, I think, that the resulting tool is anyway developer-oriented, to use
it you should know what this block status really means.. May be just s/block
status/block type/, will it be more human friendly?
[...]
@@ -1304,6 +1306,107 @@ static int check_empty_sectors(BlockBackend *blk,
int64_t offset,
return 0;
}
+#define IMG_CMP_STATUS_MASK (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | \
+ BDRV_BLOCK_ALLOCATED)
+#define IMG_CMP_STATUS_MAX (IMG_CMP_STATUS_MASK | BDRV_BLOCK_EOF)
+
+typedef struct ImgCmpStat {
+ /* stat: [ret: 0 is equal, 1 is not][status1][status2] -> n_bytes */
+ uint64_t stat[2][IMG_CMP_STATUS_MAX + 1][IMG_CMP_STATUS_MAX + 1];
`IMG_CMP_STATUS_MAX` isn’t packed tightly because it only has four bits set
(0x33). That in itself isn’t a problem, but it means that `IMG_CMP_STATUS_MAX
+ 1` is 52, and so this array’s size is 52 * 52 * 2 * sizeof(uint64_t) = 43264.
Again, that isn’t a problem in itself (although it is a bit sad that this
could fit into 16 * 16 * 2 * 8 = 4 kB), but
Re: [PATCH 5/9] qapi: Generalize struct member policy checking
John Snow writes:
> On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote:
>
>> The generated visitor functions call visit_deprecated_accept() and
>> visit_deprecated() when visiting a struct member with special feature
>> flag 'deprecated'. This makes the feature flag visible to the actual
>> visitors. I want to make feature flag 'unstable' visible there as
>> well, so I can add policy for it.
>>
>> To let me make it visible, replace these functions by
>> visit_policy_reject() and visit_policy_skip(), which take the member's
>> special features as an argument. Note that the new functions have the
>> opposite sense, i.e. the return value flips.
>>
>> Signed-off-by: Markus Armbruster
[...]
>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> index 9d9196a143..e13bbe4292 100644
>> --- a/scripts/qapi/visit.py
>> +++ b/scripts/qapi/visit.py
>> @@ -21,7 +21,7 @@
>> indent,
>> mcgen,
>> )
>> -from .gen import QAPISchemaModularCVisitor, ifcontext
>> +from .gen import QAPISchemaModularCVisitor, gen_special_features, ifcontext
>> from .schema import (
>> QAPISchema,
>> QAPISchemaEnumMember,
>> @@ -76,7 +76,6 @@ def gen_visit_object_members(name: str,
>> c_type=base.c_name())
>>
>> for memb in members:
>> -deprecated = 'deprecated' in [f.name for f in memb.features]
>> ret += memb.ifcond.gen_if()
>> if memb.optional:
>> ret += mcgen('''
>> @@ -84,14 +83,15 @@ def gen_visit_object_members(name: str,
>> ''',
>> name=memb.name, c_name=c_name(memb.name))
>> indent.increase()
>> -if deprecated:
>> +special_features = gen_special_features(memb.features)
>> +if special_features != '0':
>>
>
> Would it be possible for gen_special_features to return something false-y
> instead of '0'? Do we actually *use* the '0' return anywhere other than to
> test it to see if we should include additional code?
>
> If you actually use the '0' anywhere: Go ahead and treat this as an ack. If
> you don't, can we clean this up?
gen_special_features() returns a string holding C code for a special
features bitset. The empty bitset is 0.
The "natural" use is interpolating this value into C code. Two
instances are visible below.
The use right here is for testing "have we got special features", so we
can skip generating code that is of no use when we don't have any. It's
admittedly slightly unclean.
> (Sorry, I find the mcgen stuff hard to read in patch form and I am trying
> to give you a quick review instead of NO review.)
Any kind of review is appreciated :)
> --js
>
>
>> ret += mcgen('''
>> -if (!visit_deprecated_accept(v, "%(name)s", errp)) {
>> +if (visit_policy_reject(v, "%(name)s", %(special_features)s, errp)) {
>> return false;
>> }
>> -if (visit_deprecated(v, "%(name)s")) {
>> +if (!visit_policy_skip(v, "%(name)s", %(special_features)s)) {
>> ''',
>> - name=memb.name)
>> + name=memb.name, special_features=special_features)
These are the "natural" uses I mentioned.
If gen_special_features() returned something other than '0', we'd have
to replace it by '0' here.
>> indent.increase()
>> ret += mcgen('''
>> if (!visit_type_%(c_type)s(v, "%(name)s", >%(c_name)s, errp)) {
>> @@ -100,7 +100,7 @@ def gen_visit_object_members(name: str,
>> ''',
>> c_type=memb.type.c_name(), name=memb.name,
>> c_name=c_name(memb.name))
>> -if deprecated:
>> +if special_features != '0':
>> indent.decrease()
>> ret += mcgen('''
>> }
>> --
>> 2.31.1
>>
>>
[PATCH] block-backend: Silence clang -m32 compiler warning
Similarly to e7e588d432d31ecebc26358e47201dd108db964c, there is a
warning in block/block-backend.c that qiov->size <= INT64_MAX is always
true on machines where size_t is narrower than a uint64_t. In said
commit, we silenced this warning by casting to uint64_t.
The commit introducing this warning here
(a93d81c84afa717b0a1a6947524d8d1fbfd6bbf5) anticipated it and so tried
to address it the same way. However, it only did so in one of two
places where this comparison occurs, and so we still need to fix up the
other one.
Fixes: a93d81c84afa717b0a1a6947524d8d1fbfd6bbf5
("block-backend: convert blk_aio_ functions to int64_t bytes
paramter")
Signed-off-by: Hanna Reitz
---
block/block-backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 39cd99df2b..12ef80ea17 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1540,7 +1540,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t
offset,
QEMUIOVector *qiov, BdrvRequestFlags flags,
BlockCompletionFunc *cb, void *opaque)
{
-assert(qiov->size <= INT64_MAX);
+assert((uint64_t)qiov->size <= INT64_MAX);
return blk_aio_prwv(blk, offset, qiov->size, qiov,
blk_aio_write_entry, flags, cb, opaque);
}
--
2.31.1
Re: [PATCH 4/9] qapi: Tools for sets of special feature flags in generated code
John Snow writes:
> On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote:
>
>> New enum QapiSpecialFeature enumerates the special feature flags.
>>
>> New helper gen_special_features() returns code to represent a
>> collection of special feature flags as a bitset.
>>
>> The next few commits will put them to use.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> include/qapi/util.h| 4
>> scripts/qapi/gen.py| 13 +
>> scripts/qapi/schema.py | 3 +++
>> 3 files changed, 20 insertions(+)
>>
>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>> index 257c600f99..7a8d5c7d72 100644
>> --- a/include/qapi/util.h
>> +++ b/include/qapi/util.h
>> @@ -11,6 +11,10 @@
>> #ifndef QAPI_UTIL_H
>> #define QAPI_UTIL_H
>>
>> +typedef enum {
>> +QAPI_DEPRECATED,
>> +} QapiSpecialFeature;
>> +
>> /* QEnumLookup flags */
>> #define QAPI_ENUM_DEPRECATED 1
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index 2ec1e7b3b6..9d07b88cf6 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -29,6 +29,7 @@
>> mcgen,
>> )
>> from .schema import (
>> +QAPISchemaFeature,
>> QAPISchemaIfCond,
>> QAPISchemaModule,
>> QAPISchemaObjectType,
>> @@ -37,6 +38,18 @@
>> from .source import QAPISourceInfo
>>
>>
>> +def gen_special_features(features: QAPISchemaFeature):
>> +ret = ''
>> +sep = ''
>> +
>> +for feat in features:
>> +if feat.is_special():
>> +ret += ('%s1u << QAPI_%s' % (sep, feat.name.upper()))
>>
>
> Building the constant name here "feels" fragile, but I'll trust that the
> test suite and/or the compiler will catch us if we accidentally goof up
> this mapping. In the interest of simplicity, then, "sure, why not."
It relies on .is_special() remaining consistent with enum
QapiSpecialFeature. The C compiler should catch any screwups.
>
>> +sep = ' | '
>> +
>>
> +return ret or '0'
>> +
>>
>
> Subjectively more pythonic:
>
> special_features = [f"1u << QAPI_{feat.name.upper()}" for feat in features
> if feat.is_special()]
> ret = ' | '.join(special_features)
> return ret or '0'
>
> A bit more dense, but more functional. Up to you, but I find join() easier
> to read and reason about for the presence of separators.
Sold!
>> +
>> class QAPIGen:
>> def __init__(self, fname: str):
>> self.fname = fname
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 6d5f46509a..55f82d7389 100644
>> --- a/scripts/qapi/schema.py
>> +++ b/scripts/qapi/schema.py
>> @@ -725,6 +725,9 @@ def connect_doc(self, doc):
>> class QAPISchemaFeature(QAPISchemaMember):
>> role = 'feature'
>>
>> +def is_special(self):
>> +return self.name in ('deprecated')
>> +
>>
>
> alrighty.
>
> (Briefly wondered: is it worth naming special features as a property of the
> class, but with only two names, it's probably fine enough to leave it
> embedded in the method logic. Only a style thing and doesn't have any
> actual impact that I can imagine, so ... nevermind.)
Let's keep it simple.
>> class QAPISchemaObjectTypeMember(QAPISchemaMember):
>> def __init__(self, name, info, typ, optional, ifcond=None,
>> features=None):
>> --
>> 2.31.1
>>
>>
> Well, either way:
>
> Reviewed-by: John Snow
Thanks!
Re: [PATCH v2 1/4] qemu-img: implement compare --stat
On 26.10.21 09:53, Vladimir Sementsov-Ogievskiy wrote:
25.10.2021 19:40, Hanna Reitz wrote:
On 21.10.21 12:12, Vladimir Sementsov-Ogievskiy wrote:
With new option qemu-img compare will not stop at first mismatch, but
instead calculate statistics: how many clusters with different data,
how many clusters with equal data, how many clusters were unallocated
but become data and so on.
We compare images chunk by chunk. Chunk size depends on what
block_status returns for both images. It may return less than cluster
(remember about qcow2 subclusters), it may return more than cluster (if
several consecutive clusters share same status). Finally images may
have different cluster sizes. This all leads to ambiguity in how to
finally compare the data.
What we can say for sure is that, when we compare two qcow2 images with
same cluster size, we should compare clusters with data separately.
Otherwise, if we for example compare 10 consecutive clusters of data
where only one byte differs we'll report 10 different clusters.
Expected result in this case is 1 different cluster and 9 equal ones.
So, to serve this case and just to have some defined rule let's do the
following:
1. Select some block-size for compare procedure. In this commit it must
be specified by user, next commit will add some automatic logic and
make --block-size optional.
2. Go chunk-by-chunk using block_status as we do now with only one
differency:
If block_status() returns DATA region that intersects block-size
aligned boundary, crop this region at this boundary.
This way it's still possible to compare less than cluster and report
subcluster-level accuracy, but we newer compare more than one cluster
of data.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
docs/tools/qemu-img.rst | 18 +++-
qemu-img.c | 206
+---
qemu-img-cmds.hx | 4 +-
3 files changed, 212 insertions(+), 16 deletions(-)
Looks good to me overall! Just some technical comments below.
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index d58980aef8..21164253d4 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -159,6 +159,18 @@ Parameters to compare subcommand:
Strict mode - fail on different image size or sector allocation
+.. option:: --stat
+
+ Instead of exit on first mismatch compare the whole images and print
+ statistics on amount of different pairs of clusters, based on their
+ block-status and are they equal or not.
I’d phrase this as:
Instead of exiting on the first mismatch, compare the whole images
and print statistics on how much they differ in terms of block status
(i.e. are blocks allocated or not, do they contain data, are they
marked as containing only zeroes) and block content (a block of data
that contains only zero still has the same content as a marked-zero
block).
For me the rest starting from "and block content" sounds unclear,
seems doesn't add any information to previous (i.e. are blocks
allocated ...)
By “block content” I meant what you said by “equal or not”, i.e. what is
returned when reading from the block.
Now that I think about it again, I believe we should go with your
original “equal or not”, though, because that reflects what qemu-img
--stat prints, like so perhaps:
Instead of exiting on the first mismatch, compare the whole images and
print statistics on the amount of different pairs of blocks, based on
their block status and whether they are equal or not.
I’d still like to add something like what I had in parentheses, though,
because as a user, I’d find the “block status” and “equal or not” terms
to be a bit handwave-y. I don’t think “block status” is a common term
in our documentation, so I wanted to add some examples; and I wanted to
show by example that “equal” blocks don’t need to have the same block
status.
[...]
@@ -1304,6 +1306,107 @@ static int check_empty_sectors(BlockBackend
*blk, int64_t offset,
return 0;
}
+#define IMG_CMP_STATUS_MASK (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | \
+ BDRV_BLOCK_ALLOCATED)
+#define IMG_CMP_STATUS_MAX (IMG_CMP_STATUS_MASK | BDRV_BLOCK_EOF)
+
+typedef struct ImgCmpStat {
+ /* stat: [ret: 0 is equal, 1 is not][status1][status2] ->
n_bytes */
+ uint64_t stat[2][IMG_CMP_STATUS_MAX + 1][IMG_CMP_STATUS_MAX + 1];
`IMG_CMP_STATUS_MAX` isn’t packed tightly because it only has four
bits set (0x33). That in itself isn’t a problem, but it means that
`IMG_CMP_STATUS_MAX + 1` is 52, and so this array’s size is 52 * 52 *
2 * sizeof(uint64_t) = 43264. Again, that isn’t a problem in itself
(although it is a bit sad that this could fit into 16 * 16 * 2 * 8 =
4 kB), but in `img_compare()` [1], you put this structure on the
stack, and I believe it’s too big for that.
Hmm. May be, it's better just use GHashTables and don't bother with
these sparse arrays
Or we could use our own bits here (ALLOCATED = (1 <<
Re: [Libguestfs] [PATCH 1/9] qapi: New special feature flag "unstable"
On Tue, Oct 26, 2021 at 09:37:29AM +0200, Kevin Wolf wrote: > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: [...] > > This commit updates documentation and prepares tests. The next commit > > updates the QAPI schema. The remaining patches update the QAPI > > generator and wire up -compat policy checking. > > > > Signed-off-by: Markus Armbruster > > Obviously, replacing the old convention gets rid of the old drawbacks, > but adds a new one: While using x- makes it very obvious for a human > user that this is an unstable feature, a feature flag in the schema will > almost certainly go unnoticed in manual use. FWIW, I wondered about it too, as I like the visual reminder of the "x-" prefix. Then I thought, "how many people besides QEMU and related tooling developers -- who would read QAPI docs anyway :) -- launch experimental QEMU commands manually?" Maybe that's too big of a leap. -- /kashyap
Re: [PATCH v2 4/4] iotests: add qemu-img-compare-stat test
On 21.10.21 12:12, Vladimir Sementsov-Ogievskiy wrote: Test new feature qemu-img compare --stat. Signed-off-by: Vladimir Sementsov-Ogievskiy --- .../qemu-iotests/tests/qemu-img-compare-stat | 88 +++ .../tests/qemu-img-compare-stat.out | 106 ++ 2 files changed, 194 insertions(+) create mode 100755 tests/qemu-iotests/tests/qemu-img-compare-stat create mode 100644 tests/qemu-iotests/tests/qemu-img-compare-stat.out Reviewed-by: Hanna Reitz
Re: [PATCH v2 2/4] qemu-img: make --block-size optional for compare --stat
On 21.10.21 12:12, Vladimir Sementsov-Ogievskiy wrote:
Let's detect block-size automatically if not specified by user:
If both files define cluster-size, use minimum to be more precise.
If both files don't specify cluster-size, use default of 64K
If only one file specify cluster-size, just use it.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
docs/tools/qemu-img.rst | 7 +++-
qemu-img.c | 71 -
qemu-img-cmds.hx| 4 +--
3 files changed, 71 insertions(+), 11 deletions(-)
Looks good, just grammar nits and a request for an assertion below.
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 21164253d4..4b382ca2b0 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -170,6 +170,11 @@ Parameters to compare subcommand:
Block size for comparing with ``--stat``. This doesn't guarantee exact
size of comparing chunks, but that guarantee that data chunks being
compared will never cross aligned block-size boundary.
+ When unspecified the following logic is used:
+
+- If both files define cluster-size, use minimum to be more precise.
+- If both files don't specify cluster-size, use default of 64K
+- If only one file specify cluster-size, just use it.
s/specify/specifies/; and perhaps s/it/that/
[...]
diff --git a/qemu-img.c b/qemu-img.c
index 79a0589167..61e7f470bb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1407,6 +1407,61 @@ static void cmp_stat_print(ImgCmpStat *stat, int64_t
total_bytes)
}
}
+/* Get default value for qemu-img compare --block-size option. */
+static int img_compare_block_size(BlockDriverState *bs1,
+ BlockDriverState *bs2,
+ bool quiet)
+{
+const int default_block_size = 64 * 1024; /* 64K */
+
+int ret;
+BlockDriverInfo bdi;
+int cluster_size1, cluster_size2, block_size;
+const char *note = "Note: to alter it, set --block-size option.";
+const char *fname1 = bs1->filename;
+const char *fname2 = bs2->filename;
+
+ret = bdrv_get_info(bs1, );
+if (ret < 0 && ret != -ENOTSUP) {
+error_report("Failed to get info of %s: %s", fname1, strerror(-ret));
+return ret;
+}
+cluster_size1 = ret < 0 ? 0 : bdi.cluster_size;
+
+ret = bdrv_get_info(bs2, );
+if (ret < 0 && ret != -ENOTSUP) {
+error_report("Failed to get info of %s: %s", fname2, strerror(-ret));
+return ret;
+}
+cluster_size2 = ret < 0 ? 0 : bdi.cluster_size;
+
I’d feel better with an `assert(cluster_size1 >= 0 && cluster_size2 >=
0);` here.
+if (cluster_size1 > 0 && cluster_size2 > 0) {
+if (cluster_size1 == cluster_size2) {
+block_size = cluster_size1;
+} else {
+block_size = MIN(cluster_size1, cluster_size2);
+qprintf(quiet, "%s and %s has different cluster sizes: %d and %d "
s/has/have/
+"correspondingly. Use minimum as block-size for "
s/correspondingly/respectively/; s/Use/Using/ (“Use” sounds like an
imperative)
+"accuracy: %d. %s\n",
+fname1, fname2, cluster_size1,
+cluster_size2, block_size, note);
+}
+} else if (cluster_size1 == 0 && cluster_size2 == 0) {
+block_size = default_block_size;
+qprintf(quiet, "Neither of %s and %s has explicit cluster size. Use "
s/has/have an/; s/Use/Using/
+"default of %d bytes. %s\n", fname1, fname2, block_size, note);
+} else {
+block_size = MAX(cluster_size1, cluster_size2);
+qprintf(quiet, "%s has explicit cluster size of %d and %s "
s/has/has an/
+"doesn't have one. Use %d as block-size. %s\n",
s/Use/Using/
Hanna
+cluster_size1 ? fname1 : fname2, block_size,
+cluster_size1 ? fname2 : fname1,
+block_size, note);
+}
+
+return block_size;
+}
+
/*
* Compares two images. Exit codes:
*
Re: [PATCH v2 3/4] qemu-img: add --shallow option for qemu-img compare
On 21.10.21 12:12, Vladimir Sementsov-Ogievskiy wrote: Allow compare only top images of backing chains. This is useful to compare images with same backing file or to compare incremental images from the chain of incremental backups with "--stat" option. Signed-off-by: Vladimir Sementsov-Ogievskiy --- docs/tools/qemu-img.rst | 9 - qemu-img.c | 8 ++-- qemu-img-cmds.hx| 4 ++-- 3 files changed, 16 insertions(+), 5 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
Kashyap Chamarthy writes:
> On Mon, Oct 25, 2021 at 07:25:24AM +0200, Markus Armbruster wrote:
>> By convention, names starting with "x-" are experimental. The parts
>> of external interfaces so named may be withdrawn or changed
>> incompatibly in future releases.
>>
>> Drawback: promoting something from experimental to stable involves a
>> name change. Client code needs to be updated.
>>
>> Moreover, the convention is not universally observed:
>>
>> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>> Looks accidental, but it's ABI since 4.2.
>>
>> * QOM types "memory-backend-file", "memory-backend-memfd",
>> "memory-backend-ram", and "memory-backend-epc" have a property
>> "x-use-canonical-path-for-ramblock-id" that is documented to be
>> stable despite its name.
>
> Looks like there's another stable property with an "x-" prefix:
> "x-remote-object", part of QOM type @RemoteObjectProperties.
The union branch 'x-remote-object' isn't flagged 'unstable' (because
union branches can't have feature flags), but the enumeration value
'x-remote-object' is. Sufficient, because you can't use the branch
without using the enumeration value. Admittedly subtle.
I wrote a bit of code (appended) to make sure I don't miss names.
> Given the above "x-" properties are now stable, I take it that they
> cannot be renamed now, as they might break any tools using them? My
> guess is the tedious way is not worth it: deprecate them, and add the
> non-x variants as "synonyms".
"x-use-canonical-path-for-ramblock-id" goes back to commit fa0cb34d22
"hostmem: use object id for memory region name with >= 4.0" (v4.0). It
may have been intended to be internal back then. It wasn't anymore when
commit 8db0b20415 "machine: add missing doc for memory-backend option"
(v6.0) documented it:
And document that x-use-canonical-path-for-ramblock-id,
is considered to be stable to make sure it won't go away by accident.
x- was intended for unstable/iternal properties, and not supposed to
be stable option. However it's too late to rename (drop x-)
it as it would mean that users will have to mantain both
x-use-canonical-path-for-ramblock-id (for QEMU 5.0-5.2) versions
and prefix-less for later versions.
Igor's reasoning still applies.
"x-origin" has always been stable. Same argument.
>> We could document these exceptions, but documentation helps only
>> humans. We want to recognize "unstable" in code, like "deprecated".
>>
>> Replace the convention by a new special feature flag "unstable". It
>> will be recognized by the QAPI generator, like the existing feature
>> flag "deprecated", and unlike regular feature flags.
>
> FWIW, sounds good to me.
Thanks!
>> This commit updates documentation and prepares tests. The next commit
>> updates the QAPI schema. The remaining patches update the QAPI
>> generator and wire up -compat policy checking.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> docs/devel/qapi-code-gen.rst| 9 ++---
>> tests/qapi-schema/qapi-schema-test.json | 7 +--
>> tests/qapi-schema/qapi-schema-test.out | 5 +
>> 3 files changed, 16 insertions(+), 5 deletions(-)
>
> [...]
commit 415b71a9f6e5bc37e84895d2e767cf4cfacd279b (HEAD)
Author: Markus Armbruster
Date: Sat Oct 9 09:01:21 2021 +0200
qapi: Find x- without feature unstable DBG
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b7b3fc0ce4..f2af1d7eea 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -14,6 +14,8 @@
# TODO catching name collisions in generated code would be nice
+import sys
+
from collections import OrderedDict
import os
import re
@@ -118,6 +120,11 @@ def describe(self):
return "%s '%s'" % (self.meta, self.name)
+def check_have_feature_unstable(name, info, features):
+if name.startswith('x-') and 'unstable' not in (f.name for f in features):
+print(QAPISemError(info, f"XXX %{name} %{features}"), file=sys.stderr)
+
+
class QAPISchemaVisitor:
def visit_begin(self, schema):
pass
@@ -718,6 +725,7 @@ def __init__(self, name, info, ifcond=None, features=None):
self.features = features or []
def connect_doc(self, doc):
+check_have_feature_unstable(self.name, self.info, self.features)
super().connect_doc(doc)
if doc:
for f in self.features:
@@ -745,6 +753,7 @@ def __init__(self, name, info, typ, optional, ifcond=None,
features=None):
self.features = features or []
def check(self, schema):
+check_have_feature_unstable(self.name, self.info, self.features)
assert self.defined_in
self.type = schema.resolve_type(self._type_name, self.info,
self.describe)
@@ -789,6 +798,7 @@ def __init__(self, name, info, doc, ifcond, features,
def check(self, schema):
super().check(schema)
+check_have_feature_unstable(self.name,
Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
John Snow writes:
> On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote:
>
>> Add special feature 'unstable' everywhere the name starts with 'x-',
>> except for InputBarrierProperties member x-origin and
>> MemoryBackendProperties member x-use-canonical-path-for-ramblock-id,
>> because these two are actually stable.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> qapi/block-core.json | 123 +++
>> qapi/migration.json | 35 +---
>> qapi/misc.json | 6 ++-
>> qapi/qom.json| 11 ++--
>> 4 files changed, 130 insertions(+), 45 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6d3217abb6..ce2c1352cb 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1438,6 +1438,9 @@
>> #
>> # @x-perf: Performance options. (Since 6.0)
>> #
>> +# Features:
>> +# @unstable: Member @x-perf is experimental.
>> +#
>>
>
> It'd be a lot cooler if we could annotate the unstable member directly
> instead of confusing it with the syntax that might describe the entire
> struct/union/command/etc, but ... eh, it's just a doc field, so I'm not
> gonna press on this. I don't have the energy to get into a doc formatting
> standard discussion right now, so: sure, why not?
By design, we have a single doc comment block for the entire definition.
When Kevin invented feature flags (merge commit 4747524f9f2), he added
them just to struct types. He added "feature sections" for documenting
features. It mirrors the "argument sections" for documenting members.
Makes sense.
Aside: he neglected to update qapi-code-gen.rst section "Definition
documentation", and I failed to catch it. I'll make up for it.
Peter and I then added feature flags to the remaining definitions
(commit 23394b4c39 and 013b4efc9b). Feature sections make sense there,
too.
I then added them to struct members (commit 84ab008687). Instead of
doing something fancy for documenting feature flags of members, I simply
used the existing feature sections. This conflates member features with
struct features.
What could "something fancy" be? All we have for members is "argument
sections", which are basically a name plus descriptive text. We'd have
to add structure to that, say nest feature sections into argument
sections. I have no appetite for that right now.
>
>
>> # Note: @on-source-error and @on-target-error only affect background
>> # I/O. If an error occurs during a guest write request, the
>> device's
>> # rerror/werror actions will be used.
>> @@ -1452,7 +1455,9 @@
>> '*on-source-error': 'BlockdevOnError',
>> '*on-target-error': 'BlockdevOnError',
>> '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
>> -'*filter-node-name': 'str', '*x-perf': 'BackupPerf' } }
>> +'*filter-node-name': 'str',
>> +'*x-perf': { 'type': 'BackupPerf',
>> + 'features': [ 'unstable' ] } } }
>>
>> ##
>> # @DriveBackup:
[...]
> Seems OK, but I didn't audit for false positives/negatives. Trusting your
> judgment here. (It looks like Phil started to audit this in his reply to
> your previous commit, so I'll trust that.)
I'm pretty sure the x- names that don't get feature 'unstable' are
actually stable; see my reply to Kashyap.
I did check git history for each that does get feature 'unstable'.
Double-checking is of course welcome.
> Acked-by: John Snow
Thanks!
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
On Tue, Oct 26, 2021 at 09:15:30AM +0200, Markus Armbruster wrote:
> Kashyap Chamarthy writes:
>
> > On Mon, Oct 25, 2021 at 07:25:24AM +0200, Markus Armbruster wrote:
[...]
> > Looks like there's another stable property with an "x-" prefix:
> > "x-remote-object", part of QOM type @RemoteObjectProperties.
>
> The union branch 'x-remote-object' isn't flagged 'unstable' (because
> union branches can't have feature flags), but the enumeration value
> 'x-remote-object' is. Sufficient, because you can't use the branch
> without using the enumeration value. Admittedly subtle.
Ah, thanks for the explanation. I missed the union branch part, which
I now notice in your "[PATCH 2/9] qapi: Mark unstable QMP parts
with feature 'unstable'".
> I wrote a bit of code (appended) to make sure I don't miss names.
Thanks; looks good to me.
> > Given the above "x-" properties are now stable, I take it that they
> > cannot be renamed now, as they might break any tools using them? My
> > guess is the tedious way is not worth it: deprecate them, and add the
> > non-x variants as "synonyms".
>
> "x-use-canonical-path-for-ramblock-id" goes back to commit fa0cb34d22
> "hostmem: use object id for memory region name with >= 4.0" (v4.0). It
> may have been intended to be internal back then. It wasn't anymore when
> commit 8db0b20415 "machine: add missing doc for memory-backend option"
> (v6.0) documented it:
>
> And document that x-use-canonical-path-for-ramblock-id,
> is considered to be stable to make sure it won't go away by accident.
>
> x- was intended for unstable/iternal properties, and not supposed to
> be stable option. However it's too late to rename (drop x-)
> it as it would mean that users will have to mantain both
> x-use-canonical-path-for-ramblock-id (for QEMU 5.0-5.2) versions
> and prefix-less for later versions.
>
> Igor's reasoning still applies.
>
> "x-origin" has always been stable. Same argument.
Yep, fair enough.
[...]
> commit 415b71a9f6e5bc37e84895d2e767cf4cfacd279b (HEAD)
> Author: Markus Armbruster
> Date: Sat Oct 9 09:01:21 2021 +0200
>
> qapi: Find x- without feature unstable DBG
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index b7b3fc0ce4..f2af1d7eea 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -14,6 +14,8 @@
>
> # TODO catching name collisions in generated code would be nice
>
> +import sys
> +
> from collections import OrderedDict
> import os
> import re
> @@ -118,6 +120,11 @@ def describe(self):
> return "%s '%s'" % (self.meta, self.name)
>
>
> +def check_have_feature_unstable(name, info, features):
> +if name.startswith('x-') and 'unstable' not in (f.name for f in
> features):
> +print(QAPISemError(info, f"XXX %{name} %{features}"),
> file=sys.stderr)
> +
> +
> class QAPISchemaVisitor:
> def visit_begin(self, schema):
> pass
> @@ -718,6 +725,7 @@ def __init__(self, name, info, ifcond=None,
> features=None):
> self.features = features or []
>
> def connect_doc(self, doc):
> +check_have_feature_unstable(self.name, self.info, self.features)
> super().connect_doc(doc)
> if doc:
> for f in self.features:
> @@ -745,6 +753,7 @@ def __init__(self, name, info, typ, optional,
> ifcond=None, features=None):
> self.features = features or []
>
> def check(self, schema):
> +check_have_feature_unstable(self.name, self.info, self.features)
> assert self.defined_in
> self.type = schema.resolve_type(self._type_name, self.info,
> self.describe)
> @@ -789,6 +798,7 @@ def __init__(self, name, info, doc, ifcond, features,
>
> def check(self, schema):
> super().check(schema)
> +check_have_feature_unstable(self.name, self.info, self.features)
> if self._arg_type_name:
> self.arg_type = schema.resolve_type(
> self._arg_type_name, self.info, "command's 'data'")
> @@ -844,6 +854,7 @@ def __init__(self, name, info, doc, ifcond, features,
> arg_type, boxed):
>
> def check(self, schema):
> super().check(schema)
> +check_have_feature_unstable(self.name, self.info, self.features)
> if self._arg_type_name:
> self.arg_type = schema.resolve_type(
> self._arg_type_name, self.info, "event's 'data'")
TIL: the error class QAPISemError()
Reviewed-by: Kashyap Chamarthy
--
/kashyap
Re: [PATCH 3/9] qapi: Eliminate QCO_NO_OPTIONS for a slight simplification
John Snow writes:
> On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote:
>
>> Signed-off-by: Markus Armbruster
>> ---
>> include/qapi/qmp/dispatch.h | 1 -
>> monitor/misc.c | 3 +--
>> scripts/qapi/commands.py| 5 +
>> 3 files changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
>> index 075203dc67..0ce88200b9 100644
>> --- a/include/qapi/qmp/dispatch.h
>> +++ b/include/qapi/qmp/dispatch.h
>> @@ -21,7 +21,6 @@ typedef void (QmpCommandFunc)(QDict *, QObject **, Error
>> **);
>>
>> typedef enum QmpCommandOptions
>> {
>> -QCO_NO_OPTIONS= 0x0,
>> QCO_NO_SUCCESS_RESP = (1U << 0),
>> QCO_ALLOW_OOB = (1U << 1),
>> QCO_ALLOW_PRECONFIG = (1U << 2),
>> diff --git a/monitor/misc.c b/monitor/misc.c
>> index ffe7966870..3556b177f6 100644
>> --- a/monitor/misc.c
>> +++ b/monitor/misc.c
>> @@ -230,8 +230,7 @@ static void monitor_init_qmp_commands(void)
>>
>> qmp_init_marshal(_commands);
>>
>> -qmp_register_command(_commands, "device_add", qmp_device_add,
>> - QCO_NO_OPTIONS);
>> +qmp_register_command(_commands, "device_add", qmp_device_add, 0);
>>
>> QTAILQ_INIT(_cap_negotiation_commands);
>> qmp_register_command(_cap_negotiation_commands,
>> "qmp_capabilities",
>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> index 3654825968..c8a975528f 100644
>> --- a/scripts/qapi/commands.py
>> +++ b/scripts/qapi/commands.py
>> @@ -229,15 +229,12 @@ def gen_register_command(name: str,
>> if coroutine:
>> options += ['QCO_COROUTINE']
>>
>> -if not options:
>> -options = ['QCO_NO_OPTIONS']
>> -
>> ret = mcgen('''
>> qmp_register_command(cmds, "%(name)s",
>> qmp_marshal_%(c_name)s, %(opts)s);
>> ''',
>> name=name, c_name=c_name(name),
>> -opts=" | ".join(options))
>> +opts=' | '.join(options) or 0)
>> return ret
>>
>>
>>
> I'm not a big fan of naked constants on the C side, but the generator
> simplification is nice. I suppose it's worth the trade-off if you like it
> better this way.
>
> "eh".
I think 0 is an okay way to say "no flags, nothing to see here, move
on".
> Reviewed-by: John Snow
Thanks!
Re: [PATCH v2 1/4] qemu-img: implement compare --stat
25.10.2021 19:40, Hanna Reitz wrote:
On 21.10.21 12:12, Vladimir Sementsov-Ogievskiy wrote:
With new option qemu-img compare will not stop at first mismatch, but
instead calculate statistics: how many clusters with different data,
how many clusters with equal data, how many clusters were unallocated
but become data and so on.
We compare images chunk by chunk. Chunk size depends on what
block_status returns for both images. It may return less than cluster
(remember about qcow2 subclusters), it may return more than cluster (if
several consecutive clusters share same status). Finally images may
have different cluster sizes. This all leads to ambiguity in how to
finally compare the data.
What we can say for sure is that, when we compare two qcow2 images with
same cluster size, we should compare clusters with data separately.
Otherwise, if we for example compare 10 consecutive clusters of data
where only one byte differs we'll report 10 different clusters.
Expected result in this case is 1 different cluster and 9 equal ones.
So, to serve this case and just to have some defined rule let's do the
following:
1. Select some block-size for compare procedure. In this commit it must
be specified by user, next commit will add some automatic logic and
make --block-size optional.
2. Go chunk-by-chunk using block_status as we do now with only one
differency:
If block_status() returns DATA region that intersects block-size
aligned boundary, crop this region at this boundary.
This way it's still possible to compare less than cluster and report
subcluster-level accuracy, but we newer compare more than one cluster
of data.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
docs/tools/qemu-img.rst | 18 +++-
qemu-img.c | 206 +---
qemu-img-cmds.hx | 4 +-
3 files changed, 212 insertions(+), 16 deletions(-)
Looks good to me overall! Just some technical comments below.
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index d58980aef8..21164253d4 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -159,6 +159,18 @@ Parameters to compare subcommand:
Strict mode - fail on different image size or sector allocation
+.. option:: --stat
+
+ Instead of exit on first mismatch compare the whole images and print
+ statistics on amount of different pairs of clusters, based on their
+ block-status and are they equal or not.
I’d phrase this as:
Instead of exiting on the first mismatch, compare the whole images and print
statistics on how much they differ in terms of block status (i.e. are blocks
allocated or not, do they contain data, are they marked as containing only
zeroes) and block content (a block of data that contains only zero still has
the same content as a marked-zero block).
For me the rest starting from "and block content" sounds unclear, seems doesn't
add any information to previous (i.e. are blocks allocated ...)
+
+.. option:: --block-size BLOCK_SIZE
+
+ Block size for comparing with ``--stat``. This doesn't guarantee exact
+ size of comparing chunks, but that guarantee that data chunks being
+ compared will never cross aligned block-size boundary.
I’d do just some minor tweaks to the second sentence:
This doesn't guarantee an exact size for comparing chunks, but it does
guarantee that those chunks will never cross a block-size-aligned boundary.
OK, sounds good
+
Parameters to convert subcommand:
.. program:: qemu-img-convert
[...]
diff --git a/qemu-img.c b/qemu-img.c
index f036a1d428..79a0589167 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -83,6 +83,8 @@ enum {
OPTION_BITMAPS = 275,
OPTION_FORCE = 276,
OPTION_SKIP_BROKEN = 277,
+ OPTION_STAT = 277,
That doesn’t look ideal, I believe `OPTION_STAT` should have a different value
than `OPTION_SKIP_BROKEN`. (I guess a rebase is to blame?)
Oops
+ OPTION_BLOCK_SIZE = 278,
};
typedef enum OutputFormat {
@@ -1304,6 +1306,107 @@ static int check_empty_sectors(BlockBackend *blk,
int64_t offset,
return 0;
}
+#define IMG_CMP_STATUS_MASK (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | \
+ BDRV_BLOCK_ALLOCATED)
+#define IMG_CMP_STATUS_MAX (IMG_CMP_STATUS_MASK | BDRV_BLOCK_EOF)
+
+typedef struct ImgCmpStat {
+ /* stat: [ret: 0 is equal, 1 is not][status1][status2] -> n_bytes */
+ uint64_t stat[2][IMG_CMP_STATUS_MAX + 1][IMG_CMP_STATUS_MAX + 1];
`IMG_CMP_STATUS_MAX` isn’t packed tightly because it only has four bits set
(0x33). That in itself isn’t a problem, but it means that `IMG_CMP_STATUS_MAX
+ 1` is 52, and so this array’s size is 52 * 52 * 2 * sizeof(uint64_t) = 43264.
Again, that isn’t a problem in itself (although it is a bit sad that this
could fit into 16 * 16 * 2 * 8 = 4 kB), but in `img_compare()` [1], you put
this structure on the stack, and I believe it’s too big for that.
Hmm. May be, it's better just use GHashTables and don't
Re: [PATCH 1/9] qapi: New special feature flag "unstable"
Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben: > By convention, names starting with "x-" are experimental. The parts > of external interfaces so named may be withdrawn or changed > incompatibly in future releases. > > Drawback: promoting something from experimental to stable involves a > name change. Client code needs to be updated. > > Moreover, the convention is not universally observed: > > * QOM type "input-barrier" has properties "x-origin", "y-origin". > Looks accidental, but it's ABI since 4.2. > > * QOM types "memory-backend-file", "memory-backend-memfd", > "memory-backend-ram", and "memory-backend-epc" have a property > "x-use-canonical-path-for-ramblock-id" that is documented to be > stable despite its name. > > We could document these exceptions, but documentation helps only > humans. We want to recognize "unstable" in code, like "deprecated". > > Replace the convention by a new special feature flag "unstable". It > will be recognized by the QAPI generator, like the existing feature > flag "deprecated", and unlike regular feature flags. > > This commit updates documentation and prepares tests. The next commit > updates the QAPI schema. The remaining patches update the QAPI > generator and wire up -compat policy checking. > > Signed-off-by: Markus Armbruster Obviously, replacing the old convention gets rid of the old drawbacks, but adds a new one: While using x- makes it very obvious for a human user that this is an unstable feature, a feature flag in the schema will almost certainly go unnoticed in manual use. Kevin
