Re: [PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces

[Markus Armbruster]

"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 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Markus Armbruster]

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"

[Markus Armbruster]

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 v4 5/5] block: Deprecate transaction type drive-backup

[Markus Armbruster]

John Snow  writes:

> On Mon, Oct 25, 2021 at 12:24 AM Markus Armbruster 
> wrote:
>
>> Several moons ago, Vladimir posted
>>
>> Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
>> Date: Wed,  5 May 2021 16:58:03 +0300
>> Message-Id: <20210505135803.67896-4-vsement...@virtuozzo.com>
>> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html
>>
>> with this
>>
>> TODO: We also need to deprecate drive-backup transaction action..
>> But union members in QAPI doesn't support 'deprecated' feature. I tried
>> to dig a bit, but failed :/ Markus, could you please help with it? At
>> least by advice?
>>
>> This is one way to resolve it.  Sorry it took so long.
>>
>>
> I'll share the blame for not pushing back on the other series, but ...
>
>
>> John explored another way, namely adding feature flags to union
>> branches.  Could also be useful, say to add different features to
>> branches in multiple unions sharing the same tag enum.
>>
>>
> ... this way seems simpler for now, and I trust your intuition on what's
> easier to support as I don't have a solid grasp of the C interfaces at play
> for actually parsing the input. We can always revisit the other thing later
> if/when we need it.
>
>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  qapi/transaction.json | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index d175b5f863..381a2df782 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -54,6 +54,10 @@
>>  # @blockdev-snapshot-sync: since 1.1
>>  # @drive-backup: Since 1.6
>>  #
>> +# Features:
>> +# @deprecated: Member @drive-backup is deprecated.  Use member
>> +#  @blockdev-backup instead.
>> +#
>>  # Since: 1.1
>>  ##
>>  { 'enum': 'TransactionActionKind',
>> @@ -62,7 +66,7 @@
>>  'block-dirty-bitmap-disable', 'block-dirty-bitmap-merge',
>>  'blockdev-backup', 'blockdev-snapshot',
>>  'blockdev-snapshot-internal-sync', 'blockdev-snapshot-sync',
>> -'drive-backup' ] }
>> +{ 'name': 'drive-backup', 'features': [ 'deprecated' ] } ] }
>>
>>  ##
>>  # @AbortWrapper:
>> --
>> 2.31.1
>>
>>
> Seems pretty clean to me overall. What's the reason for wanting it to be
> RFC?

I believe it depends on the remainder of Vladimir's series.

[PATCH v2 4/4] MAINTAINERS: Agree to maintain nanoMIPS TCG frontend

[Philippe Mathieu-Daudé]

As of this commit, the nanoMIPS toolchains haven't been merged
in mainstream projects. However MediaTek provides a toolchain:
https://github.com/MediaTek-Labs/nanomips-gnu-toolchain/releases/tag/nanoMIPS-2021.02-01

QEMU deprecation policy schedules code for removal. If we don't
need / want to remove, we should un-deprecated [*].

Since I now have spent more time with the ISA, I agree to maintain
it along with the other MIPS ISA. Therefore remove its deprecation
notice.

For historical notes, see commit a60442eb8 ("Deprecate nanoMIPS ISA").

[*] https://lore.kernel.org/qemu-devel/yvx7ygquenp83...@redhat.com/

Cc: Vince Del Vecchio 
Cc: Petar Jovanovic 
Reviewed-by: Jiaxun Yang 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: un-deprecate (danpb)
---
 docs/about/deprecated.rst | 23 ---
 MAINTAINERS   |  6 +-
 2 files changed, 1 insertion(+), 28 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 0bed6ecb1da..5f4e4eeb2b0 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -246,13 +246,6 @@ System emulator CPUS
 ``Icelake-Client`` CPU Models are deprecated. Use ``Icelake-Server`` CPU
 Models instead.
 
-MIPS ``I7200`` CPU Model (since 5.2)
-
-
-The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
-(the ISA has never been upstreamed to a compiler toolchain). Therefore
-this CPU is also deprecated.
-
 
 QEMU API (QAPI) events
 --
@@ -342,13 +335,6 @@ The ``ppc64abi32`` architecture has a number of issues 
which regularly
 trip up our CI testing and is suspected to be quite broken. For that
 reason the maintainers strongly suspect no one actually uses it.
 
-MIPS ``I7200`` CPU (since 5.2)
-''
-
-The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
-(the ISA has never been upstreamed to a compiler toolchain). Therefore
-this CPU is also deprecated.
-
 Related binaries
 
 
@@ -380,12 +366,3 @@ point to a version that doesn't break runnability 
guarantees
 versions, aliases will point to newer CPU model versions
 depending on the machine type, so management software must
 resolve CPU model aliases before starting a virtual machine.
-
-Guest Emulator ISAs

-
-nanoMIPS ISA
-
-
-The ``nanoMIPS`` ISA has never been upstreamed to any compiler toolchain.
-As it is hard to generate binaries for it, declare it deprecated.
diff --git a/MAINTAINERS b/MAINTAINERS
index efcfa57cd6a..71b198139c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -237,14 +237,10 @@ R: Aleksandar Rikalo 
 S: Odd Fixes
 F: target/mips/
 F: disas/mips.c
+X: disas/nanomips.*
 F: docs/system/cpu-models-mips.rst.inc
 F: tests/tcg/mips/
 
-MIPS TCG CPUs (nanoMIPS ISA)
-S: Orphan
-F: disas/nanomips.*
-F: target/mips/tcg/*nanomips*
-
 NiosII TCG CPUs
 M: Chris Wulff 
 M: Marek Vasut 
-- 
2.31.1

[PATCH v2 1/4] MAINTAINERS: Add MIPS general architecture support entry

[Philippe Mathieu-Daudé]

The architecture is covered in TCG (frontend and backend)
and hardware models. Add a generic section matching the
'mips' word in patch subjects.

Reviewed-by: Jiaxun Yang 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20211004092515.3819836-2-f4...@amsat.org>
---
 MAINTAINERS | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 894dc431052..5369fe2a129 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -109,6 +109,12 @@ K: ^Subject:.*(?i)s390x?
 T: git https://gitlab.com/cohuck/qemu.git s390-next
 L: qemu-s3...@nongnu.org
 
+MIPS general architecture support
+M: Philippe Mathieu-Daudé 
+R: Jiaxun Yang 
+S: Odd Fixes
+K: ^Subject:.*(?i)mips
+
 Guest CPU cores (TCG)
 -
 Overall TCG CPUs
@@ -242,7 +248,6 @@ F: include/hw/mips/
 F: include/hw/misc/mips_*
 F: include/hw/timer/mips_gictimer.h
 F: tests/tcg/mips/
-K: ^Subject:.*(?i)mips
 
 MIPS TCG CPUs (nanoMIPS ISA)
 S: Orphan
-- 
2.31.1

[PATCH v2 2/4] MAINTAINERS: Add entries to cover MIPS CPS / GIC hardware

[Philippe Mathieu-Daudé]

MIPS CPS and GIC models are unrelated to the TCG frontend.
Move them as new sections under the 'Devices' group.

Cc: Paul Burton 
Reviewed-by: Jiaxun Yang 
Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5369fe2a129..62a37aa94b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -239,14 +239,8 @@ F: target/mips/
 F: configs/devices/mips*/*
 F: disas/mips.c
 F: docs/system/cpu-models-mips.rst.inc
-F: hw/intc/mips_gic.c
 F: hw/mips/
-F: hw/misc/mips_*
-F: hw/timer/mips_gictimer.c
-F: include/hw/intc/mips_gic.h
 F: include/hw/mips/
-F: include/hw/misc/mips_*
-F: include/hw/timer/mips_gictimer.h
 F: tests/tcg/mips/
 
 MIPS TCG CPUs (nanoMIPS ISA)
@@ -2270,6 +2264,20 @@ S: Odd Fixes
 F: hw/intc/openpic.c
 F: include/hw/ppc/openpic.h
 
+MIPS CPS
+M: Philippe Mathieu-Daudé 
+S: Odd Fixes
+F: hw/misc/mips_*
+F: include/hw/misc/mips_*
+
+MIPS GIC
+M: Philippe Mathieu-Daudé 
+S: Odd Fixes
+F: hw/intc/mips_gic.c
+F: hw/timer/mips_gictimer.c
+F: include/hw/intc/mips_gic.h
+F: include/hw/timer/mips_gictimer.h
+
 Subsystems
 --
 Overall Audio backends
-- 
2.31.1

[PATCH v2 0/4] MAINTAINERS: Sanitize 'MIPS TCG CPUs' section

[Philippe Mathieu-Daudé]

Move various files unrelated to MIPS TCG frontend into
new sections.

Since v1:
- Do not add Paul without his consent
- un-deprecate nanoMIPS

Philippe Mathieu-Daudé (4):
  MAINTAINERS: Add MIPS general architecture support entry
  MAINTAINERS: Add entries to cover MIPS CPS / GIC hardware
  MAINTAINERS: Split MIPS TCG frontend vs MIPS machines/hardware
  MAINTAINERS: Agree to maintain nanoMIPS TCG frontend

 docs/about/deprecated.rst | 23 -
 MAINTAINERS   | 43 +--
 2 files changed, 28 insertions(+), 38 deletions(-)

-- 
2.31.1

[PATCH v2 3/4] MAINTAINERS: Split MIPS TCG frontend vs MIPS machines/hardware

[Philippe Mathieu-Daudé]

Hardware emulated models don't belong to the TCG MAINTAINERS
section. Move them to a new 'Overall MIPS Machines' section
in the 'MIPS Machines' group.

Reviewed-by: Jiaxun Yang 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20211004092515.3819836-4-f4...@amsat.org>
---
 MAINTAINERS | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 62a37aa94b5..efcfa57cd6a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -236,11 +236,8 @@ R: Jiaxun Yang 
 R: Aleksandar Rikalo 
 S: Odd Fixes
 F: target/mips/
-F: configs/devices/mips*/*
 F: disas/mips.c
 F: docs/system/cpu-models-mips.rst.inc
-F: hw/mips/
-F: include/hw/mips/
 F: tests/tcg/mips/
 
 MIPS TCG CPUs (nanoMIPS ISA)
@@ -1169,6 +1166,13 @@ F: hw/microblaze/petalogix_ml605_mmu.c
 
 MIPS Machines
 -
+Overall MIPS Machines
+M: Philippe Mathieu-Daudé 
+S: Odd Fixes
+F: configs/devices/mips*/*
+F: hw/mips/
+F: include/hw/mips/
+
 Jazz
 M: Hervé Poussineau 
 R: Aleksandar Rikalo 
-- 
2.31.1

Re: [RFC] Allowing SEV attestation

[Jim Fehlig]

On 5/6/21 04:22, Michal Prívozník wrote:

Dear list,


Hi Michal,

This thread has been quiet for a long time, but I wanted to check if any work 
has been done to provide an sev-inject-launch-secret equivalent for libvirt. 
AFAICT, there was agreement this missing piece is needed to solve the 
attestation puzzle. Did you make any progress? If so, I can help with testing 
and review. If not, I can take a stab at it.


Regards,
Jim



in the light of recent development of secure virtualization (for instance AMD
SEV-SNP [1]) I'd like us to be prepared for when QEMU adopts these new
technologies and thus would like to discuss our options. So far, I've came
across AMD SEV-SNP [2]. While it's true that we do support SEV, we need to
adopt its newer version too.

Consider the following example: you loan a VM on a public cloud and you want
to process some private data there. Of course, the data has to be stored on an
encrypted disk/partition. But encrypting guest memory (confidentiality)
prevents you only from finding the key in a memory dump. It doesn't help if
the guest image, initial guest memory, hypervisor or firmware were tampered 
with.

This is where attestation comes to help - it enables the guest owner (which in
this example is different to the one running it) verify - with cryptographic
level of confidence - that data confidentiality and integrity can be ensured.

Depending on the technology, attestation will be done at different times in
the VM lifecycle. When it's done before starting guest vCPUs, it's called
pre-attestation and when it's done after the guest has launched, it's called
post-attestation. For example, AMD SEV and SEV-ES require pre-attestation.
SEV-SNP allows for post attestation. The crux of the issue here is that *when*
and *how* the guest owner will interact with the VM for attestation will be
different depending on which technology is being used.

It looks like QEMU will expose commands needed for attestation via QMP [3].
But question then is, how to expose those at Libvirt level? Should we allow
users to bypass Libvirt and communicate to QEMU directly or wrap those QMPs in
public APIs, or something completely different?

Will those APIs be generic enough to serve other technologies too? For
instance, given set of APIs in given order works for SEV but might not work
for Intel's TDX.


1: 
https://kvmforum2019.sched.com/event/Tmwl/improving-and-expanding-sev-support-thomas-lendacky-amd

2: 
https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

3: https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03689.html


Michal

Re: [PATCH v4 5/5] block: Deprecate transaction type drive-backup

[John Snow]

On Mon, Oct 25, 2021 at 12:24 AM Markus Armbruster 
wrote:

> Several moons ago, Vladimir posted
>
> Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
> Date: Wed,  5 May 2021 16:58:03 +0300
> Message-Id: <20210505135803.67896-4-vsement...@virtuozzo.com>
> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html
>
> with this
>
> TODO: We also need to deprecate drive-backup transaction action..
> But union members in QAPI doesn't support 'deprecated' feature. I tried
> to dig a bit, but failed :/ Markus, could you please help with it? At
> least by advice?
>
> This is one way to resolve it.  Sorry it took so long.
>
>
I'll share the blame for not pushing back on the other series, but ...


> John explored another way, namely adding feature flags to union
> branches.  Could also be useful, say to add different features to
> branches in multiple unions sharing the same tag enum.
>
>
... this way seems simpler for now, and I trust your intuition on what's
easier to support as I don't have a solid grasp of the C interfaces at play
for actually parsing the input. We can always revisit the other thing later
if/when we need it.


> Signed-off-by: Markus Armbruster 
> ---
>  qapi/transaction.json | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index d175b5f863..381a2df782 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -54,6 +54,10 @@
>  # @blockdev-snapshot-sync: since 1.1
>  # @drive-backup: Since 1.6
>  #
> +# Features:
> +# @deprecated: Member @drive-backup is deprecated.  Use member
> +#  @blockdev-backup instead.
> +#
>  # Since: 1.1
>  ##
>  { 'enum': 'TransactionActionKind',
> @@ -62,7 +66,7 @@
>  'block-dirty-bitmap-disable', 'block-dirty-bitmap-merge',
>  'blockdev-backup', 'blockdev-snapshot',
>  'blockdev-snapshot-internal-sync', 'blockdev-snapshot-sync',
> -'drive-backup' ] }
> +{ 'name': 'drive-backup', 'features': [ 'deprecated' ] } ] }
>
>  ##
>  # @AbortWrapper:
> --
> 2.31.1
>
>
Seems pretty clean to me overall. What's the reason for wanting it to be
RFC?

Re: [PATCH v4 3/5] qapi: Move compat policy from QObject to generic visitor

[John Snow]

On Mon, Oct 25, 2021 at 12:24 AM Markus Armbruster 
wrote:

> The next commit needs to access compat policy from the generic visitor
> core.  Move it there from qobject input and output visitor.
>
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Eric Blake 
>

"LGTM".

Re: [PATCH v4 2/5] qapi: Add feature flags to enum members

[John Snow]

On Mon, Oct 25, 2021 at 12:24 AM Markus Armbruster 
wrote:

> This is quite similar to commit 84ab008687 "qapi: Add feature flags to
> struct members", only for enums instead of structs.
>
> Special feature flag 'deprecated' is silently ignored there.  This is
> okay only because it will be implemented shortly.
>
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Eric Blake 
>

Reviewed-by: John Snow 


> ---
>  docs/devel/qapi-code-gen.rst  | 16 +-
>  qapi/compat.json  |  2 ++
>  qapi/introspect.json  |  5 -
>  scripts/qapi/expr.py  |  3 ++-
>  scripts/qapi/introspect.py|  5 +++--
>  scripts/qapi/schema.py| 22 +--
>  tests/qapi-schema/doc-good.json   |  5 -
>  tests/qapi-schema/doc-good.out|  3 +++
>  tests/qapi-schema/doc-good.txt|  3 +++
>  .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
>  tests/qapi-schema/qapi-schema-test.json   |  3 ++-
>  tests/qapi-schema/qapi-schema-test.out|  1 +
>  tests/qapi-schema/test-qapi.py|  1 +
>  13 files changed, 57 insertions(+), 14 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index d267889d2c..4071c9074a 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -200,7 +200,9 @@ Syntax::
>   '*if': COND,
>   '*features': FEATURES }
>  ENUM-VALUE = STRING
> -   | { 'name': STRING, '*if': COND }
> +   | { 'name': STRING,
> +   '*if': COND,
> +   '*features': FEATURES }
>
>  Member 'enum' names the enum type.
>
> @@ -706,8 +708,10 @@ QEMU shows a certain behaviour.
>  Special features
>  
>
> -Feature "deprecated" marks a command, event, or struct member as
> -deprecated.  It is not supported elsewhere so far.
> +Feature "deprecated" marks a command, event, enum value, or struct
> +member as deprecated.  It is not supported elsewhere so far.
> +Interfaces so marked may be withdrawn in future releases in accordance
> +with QEMU's deprecation policy.
>
>
>  Naming rules and reserved names
> @@ -1157,7 +1161,8 @@ and "variants".
>
>  "members" is a JSON array describing the object's common members, if
>  any.  Each element is a JSON object with members "name" (the member's
> -name), "type" (the name of its type), and optionally "default".  The
> +name), "type" (the name of its type), "features" (a JSON array of
> +feature strings), and "default".  The latter two are optional.  The
>  member is optional if "default" is present.  Currently, "default" can
>  only have value null.  Other values are reserved for future
>  extensions.  The "members" array is in no particular order; clients
> @@ -1234,7 +1239,8 @@ The SchemaInfo for an enumeration type has meta-type
> "enum" and
>  variant member "members".
>
>  "members" is a JSON array describing the enumeration values.  Each
> -element is a JSON object with member "name" (the member's name).  The
> +element is a JSON object with member "name" (the member's name), and
> +optionally "features" (a JSON array of feature strings).  The
>  "members" array is in no particular order; clients must search the
>  entire array when learning whether a particular value is supported.
>
> diff --git a/qapi/compat.json b/qapi/compat.json
> index ae3afc22df..1d2b76f00c 100644
> --- a/qapi/compat.json
> +++ b/qapi/compat.json
> @@ -42,6 +42,8 @@
>  # with feature 'deprecated'.  We may want to extend it to cover
>  # semantic aspects, CLI, and experimental features.
>  #
> +# Limitation: not implemented for deprecated enumeration values.
> +#
>  # @deprecated-input: how to handle deprecated input (default 'accept')
>  # @deprecated-output: how to handle deprecated output (default 'accept')
>  #
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 9683e884f8..183148b2e9 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -167,10 +167,13 @@
>  #
>  # @name: the member's name, as defined in the QAPI schema.
>  #
> +# @features: names of features associated with the member, in no
> +#particular order.
> +#
>  # Since: 6.2
>  ##
>  { 'struct': 'SchemaInfoEnumMember',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str', '*features': [ 'str' ] } }
>
>  ##
>  # @SchemaInfoArray:
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 819ea6ad97..3cb389e875 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -472,7 +472,7 @@ def check_enum(expr: _JSONObject, info:
> QAPISourceInfo) -> None:
>for m in members]
>  for member in members:
>  source = "'data' member"
> -check_keys(member, info, source, ['name'], ['if'])
> +check_keys(member, info, source, ['name'], ['if', 'features'])
>  member_name = 

Re: [PATCH v4 1/5] qapi: Enable enum member introspection to show more than name

[John Snow]

On Mon, Oct 25, 2021 at 12:24 AM Markus Armbruster 
wrote:

> The next commit will add feature flags to enum members.  There's a
> problem, though: query-qmp-schema shows an enum type's members as an
> array of member names (SchemaInfoEnum member @values).  If it showed
> an array of objects with a name member, we could simply add more
> members to these objects.  Since it's just strings, we can't.
>
> I can see three ways to correct this design mistake:
>
> 1. Do it the way we should have done it, plus compatibility goo.
>
>We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum.  Since
>changing @values would be a compatibility break, add a new member
>@members instead.
>
>@values is now redundant.  In my testing, output of
>qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).
>
>We can deprecate @values now and drop it later.  This will break
>outmoded clients.  Well-behaved clients such as libvirt are
>expected to break cleanly.
>
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>
>@values does not become redundant.  @members augments it.  Somewhat
>cumbersome, but output of query-qmp-schema grows only as we make
>enum members non-boring.
>
>There is nothing to deprecate here.
>
> 3. Versioned query-qmp-schema.
>
>query-qmp-schema provides either @values or @members.  The QMP
>client can select which version it wants.  There is no redundant
>output.
>
>We can deprecate old versions and eventually drop them.  This will
>break outmoded clients.  Breaking cleanly is easier than for 1.
>
>While 1 and 2 operate within the common rules for compatible
>evolution apply (section "Compatibility considerations" in
>docs/devel/qapi-code-gen.rst), 3 bypasses them.  Attractive when
>operating within the rules is just too awkward.  Not the case here.
>
> This commit implements 1.  Libvirt developers prefer it.
>
> Deprecate @values in favour of @members.  Since query-qmp-schema
> compatibility is pretty fundamental for management applications, an
> extended grace period is advised.
>
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Eric Blake 
> Tested-by: Peter Krempa 
> Acked-by: Peter Krempa 
>

Reviewed-by: John Snow 

Re: [PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces

[Dr. David Alan Gilbert]

* 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: [libvirt PATCH 1/4] PCI VPD: handle additional edge cases

[Dmitrii Shcherbakov]

Hi Daniel,

> Something is still not right with the logic here as this error report
is triggering on my machines. If I comment out this check, then I can
get data reported by libvirt

The VPD example you shared previously has multiple violations of the VPD format:

* Invalid field values ("N/A" in the read-only section, 0xFF in the
read-write section);
* The lack of the RW field at the end of the read-write section.

Previously, the VPD was discarded due to invalid values.

I thought honoring the RW requirement would still make sense and kept
the RW presence check.

I added it as a test case here (converted to hex):
https://listman.redhat.com/archives/libvir-list/2021-October/msg00871.html

The spec says the following (PCIe 4.0 "6.28.2.3 Read/Write Fields"):

"RW | Remaining Read/Write Area |
This descriptor is used to identify the unused portion of the
read/write space. The product
vendor initializes this parameter based on the size of the read/write
space or the space remaining
following the Vx VPD items. One or more of the Vx, Yx, and RW items
are required."

I can also confirm that by looking at the hexdump and looking for hex
values for R (0x52) and W (0x57):

hexdump -e '16/1 "0x%02x, " "\n"' vpd

I could ease it up and allow it to be missing as we do not care much
about it when reading so long as the resource
length is correct. However, if we were to allow Libvirt to write to
the read-write section in the future we would need to
know how much free space is there in the first place.

Re: [libvirt PATCH 1/4] PCI VPD: handle additional edge cases

[Daniel P . Berrangé]

On Fri, Oct 22, 2021 at 03:45:42PM +0300, Dmitrii Shcherbakov wrote:
> * RV and RW fields must be at the last position in their respective
>   section (per the conditions in the spec). Therefore, the parser now
>   stops iterating over fields as soon as it encounters one of those
>   fields and checks whether the end of the resource has been reached;
> * Individual fields must have a valid length - the parser needs to check
>   for invalid length values that violate boundary conditions of the
>   resource.
> * A zero-length field may be the last one in the resource, however, the
>   boundary check is currently too strict to allow that.
> 
> Signed-off-by: Dmitrii Shcherbakov 
> ---
>  src/util/virpcivpd.c  | 28 +++
>  tests/virpcivpdtest.c | 84 +++
>  2 files changed, 106 insertions(+), 6 deletions(-)
> 
> diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
> index 8856bca459..cd49031fa4 100644
> --- a/src/util/virpcivpd.c
> +++ b/src/util/virpcivpd.c
> @@ -466,8 +466,12 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
> uint16_t resPos, uint16_t re
>  
>  bool hasChecksum = false;
>  bool hasRW = false;
> +bool endReached = false;
>  
> -while (fieldPos + 3 < resPos + resDataLen) {
> +/* Note the equal sign - fields may have a zero length in which case 
> they will
> + * just occupy 3 header bytes. In the in case of the RW field this may 
> mean that
> + * no more space is left in the section. */
> +while (fieldPos + 3 <= resPos + resDataLen) {
>  /* Keyword resources consist of keywords (2 ASCII bytes per the 
> spec) and 1-byte length. */
>  if (virPCIVPDReadVPDBytes(vpdFileFd, buf, 3, fieldPos, csum) != 3) {
>  /* Invalid field encountered which means the resource itself is 
> invalid too. Report
> @@ -518,6 +522,13 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
> uint16_t resPos, uint16_t re
>  return false;
>  }
>  
> +if (resPos + resDataLen < fieldPos + fieldDataLen) {
> +/* In this case the field cannot simply be skipped since the 
> position of the
> + * next field is determined based on the length of a previous 
> field. */
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("A field data length violates the resource 
> length boundary."));
> +return false;
> +}
>  if (virPCIVPDReadVPDBytes(vpdFileFd, buf, bytesToRead, fieldPos, 
> csum) != bytesToRead) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Could not parse a resource field data - VPD 
> has invalid format"));
> @@ -546,12 +557,13 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
> uint16_t resPos, uint16_t re
>  hasChecksum = true;
>  g_free(g_steal_pointer());
>  g_free(g_steal_pointer());
> -continue;
> +break;
>  } else if (fieldFormat == 
> VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR) {
>  /* Skip the read-write space since it is used for indication 
> only. */
>  hasRW = true;
>  g_free(g_steal_pointer());
>  g_free(g_steal_pointer());
> +break;
>  } else if (fieldFormat == 
> VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST) {
>  /* Skip unknown fields */
>  g_free(g_steal_pointer());
> @@ -579,13 +591,17 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
> uint16_t resPos, uint16_t re
>  g_free(g_steal_pointer());
>  g_free(g_steal_pointer());
>  }
> -if (readOnly && !hasChecksum) {
> +
> +/* May have exited the loop prematurely in case RV or RW were 
> encountered and
> + * they were not the last fields in the section. */
> +endReached = (fieldPos >= resPos + resDataLen);
> +if (readOnly && !(hasChecksum && endReached)) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("VPD-R does not contain the mandatory RV field"));
> +_("VPD-R does not contain the mandatory RV field as the last 
> field"));
>  return false;
> -} else if (!readOnly && !hasRW) {
> +} else if (!readOnly && !(hasRW && endReached)) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("VPD-W does not contain the mandatory RW field"));
> +   _("VPD-W does not contain the mandatory RW field as 
> the last field"));
>  return false;

Something is still not right with the logic here as this error report
is triggering on my machines. If I comment out this check, then I can
get data reported by libvirt


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-

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[John Snow]

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: [libvirt PATCH 3/4] PCI VPD: Skip fields with invalid values

[Daniel P . Berrangé]

On Fri, Oct 22, 2021 at 03:45:44PM +0300, Dmitrii Shcherbakov wrote:
> While invalid values need to be ignored when presenting VPD data to the
> user, it would be good to attempt to parse a valid portion of the VPD
> instead of marking it invalid as a whole.
> 
> The particular example encountered on real hardware was twofold:
> 
> * "N/A" strings present in read-only fields. This would not be a useful
>   valid value for a field, not to mention that if the serial number
>   field with the "N/A" value was parsed, it would confuse higher-level
>   software because this isn't a unique serial for a device;


The higher level software needs to acccept that the BIOS vendor
might have included garbage and have a plan to deal with this.
Libvirt should apply a policy in this regards as it cann result
in libvirt rejecting valid data.

For example by rejecting "/" as a character,  we fail to report
valid data from mymachine.

If I allow "/" I get:


  HP Ethernet 1Gb 2-port 361i Adapter
  
N/A
N/A
N/A
4W/1W PCIeG2x4 2p 1GbE RJ45 Intel 
i350
  
  
N/A
5.7.06
2.8.20
1.5.35
  


sure the "n/a" is unhelpful, but libvirt is right to
include it.

I think perhaps rather than allow-listing the characters, we need
to just accept any field value that contains printable characters.

eg instead of the strspn, I'd suggest we do as "isprint()" check
on each character

> * 0xFF bytes present in VPD-W field values. Those bytes are not valid
>   values and were probably used by the vendor as placeholders. Ignoring
>   the whole VPD because of that would be too strict.
> 
> Signed-off-by: Dmitrii Shcherbakov 
> ---
>  src/util/virpcivpd.c  |   9 ++--
>  tests/virpcivpdtest.c | 105 ++
>  2 files changed, 102 insertions(+), 12 deletions(-)
> 
> diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
> index cd49031fa4..8c2b17c3a6 100644
> --- a/src/util/virpcivpd.c
> +++ b/src/util/virpcivpd.c
> @@ -544,9 +544,12 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
> uint16_t resPos, uint16_t re
>   */
>  fieldValue = g_strstrip(g_strndup((char *)buf, fieldDataLen));
>  if (!virPCIVPDResourceIsValidTextValue(fieldValue)) {

This method itself calls virReportError, so we still get stuff reported at
level ERROR in the logs. We need to remove the virReportError call in
this method.

> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("Field value contains invalid characters"));
> -return false;
> +/* Skip fields with invalid values - this is safe assuming 
> field length is
> + * correctly specified. */
> +VIR_DEBUG("A value for field %s contains invalid 
> characters", fieldKeyword);

We should include  fieldValue in the log, even if it might have non-printable
chars, as it'll be important debugging info.

> +g_free(g_steal_pointer());
> +g_free(g_steal_pointer());
> +continue;
>  }
>  } else if (fieldFormat == 
> VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) {
>  if (*csum) {

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 7/9] qapi: Generalize enum member policy checking

[John Snow]

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

[Philippe Mathieu-Daudé]

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"

[Daniel P . Berrangé]

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 :|

[PATCH v2] qemu_block: Refactor qemuBlockExportAddNBD()

[Kristina Hanicova]

This is v2 of: 
https://listman.redhat.com/archives/libvir-list/2021-October/msg01035.html

Diff to v1:
* negation of 'if' with switched body (proposed by Peter)

Signed-off-by: Kristina Hanicova 
---
 src/qemu/qemu_block.c | 33 +
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 34fdec2c4b..b8e70f6a83 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -3592,28 +3592,21 @@ qemuBlockExportAddNBD(virDomainObj *vm,
   const char *bitmap)
 {
 qemuDomainObjPrivate *priv = vm->privateData;
+g_autoptr(virJSONValue) nbdprops = NULL;
+const char *bitmaps[2] = { bitmap, NULL };
 
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD)) {
-g_autoptr(virJSONValue) nbdprops = NULL;
-const char *bitmaps[2] = { bitmap, NULL };
+/* older qemu versions didn't support configuring the exportname and
+ * took the 'drivealias' as the export name */
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
+return qemuMonitorNBDServerAdd(priv->mon, drivealias, NULL, writable, 
NULL);
 
-if (!(nbdprops = qemuBlockExportGetNBDProps(src->nodeformat,
-exportname,
-writable,
-bitmaps)))
-return -1;
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD))
+return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat,
+   exportname, writable, bitmap);
 
-return qemuMonitorBlockExportAdd(priv->mon, );
-} else {
-return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat,
-   exportname, writable, bitmap);
-}
-} else {
-/* older qemu versions didn't support configuring the exportname and
- * took the 'drivealias' as the export name */
-return qemuMonitorNBDServerAdd(priv->mon, drivealias, NULL, writable, 
NULL);
-}
+if (!(nbdprops = qemuBlockExportGetNBDProps(src->nodeformat, exportname,
+writable, bitmaps)))
+return -1;
 
-return 0;
+return qemuMonitorBlockExportAdd(priv->mon, );
 }
-- 
2.31.1

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

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: [PATCH] qemu_block: Refactor qemuBlockExportAddNBD()

[Peter Krempa]

On Tue, Oct 26, 2021 at 16:58:22 +0200, Kristina Hanicova wrote:
> I think this patch improves readability of the function by
> removing unnecessary 'else' branches after 'return' and reworking
> the code a bit.
> 
> Signed-off-by: Kristina Hanicova 
> ---
>  src/qemu/qemu_block.c | 33 +
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 34fdec2c4b..4e532d29c0 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -3592,28 +3592,21 @@ qemuBlockExportAddNBD(virDomainObj *vm,
>const char *bitmap)
>  {
>  qemuDomainObjPrivate *priv = vm->privateData;
> +g_autoptr(virJSONValue) nbdprops = NULL;
> +const char *bitmaps[2] = { bitmap, NULL };
>  
> -if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> -if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD)) {
> -g_autoptr(virJSONValue) nbdprops = NULL;
> -const char *bitmaps[2] = { bitmap, NULL };
> +/* older qemu versions didn't support configuring the exportname and
> + * took the 'drivealias' as the export name */
> +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
> +return qemuMonitorNBDServerAdd(priv->mon, drivealias, NULL, 
> writable, NULL);
>  
> -if (!(nbdprops = qemuBlockExportGetNBDProps(src->nodeformat,
> -exportname,
> -writable,
> -bitmaps)))
> -return -1;
> +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD))
> +return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat,
> +   exportname, writable, bitmap);

These two are okay, they invoke a different code path when features are
not supported ...

It will indeed help as we can simply delete them when the caps become
obsolete ...

>  
> -return qemuMonitorBlockExportAdd(priv->mon, );
> -} else {
> -return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat,
> -   exportname, writable, bitmap);
> -}
> -} else {
> -/* older qemu versions didn't support configuring the exportname and
> - * took the 'drivealias' as the export name */
> -return qemuMonitorNBDServerAdd(priv->mon, drivealias, NULL, 
> writable, NULL);
> -}
> +if ((nbdprops = qemuBlockExportGetNBDProps(src->nodeformat, exportname,
> +   writable, bitmaps)))
> +return qemuMonitorBlockExportAdd(priv->mon, );

but this is weird and doesn't conform to what we usually do. Write it
as:

if (!(nbdprops = qemuBlockExportGetNBDProps(src->nodeformat, exportname,
writable, bitmaps)))
return -1;

return qemuMonitorBlockExportAdd(priv->mon, );


>  
> -return 0;
> +return -1;

And get rid of the above.

>  }
> -- 
> 2.31.1
> 

Re: [PATCH 1/3] virthreadpool: Allow setting identity for workers

[Daniel P . Berrangé]

On Tue, Oct 26, 2021 at 04:49:55PM +0200, Michal Prívozník wrote:
> On 10/26/21 4:39 PM, Daniel P. Berrangé wrote:
> > On Tue, Oct 26, 2021 at 12:17:06PM +0200, Michal Privoznik wrote:
> >> In some cases the worker func running inside the pool may rely on
> >> virIdentity. While worker func could check for identity and set
> >> one it is not optimal - it may not have access to the identity of
> >> the thread creating the pool and thus would have to call
> >> virIdentityGetSystem(). Allow passing identity when creating the
> >> pool.
> > 
> > I wonder if we should have an identity passed via virThreadPoolSendJob,
> > so whatever queues the job can preserve its identity ?
> 
> I thought about this too, but the threat that's doing that doesn't have
> an identity set either:
> 
> #0  virThreadPoolSendJob (pool=0x7f2ff010a370, priority=0, 
> jobData=0x7f2fdc0b1f10) at ../src/util/virthreadpool.c:390
> #1  0x7f30301f5240 in qemuProcessEventSubmit (driver=0x7f2ff0022370, 
> event=0x7f3012ffc880) at ../src/qemu/qemu_process.c:300
> #2  0x7f30301f8d13 in qemuProcessHandleSerialChanged (mon=0x7f2ff00295d0, 
> vm=0x7f2ff0037830, devAlias=0x7f2fdc12f9e0 "channel0", connected=false, 
> opaque=0x7f2ff0022370) at ../src/qemu/qemu_process.c:1540
> #3  0x7f30301c37f2 in qemuMonitorEmitSerialChange (mon=0x7f2ff00295d0, 
> devAlias=0x7f2fdc12f9e0 "channel0", connected=false) at 
> ../src/qemu/qemu_monitor.c:1373
> #4  0x7f30301d48c8 in qemuMonitorJSONHandleSerialChange 
> (mon=0x7f2ff00295d0, data=0x7f2fdc0b2b40) at 
> ../src/qemu/qemu_monitor_json.c:1145
> #5  0x7f30301d1725 in qemuMonitorJSONIOProcessEvent (mon=0x7f2ff00295d0, 
> obj=0x7f2fdc0b2770) at ../src/qemu/qemu_monitor_json.c:208
> #6  0x7f30301d1933 in qemuMonitorJSONIOProcessLine (mon=0x7f2ff00295d0, 
> line=0x7f2fdc0b4cd0 "{\"timestamp\": {\"seconds\": 1635259527, 
> \"microseconds\": 380958}, \"event\": \"VSERPORT_CHANGE\", \"data\": 
> {\"open\": false, \"id\": \"channel0\"}}", msg=0x0) at 
> ../src/qemu/qemu_monitor_json.c:236
> #7  0x7f30301d1bcc in qemuMonitorJSONIOProcess (mon=0x7f2ff00295d0, 
> data=0x7f2fdc12f590 "{\"timestamp\": {\"seconds\": 1635259527, 
> \"microseconds\": 380958}, \"event\": \"VSERPORT_CHANGE\", \"data\": 
> {\"open\": false, \"id\": \"channel0\"}}\r\n", len=135, msg=0x0) at 
> ../src/qemu/qemu_monitor_json.c:274
> #8  0x7f30301c0400 in qemuMonitorIOProcess (mon=0x7f2ff00295d0) at 
> ../src/qemu/qemu_monitor.c:347
> #9  0x7f30301c0c80 in qemuMonitorIO (socket=0x7f2ff0017280, cond=0, 
> opaque=0x7f2ff00295d0) at ../src/qemu/qemu_monitor.c:573
> #10 0x7f3036ba8c77 in socket_source_dispatch () at 
> /usr/lib64/libgio-2.0.so.0
> #11 0x7f3036d8b9d0 in g_main_context_dispatch () at 
> /usr/lib64/libglib-2.0.so.0
> #12 0x7f3036d8bd78 in g_main_context_iterate.constprop () at 
> /usr/lib64/libglib-2.0.so.0
> #13 0x7f3036d8c06b in g_main_loop_run () at /usr/lib64/libglib-2.0.so.0
> #14 0x7f3036f81d7b in virEventThreadWorker (opaque=0x7f2ff00b10c0) at 
> ../src/util/vireventthread.c:124
> #15 0x7f3036db52bd in g_thread_proxy () at /usr/lib64/libglib-2.0.so.0
> #16 0x7f3036235e3e in start_thread () at /lib64/libpthread.so.0
> #17 0x7f30369ffd6f in clone () at /lib64/libc.so.6
> 
> virThreadPoolSendJob 19 $ call virIdentityGetCurrent()
> $1 = 0x0

Hmm, yes, because that's from the event loop thread.

This actually makes me wonder if we actually need the worker pool at all
here. The worker pool is global to the driver, so events for all VMs are
being processed in the pool and so will contend for resource.

This made sense when all monitor I/O was procssed by the main event
loop thread, but now we have a per-VM event loop thread. Seems like
we could just process events directly in the per-VM thread potentially,
unless something they do synchronously needs to do more monitor I/O

None the less, that's too big of a change todo in a hurry, so lets
go with your patches here.

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"

[Daniel P . Berrangé]

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 :|

[PATCH] qemu_block: Refactor qemuBlockExportAddNBD()

[Kristina Hanicova]

I think this patch improves readability of the function by
removing unnecessary 'else' branches after 'return' and reworking
the code a bit.

Signed-off-by: Kristina Hanicova 
---
 src/qemu/qemu_block.c | 33 +
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 34fdec2c4b..4e532d29c0 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -3592,28 +3592,21 @@ qemuBlockExportAddNBD(virDomainObj *vm,
   const char *bitmap)
 {
 qemuDomainObjPrivate *priv = vm->privateData;
+g_autoptr(virJSONValue) nbdprops = NULL;
+const char *bitmaps[2] = { bitmap, NULL };
 
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD)) {
-g_autoptr(virJSONValue) nbdprops = NULL;
-const char *bitmaps[2] = { bitmap, NULL };
+/* older qemu versions didn't support configuring the exportname and
+ * took the 'drivealias' as the export name */
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
+return qemuMonitorNBDServerAdd(priv->mon, drivealias, NULL, writable, 
NULL);
 
-if (!(nbdprops = qemuBlockExportGetNBDProps(src->nodeformat,
-exportname,
-writable,
-bitmaps)))
-return -1;
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD))
+return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat,
+   exportname, writable, bitmap);
 
-return qemuMonitorBlockExportAdd(priv->mon, );
-} else {
-return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat,
-   exportname, writable, bitmap);
-}
-} else {
-/* older qemu versions didn't support configuring the exportname and
- * took the 'drivealias' as the export name */
-return qemuMonitorNBDServerAdd(priv->mon, drivealias, NULL, writable, 
NULL);
-}
+if ((nbdprops = qemuBlockExportGetNBDProps(src->nodeformat, exportname,
+   writable, bitmaps)))
+return qemuMonitorBlockExportAdd(priv->mon, );
 
-return 0;
+return -1;
 }
-- 
2.31.1

Re: [PATCH 1/3] virthreadpool: Allow setting identity for workers

[Michal Prívozník]

On 10/26/21 4:39 PM, Daniel P. Berrangé wrote:
> On Tue, Oct 26, 2021 at 12:17:06PM +0200, Michal Privoznik wrote:
>> In some cases the worker func running inside the pool may rely on
>> virIdentity. While worker func could check for identity and set
>> one it is not optimal - it may not have access to the identity of
>> the thread creating the pool and thus would have to call
>> virIdentityGetSystem(). Allow passing identity when creating the
>> pool.
> 
> I wonder if we should have an identity passed via virThreadPoolSendJob,
> so whatever queues the job can preserve its identity ?

I thought about this too, but the threat that's doing that doesn't have
an identity set either:

#0  virThreadPoolSendJob (pool=0x7f2ff010a370, priority=0, 
jobData=0x7f2fdc0b1f10) at ../src/util/virthreadpool.c:390
#1  0x7f30301f5240 in qemuProcessEventSubmit (driver=0x7f2ff0022370, 
event=0x7f3012ffc880) at ../src/qemu/qemu_process.c:300
#2  0x7f30301f8d13 in qemuProcessHandleSerialChanged (mon=0x7f2ff00295d0, 
vm=0x7f2ff0037830, devAlias=0x7f2fdc12f9e0 "channel0", connected=false, 
opaque=0x7f2ff0022370) at ../src/qemu/qemu_process.c:1540
#3  0x7f30301c37f2 in qemuMonitorEmitSerialChange (mon=0x7f2ff00295d0, 
devAlias=0x7f2fdc12f9e0 "channel0", connected=false) at 
../src/qemu/qemu_monitor.c:1373
#4  0x7f30301d48c8 in qemuMonitorJSONHandleSerialChange 
(mon=0x7f2ff00295d0, data=0x7f2fdc0b2b40) at 
../src/qemu/qemu_monitor_json.c:1145
#5  0x7f30301d1725 in qemuMonitorJSONIOProcessEvent (mon=0x7f2ff00295d0, 
obj=0x7f2fdc0b2770) at ../src/qemu/qemu_monitor_json.c:208
#6  0x7f30301d1933 in qemuMonitorJSONIOProcessLine (mon=0x7f2ff00295d0, 
line=0x7f2fdc0b4cd0 "{\"timestamp\": {\"seconds\": 1635259527, 
\"microseconds\": 380958}, \"event\": \"VSERPORT_CHANGE\", \"data\": {\"open\": 
false, \"id\": \"channel0\"}}", msg=0x0) at ../src/qemu/qemu_monitor_json.c:236
#7  0x7f30301d1bcc in qemuMonitorJSONIOProcess (mon=0x7f2ff00295d0, 
data=0x7f2fdc12f590 "{\"timestamp\": {\"seconds\": 1635259527, 
\"microseconds\": 380958}, \"event\": \"VSERPORT_CHANGE\", \"data\": {\"open\": 
false, \"id\": \"channel0\"}}\r\n", len=135, msg=0x0) at 
../src/qemu/qemu_monitor_json.c:274
#8  0x7f30301c0400 in qemuMonitorIOProcess (mon=0x7f2ff00295d0) at 
../src/qemu/qemu_monitor.c:347
#9  0x7f30301c0c80 in qemuMonitorIO (socket=0x7f2ff0017280, cond=0, 
opaque=0x7f2ff00295d0) at ../src/qemu/qemu_monitor.c:573
#10 0x7f3036ba8c77 in socket_source_dispatch () at 
/usr/lib64/libgio-2.0.so.0
#11 0x7f3036d8b9d0 in g_main_context_dispatch () at 
/usr/lib64/libglib-2.0.so.0
#12 0x7f3036d8bd78 in g_main_context_iterate.constprop () at 
/usr/lib64/libglib-2.0.so.0
#13 0x7f3036d8c06b in g_main_loop_run () at /usr/lib64/libglib-2.0.so.0
#14 0x7f3036f81d7b in virEventThreadWorker (opaque=0x7f2ff00b10c0) at 
../src/util/vireventthread.c:124
#15 0x7f3036db52bd in g_thread_proxy () at /usr/lib64/libglib-2.0.so.0
#16 0x7f3036235e3e in start_thread () at /lib64/libpthread.so.0
#17 0x7f30369ffd6f in clone () at /lib64/libc.so.6

virThreadPoolSendJob 19 $ call virIdentityGetCurrent()
$1 = 0x0


Michal

Re: [PATCH 2/3] qemu: Set "qemu-event" thread identity

[Michal Prívozník]

On 10/26/21 4:31 PM, Daniel P. Berrangé wrote:
> On Tue, Oct 26, 2021 at 12:17:07PM +0200, Michal Privoznik wrote:
>> It may happen that qemuProcessStop() is called from "qemu-event"
>> thread. But this thread doesn't have any virIdentity set
>> (virIdentity being thread local) and therefore it may be unable
>> to open connection to secondary drivers. It is unable to do so
>> in split daemon scenario, because in there opening a connection
>> is coupled with copying current thread identity onto the
>> connection. Code-wise, virIdentityGetCurrent() returns NULL which
>> in turn makes virGetConnectGeneric() fail. This problem does not
>> occur in monolithic daemon scenario, because no identity copying
>> is done there.
>>
>> Long story short, inability to open secondary driver connection
>> can lead to unwanted results. Therefore, do what
>> qemuProcessReconnectHelper() does - set the new thread identity
>> to be the one of the caller.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2013573
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_driver.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a8bf0ecc6f..70b5f37e6b 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -571,6 +571,7 @@ qemuStateInitialize(bool privileged,
>>  size_t i;
>>  const char *defsecmodel = NULL;
>>  g_autofree virSecurityManager **sec_managers = NULL;
>> +g_autoptr(virIdentity) identity = virIdentityGetCurrent();
> 
> Note, qemuStateInitialize will run with the system identity, so this
> is functionally the same as the next patch.

Yes, but for some reason that patch looks like a hack to me. But I can
polish it and we can go with that one instead.

Michal

Re: [PATCH 1/3] virthreadpool: Allow setting identity for workers

[Daniel P . Berrangé]

On Tue, Oct 26, 2021 at 12:17:06PM +0200, Michal Privoznik wrote:
> In some cases the worker func running inside the pool may rely on
> virIdentity. While worker func could check for identity and set
> one it is not optimal - it may not have access to the identity of
> the thread creating the pool and thus would have to call
> virIdentityGetSystem(). Allow passing identity when creating the
> pool.

I wonder if we should have an identity passed via virThreadPoolSendJob,
so whatever queues the job can preserve its identity ?

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/nwfilter/nwfilter_dhcpsnoop.c |  1 +
>  src/qemu/qemu_driver.c|  4 +++-
>  src/rpc/virnetserver.c|  1 +
>  src/util/virthreadpool.c  | 12 
>  src/util/virthreadpool.h  | 12 +++-
>  5 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
> b/src/nwfilter/nwfilter_dhcpsnoop.c
> index 4b62a7b661..b0297f0741 100644
> --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> @@ -1333,6 +1333,7 @@ virNWFilterDHCPSnoopThread(void *req0)
>  worker = virThreadPoolNewFull(1, 1, 0,
>virNWFilterDHCPDecodeWorker,
>"dhcp-decode",
> +  NULL,
>req);
>  }
>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6997dc7dea..a8bf0ecc6f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -914,7 +914,9 @@ qemuStateInitialize(bool privileged,
>   * running domains since there might occur some QEMU monitor
>   * events that will be dispatched to the worker pool */
>  qemu_driver->workerPool = virThreadPoolNewFull(0, 1, 0, 
> qemuProcessEventHandler,
> -   "qemu-event", 
> qemu_driver);
> +   "qemu-event",
> +   NULL,
> +   qemu_driver);
>  if (!qemu_driver->workerPool)
>  goto error;
>  
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index dc8f32b095..c7b4939398 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -378,6 +378,7 @@ virNetServer *virNetServerNew(const char *name,
>priority_workers,
>virNetServerHandleJob,
>"rpc-worker",
> +  NULL,
>srv)))
>  goto error;
>  
> diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
> index 92b7cac286..7bf4333885 100644
> --- a/src/util/virthreadpool.c
> +++ b/src/util/virthreadpool.c
> @@ -55,6 +55,8 @@ struct _virThreadPool {
>  virThreadPoolJobList jobList;
>  size_t jobQueueDepth;
>  
> +virIdentity *identity;
> +
>  virMutex mutex;
>  virCond cond;
>  virCond quit_cond;
> @@ -99,6 +101,9 @@ static void virThreadPoolWorker(void *opaque)
>  
>  virMutexLock(>mutex);
>  
> +if (pool->identity)
> +virIdentitySetCurrent(pool->identity);
> +
>  while (1) {
>  /* In order to support async worker termination, we need ensure that
>   * both busy and free workers know if they need to terminated. Thus,
> @@ -219,6 +224,7 @@ virThreadPoolNewFull(size_t minWorkers,
>   size_t prioWorkers,
>   virThreadPoolJobFunc func,
>   const char *name,
> + virIdentity *identity,
>   void *opaque)
>  {
>  virThreadPool *pool;
> @@ -234,6 +240,9 @@ virThreadPoolNewFull(size_t minWorkers,
>  pool->jobName = name;
>  pool->jobOpaque = opaque;
>  
> +if (identity)
> +pool->identity = g_object_ref(identity);
> +
>  if (virMutexInit(>mutex) < 0)
>  goto error;
>  if (virCondInit(>cond) < 0)
> @@ -300,6 +309,9 @@ void virThreadPoolFree(virThreadPool *pool)
>  virMutexLock(>mutex);
>  virThreadPoolDrainLocked(pool);
>  
> +if (pool->identity)
> +g_object_unref(pool->identity);
> +
>  g_free(pool->workers);
>  virMutexUnlock(>mutex);
>  virMutexDestroy(>mutex);
> diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h
> index 619d128e9a..c6b9f31916 100644
> --- a/src/util/virthreadpool.h
> +++ b/src/util/virthreadpool.h
> @@ -22,17 +22,19 @@
>  #pragma once
>  
>  #include "internal.h"
> +#include "viridentity.h"
>  
>  typedef struct _virThreadPool virThreadPool;
>  
>  typedef void (*virThreadPoolJobFunc)(void *jobdata, void *opaque);
>  
>  virThreadPool *virThreadPoolNewFull(size_t minWorkers,
> -  size_t maxWorkers,
> - 

Re: [PATCH 2/3] qemu: Set "qemu-event" thread identity

[Daniel P . Berrangé]

On Tue, Oct 26, 2021 at 12:17:07PM +0200, Michal Privoznik wrote:
> It may happen that qemuProcessStop() is called from "qemu-event"
> thread. But this thread doesn't have any virIdentity set
> (virIdentity being thread local) and therefore it may be unable
> to open connection to secondary drivers. It is unable to do so
> in split daemon scenario, because in there opening a connection
> is coupled with copying current thread identity onto the
> connection. Code-wise, virIdentityGetCurrent() returns NULL which
> in turn makes virGetConnectGeneric() fail. This problem does not
> occur in monolithic daemon scenario, because no identity copying
> is done there.
> 
> Long story short, inability to open secondary driver connection
> can lead to unwanted results. Therefore, do what
> qemuProcessReconnectHelper() does - set the new thread identity
> to be the one of the caller.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2013573
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a8bf0ecc6f..70b5f37e6b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -571,6 +571,7 @@ qemuStateInitialize(bool privileged,
>  size_t i;
>  const char *defsecmodel = NULL;
>  g_autofree virSecurityManager **sec_managers = NULL;
> +g_autoptr(virIdentity) identity = virIdentityGetCurrent();

Note, qemuStateInitialize will run with the system identity, so this
is functionally the same as the next patch.

>  
>  qemu_driver = g_new0(virQEMUDriver, 1);
>  
> @@ -915,7 +916,7 @@ qemuStateInitialize(bool privileged,
>   * events that will be dispatched to the worker pool */
>  qemu_driver->workerPool = virThreadPoolNewFull(0, 1, 0, 
> qemuProcessEventHandler,
> "qemu-event",
> -   NULL,
> +   identity,
> qemu_driver);
>  if (!qemu_driver->workerPool)
>  goto error;

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 1/1] Fix some typos

[Ján Tomko]

On a Tuesday in 2021, Tim Wiederhake wrote:

Signed-off-by: Tim Wiederhake 
---
NEWS.rst| 2 +-
src/conf/domain_conf.c  | 2 +-
src/qemu/qemu_command.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/NEWS.rst b/NEWS.rst
index 5a7570d0db..5f1cf19940 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -64,7 +64,7 @@ v7.9.0 (unreleased)

Libvirt started using JSON directly with the ``-device`` commandline
parameter as it's considered the preferred stable syntax for further QEMU
-releases. If any problems with the conversion are encoutered please
+releases. If any problems with the conversion are encountered please


Hmm, how did that get past review?


report them as soon as possible.

* **Bug fixes**


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[libvirt PATCH 0/1] Fix some typos

[Tim Wiederhake]

Found by "codespell", see proposed script at
https://listman.redhat.com/archives/libvir-list/2021-October/msg00015.html

Cheers,
Tim

Tim Wiederhake (1):
  Fix some typos

 NEWS.rst| 2 +-
 src/conf/domain_conf.c  | 2 +-
 src/qemu/qemu_command.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.31.1

[libvirt PATCH 1/1] Fix some typos

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 NEWS.rst| 2 +-
 src/conf/domain_conf.c  | 2 +-
 src/qemu/qemu_command.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/NEWS.rst b/NEWS.rst
index 5a7570d0db..5f1cf19940 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -64,7 +64,7 @@ v7.9.0 (unreleased)
 
 Libvirt started using JSON directly with the ``-device`` commandline
 parameter as it's considered the preferred stable syntax for further QEMU
-releases. If any problems with the conversion are encoutered please
+releases. If any problems with the conversion are encountered please
 report them as soon as possible.
 
 * **Bug fixes**
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 48c6ee9865..8a7d241df1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16601,7 +16601,7 @@ virDomainMemoryFindInactiveByDef(virDomainDef *def,
 
 /**
  * virDomainMemoryFindByDeviceInfo:
- * @def: domain defintion
+ * @def: domain definition
  * @info: device info to match
  * @pos: store position within array
  *
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7374b2beca..77977c396e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -694,7 +694,7 @@ qemuBuildDeviceAddressProps(virJSONValue *props,
  * @virtio: the returned video device is a 'virtio' device
  * @virtioBusSuffix: the returned device needs to get the bus-suffix
  *
- * Returns the model fo the device for @video and @qemuCaps. @virtio and
+ * Returns the model of the device for @video and @qemuCaps. @virtio and
  * @virtioBusSuffix are filled with the corresponding flags.
  */
 static const char *
@@ -1106,7 +1106,7 @@ qemuBuildVirtioDevProps(virDomainDeviceType devtype,
 
 /* We temporarily cast the const away here, but that's safe to do
  * because the called function simply sets the correct member of
- * device to devdata based on devtype. Futher uses of device will
+ * device to devdata based on devtype. Further uses of device will
  * not touch its contents */
 virDomainDeviceSetData((virDomainDeviceDef *) , (void *) devdata);
 
-- 
2.31.1

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Kevin Wolf]

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 8/9] qapi: Factor out compat_policy_input_ok()

[Philippe Mathieu-Daudé]

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 0/5] trace: inroduce qmp: trace namespace

[Markus Armbruster]

Stefan Hajnoczi  writes:

> On Thu, Oct 14, 2021 at 06:22:32PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 12.10.2021 14:49, Markus Armbruster wrote:
>> > Vladimir Sementsov-Ogievskiy  writes:

[...]

>> Another possible way is to update QAPI code generator to insert a personal
>> trace point for each qmp command.. That seems more complicated to implement,
>> but I can try.
>
> That's what came to mind when I saw this series too. The QAPI generator
> can create a trace event for each QMP command. That way each command has
> a dedicated trace event that can be enabled/disabled in the usual way
> (e.g. built-in "trace" monitor command, SystemTap scripts, etc) without
> introducing special syntax.

I like how this way stays entirely within the existing interface.  I
couldn't tell you how to implement it, though :)

[PATCH 3/3] *** DO NOT MERGE ***

[Michal Privoznik]

One problem with qemuProcessStop() is that it may run from
"qemu-event" thread. This thread processes events from all
guests, meaning there's just one thread that serves all running
guests. But as such has no virIdentity set, which looks correct
on the first glance - there's no identity to associate it with
anyway.

But, the problem arises when we need to connect to other daemons
(e.g. virtnetworkd) to call APIs (e.g. delete a port). Here we
want the thread to have the identity that of the guest. But since
we haven't set any identity, opening connection to secondary
drivers fails (because virIdentityGetCurrent() returns NULL which
is treated by virGetConnectGeneric() as an error).

As a terrible hack we can set the system identity.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2013573
Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_process.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d5f8a47ac2..7ae1974710 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7888,6 +7888,7 @@ void qemuProcessStop(virQEMUDriver *driver,
 g_autofree char *timestamp = NULL;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 g_autoptr(virConnect) conn = NULL;
+g_autoptr(virIdentity) ident = NULL;
 
 VIR_DEBUG("Shutting down vm=%p name=%s id=%d pid=%lld, "
   "reason=%s, asyncJob=%s, flags=0x%x",
@@ -7901,6 +7902,9 @@ void qemuProcessStop(virQEMUDriver *driver,
  * reporting so we don't squash a legit error. */
 virErrorPreserveLast(_err);
 
+ident = virIdentityGetSystem();
+virIdentitySetCurrent(ident);
+
 if (asyncJob != QEMU_ASYNC_JOB_NONE) {
 if (qemuDomainObjBeginNestedJob(driver, vm, asyncJob) < 0)
 goto cleanup;
-- 
2.32.0

[PATCH 2/3] qemu: Set "qemu-event" thread identity

[Michal Privoznik]

It may happen that qemuProcessStop() is called from "qemu-event"
thread. But this thread doesn't have any virIdentity set
(virIdentity being thread local) and therefore it may be unable
to open connection to secondary drivers. It is unable to do so
in split daemon scenario, because in there opening a connection
is coupled with copying current thread identity onto the
connection. Code-wise, virIdentityGetCurrent() returns NULL which
in turn makes virGetConnectGeneric() fail. This problem does not
occur in monolithic daemon scenario, because no identity copying
is done there.

Long story short, inability to open secondary driver connection
can lead to unwanted results. Therefore, do what
qemuProcessReconnectHelper() does - set the new thread identity
to be the one of the caller.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2013573
Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a8bf0ecc6f..70b5f37e6b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -571,6 +571,7 @@ qemuStateInitialize(bool privileged,
 size_t i;
 const char *defsecmodel = NULL;
 g_autofree virSecurityManager **sec_managers = NULL;
+g_autoptr(virIdentity) identity = virIdentityGetCurrent();
 
 qemu_driver = g_new0(virQEMUDriver, 1);
 
@@ -915,7 +916,7 @@ qemuStateInitialize(bool privileged,
  * events that will be dispatched to the worker pool */
 qemu_driver->workerPool = virThreadPoolNewFull(0, 1, 0, 
qemuProcessEventHandler,
"qemu-event",
-   NULL,
+   identity,
qemu_driver);
 if (!qemu_driver->workerPool)
 goto error;
-- 
2.32.0

[PATCH 1/3] virthreadpool: Allow setting identity for workers

[Michal Privoznik]

In some cases the worker func running inside the pool may rely on
virIdentity. While worker func could check for identity and set
one it is not optimal - it may not have access to the identity of
the thread creating the pool and thus would have to call
virIdentityGetSystem(). Allow passing identity when creating the
pool.

Signed-off-by: Michal Privoznik 
---
 src/nwfilter/nwfilter_dhcpsnoop.c |  1 +
 src/qemu/qemu_driver.c|  4 +++-
 src/rpc/virnetserver.c|  1 +
 src/util/virthreadpool.c  | 12 
 src/util/virthreadpool.h  | 12 +++-
 5 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 4b62a7b661..b0297f0741 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -1333,6 +1333,7 @@ virNWFilterDHCPSnoopThread(void *req0)
 worker = virThreadPoolNewFull(1, 1, 0,
   virNWFilterDHCPDecodeWorker,
   "dhcp-decode",
+  NULL,
   req);
 }
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6997dc7dea..a8bf0ecc6f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -914,7 +914,9 @@ qemuStateInitialize(bool privileged,
  * running domains since there might occur some QEMU monitor
  * events that will be dispatched to the worker pool */
 qemu_driver->workerPool = virThreadPoolNewFull(0, 1, 0, 
qemuProcessEventHandler,
-   "qemu-event", qemu_driver);
+   "qemu-event",
+   NULL,
+   qemu_driver);
 if (!qemu_driver->workerPool)
 goto error;
 
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index dc8f32b095..c7b4939398 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -378,6 +378,7 @@ virNetServer *virNetServerNew(const char *name,
   priority_workers,
   virNetServerHandleJob,
   "rpc-worker",
+  NULL,
   srv)))
 goto error;
 
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index 92b7cac286..7bf4333885 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -55,6 +55,8 @@ struct _virThreadPool {
 virThreadPoolJobList jobList;
 size_t jobQueueDepth;
 
+virIdentity *identity;
+
 virMutex mutex;
 virCond cond;
 virCond quit_cond;
@@ -99,6 +101,9 @@ static void virThreadPoolWorker(void *opaque)
 
 virMutexLock(>mutex);
 
+if (pool->identity)
+virIdentitySetCurrent(pool->identity);
+
 while (1) {
 /* In order to support async worker termination, we need ensure that
  * both busy and free workers know if they need to terminated. Thus,
@@ -219,6 +224,7 @@ virThreadPoolNewFull(size_t minWorkers,
  size_t prioWorkers,
  virThreadPoolJobFunc func,
  const char *name,
+ virIdentity *identity,
  void *opaque)
 {
 virThreadPool *pool;
@@ -234,6 +240,9 @@ virThreadPoolNewFull(size_t minWorkers,
 pool->jobName = name;
 pool->jobOpaque = opaque;
 
+if (identity)
+pool->identity = g_object_ref(identity);
+
 if (virMutexInit(>mutex) < 0)
 goto error;
 if (virCondInit(>cond) < 0)
@@ -300,6 +309,9 @@ void virThreadPoolFree(virThreadPool *pool)
 virMutexLock(>mutex);
 virThreadPoolDrainLocked(pool);
 
+if (pool->identity)
+g_object_unref(pool->identity);
+
 g_free(pool->workers);
 virMutexUnlock(>mutex);
 virMutexDestroy(>mutex);
diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h
index 619d128e9a..c6b9f31916 100644
--- a/src/util/virthreadpool.h
+++ b/src/util/virthreadpool.h
@@ -22,17 +22,19 @@
 #pragma once
 
 #include "internal.h"
+#include "viridentity.h"
 
 typedef struct _virThreadPool virThreadPool;
 
 typedef void (*virThreadPoolJobFunc)(void *jobdata, void *opaque);
 
 virThreadPool *virThreadPoolNewFull(size_t minWorkers,
-  size_t maxWorkers,
-  size_t prioWorkers,
-  virThreadPoolJobFunc func,
-  const char *name,
-  void *opaque) ATTRIBUTE_NONNULL(4);
+size_t maxWorkers,
+size_t prioWorkers,
+virThreadPoolJobFunc func,
+   

[PATCH 0/3] qemu: Set "qemu-event" thread identity

[Michal Privoznik]

I propose to merge only the first two patches. The third one is just an
alternative to the first two, but honestly, I view it as a terrible
hack. Nevertheless, a hack worth discussion.

Michal Prívozník (3):
  virthreadpool: Allow setting identity for workers
  qemu: Set "qemu-event" thread identity
  *** DO NOT MERGE ***

 src/nwfilter/nwfilter_dhcpsnoop.c |  1 +
 src/qemu/qemu_driver.c|  5 -
 src/qemu/qemu_process.c   |  4 
 src/rpc/virnetserver.c|  1 +
 src/util/virthreadpool.c  | 12 
 src/util/virthreadpool.h  | 12 +++-
 6 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.32.0

Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()

[Markus Armbruster]

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

[Markus Armbruster]

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

[Markus Armbruster]

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"

[Markus Armbruster]

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"

[Daniel P . Berrangé]

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"

[Dr. David Alan Gilbert]

* 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 5/9] qapi: Generalize struct member policy checking

[Markus Armbruster]

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
>>
>>

Entering freeze for libvirt-7.9.0

[Jiri Denemark]

I have just tagged v7.9.0-rc1 in the repository and pushed signed
tarballs and source RPMs to https://libvirt.org/sources/

Please give the release candidate some testing and in case you find a
serious issue which should have a fix in the upcoming release, feel
free to reply to this thread to make sure the issue is more visible.

If you have not done so yet, please update NEWS.rst to document any
significant change you made since the last release.

Thanks,

Jirka

Re: [PATCH 4/9] qapi: Tools for sets of special feature flags in generated code

[Markus Armbruster]

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: [Libguestfs] [PATCH 1/9] qapi: New special feature flag "unstable"

[Kashyap Chamarthy]

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 0/2] domainMemoryDeviceSizeChange: Two simple improvements

[Peter Krempa]

On Mon, Oct 25, 2021 at 15:25:42 +0200, Michal Privoznik wrote:
> *** BLURB HERE ***
> 
> Michal Prívozník (2):
>   qemu_monitor: Make domainMemoryDeviceSizeChange cb return void
>   qemuProcessHandleMemoryDeviceSizeChange: Use qemuProcessEventSubmit()

Reviewed-by: Peter Krempa 

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Kashyap Chamarthy]

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 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Markus Armbruster]

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 3/9] qapi: Eliminate QCO_NO_OPTIONS for a slight simplification

[Markus Armbruster]

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: The exact meaning of DEVICE_REMOVED event

[Peter Krempa]

On Tue, Oct 26, 2021 at 15:13:22 +0800, Han Han wrote:
> Hi, developers
> I has a question about the event VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED. When
> will it occur? Detach device from an active VM? Or an inactive VM? Or both?
> I get a code comment from the callback of this event
> https://gitlab.com/libvirt/libvirt/-/blob/master/include/libvirt/libvirt-domain.h#L4151
> "This callback occurs when a device is removed from the domain"
> 
> But it is not clear the domain could be active or inactive or both.

The wording is indeed a bit confusing. At this point the callback is
emitted only when the VM is running and only when the guest OS
cooperates with the device removal.

Re: [libvirt PATCH] util: Drop pointless NUL_TERMINATE macro

[Peter Krempa]

On Mon, Oct 25, 2021 at 17:32:05 +0200, Jiri Denemark wrote:
> It's only used once and open coding it is at least as clear as using the
> macro.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/internal.h | 2 --
>  src/util/virutil.c | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Kevin Wolf]

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

答复: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface

[张金生]

Hi Yalan,


1)   For inbound, we can use `ovs-vsctl list qos` and `ovs-vsctl list 
queue`  to check them from the openvswitch side. Values can be found in 
other_config.  Inbound is in kbyte when set qos with `virshdomiftune …`, 
well it is in bit in ovs, Therefore, when inbound.average is set to 100, the 
corresponding value will be  set to 819200 in ovs.

2)   For outbound, it is in kbyte in libvirt and ingress_policing_XX in ovs 
interface is in kbit.

3)   Ovs use tc to set qos, so we can see output from tc command.
This patch is to unify the qos control and query on ovs ports.
The conversion explanation is added in this patch: 
https://listman.redhat.com/archives/libvir-list/2021-August/msg00422.html
And there are 6 following patches to fix some bugs. See 
https://listman.redhat.com/archives/libvir-list/2021-August/msg00423.html

---
Best Regards,
Jinsheng Zhang

发件人: Yalan Zhang [mailto:yalzh...@redhat.com]
发送时间: 2021年10月25日 17:54
收件人: Michal Prívozník; Jinsheng Zhang (张金生)-云服务集团
抄送: libvir-list@redhat.com; Norman Shen(申嘉童); zhangjl02
主题: Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface

Hi Jinsheng,

I have tested the patch and have some questions, could you please help to 
confirm?
1) For inbound, how to check it from the openvswitch side? tc will still show 
the statistics, is that expected?
2) For outbound, the peak is ignored. I just can not understand the 
"ingress_policing_burst: 2048", how can it come from the setting 
"outbound.burst : 256"?
3) Is the output from tc command expected?

Test inbound:
1. start vm with setting as below:
 
  
  





  

...


2.

# virsh domiftune rhel vnet5

inbound.average: 100

inbound.peak   : 200

inbound.burst  : 256

inbound.floor  : 0

outbound.average: 0

outbound.peak  : 0

outbound.burst : 0

# ip l

17: vnet5:  mtu 1500 qdisc htb master 
ovs-system state UNKNOWN mode DEFAULT group default qlen 1000

link/ether fe:54:00:4d:43:5a brd ff:ff:ff:ff:ff:ff

# ovs-vsctl show interface

…...

ingress_policing_burst: 0

ingress_policing_kpkts_burst: 0

ingress_policing_kpkts_rate: 0

ingress_policing_rate: 0

…...

name: vnet5


#  tc -d class show  dev vnet5

class htb 1:1 parent 1:fffe prio 0 quantum 10240 rate 819200bit ceil 1638Kbit 
linklayer ethernet burst 256Kb/1 mpu 0b cburst 256Kb/1 mpu 0b level 0

class htb 1:fffe root rate 1638Kbit ceil 1638Kbit linklayer ethernet burst 
1499b/1 mpu 0b cburst 1499b/1 mpu 0b level 7

#  tc -d filter show dev vnet5 parent :
(no outputs)

For outbound:

# virsh dumpxml rhel | grep /bandwidth -B2

 



  

# virsh domiftune rhel vnet9

inbound.average: 0

inbound.peak   : 0

inbound.burst  : 0

inbound.floor  : 0

outbound.average: 100

outbound.peak  : 200

outbound.burst : 256

# ovs-vsctl list interface

ingress_policing_burst: 2048

ingress_policing_kpkts_burst: 0

ingress_policing_kpkts_rate: 0

ingress_policing_rate: 800
...

# tc -d filter show dev vnet9 parent :

filter protocol all pref 49 basic chain 0

filter protocol all pref 49 basic chain 0 handle 0x1

   action order 1:  police 0x1 rate 800Kbit burst 256Kb mtu 64Kb action 
drop/pipe overhead 0b linklayer unspec

   ref 1 bind 1

# tc -d class show  dev vnet9

(no outputs)


---
Best Regards,
Yalan Zhang
IRC: yalzhang


On Mon, Jul 12, 2021 at 3:43 PM Michal Prívozník 
mailto:mpriv...@redhat.com>> wrote:
On 7/9/21 3:31 PM, Jinsheng Zhang (张金生)-云服务集团 wrote:
> Here is my signed-off-by line
>
> Signed-off-by: zhangj...@inspur.com
>
> Thanks again for reminding:) .

Perfect.

Reviewed-by: Michal Privoznik mailto:mpriv...@redhat.com>>

and pushed. Congratulations on your first libvirt contribution!

Michal

Re: [libvirt PATCH] storage_file: Compute QCOW2 cluster size as ULL

[Pavel Hrdina]

On Mon, Oct 25, 2021 at 05:32:43PM +0200, Jiri Denemark wrote:
> While the QCOW2 cluster size is represented in only 4 bits in the QCOW2
> header and thus 1 << cluster_size cannot overflow int,
> qcow2GetClusterSize is supposed to return unsigned long long so we can
> just compute the result as ULL rather than computing it as int and
> promoting to unsigned long long.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/storage_file/storage_file_probe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

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, 

The exact meaning of DEVICE_REMOVED event

[Han Han]

Hi, developers
I has a question about the event VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED. When
will it occur? Detach device from an active VM? Or an inactive VM? Or both?
I get a code comment from the callback of this event
https://gitlab.com/libvirt/libvirt/-/blob/master/include/libvirt/libvirt-domain.h#L4151
"This callback occurs when a device is removed from the domain"

But it is not clear the domain could be active or inactive or both.