Re: [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features)

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* Re: [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features)
       [not found]             ` <87y34v42z0.fsf_-_@dusky.pond.sub.org>
@ 2019-04-01 11:54               ` Kevin Wolf
  2019-04-01 13:11                 ` [Qemu-devel] Options for 4.0 Markus Armbruster
  2019-04-01 12:20               ` [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features) Peter Krempa
  2019-04-01 20:02               ` Markus Armbruster
  2 siblings, 1 reply; 18+ messages in thread
From: Kevin Wolf @ 2019-04-01 11:54 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Eric Blake, pkrempa, stefanha, qemu-block, qemu-devel

Am 31.03.2019 um 14:56 hat Markus Armbruster geschrieben:
> Let's review our options for 4.0.
> 
> Please note my analysis is handicapped by incomplete information, in
> particular on libvirt's needs.
> 
> Terminology:
> 
> * "Hard read-write" semantics: open read/write.
> 
> * "Hard read-only" semantics: open read-only.
> 
> * "Dynamic read-only" semantics: open read-only, reopen read/write when
>   a writer attaches, reopen read-only when the last writer detaches.
> 
> * "Fallback read-only" semantics:. try to open read/write, fall back to
>   read-only.
> 
> We have a use case for dynamic read-only: libvirt.  I'm not aware of a
> use case for fallback read-only.

The situation is a bit more complex than this:

libvirt requires dynamic read-only for file-posix. It prefers dynamic
read-only for other drivers, but can live with fallback read-only there.

Only file-posix implements dynamic read-only today. Other drivers
implement only fallback read-only.

Therefore libvirt has a use case for using fallback read-only for
non-file-posix drivers.

> Behavior before 3.1:
> 
> * read-only=on: hard read-only
> 
> * read-only=off: hard read-write
> 
> Behavior in 3.1: new parameter auto-read-only, default off.
> 
> * read-only=on: hard read-only (no change)
> 
> * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>   silently ignored)
> 
> * read-only=off: hard read-write
> 
> * read-only=off,auto-read-only=on: depends on the driver
> 
>   - file-posix.c's drivers: fallback read-only
>   - some other drivers: fallback read-only
>   - remaining drivers: hard read/write
> 
> Behavior in 4.0 so far:
> 
> * read-only=on: hard read-only (no change)
> 
> * read-only=on,auto-read-only=on: hard read-only (no change)
> 
> * read-only=off: hard read-write (no change)
> 
> * read-only=off,auto-read-only=on: depends on the driver
> 
>   - file-posix.c's drivers: dynamic read-only
>   - some other drivers: fallback read-only (no change)
>   - remaining drivers: hard read/write (no change)
> 
> 
> Option 1: Rename @auto-read-only.
> 
> Rationale: we released auto-read-only in v3.1 with unproven fallback
> read-only semantics.  It turned out not to be useful.

It turned out not to be useful for file-posix in the context of libvirt
with sVirt enabled. That's a different thing than just "not useful".

> Admit the mistake, retract it.  Release our next attempt in 4.0 under
> a suitable new name with fallback read-only semantics.

I'm confused. Do you mean s/fallback/dynamic/?

file-posix is the only driver than implements dynamic read-only, so this
is not a suitable replacement for auto-read-only, which is supported by
many other drivers, too (with fallback read-only semantics, which is
useful enough for libvirt for those drivers). Removing auto-read-only
without adding a replacement for all drivers that support it would be a
regression.

> CON:
> 
> * Compatibility break.  No-go if there are users.  Users seem quite
>   unlikely, though.
> 
> * Still unproven, albeit less so: this time we have some unreleased
>   (unfinished?) libvirt code.

We had this last time, too. Peter just didn't realise that he hadn't
sVirt enabled in his development build...

> * Semantics are still largely left to drivers, and the schema can't tell
>   which one does what.  Awkward.
> 
> * Unless we're fairly confident we won't upgrade drivers from "hard" to
>   "fallback" to "dynamic", this merely kicks the can down the road:
>   we'll face the exact same "how can libvirt find out" problem on every
>   upgrade.

Hm, maybe you meant something different above. Would the new command
actually be dynamic read-only only for file-posix and fallback read-only
for everything else?

> PRO:
> 
> * When libvirt sees the new name, it can rely on file-posix.c's drivers
>   providing dynamic read-only semantics.
> 
> 
> Option 2: Add query-qemu-features command, return
> file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
> 
> Rationale: we released auto-read-only in v3.1 with unproven fallback
> read-only semantics.  It turned out not to be useful.  Admit the
> mistake, and patch it up in 4.0.  Libirt needs to know whether it's
> patche up, and this is a simple way to tell it.
> 
> CON:
> 
> * All of option 1's, except for the compatibility break
> 
> * Uses query-qemu-features to expose a property of the build.  I
>   consider that a mistake.
> 
> PRO
> 
> * Libvirt can use either query-qmp-schema or query-qemu-features to find
>   out whether it can can rely on file-posix.c's drivers providing
>   dynamic read-only semantics.  To make query-qemu-features usable, we
>   need to promise query-qemu-features never returns false for it.  To
>   make query-qemu-features usable, we need to promise the value will
>   remain valid for the remainder of the QEMU run (defeats caching) or
>   for any future run of the same QEMU binary (enables caching).

I don't agree with your assessment here.

query-qmp-schema, as its name says, queries the QMP schema, which
describes the structure of QMP communication, not generally features of
the QEMU build.

Features are often related in some way to a QAPI type and clients query
the schema anyway, so it's convenient to add additional information
about features there. But it's not the only place where it makes sense.

The schema for query-qemu-features should tell you whether this QEMU
version even knows about a feature like this, i.e. whether it is capable
of providing this information in a QMP response. Only executing the
command returns whether the feature is actually supported in this build.
Inferring the value of a field from its presence in the schema feels
wrong.

> Option 3: Add @dynamic-read-only to the drivers that provide dynamic
> read-only, default to value of auto-read-only, promise we'll never add
> it to drivers that don't.
> 
> Rationale: give users explicit control over dynamic vs. fallback for all
> drivers that can provide dynamic.  This makes some sense as an
> interface, as long as you ignore the fact that no driver implements both
> dynamic and fallback.  I can't see why a driver couldn't implement both.
> It also makes dynamic support visible in the schema.

Except that we don't even give users more control (the drivers still
provide what they provide), but we require users to specify that they
actually expect what the driver provides.

> Behavior (three bools -> eight cases):
> 
> * read-only=on: hard read-only (no change)
> 
>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
> 
> * read-only=on,auto-read-only=on: hard read-only (no change)
> 
>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
> 
> * read-only=off: hard read-write (no change)
> 
>   Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
> 
> * read-only=off,auto-read-only=on:
> 
>   Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
> 
>   - file-posix.c's drivers: dynamic read-only (no change,
>     dynamic-read-only=on is the default)
>   - some other drivers: fallback read-only (no change)
>   - remaining drivers: hard read/write (no change)
> 
> * read-only=off,auto-read-only=on,dynamic-read-only=off
> 
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
> 
> * read-only=off,auto-read-only=off,dynamic-read-only=on
> 
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
> 
> * read-only=on,dynamic-read-only=on
> 
>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
> 
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
> 
> * read-only=on,auto-read-only=on,dynamic-read-only=off
> 
>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
> 
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
> 
> CON:
> 
> * Two bools (read-only and auto-read-only) to select from three choices
>   was already ugly.  Three bools (the same plus dynamic-read-only) to
>   select from four choices is even uglier.
> 
> * The explicit control is just a facade so far: since only the default
>   setting is implemented, it doesn't actually control anything.
> 
> PRO:
> 
> * When libvirt sees a driver providing a dynamic-read-only parameter, it
>   knows it can rely on the driver providing dynamic read-only semantics.
> 
> * Adding dynamic read-only capability to drivers creates no
>   introspection problems: we simply add dynamic-read-only to their
>   parameters.

Option 4:

Add a dummy option to BlockdevOptionsFile:

    '*x-auto-read-only-is-dynamic': { 'type': 'null',
                                      'if': 'defined(CONFIG_POSIX)' }

Specifying it has no effect, so the ridiculous complexity of three bools
to select from three options is avoided. Its presence in the schema
indicates that file-posix implements dynamic auto-read-only.

Basically this is features flags in the schema without having proper
feature flags yet.

Once we add real annotations (hopefully 4.1), this dummy field can be
removed again.

Kevin

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 11:54               ` [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features) Kevin Wolf
@ 2019-04-01 13:11                 ` Markus Armbruster
  2019-04-01 14:08                   ` Kevin Wolf
  0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2019-04-01 13:11 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, qemu-block, stefanha, qemu-devel

Kevin Wolf <kwolf@redhat.com> writes:

> Am 31.03.2019 um 14:56 hat Markus Armbruster geschrieben:
>> Let's review our options for 4.0.
>> 
>> Please note my analysis is handicapped by incomplete information, in
>> particular on libvirt's needs.
>> 
>> Terminology:
>> 
>> * "Hard read-write" semantics: open read/write.
>> 
>> * "Hard read-only" semantics: open read-only.
>> 
>> * "Dynamic read-only" semantics: open read-only, reopen read/write when
>>   a writer attaches, reopen read-only when the last writer detaches.
>> 
>> * "Fallback read-only" semantics:. try to open read/write, fall back to
>>   read-only.
>> 
>> We have a use case for dynamic read-only: libvirt.  I'm not aware of a
>> use case for fallback read-only.
>
> The situation is a bit more complex than this:
>
> libvirt requires dynamic read-only for file-posix. It prefers dynamic
> read-only for other drivers, but can live with fallback read-only there.

Aha!

> Only file-posix implements dynamic read-only today. Other drivers
> implement only fallback read-only.
>
> Therefore libvirt has a use case for using fallback read-only for
> non-file-posix drivers.
>
>> Behavior before 3.1:
>> 
>> * read-only=on: hard read-only
>> 
>> * read-only=off: hard read-write
>> 
>> Behavior in 3.1: new parameter auto-read-only, default off.
>> 
>> * read-only=on: hard read-only (no change)
>> 
>> * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>>   silently ignored)
>> 
>> * read-only=off: hard read-write
>> 
>> * read-only=off,auto-read-only=on: depends on the driver
>> 
>>   - file-posix.c's drivers: fallback read-only
>>   - some other drivers: fallback read-only
>>   - remaining drivers: hard read/write
>> 
>> Behavior in 4.0 so far:
>> 
>> * read-only=on: hard read-only (no change)
>> 
>> * read-only=on,auto-read-only=on: hard read-only (no change)
>> 
>> * read-only=off: hard read-write (no change)
>> 
>> * read-only=off,auto-read-only=on: depends on the driver
>> 
>>   - file-posix.c's drivers: dynamic read-only
>>   - some other drivers: fallback read-only (no change)
>>   - remaining drivers: hard read/write (no change)
>> 
>> 
>> Option 1: Rename @auto-read-only.
>> 
>> Rationale: we released auto-read-only in v3.1 with unproven fallback
>> read-only semantics.  It turned out not to be useful.
>
> It turned out not to be useful for file-posix in the context of libvirt
> with sVirt enabled. That's a different thing than just "not useful".

Right.  It turned out to be insufficient, which is not the same as "not
useful".

>> Admit the mistake, retract it.  Release our next attempt in 4.0 under
>> a suitable new name with fallback read-only semantics.
>
> I'm confused. Do you mean s/fallback/dynamic/?

Messed up prose, sorry.

> file-posix is the only driver than implements dynamic read-only, so this
> is not a suitable replacement for auto-read-only, which is supported by
> many other drivers, too (with fallback read-only semantics, which is
> useful enough for libvirt for those drivers). Removing auto-read-only
> without adding a replacement for all drivers that support it would be a
> regression.

Our next attempt is actually "dynamic read-only with file-posix.c's
drivers, fallback read-only with some other drivers, hard read/write
with the remaining drivers".  All enabled by a single flag.

Documenting this with sufficient precision without making a mess of it
won't be easy.

In my opinion, we did make a mess of it in 3.1: we specified only what
QEMU *may* do, not what it actually does.  Am I confused or was libvirt
expected to rely on actual behavior and not just the specification?

>> CON:
>> 
>> * Compatibility break.  No-go if there are users.  Users seem quite
>>   unlikely, though.
>> 
>> * Still unproven, albeit less so: this time we have some unreleased
>>   (unfinished?) libvirt code.
>
> We had this last time, too. Peter just didn't realise that he hadn't
> sVirt enabled in his development build...

I see.  High pressure is bound to lead to mistakes.

>> * Semantics are still largely left to drivers, and the schema can't tell
>>   which one does what.  Awkward.
>> 
>> * Unless we're fairly confident we won't upgrade drivers from "hard" to
>>   "fallback" to "dynamic", this merely kicks the can down the road:
>>   we'll face the exact same "how can libvirt find out" problem on every
>>   upgrade.
>
> Hm, maybe you meant something different above. Would the new command
> actually be dynamic read-only only for file-posix and fallback read-only
> for everything else?

Yes.

>> PRO:
>> 
>> * When libvirt sees the new name, it can rely on file-posix.c's drivers
>>   providing dynamic read-only semantics.
>> 
>> 
>> Option 2: Add query-qemu-features command, return
>> file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
>> 
>> Rationale: we released auto-read-only in v3.1 with unproven fallback
>> read-only semantics.  It turned out not to be useful.  Admit the
>> mistake, and patch it up in 4.0.  Libirt needs to know whether it's
>> patche up, and this is a simple way to tell it.
>> 
>> CON:
>> 
>> * All of option 1's, except for the compatibility break
>> 
>> * Uses query-qemu-features to expose a property of the build.  I
>>   consider that a mistake.
>> 
>> PRO
>> 
>> * Libvirt can use either query-qmp-schema or query-qemu-features to find
>>   out whether it can can rely on file-posix.c's drivers providing
>>   dynamic read-only semantics.  To make query-qemu-features usable, we

"To make query-qmp-schema usable", of course.

>>   need to promise query-qemu-features never returns false for it.  To
>>   make query-qemu-features usable, we need to promise the value will
>>   remain valid for the remainder of the QEMU run (defeats caching) or
>>   for any future run of the same QEMU binary (enables caching).
>
> I don't agree with your assessment here.
>
> query-qmp-schema, as its name says, queries the QMP schema, which
> describes the structure of QMP communication, not generally features of
> the QEMU build.
>
> Features are often related in some way to a QAPI type and clients query
> the schema anyway, so it's convenient to add additional information
> about features there. But it's not the only place where it makes sense.

I've never denied the existence of use cases for query-qemu-features.
I just believe this ain't one :)

> The schema for query-qemu-features should tell you whether this QEMU
> version even knows about a feature like this, i.e. whether it is capable
> of providing this information in a QMP response. Only executing the
> command returns whether the feature is actually supported in this build.
> Inferring the value of a field from its presence in the schema feels
> wrong.

That depends.

If we specify that query-qemu-features never returns false for a certain
feature, there is no need to actually run it to obtain the feature's
value.  If you prefer to run query-qemu-features anyway, go ahead and
run it to your heart's content.  You'll still rely on "never returns
false", because that's the part that tells you for how long the response
remains valid.

If we don't specify "never returns false", we get to specify directly
for how long the response remains valid.

>> Option 3: Add @dynamic-read-only to the drivers that provide dynamic
>> read-only, default to value of auto-read-only, promise we'll never add
>> it to drivers that don't.
>> 
>> Rationale: give users explicit control over dynamic vs. fallback for all
>> drivers that can provide dynamic.  This makes some sense as an
>> interface, as long as you ignore the fact that no driver implements both
>> dynamic and fallback.  I can't see why a driver couldn't implement both.
>> It also makes dynamic support visible in the schema.
>
> Except that we don't even give users more control (the drivers still
> provide what they provide), but we require users to specify that they
> actually expect what the driver provides.

That would be a feature.  Declaring needs explicitly is *good*.

We don't actually require it here because of defaults.

>> Behavior (three bools -> eight cases):
>> 
>> * read-only=on: hard read-only (no change)
>> 
>>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
>> 
>> * read-only=on,auto-read-only=on: hard read-only (no change)
>> 
>>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
>> 
>> * read-only=off: hard read-write (no change)
>> 
>>   Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
>> 
>> * read-only=off,auto-read-only=on:
>> 
>>   Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
>> 
>>   - file-posix.c's drivers: dynamic read-only (no change,
>>     dynamic-read-only=on is the default)
>>   - some other drivers: fallback read-only (no change)
>>   - remaining drivers: hard read/write (no change)
>> 
>> * read-only=off,auto-read-only=on,dynamic-read-only=off
>> 
>>   - file-posix.c's drivers: error
>>   - all other drivers: N/A
>> 
>> * read-only=off,auto-read-only=off,dynamic-read-only=on
>> 
>>   - file-posix.c's drivers: error
>>   - all other drivers: N/A
>> 
>> * read-only=on,dynamic-read-only=on
>> 
>>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
>> 
>>   - file-posix.c's drivers: error
>>   - all other drivers: N/A
>> 
>> * read-only=on,auto-read-only=on,dynamic-read-only=off
>> 
>>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
>> 
>>   - file-posix.c's drivers: error
>>   - all other drivers: N/A
>> 
>> CON:
>> 
>> * Two bools (read-only and auto-read-only) to select from three choices
>>   was already ugly.  Three bools (the same plus dynamic-read-only) to
>>   select from four choices is even uglier.
>> 
>> * The explicit control is just a facade so far: since only the default
>>   setting is implemented, it doesn't actually control anything.
>> 
>> PRO:
>> 
>> * When libvirt sees a driver providing a dynamic-read-only parameter, it
>>   knows it can rely on the driver providing dynamic read-only semantics.
>> 
>> * Adding dynamic read-only capability to drivers creates no
>>   introspection problems: we simply add dynamic-read-only to their
>>   parameters.
>
> Option 4:
>
> Add a dummy option to BlockdevOptionsFile:
>
>     '*x-auto-read-only-is-dynamic': { 'type': 'null',
>                                       'if': 'defined(CONFIG_POSIX)' }
>
> Specifying it has no effect, so the ridiculous complexity of three bools
> to select from three options is avoided. Its presence in the schema
> indicates that file-posix implements dynamic auto-read-only.
>
> Basically this is features flags in the schema without having proper
> feature flags yet.
>
> Once we add real annotations (hopefully 4.1), this dummy field can be
> removed again.

Exact same patch as for option 3, with the parameter renamed and the
sanity check for non-sensical use dropped.  *Shrug*

Puts more pressure on me to deliver QAPI feature flags sooner rather
than later.  My QAPI pressure control valve has been shedding pressure
for a while, so "bring it on".  I'd advise against holding your breath,
though.

SIGCHLD, back later.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 13:11                 ` [Qemu-devel] Options for 4.0 Markus Armbruster
@ 2019-04-01 14:08                   ` Kevin Wolf
  2019-04-01 15:31                     ` Markus Armbruster
  2019-04-01 19:54                     ` Markus Armbruster
  0 siblings, 2 replies; 18+ messages in thread
From: Kevin Wolf @ 2019-04-01 14:08 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: pkrempa, qemu-block, stefanha, qemu-devel

Am 01.04.2019 um 15:11 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > Option 4:
> >
> > Add a dummy option to BlockdevOptionsFile:
> >
> >     '*x-auto-read-only-is-dynamic': { 'type': 'null',
> >                                       'if': 'defined(CONFIG_POSIX)' }
> >
> > Specifying it has no effect, so the ridiculous complexity of three bools
> > to select from three options is avoided. Its presence in the schema
> > indicates that file-posix implements dynamic auto-read-only.
> >
> > Basically this is features flags in the schema without having proper
> > feature flags yet.
> >
> > Once we add real annotations (hopefully 4.1), this dummy field can be
> > removed again.
> 
> Exact same patch as for option 3, with the parameter renamed and the
> sanity check for non-sensical use dropped.  *Shrug*
> 
> Puts more pressure on me to deliver QAPI feature flags sooner rather
> than later.  My QAPI pressure control valve has been shedding pressure
> for a while, so "bring it on".  I'd advise against holding your breath,
> though.

Okay, let's stop beating around the bush.

Nobody has told me so explicitly during this discussion, but implicitly
I understand that everyone seems to think doing annotations in the
schema is what we really want to have.

Instead of continuing to argue which option to get around it is the
least ugly one - how bad is the following attempt at just implementing
what we really want?

Only for structs for now, but that's all we need at the moment. Should
be easy to extend during the 4.1 cycle (or whenever we need something
else).

Note that even if there are bugs in it, they are not user-visible and
can just be fixed in 4.1. Currently, a diff between old and new
query-qmp-schema output looks sane - just the added 'features' list
where we want it.

Kevin



diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..d74bf4a88a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2852,7 +2852,8 @@
             '*aio': 'BlockdevAioOptions',
 	    '*drop-cache': {'type': 'bool',
 	                    'if': 'defined(CONFIG_LINUX)'},
-            '*x-check-cache-dropped': 'bool' } }
+            '*x-check-cache-dropped': 'bool' },
+  'features': [ 'dynamic-auto-read-only' ] }
 
 ##
 # @BlockdevOptionsNull:
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index f07869ec73..668ebb654a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -886,12 +886,22 @@ def check_enum(expr, info):
 def check_struct(expr, info):
     name = expr['struct']
     members = expr['data']
+    features = expr.get('features')
 
     check_type(info, "'data' for struct '%s'" % name, members,
                allow_dict=True, allow_optional=True)
     check_type(info, "'base' for struct '%s'" % name, expr.get('base'),
                allow_metas=['struct'])
 
+    if features:
+        if not isinstance(features, list):
+            raise QAPISemError(info,
+                               "Struct '%s' requires an array for 'features'" %
+                               name)
+        for s in features:
+            if not isinstance(s, str):
+                raise QAPISemError(info, "'features' for struct '%s' must "
+                                         "contain strings" % name)
 
 def check_known_keys(info, source, keys, required, optional):
 
@@ -986,7 +996,7 @@ def check_exprs(exprs):
             normalize_members(expr['data'])
         elif 'struct' in expr:
             meta = 'struct'
-            check_keys(expr_elem, 'struct', ['data'], ['base', 'if'])
+            check_keys(expr_elem, 'struct', ['data'], ['base', 'if', 'features'])
             normalize_members(expr['data'])
             struct_types[expr[meta]] = expr
         elif 'command' in expr:
@@ -1129,7 +1139,8 @@ class QAPISchemaVisitor(object):
     def visit_object_type(self, name, info, ifcond, base, members, variants):
         pass
 
-    def visit_object_type_flat(self, name, info, ifcond, members, variants):
+    def visit_object_type_flat(self, name, info, ifcond, members, variants,
+                               features):
         pass
 
     def visit_alternate_type(self, name, info, ifcond, variants):
@@ -1290,7 +1301,7 @@ class QAPISchemaArrayType(QAPISchemaType):
 
 class QAPISchemaObjectType(QAPISchemaType):
     def __init__(self, name, info, doc, ifcond,
-                 base, local_members, variants):
+                 base, local_members, variants, features):
         # struct has local_members, optional base, and no variants
         # flat union has base, variants, and no local_members
         # simple union has local_members, variants, and no base
@@ -1307,6 +1318,7 @@ class QAPISchemaObjectType(QAPISchemaType):
         self.local_members = local_members
         self.variants = variants
         self.members = None
+        self.features = features
 
     def check(self, schema):
         QAPISchemaType.check(self, schema)
@@ -1370,7 +1382,8 @@ class QAPISchemaObjectType(QAPISchemaType):
         visitor.visit_object_type(self.name, self.info, self.ifcond,
                                   self.base, self.local_members, self.variants)
         visitor.visit_object_type_flat(self.name, self.info, self.ifcond,
-                                       self.members, self.variants)
+                                       self.members, self.variants,
+                                       self.features)
 
 
 class QAPISchemaMember(object):
@@ -1675,7 +1688,7 @@ class QAPISchema(object):
                   ('null',   'null',    'QNull' + pointer_suffix)]:
             self._def_builtin_type(*t)
         self.the_empty_object_type = QAPISchemaObjectType(
-            'q_empty', None, None, None, None, [], None)
+            'q_empty', None, None, None, None, [], None, [])
         self._def_entity(self.the_empty_object_type)
 
         qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
@@ -1721,7 +1734,7 @@ class QAPISchema(object):
             assert ifcond == typ._ifcond # pylint: disable=protected-access
         else:
             self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
-                                                  None, members, None))
+                                                  None, members, None, []))
         return name
 
     def _def_enum_type(self, expr, info, doc):
@@ -1752,9 +1765,10 @@ class QAPISchema(object):
         base = expr.get('base')
         data = expr['data']
         ifcond = expr.get('if')
+        features = expr.get('features')
         self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base,
                                               self._make_members(data, info),
-                                              None))
+                                              None, features))
 
     def _make_variant(self, case, typ, ifcond):
         return QAPISchemaObjectTypeVariant(case, typ, ifcond)
@@ -1795,7 +1809,7 @@ class QAPISchema(object):
             QAPISchemaObjectType(name, info, doc, ifcond, base, members,
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_member,
-                                                              variants)))
+                                                              variants), []))
 
     def _def_alternate_type(self, expr, info, doc):
         name = expr['alternate']
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index f7f2ca07e4..f93a31aaa8 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s;
         self._gen_qlit('[' + element + ']', 'array', {'element-type': element},
                        ifcond)
 
-    def visit_object_type_flat(self, name, info, ifcond, members, variants):
+    def visit_object_type_flat(self, name, info, ifcond, members, variants,
+                               features):
         obj = {'members': [self._gen_member(m) for m in members]}
         if variants:
             obj.update(self._gen_variants(variants.tag_member.name,
                                           variants.variants))
+        if features:
+            obj['features'] = features
+
         self._gen_qlit(name, 'object', obj, ifcond)
 
     def visit_alternate_type(self, name, info, ifcond, variants):

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 14:08                   ` Kevin Wolf
@ 2019-04-01 15:31                     ` Markus Armbruster
  2019-04-01 19:54                     ` Markus Armbruster
  1 sibling, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2019-04-01 15:31 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, stefanha, qemu-block, qemu-devel

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.04.2019 um 15:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > Option 4:
>> >
>> > Add a dummy option to BlockdevOptionsFile:
>> >
>> >     '*x-auto-read-only-is-dynamic': { 'type': 'null',
>> >                                       'if': 'defined(CONFIG_POSIX)' }
>> >
>> > Specifying it has no effect, so the ridiculous complexity of three bools
>> > to select from three options is avoided. Its presence in the schema
>> > indicates that file-posix implements dynamic auto-read-only.
>> >
>> > Basically this is features flags in the schema without having proper
>> > feature flags yet.
>> >
>> > Once we add real annotations (hopefully 4.1), this dummy field can be
>> > removed again.
>> 
>> Exact same patch as for option 3, with the parameter renamed and the
>> sanity check for non-sensical use dropped.  *Shrug*
>> 
>> Puts more pressure on me to deliver QAPI feature flags sooner rather
>> than later.  My QAPI pressure control valve has been shedding pressure
>> for a while, so "bring it on".  I'd advise against holding your breath,
>> though.
>
> Okay, let's stop beating around the bush.
>
> Nobody has told me so explicitly during this discussion, but implicitly
> I understand that everyone seems to think doing annotations in the
> schema is what we really want to have.

I think it would make a useful addition to QAPI.

I don't think it's the ideal solution to the problem at hand.  But the
problem at hand may not have ideal solutions.

We put @auto-read-only in 3.1 unproven.  As specified, it gives QEMU
wide latitude to open read-only instead of read-write, and to reopen.
It doesn't *require* QEMU to do anything in particular.

3.1's implementation conforms to this specification (it would be hard
not to).

Libvirt needs more than is specified (it would be hard not to), and more
than is implemented in 3.1.

We changed the implementation in 4.0 to give libvirt what it needs (to
the best of our knowledge).  This implementation still conforms to the
specification (again, hard not to).

Libvirt needs to know whether it's dealing with a 3.1 implementation or
a 4.0 implementation.  A feature bit in the schema can expose this
cleanly in introspection.

But it's still an awful interface, to be frank.  Let's try to write a
doc comment for it.

# @read-only:     whether the block device should be read-only (default: false).
#                 Note that some block drivers support only read-only access,
#                 either generally or in certain configurations. In this case,
#                 the default value does not work and the option must be
#                 specified explicitly.
# @auto-read-only: if true and @read-only is false, QEMU may automatically
#                  decide not to open the image read-write as requested, but
#                  fall back to read-only instead (and switch between the modes
#                  later), e.g. depending on whether the image file is writable
#                  or whether a writing user is attached to the node.
#                  If the variant part corresponding to @driver carries
#                  feature flag dynamic-auto-read-only, then QEMU will
#                  always open the image read-only, reopen it read/write
#                  when the first writing user attaches to the node, and
#                  reopen it read-only when the last writing user
#                  detaches from the node.
#                  (default: false, since 3.1)

This still underspecifies fallback read-only.  If libvirt wants to rely
on it, we'll get to tighten up that part.

Behavior is governed by three separate entities: feature flag
@dynamic-read-only, members @read-only and @auto-read-only.  Bonus: the
feature flag lives somewhere else.  Where it'll also need documentation.

Now compare to this hypothetical interface: @read-only can be true,
false, "dynamic" (some drivers only) or "fallback (some drivers only,
assuming we even need it).

To make introspection show the values the driver accepts, @read-only has
to move from BlockdevOptions into the BlockdevOptionsFoo.  In some of
them, it remains bool, in BlockdevOptionsFile it becomes an alternate of
bool and an enum with the single value "dynamic", and so forth.

Perhaps this implementing this would be too onerous.

> Instead of continuing to argue which option to get around it is the
> least ugly one - how bad is the following attempt at just implementing
> what we really want?
>
> Only for structs for now, but that's all we need at the moment. Should
> be easy to extend during the 4.1 cycle (or whenever we need something
> else).
>
> Note that even if there are bugs in it, they are not user-visible and
> can just be fixed in 4.1. Currently, a diff between old and new
> query-qmp-schema output looks sane - just the added 'features' list
> where we want it.

I'll try to review the patch later today.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 14:08                   ` Kevin Wolf
  2019-04-01 15:31                     ` Markus Armbruster
@ 2019-04-01 19:54                     ` Markus Armbruster
  1 sibling, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2019-04-01 19:54 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, stefanha, qemu-block, qemu-devel

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.04.2019 um 15:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > Option 4:
>> >
>> > Add a dummy option to BlockdevOptionsFile:
>> >
>> >     '*x-auto-read-only-is-dynamic': { 'type': 'null',
>> >                                       'if': 'defined(CONFIG_POSIX)' }
>> >
>> > Specifying it has no effect, so the ridiculous complexity of three bools
>> > to select from three options is avoided. Its presence in the schema
>> > indicates that file-posix implements dynamic auto-read-only.
>> >
>> > Basically this is features flags in the schema without having proper
>> > feature flags yet.
>> >
>> > Once we add real annotations (hopefully 4.1), this dummy field can be
>> > removed again.
>> 
>> Exact same patch as for option 3, with the parameter renamed and the
>> sanity check for non-sensical use dropped.  *Shrug*
>> 
>> Puts more pressure on me to deliver QAPI feature flags sooner rather
>> than later.  My QAPI pressure control valve has been shedding pressure
>> for a while, so "bring it on".  I'd advise against holding your breath,
>> though.
>
> Okay, let's stop beating around the bush.
>
> Nobody has told me so explicitly during this discussion, but implicitly
> I understand that everyone seems to think doing annotations in the
> schema is what we really want to have.
>
> Instead of continuing to argue which option to get around it is the
> least ugly one - how bad is the following attempt at just implementing
> what we really want?
>
> Only for structs for now, but that's all we need at the moment. Should
> be easy to extend during the 4.1 cycle (or whenever we need something
> else).
>
> Note that even if there are bugs in it, they are not user-visible and
> can just be fixed in 4.1. Currently, a diff between old and new
> query-qmp-schema output looks sane - just the added 'features' list
> where we want it.

[Patch snipped...]

Caveat emptor: at this time of day, I'm perfectly capable of missing the
obvious.  Assuming I don't: for once, something doesn't turn out six
times as hard as anticipated.

Missing:

* Test coverage: two negative tests for the two errors, positive
  coverage in qapi-schema-test.json

* Update to introspect.json (without this, qmp-cmd-test fails, and with
  qapi-schema-test.json updated, test /visitor/input/qapi-introspect
  will fail, too),

* Update a few tests/qapi-schema/*.err

* Update docs/devel/qapi-code-gen.txt

* A general idea how to document feature flags

* Depending on that, perhaps an update to the documentation generator
  scripts/qapi/doc.py

* A rough idea on where else feature flags need to go

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features)
       [not found]             ` <87y34v42z0.fsf_-_@dusky.pond.sub.org>
  2019-04-01 11:54               ` [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features) Kevin Wolf
@ 2019-04-01 12:20               ` Peter Krempa
  2019-04-01 14:55                 ` [Qemu-devel] Options for 4.0 Markus Armbruster
  2019-04-01 20:02               ` Markus Armbruster
  2 siblings, 1 reply; 18+ messages in thread
From: Peter Krempa @ 2019-04-01 12:20 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Kevin Wolf, Eric Blake, stefanha, qemu-block, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 8088 bytes --]

On Sun, Mar 31, 2019 at 14:56:19 +0200, Markus Armbruster wrote:
> Let's review our options for 4.0.
> 
> Please note my analysis is handicapped by incomplete information, in
> particular on libvirt's needs.

Libvirt's needs are "simple" we need to do block-commit. It has a few
caveats though. The selinux labels for backing images don't allow write
operation when starting qemu. When doing block-commit libvirt relabels
the backing chain prior to using block-commit.

That works with -drive as qemu reopens the files internally, but does
not work when using -blockdev.

Since we need to keep the images RO until doing  the blockjob there
needs to be a way when either qemu automagically does what libvirt needs
even if the image did not have the write permission originally.

auto-read-only was a naive impl when qemu attempted to keep the storage
access nodes RW. At the point when I was testing it I didn't enable
s-virt as it's a hassle when you want to run git versions of libvirt and
qemu and thus didn't see the problem with missing permissions.

The dynamic version attempts to fix that. That is still the automagic
approach. I don't really mind also doing a hands-on approach where we'd
need to tell qemu to reopen given nodes.

I'm not sure what ends up being less work.

> Terminology:
> 
> * "Hard read-write" semantics: open read/write.
> 
> * "Hard read-only" semantics: open read-only.
> 
> * "Dynamic read-only" semantics: open read-only, reopen read/write when
>   a writer attaches, reopen read-only when the last writer detaches.
> 
> * "Fallback read-only" semantics:. try to open read/write, fall back to
>   read-only.
> 
> We have a use case for dynamic read-only: libvirt.  I'm not aware of a
> use case for fallback read-only.
> 
> Behavior before 3.1:
> 
> * read-only=on: hard read-only
> 
> * read-only=off: hard read-write
> 
> Behavior in 3.1: new parameter auto-read-only, default off.
> 
> * read-only=on: hard read-only (no change)
> 
> * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>   silently ignored)
> 
> * read-only=off: hard read-write
> 
> * read-only=off,auto-read-only=on: depends on the driver
> 
>   - file-posix.c's drivers: fallback read-only
>   - some other drivers: fallback read-only
>   - remaining drivers: hard read/write
> 
> Behavior in 4.0 so far:
> 
> * read-only=on: hard read-only (no change)
> 
> * read-only=on,auto-read-only=on: hard read-only (no change)
> 
> * read-only=off: hard read-write (no change)
> 
> * read-only=off,auto-read-only=on: depends on the driver
> 
>   - file-posix.c's drivers: dynamic read-only
>   - some other drivers: fallback read-only (no change)
>   - remaining drivers: hard read/write (no change)
> 
> 
> Option 1: Rename @auto-read-only.
> 
> Rationale: we released auto-read-only in v3.1 with unproven fallback
> read-only semantics.  It turned out not to be useful.  Admit the
> mistake, retract it.  Release our next attempt in 4.0 under a suitable
> new name with fallback read-only semantics.

I concur. Nobody probably uses this. Setting up selinux and blockdev is
not a trivial excercise.

> CON:
> 
> * Compatibility break.  No-go if there are users.  Users seem quite
>   unlikely, though.
> 
> * Still unproven, albeit less so: this time we have some unreleased
>   (unfinished?) libvirt code.

unfinished and thus also unreleased.

> 
> * Semantics are still largely left to drivers, and the schema can't tell
>   which one does what.  Awkward.
> 
> * Unless we're fairly confident we won't upgrade drivers from "hard" to
>   "fallback" to "dynamic", this merely kicks the can down the road:
>   we'll face the exact same "how can libvirt find out" problem on every
>   upgrade.
> 
> PRO:
> 
> * When libvirt sees the new name, it can rely on file-posix.c's drivers
>   providing dynamic read-only semantics.
> 
> 
> Option 2: Add query-qemu-features command, return
> file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
> 
> Rationale: we released auto-read-only in v3.1 with unproven fallback
> read-only semantics.  It turned out not to be useful.  Admit the
> mistake, and patch it up in 4.0.  Libirt needs to know whether it's
> patche up, and this is a simple way to tell it.
> 
> CON:
> 
> * All of option 1's, except for the compatibility break
> 
> * Uses query-qemu-features to expose a property of the build.  I
>   consider that a mistake.
> 
> PRO
> 
> * Libvirt can use either query-qmp-schema or query-qemu-features to find
>   out whether it can can rely on file-posix.c's drivers providing
>   dynamic read-only semantics.  To make query-qemu-features usable, we
>   need to promise query-qemu-features never returns false for it.  To
>   make query-qemu-features usable, we need to promise the value will
>   remain valid for the remainder of the QEMU run (defeats caching) or
>   for any future run of the same QEMU binary (enables caching).
> 
> 
> Option 3: Add @dynamic-read-only to the drivers that provide dynamic
> read-only, default to value of auto-read-only, promise we'll never add
> it to drivers that don't.
> 
> Rationale: give users explicit control over dynamic vs. fallback for all
> drivers that can provide dynamic.  This makes some sense as an
> interface, as long as you ignore the fact that no driver implements both
> dynamic and fallback.  I can't see why a driver couldn't implement both.
> It also makes dynamic support visible in the schema.
> 
> Behavior (three bools -> eight cases):
> 
> * read-only=on: hard read-only (no change)
> 
>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
> 
> * read-only=on,auto-read-only=on: hard read-only (no change)
> 
>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
> 
> * read-only=off: hard read-write (no change)
> 
>   Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
> 
> * read-only=off,auto-read-only=on:
> 
>   Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
> 
>   - file-posix.c's drivers: dynamic read-only (no change,
>     dynamic-read-only=on is the default)
>   - some other drivers: fallback read-only (no change)
>   - remaining drivers: hard read/write (no change)
> 
> * read-only=off,auto-read-only=on,dynamic-read-only=off
> 
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
> 
> * read-only=off,auto-read-only=off,dynamic-read-only=on
> 
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
> 
> * read-only=on,dynamic-read-only=on
> 
>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
> 
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
> 
> * read-only=on,auto-read-only=on,dynamic-read-only=off
> 
>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
> 
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
> 
> CON:
> 
> * Two bools (read-only and auto-read-only) to select from three choices
>   was already ugly.  Three bools (the same plus dynamic-read-only) to
>   select from four choices is even uglier.
> 
> * The explicit control is just a facade so far: since only the default
>   setting is implemented, it doesn't actually control anything.
> 
> PRO:
> 
> * When libvirt sees a driver providing a dynamic-read-only parameter, it
>   knows it can rely on the driver providing dynamic read-only semantics.
> 
> * Adding dynamic read-only capability to drivers creates no
>   introspection problems: we simply add dynamic-read-only to their
>   parameters.

In the end libvirt does not care that much if the storage is open read
only or read write. We need the format node or guest frontend to deny
writes if the disk is declared as read-only. We also need to be able to
do blockjobs. The requirement is that images may not be accessible in
R/W mode when qemu is starting but later may become. Then it's expected
that block-commit will succeed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 12:20               ` [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features) Peter Krempa
@ 2019-04-01 14:55                 ` Markus Armbruster
  2019-04-01 16:07                   ` Peter Krempa
  0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2019-04-01 14:55 UTC (permalink / raw)
  To: Peter Krempa; +Cc: Kevin Wolf, qemu-block, stefanha, qemu-devel

Peter Krempa <pkrempa@redhat.com> writes:

> On Sun, Mar 31, 2019 at 14:56:19 +0200, Markus Armbruster wrote:
>> Let's review our options for 4.0.
>> 
>> Please note my analysis is handicapped by incomplete information, in
>> particular on libvirt's needs.
>
> Libvirt's needs are "simple" we need to do block-commit. It has a few
> caveats though. The selinux labels for backing images don't allow write
> operation when starting qemu. When doing block-commit libvirt relabels
> the backing chain prior to using block-commit.

Paraphrasing, to make sure I got you.

Consider the common read/write COW image with a backing image.

Libvirt wants to grant QEMU read/write access to the COW and read-only
access to the backing image.  It does that with SELinux.  QEMU has to
open the backing image read-only (read/write would fail).

But libvirt also wants to do block-commit.  Libvirt first relabels to
grant QEMU read/write access to the backing image, then $something makes
QEMU reopen the backing image read/write, block-commit does its job,
$something makes QEMU reopen the backing image read-only, libvirt
relabels to revoke read/write access.

Correct?

With dynamic read-only, $something is the block commit job (a writer)
attaching and detaching to the backing image node.

> That works with -drive as qemu reopens the files internally, but does
> not work when using -blockdev.

Happy to take your word for it.

> Since we need to keep the images RO until doing  the blockjob there
> needs to be a way when either qemu automagically does what libvirt needs
> even if the image did not have the write permission originally.
>
> auto-read-only was a naive impl when qemu attempted to keep the storage
> access nodes RW. At the point when I was testing it I didn't enable
> s-virt as it's a hassle when you want to run git versions of libvirt and
> qemu and thus didn't see the problem with missing permissions.

Yes, fallback read-only is useless for the scenario above.

Kevin wrote "libvirt requires dynamic read-only for file-posix. It
prefers dynamic read-only for other drivers, but can live with fallback
read-only there."  I'm not sure how it can live with fallback read-only.
Is it because only *files* can labelled with SELinux?

If yes, then my next question is how libvirt makes use of fallback
read-only for non-files.  Can you give me an example?

> The dynamic version attempts to fix that. That is still the automagic
> approach. I don't really mind also doing a hands-on approach where we'd
> need to tell qemu to reopen given nodes.

Aha!  You just corrected my overly narrow idea of the solution space.

> I'm not sure what ends up being less work.

"Less work" is an important consideration.

We may be able to accept some extra work to get a saner interface.
Depends on how much saner and how much extra exactly, and on who needs
to do the work.

>> Terminology:
>> 
>> * "Hard read-write" semantics: open read/write.
>> 
>> * "Hard read-only" semantics: open read-only.
>> 
>> * "Dynamic read-only" semantics: open read-only, reopen read/write when
>>   a writer attaches, reopen read-only when the last writer detaches.
>> 
>> * "Fallback read-only" semantics:. try to open read/write, fall back to
>>   read-only.
>> 
>> We have a use case for dynamic read-only: libvirt.  I'm not aware of a
>> use case for fallback read-only.
>> 
>> Behavior before 3.1:
>> 
>> * read-only=on: hard read-only
>> 
>> * read-only=off: hard read-write
>> 
>> Behavior in 3.1: new parameter auto-read-only, default off.
>> 
>> * read-only=on: hard read-only (no change)
>> 
>> * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>>   silently ignored)
>> 
>> * read-only=off: hard read-write
>> 
>> * read-only=off,auto-read-only=on: depends on the driver
>> 
>>   - file-posix.c's drivers: fallback read-only
>>   - some other drivers: fallback read-only
>>   - remaining drivers: hard read/write
>> 
>> Behavior in 4.0 so far:
>> 
>> * read-only=on: hard read-only (no change)
>> 
>> * read-only=on,auto-read-only=on: hard read-only (no change)
>> 
>> * read-only=off: hard read-write (no change)
>> 
>> * read-only=off,auto-read-only=on: depends on the driver
>> 
>>   - file-posix.c's drivers: dynamic read-only
>>   - some other drivers: fallback read-only (no change)
>>   - remaining drivers: hard read/write (no change)
>> 
>> 
>> Option 1: Rename @auto-read-only.
>> 
>> Rationale: we released auto-read-only in v3.1 with unproven fallback
>> read-only semantics.  It turned out not to be useful.  Admit the
>> mistake, retract it.  Release our next attempt in 4.0 under a suitable
>> new name with fallback read-only semantics.
>
> I concur. Nobody probably uses this. Setting up selinux and blockdev is
> not a trivial excercise.

Right.

>> CON:
>> 
>> * Compatibility break.  No-go if there are users.  Users seem quite
>>   unlikely, though.
>> 
>> * Still unproven, albeit less so: this time we have some unreleased
>>   (unfinished?) libvirt code.
>
> unfinished and thus also unreleased.

Noted.

>> 
>> * Semantics are still largely left to drivers, and the schema can't tell
>>   which one does what.  Awkward.
>> 
>> * Unless we're fairly confident we won't upgrade drivers from "hard" to
>>   "fallback" to "dynamic", this merely kicks the can down the road:
>>   we'll face the exact same "how can libvirt find out" problem on every
>>   upgrade.
>> 
>> PRO:
>> 
>> * When libvirt sees the new name, it can rely on file-posix.c's drivers
>>   providing dynamic read-only semantics.
>> 
>> 
>> Option 2: Add query-qemu-features command, return
>> file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
>> 
>> Rationale: we released auto-read-only in v3.1 with unproven fallback
>> read-only semantics.  It turned out not to be useful.  Admit the
>> mistake, and patch it up in 4.0.  Libirt needs to know whether it's
>> patche up, and this is a simple way to tell it.
>> 
>> CON:
>> 
>> * All of option 1's, except for the compatibility break
>> 
>> * Uses query-qemu-features to expose a property of the build.  I
>>   consider that a mistake.
>> 
>> PRO
>> 
>> * Libvirt can use either query-qmp-schema or query-qemu-features to find
>>   out whether it can can rely on file-posix.c's drivers providing
>>   dynamic read-only semantics.  To make query-qemu-features usable, we
>>   need to promise query-qemu-features never returns false for it.  To
>>   make query-qemu-features usable, we need to promise the value will
>>   remain valid for the remainder of the QEMU run (defeats caching) or
>>   for any future run of the same QEMU binary (enables caching).
>> 
>> 
>> Option 3: Add @dynamic-read-only to the drivers that provide dynamic
>> read-only, default to value of auto-read-only, promise we'll never add
>> it to drivers that don't.
>> 
>> Rationale: give users explicit control over dynamic vs. fallback for all
>> drivers that can provide dynamic.  This makes some sense as an
>> interface, as long as you ignore the fact that no driver implements both
>> dynamic and fallback.  I can't see why a driver couldn't implement both.
>> It also makes dynamic support visible in the schema.
>> 
>> Behavior (three bools -> eight cases):
>> 
>> * read-only=on: hard read-only (no change)
>> 
>>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
>> 
>> * read-only=on,auto-read-only=on: hard read-only (no change)
>> 
>>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
>> 
>> * read-only=off: hard read-write (no change)
>> 
>>   Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
>> 
>> * read-only=off,auto-read-only=on:
>> 
>>   Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
>> 
>>   - file-posix.c's drivers: dynamic read-only (no change,
>>     dynamic-read-only=on is the default)
>>   - some other drivers: fallback read-only (no change)
>>   - remaining drivers: hard read/write (no change)
>> 
>> * read-only=off,auto-read-only=on,dynamic-read-only=off
>> 
>>   - file-posix.c's drivers: error
>>   - all other drivers: N/A
>> 
>> * read-only=off,auto-read-only=off,dynamic-read-only=on
>> 
>>   - file-posix.c's drivers: error
>>   - all other drivers: N/A
>> 
>> * read-only=on,dynamic-read-only=on
>> 
>>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
>> 
>>   - file-posix.c's drivers: error
>>   - all other drivers: N/A
>> 
>> * read-only=on,auto-read-only=on,dynamic-read-only=off
>> 
>>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
>> 
>>   - file-posix.c's drivers: error
>>   - all other drivers: N/A
>> 
>> CON:
>> 
>> * Two bools (read-only and auto-read-only) to select from three choices
>>   was already ugly.  Three bools (the same plus dynamic-read-only) to
>>   select from four choices is even uglier.
>> 
>> * The explicit control is just a facade so far: since only the default
>>   setting is implemented, it doesn't actually control anything.
>> 
>> PRO:
>> 
>> * When libvirt sees a driver providing a dynamic-read-only parameter, it
>>   knows it can rely on the driver providing dynamic read-only semantics.
>> 
>> * Adding dynamic read-only capability to drivers creates no
>>   introspection problems: we simply add dynamic-read-only to their
>>   parameters.
>
> In the end libvirt does not care that much if the storage is open read
> only or read write. We need the format node or guest frontend to deny
> writes if the disk is declared as read-only. We also need to be able to
> do blockjobs. The requirement is that images may not be accessible in
> R/W mode when qemu is starting but later may become. Then it's expected
> that block-commit will succeed.

Fallback read-only can't help there.

Dynamic read-only should work.  Explicit reopen triggers could also
work.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 14:55                 ` [Qemu-devel] Options for 4.0 Markus Armbruster
@ 2019-04-01 16:07                   ` Peter Krempa
  2019-04-01 16:22                     ` Markus Armbruster
  0 siblings, 1 reply; 18+ messages in thread
From: Peter Krempa @ 2019-04-01 16:07 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Kevin Wolf, qemu-block, stefanha, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 5205 bytes --]

On Mon, Apr 01, 2019 at 16:55:50 +0200, Markus Armbruster wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
> > On Sun, Mar 31, 2019 at 14:56:19 +0200, Markus Armbruster wrote:
> >> Let's review our options for 4.0.
> >> 
> >> Please note my analysis is handicapped by incomplete information, in
> >> particular on libvirt's needs.
> >
> > Libvirt's needs are "simple" we need to do block-commit. It has a few
> > caveats though. The selinux labels for backing images don't allow write
> > operation when starting qemu. When doing block-commit libvirt relabels
> > the backing chain prior to using block-commit.
> 
> Paraphrasing, to make sure I got you.
> 
> Consider the common read/write COW image with a backing image.
> 
> Libvirt wants to grant QEMU read/write access to the COW and read-only
> access to the backing image.  It does that with SELinux.  QEMU has to
> open the backing image read-only (read/write would fail).
> 
> But libvirt also wants to do block-commit.  Libvirt first relabels to
> grant QEMU read/write access to the backing image, then $something makes
> QEMU reopen the backing image read/write, block-commit does its job,
> $something makes QEMU reopen the backing image read-only, libvirt
> relabels to revoke read/write access.
> 
> Correct?

Yes.

> 
> With dynamic read-only, $something is the block commit job (a writer)
> attaching and detaching to the backing image node.
> 
> > That works with -drive as qemu reopens the files internally, but does
> > not work when using -blockdev.
> 
> Happy to take your word for it.
> 
> > Since we need to keep the images RO until doing  the blockjob there
> > needs to be a way when either qemu automagically does what libvirt needs
> > even if the image did not have the write permission originally.
> >
> > auto-read-only was a naive impl when qemu attempted to keep the storage
> > access nodes RW. At the point when I was testing it I didn't enable
> > s-virt as it's a hassle when you want to run git versions of libvirt and
> > qemu and thus didn't see the problem with missing permissions.
> 
> Yes, fallback read-only is useless for the scenario above.
> 
> Kevin wrote "libvirt requires dynamic read-only for file-posix. It
> prefers dynamic read-only for other drivers, but can live with fallback
> read-only there."  I'm not sure how it can live with fallback read-only.
> Is it because only *files* can labelled with SELinux?
> 
> If yes, then my next question is how libvirt makes use of fallback
> read-only for non-files.  Can you give me an example?

For non-files libvirt usually does not have means to modify the access
permissions.

A simple example may be a NBD export. If the NBD server exports the
image readonly libvirt can't do much about it. Arguably you can consider
a situation where the user modifies the access to read-write manually
and then invokes the libvirt virDomainBlockCommit API and thus still
expects the automagic reopen to happen correctly. At this point I'm not
really certain to which extend this would work currently with -drive as
there are known issues when attempting to point to a non-file image by
path rather than node-name.

> > The dynamic version attempts to fix that. That is still the automagic
> > approach. I don't really mind also doing a hands-on approach where we'd
> > need to tell qemu to reopen given nodes.
> 
> Aha!  You just corrected my overly narrow idea of the solution space.
> 
> > I'm not sure what ends up being less work.
> 
> "Less work" is an important consideration.
> 
> We may be able to accept some extra work to get a saner interface.
> Depends on how much saner and how much extra exactly, and on who needs
> to do the work.

I agree. While the 'explicit reopen' approach may be more work for
libvirt I'll happily accept that solution.

> 
> >> Terminology:
> >> 
> >> * "Hard read-write" semantics: open read/write.
> >> 
> >> * "Hard read-only" semantics: open read-only.
> >> 
> >> * "Dynamic read-only" semantics: open read-only, reopen read/write when
> >>   a writer attaches, reopen read-only when the last writer detaches.

[...]

> >> * When libvirt sees a driver providing a dynamic-read-only parameter, it
> >>   knows it can rely on the driver providing dynamic read-only semantics.
> >> 
> >> * Adding dynamic read-only capability to drivers creates no
> >>   introspection problems: we simply add dynamic-read-only to their
> >>   parameters.
> >
> > In the end libvirt does not care that much if the storage is open read
> > only or read write. We need the format node or guest frontend to deny
> > writes if the disk is declared as read-only. We also need to be able to
> > do blockjobs. The requirement is that images may not be accessible in
> > R/W mode when qemu is starting but later may become. Then it's expected
> > that block-commit will succeed.
> 
> Fallback read-only can't help there.
> 
> Dynamic read-only should work.  Explicit reopen triggers could also
> work.

The explicit trigger would work. With the tighter control of blockjobs
we should be able to control the lifecycle properly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 16:07                   ` Peter Krempa
@ 2019-04-01 16:22                     ` Markus Armbruster
  2019-04-02  8:50                       ` Kevin Wolf
  0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2019-04-01 16:22 UTC (permalink / raw)
  To: Peter Krempa; +Cc: Kevin Wolf, stefanha, qemu-block, qemu-devel

Peter Krempa <pkrempa@redhat.com> writes:

> On Mon, Apr 01, 2019 at 16:55:50 +0200, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>> 
>> > On Sun, Mar 31, 2019 at 14:56:19 +0200, Markus Armbruster wrote:
>> >> Let's review our options for 4.0.
>> >> 
>> >> Please note my analysis is handicapped by incomplete information, in
>> >> particular on libvirt's needs.
>> >
>> > Libvirt's needs are "simple" we need to do block-commit. It has a few
>> > caveats though. The selinux labels for backing images don't allow write
>> > operation when starting qemu. When doing block-commit libvirt relabels
>> > the backing chain prior to using block-commit.
>> 
>> Paraphrasing, to make sure I got you.
>> 
>> Consider the common read/write COW image with a backing image.
>> 
>> Libvirt wants to grant QEMU read/write access to the COW and read-only
>> access to the backing image.  It does that with SELinux.  QEMU has to
>> open the backing image read-only (read/write would fail).
>> 
>> But libvirt also wants to do block-commit.  Libvirt first relabels to
>> grant QEMU read/write access to the backing image, then $something makes
>> QEMU reopen the backing image read/write, block-commit does its job,
>> $something makes QEMU reopen the backing image read-only, libvirt
>> relabels to revoke read/write access.
>> 
>> Correct?
>
> Yes.

Thank you.

>> With dynamic read-only, $something is the block commit job (a writer)
>> attaching and detaching to the backing image node.
>> 
>> > That works with -drive as qemu reopens the files internally, but does
>> > not work when using -blockdev.
>> 
>> Happy to take your word for it.
>> 
>> > Since we need to keep the images RO until doing  the blockjob there
>> > needs to be a way when either qemu automagically does what libvirt needs
>> > even if the image did not have the write permission originally.
>> >
>> > auto-read-only was a naive impl when qemu attempted to keep the storage
>> > access nodes RW. At the point when I was testing it I didn't enable
>> > s-virt as it's a hassle when you want to run git versions of libvirt and
>> > qemu and thus didn't see the problem with missing permissions.
>> 
>> Yes, fallback read-only is useless for the scenario above.
>> 
>> Kevin wrote "libvirt requires dynamic read-only for file-posix. It
>> prefers dynamic read-only for other drivers, but can live with fallback
>> read-only there."  I'm not sure how it can live with fallback read-only.
>> Is it because only *files* can labelled with SELinux?
>> 
>> If yes, then my next question is how libvirt makes use of fallback
>> read-only for non-files.  Can you give me an example?
>
> For non-files libvirt usually does not have means to modify the access
> permissions.
>
> A simple example may be a NBD export. If the NBD server exports the
> image readonly libvirt can't do much about it. Arguably you can consider
> a situation where the user modifies the access to read-write manually
> and then invokes the libvirt virDomainBlockCommit API and thus still
> expects the automagic reopen to happen correctly. At this point I'm not
> really certain to which extend this would work currently with -drive as
> there are known issues when attempting to point to a non-file image by
> path rather than node-name.

Both automagic reopen and explicit reopen work here.  Both succeed when
the user has granted access manually.  Libvirt would have to try the
explicit reopen even in cases where it can't grant access itself.

Sounds like you don't have a use case for fallback read-only.  Correct?

>> > The dynamic version attempts to fix that. That is still the automagic
>> > approach. I don't really mind also doing a hands-on approach where we'd
>> > need to tell qemu to reopen given nodes.
>> 
>> Aha!  You just corrected my overly narrow idea of the solution space.
>> 
>> > I'm not sure what ends up being less work.
>> 
>> "Less work" is an important consideration.
>> 
>> We may be able to accept some extra work to get a saner interface.
>> Depends on how much saner and how much extra exactly, and on who needs
>> to do the work.
>
> I agree. While the 'explicit reopen' approach may be more work for
> libvirt I'll happily accept that solution.

Would x-blockdev-reopen do?

What exactly is hold it in x-space?

I guess a specialized blockdev-set-read-only would do as well.

>> >> Terminology:
>> >> 
>> >> * "Hard read-write" semantics: open read/write.
>> >> 
>> >> * "Hard read-only" semantics: open read-only.
>> >> 
>> >> * "Dynamic read-only" semantics: open read-only, reopen read/write when
>> >>   a writer attaches, reopen read-only when the last writer detaches.
>
> [...]
>
>> >> * When libvirt sees a driver providing a dynamic-read-only parameter, it
>> >>   knows it can rely on the driver providing dynamic read-only semantics.
>> >> 
>> >> * Adding dynamic read-only capability to drivers creates no
>> >>   introspection problems: we simply add dynamic-read-only to their
>> >>   parameters.
>> >
>> > In the end libvirt does not care that much if the storage is open read
>> > only or read write. We need the format node or guest frontend to deny
>> > writes if the disk is declared as read-only. We also need to be able to
>> > do blockjobs. The requirement is that images may not be accessible in
>> > R/W mode when qemu is starting but later may become. Then it's expected
>> > that block-commit will succeed.
>> 
>> Fallback read-only can't help there.
>> 
>> Dynamic read-only should work.  Explicit reopen triggers could also
>> work.
>
> The explicit trigger would work. With the tighter control of blockjobs
> we should be able to control the lifecycle properly.

Good to know, thanks!

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 16:22                     ` Markus Armbruster
@ 2019-04-02  8:50                       ` Kevin Wolf
  0 siblings, 0 replies; 18+ messages in thread
From: Kevin Wolf @ 2019-04-02  8:50 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Peter Krempa, stefanha, qemu-block, qemu-devel

Am 01.04.2019 um 18:22 hat Markus Armbruster geschrieben:
> Peter Krempa <pkrempa@redhat.com> writes:
> > On Mon, Apr 01, 2019 at 16:55:50 +0200, Markus Armbruster wrote:
> >> Peter Krempa <pkrempa@redhat.com> writes:
> >> > That works with -drive as qemu reopens the files internally, but does
> >> > not work when using -blockdev.
> >> 
> >> Happy to take your word for it.

The difference is actually not -blockdev vs. -drive, but whether you
define the 'file' node separately in its own -blockdev option or whether
you create it in the context of the format node (using file.* dotted
syntax for its options).

Effectively this doesn't change what Peter says because -drive only
supports the latter, and for -blockdev we only want to use the former.

> >> > Since we need to keep the images RO until doing  the blockjob there
> >> > needs to be a way when either qemu automagically does what libvirt needs
> >> > even if the image did not have the write permission originally.
> >> >
> >> > auto-read-only was a naive impl when qemu attempted to keep the storage
> >> > access nodes RW. At the point when I was testing it I didn't enable
> >> > s-virt as it's a hassle when you want to run git versions of libvirt and
> >> > qemu and thus didn't see the problem with missing permissions.
> >> 
> >> Yes, fallback read-only is useless for the scenario above.
> >> 
> >> Kevin wrote "libvirt requires dynamic read-only for file-posix. It
> >> prefers dynamic read-only for other drivers, but can live with fallback
> >> read-only there."  I'm not sure how it can live with fallback read-only.
> >> Is it because only *files* can labelled with SELinux?
> >> 
> >> If yes, then my next question is how libvirt makes use of fallback
> >> read-only for non-files.  Can you give me an example?
> >
> > For non-files libvirt usually does not have means to modify the access
> > permissions.
> >
> > A simple example may be a NBD export. If the NBD server exports the
> > image readonly libvirt can't do much about it. Arguably you can consider
> > a situation where the user modifies the access to read-write manually
> > and then invokes the libvirt virDomainBlockCommit API and thus still
> > expects the automagic reopen to happen correctly. At this point I'm not
> > really certain to which extend this would work currently with -drive as
> > there are known issues when attempting to point to a non-file image by
> > path rather than node-name.
> 
> Both automagic reopen and explicit reopen work here.  Both succeed when
> the user has granted access manually.  Libvirt would have to try the
> explicit reopen even in cases where it can't grant access itself.
> 
> Sounds like you don't have a use case for fallback read-only.  Correct?

If everything supports dynamic read-only, I don't think anyone would
have a use for fallback read-only. The long-term vision was that we
would remove the read-only option and add a new force-read-only option
that would allow selecting between "force read-only" and "do whatever is
needed to support what the attached parents requested". This should be
all we really need. I don't think there is even a use case for a hard
read-write option.

It's just that dynamic read-only is a bit harder to implement, so we
started with fallback read-only as "good enough for libvirt" (seemingly,
turned out to be wrong for file-posix).

The definition of the option was kept intentionally vague so that it
would allow switching to dynamic read-only. The idea was that it only
turns an error into working cases, so we can safely do the switch and
libvirt would automatically be upgraded from an error case to a working
case, too.

Of course, in this reasoning we forgot that libvirt might want to just
switch back to -drive in the cases that would error out instead of
actually receiving that error.

> >> > The dynamic version attempts to fix that. That is still the automagic
> >> > approach. I don't really mind also doing a hands-on approach where we'd
> >> > need to tell qemu to reopen given nodes.
> >> 
> >> Aha!  You just corrected my overly narrow idea of the solution space.
> >> 
> >> > I'm not sure what ends up being less work.
> >> 
> >> "Less work" is an important consideration.
> >> 
> >> We may be able to accept some extra work to get a saner interface.
> >> Depends on how much saner and how much extra exactly, and on who needs
> >> to do the work.
> >
> > I agree. While the 'explicit reopen' approach may be more work for
> > libvirt I'll happily accept that solution.
> 
> Would x-blockdev-reopen do?
> 
> What exactly is hold it in x-space?
> 
> I guess a specialized blockdev-set-read-only would do as well.

x-blockdev-reopen was only just added. It provides the same options as
blockdev-add, so a first estimation would be that it's roughly the same
complexity. Experience from blockdev-add tells us that getting it stable
the next release was completely unrealistic because we didn't even fully
understand the problems yet. I expect the same for x-blockdev-reopen.

Maybe what impresses you more is that x-blockdev-reopen doesn't only
change options like read-only, but also references to other nodes. So
it's the dynamic graph reconfiguration thing we've talking about for
years. This seems hard to get right in the first attempt.

But we also know a few more concrete shortcomings. For example, changing
child nodes doesn't work if iothreads are involved.

Also: Not all option that could theoretically be changed in
x-blockdev-reopen are actually supported yet. If we mark it stable
before everything is covered, we'll need some kind of schema annotations
again that describe in which version changing which option was added.
Maybe this time we'd better figure out how to do that before creating
the problem.

For blockdev-add, we decided to only mark it stable as soon as full
feature parity with -drive was achieved (except for options that we
_really_ don't want to have in QAPI).

So I'm very doubtful of marking x-blockdev-reopen stable in 4.1.

A specialised blockdev-set-read-only seems more realistic for 4.1, but
it still wouldn't solve the problem now in 4.0 and once we have a stable
blockdev-reopen, it's duplicated functionality.

Kevin

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
       [not found]             ` <87y34v42z0.fsf_-_@dusky.pond.sub.org>
  2019-04-01 11:54               ` [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features) Kevin Wolf
  2019-04-01 12:20               ` [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features) Peter Krempa
@ 2019-04-01 20:02               ` Markus Armbruster
  2019-04-02  8:19                 ` Kevin Wolf
  2 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2019-04-01 20:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Eric Blake, pkrempa, stefanha, qemu-block

Markus Armbruster <armbru@redhat.com> writes:

> Let's review our options for 4.0.
>
> Please note my analysis is handicapped by incomplete information, in
> particular on libvirt's needs.
>
> Terminology:
>
> * "Hard read-write" semantics: open read/write.
>
> * "Hard read-only" semantics: open read-only.
>
> * "Dynamic read-only" semantics: open read-only, reopen read/write when
>   a writer attaches, reopen read-only when the last writer detaches.
>
> * "Fallback read-only" semantics:. try to open read/write, fall back to
>   read-only.
>
> We have a use case for dynamic read-only: libvirt.  I'm not aware of a
> use case for fallback read-only.
>
> Behavior before 3.1:
>
> * read-only=on: hard read-only
>
> * read-only=off: hard read-write
>
> Behavior in 3.1: new parameter auto-read-only, default off.
>
> * read-only=on: hard read-only (no change)
>
> * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>   silently ignored)
>
> * read-only=off: hard read-write
>
> * read-only=off,auto-read-only=on: depends on the driver
>
>   - file-posix.c's drivers: fallback read-only
>   - some other drivers: fallback read-only
>   - remaining drivers: hard read/write
>
> Behavior in 4.0 so far:
>
> * read-only=on: hard read-only (no change)
>
> * read-only=on,auto-read-only=on: hard read-only (no change)
>
> * read-only=off: hard read-write (no change)
>
> * read-only=off,auto-read-only=on: depends on the driver
>
>   - file-posix.c's drivers: dynamic read-only
>   - some other drivers: fallback read-only (no change)
>   - remaining drivers: hard read/write (no change)
>
>
> Option 1: Rename @auto-read-only.
>
> Rationale: we released auto-read-only in v3.1 with unproven fallback
> read-only semantics.  It turned out not to be useful.  Admit the
> mistake, retract it.  Release our next attempt in 4.0 under a suitable
> new name with fallback read-only semantics.
>
> CON:
>
> * Compatibility break.  No-go if there are users.  Users seem quite
>   unlikely, though.
>
> * Still unproven, albeit less so: this time we have some unreleased
>   (unfinished?) libvirt code.
>
> * Semantics are still largely left to drivers, and the schema can't tell
>   which one does what.  Awkward.
>
> * Unless we're fairly confident we won't upgrade drivers from "hard" to
>   "fallback" to "dynamic", this merely kicks the can down the road:
>   we'll face the exact same "how can libvirt find out" problem on every
>   upgrade.
>
> PRO:
>
> * When libvirt sees the new name, it can rely on file-posix.c's drivers
>   providing dynamic read-only semantics.
>
>
> Option 2: Add query-qemu-features command, return
> file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
>
> Rationale: we released auto-read-only in v3.1 with unproven fallback
> read-only semantics.  It turned out not to be useful.  Admit the
> mistake, and patch it up in 4.0.  Libirt needs to know whether it's
> patche up, and this is a simple way to tell it.
>
> CON:
>
> * All of option 1's, except for the compatibility break
>
> * Uses query-qemu-features to expose a property of the build.  I
>   consider that a mistake.
>
> PRO
>
> * Libvirt can use either query-qmp-schema or query-qemu-features to find
>   out whether it can can rely on file-posix.c's drivers providing
>   dynamic read-only semantics.  To make query-qemu-features usable, we
>   need to promise query-qemu-features never returns false for it.  To
>   make query-qemu-features usable, we need to promise the value will
>   remain valid for the remainder of the QEMU run (defeats caching) or
>   for any future run of the same QEMU binary (enables caching).
>
>
> Option 3: Add @dynamic-read-only to the drivers that provide dynamic
> read-only, default to value of auto-read-only, promise we'll never add
> it to drivers that don't.
>
> Rationale: give users explicit control over dynamic vs. fallback for all
> drivers that can provide dynamic.  This makes some sense as an
> interface, as long as you ignore the fact that no driver implements both
> dynamic and fallback.  I can't see why a driver couldn't implement both.
> It also makes dynamic support visible in the schema.
>
> Behavior (three bools -> eight cases):
>
> * read-only=on: hard read-only (no change)
>
>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
>
> * read-only=on,auto-read-only=on: hard read-only (no change)
>
>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
>
> * read-only=off: hard read-write (no change)
>
>   Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
>
> * read-only=off,auto-read-only=on:
>
>   Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
>
>   - file-posix.c's drivers: dynamic read-only (no change,
>     dynamic-read-only=on is the default)
>   - some other drivers: fallback read-only (no change)
>   - remaining drivers: hard read/write (no change)
>
> * read-only=off,auto-read-only=on,dynamic-read-only=off
>
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
>
> * read-only=off,auto-read-only=off,dynamic-read-only=on
>
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
>
> * read-only=on,dynamic-read-only=on
>
>   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
>
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
>
> * read-only=on,auto-read-only=on,dynamic-read-only=off
>
>   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
>
>   - file-posix.c's drivers: error
>   - all other drivers: N/A
>
> CON:
>
> * Two bools (read-only and auto-read-only) to select from three choices
>   was already ugly.  Three bools (the same plus dynamic-read-only) to
>   select from four choices is even uglier.
>
> * The explicit control is just a facade so far: since only the default
>   setting is implemented, it doesn't actually control anything.
>
> PRO:
>
> * When libvirt sees a driver providing a dynamic-read-only parameter, it
>   knows it can rely on the driver providing dynamic read-only semantics.
>
> * Adding dynamic read-only capability to drivers creates no
>   introspection problems: we simply add dynamic-read-only to their
>   parameters.
>
> Code sketch appended

Option 4 [Kevin]: Add a dummy option to BlockdevOptionsFile.
Message-ID: <20190401115410.GB4935@localhost.localdomain>

A variation of option 3, code is almost identical.  Where option 3 tries
to evolve what we have into the least bad permanent interface, option 4
only wants to serve as a stop gap until we get a better solution, such
as option 5.

Option 5 [Kevin]: Add feature flags to the QAPI language, use one to say
"this block driver provides dynamic read-only".
Message-ID: <20190401140842.GD4935@localhost.localdomain>

Option 6 [Peter]: Trigger reopen explicitly via QMP.
Message-ID: <20190401122054.GA2686@andariel.pipo.sk>
[Me] This leaves auto-read-only without a use case; deprecate?


Given that the libvirt code isn't ready, why do we need this done and
declared stable in 4.0?  We're going to tag -rc2 tomorrow...

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-01 20:02               ` Markus Armbruster
@ 2019-04-02  8:19                 ` Kevin Wolf
  2019-04-02  8:42                   ` Peter Krempa
  2019-04-02  9:09                   ` Markus Armbruster
  0 siblings, 2 replies; 18+ messages in thread
From: Kevin Wolf @ 2019-04-02  8:19 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, Eric Blake, pkrempa, stefanha, qemu-block

Am 01.04.2019 um 22:02 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Let's review our options for 4.0.
> >
> > Please note my analysis is handicapped by incomplete information, in
> > particular on libvirt's needs.
> >
> > Terminology:
> >
> > * "Hard read-write" semantics: open read/write.
> >
> > * "Hard read-only" semantics: open read-only.
> >
> > * "Dynamic read-only" semantics: open read-only, reopen read/write when
> >   a writer attaches, reopen read-only when the last writer detaches.
> >
> > * "Fallback read-only" semantics:. try to open read/write, fall back to
> >   read-only.
> >
> > We have a use case for dynamic read-only: libvirt.  I'm not aware of a
> > use case for fallback read-only.
> >
> > Behavior before 3.1:
> >
> > * read-only=on: hard read-only
> >
> > * read-only=off: hard read-write
> >
> > Behavior in 3.1: new parameter auto-read-only, default off.
> >
> > * read-only=on: hard read-only (no change)
> >
> > * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
> >   silently ignored)
> >
> > * read-only=off: hard read-write
> >
> > * read-only=off,auto-read-only=on: depends on the driver
> >
> >   - file-posix.c's drivers: fallback read-only
> >   - some other drivers: fallback read-only
> >   - remaining drivers: hard read/write
> >
> > Behavior in 4.0 so far:
> >
> > * read-only=on: hard read-only (no change)
> >
> > * read-only=on,auto-read-only=on: hard read-only (no change)
> >
> > * read-only=off: hard read-write (no change)
> >
> > * read-only=off,auto-read-only=on: depends on the driver
> >
> >   - file-posix.c's drivers: dynamic read-only
> >   - some other drivers: fallback read-only (no change)
> >   - remaining drivers: hard read/write (no change)
> >
> >
> > Option 1: Rename @auto-read-only.
> >
> > Rationale: we released auto-read-only in v3.1 with unproven fallback
> > read-only semantics.  It turned out not to be useful.  Admit the
> > mistake, retract it.  Release our next attempt in 4.0 under a suitable
> > new name with fallback read-only semantics.
> >
> > CON:
> >
> > * Compatibility break.  No-go if there are users.  Users seem quite
> >   unlikely, though.
> >
> > * Still unproven, albeit less so: this time we have some unreleased
> >   (unfinished?) libvirt code.
> >
> > * Semantics are still largely left to drivers, and the schema can't tell
> >   which one does what.  Awkward.
> >
> > * Unless we're fairly confident we won't upgrade drivers from "hard" to
> >   "fallback" to "dynamic", this merely kicks the can down the road:
> >   we'll face the exact same "how can libvirt find out" problem on every
> >   upgrade.
> >
> > PRO:
> >
> > * When libvirt sees the new name, it can rely on file-posix.c's drivers
> >   providing dynamic read-only semantics.
> >
> >
> > Option 2: Add query-qemu-features command, return
> > file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
> >
> > Rationale: we released auto-read-only in v3.1 with unproven fallback
> > read-only semantics.  It turned out not to be useful.  Admit the
> > mistake, and patch it up in 4.0.  Libirt needs to know whether it's
> > patche up, and this is a simple way to tell it.
> >
> > CON:
> >
> > * All of option 1's, except for the compatibility break
> >
> > * Uses query-qemu-features to expose a property of the build.  I
> >   consider that a mistake.
> >
> > PRO
> >
> > * Libvirt can use either query-qmp-schema or query-qemu-features to find
> >   out whether it can can rely on file-posix.c's drivers providing
> >   dynamic read-only semantics.  To make query-qemu-features usable, we
> >   need to promise query-qemu-features never returns false for it.  To
> >   make query-qemu-features usable, we need to promise the value will
> >   remain valid for the remainder of the QEMU run (defeats caching) or
> >   for any future run of the same QEMU binary (enables caching).
> >
> >
> > Option 3: Add @dynamic-read-only to the drivers that provide dynamic
> > read-only, default to value of auto-read-only, promise we'll never add
> > it to drivers that don't.
> >
> > Rationale: give users explicit control over dynamic vs. fallback for all
> > drivers that can provide dynamic.  This makes some sense as an
> > interface, as long as you ignore the fact that no driver implements both
> > dynamic and fallback.  I can't see why a driver couldn't implement both.
> > It also makes dynamic support visible in the schema.
> >
> > Behavior (three bools -> eight cases):
> >
> > * read-only=on: hard read-only (no change)
> >
> >   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
> >
> > * read-only=on,auto-read-only=on: hard read-only (no change)
> >
> >   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
> >
> > * read-only=off: hard read-write (no change)
> >
> >   Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
> >
> > * read-only=off,auto-read-only=on:
> >
> >   Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
> >
> >   - file-posix.c's drivers: dynamic read-only (no change,
> >     dynamic-read-only=on is the default)
> >   - some other drivers: fallback read-only (no change)
> >   - remaining drivers: hard read/write (no change)
> >
> > * read-only=off,auto-read-only=on,dynamic-read-only=off
> >
> >   - file-posix.c's drivers: error
> >   - all other drivers: N/A
> >
> > * read-only=off,auto-read-only=off,dynamic-read-only=on
> >
> >   - file-posix.c's drivers: error
> >   - all other drivers: N/A
> >
> > * read-only=on,dynamic-read-only=on
> >
> >   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
> >
> >   - file-posix.c's drivers: error
> >   - all other drivers: N/A
> >
> > * read-only=on,auto-read-only=on,dynamic-read-only=off
> >
> >   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
> >
> >   - file-posix.c's drivers: error
> >   - all other drivers: N/A
> >
> > CON:
> >
> > * Two bools (read-only and auto-read-only) to select from three choices
> >   was already ugly.  Three bools (the same plus dynamic-read-only) to
> >   select from four choices is even uglier.
> >
> > * The explicit control is just a facade so far: since only the default
> >   setting is implemented, it doesn't actually control anything.
> >
> > PRO:
> >
> > * When libvirt sees a driver providing a dynamic-read-only parameter, it
> >   knows it can rely on the driver providing dynamic read-only semantics.
> >
> > * Adding dynamic read-only capability to drivers creates no
> >   introspection problems: we simply add dynamic-read-only to their
> >   parameters.
> >
> > Code sketch appended
> 
> Option 4 [Kevin]: Add a dummy option to BlockdevOptionsFile.
> Message-ID: <20190401115410.GB4935@localhost.localdomain>
> 
> A variation of option 3, code is almost identical.  Where option 3 tries
> to evolve what we have into the least bad permanent interface, option 4
> only wants to serve as a stop gap until we get a better solution, such
> as option 5.

I think code-wise, this is actually very different from option 3. We
need to actually process the dynamic-read-only option with option 3, and
I can see things go wrong easily, which is not something to do for -rc2.

Option 4, in contrast, is purely a minimal schema change without
touching any C source file.

> Option 5 [Kevin]: Add feature flags to the QAPI language, use one to say
> "this block driver provides dynamic read-only".
> Message-ID: <20190401140842.GD4935@localhost.localdomain>

More complex than option 4, but the advantage here is that none of the
new code is actually externally visible, so if the implementation turns
out buggy, we can fix it without having to maintain compatibility with
the broken state.

The actually visible change, the query-qmp-schema output, is almost as
minimal as option 4 and can manually be verified.

A bit more invasive than option 4, but I'd consider it still doable for
-rc2 if we decide quickly.

> Option 6 [Peter]: Trigger reopen explicitly via QMP.
> Message-ID: <20190401122054.GA2686@andariel.pipo.sk>
> [Me] This leaves auto-read-only without a use case; deprecate?

We have made some progress there with the new x-blockdev-reopen command.
There are a few things that we know must be addressed before we can
declare it stable, and probably there are more such things that we don't
know of.

x-blockdev-reopen takes the same options as blockdev-add and as such has
a similar complexity. The experience with blockdev-add makes me doubt
that we can mark it stable even for 4.1. So this might be a long-term
solution, but we'd have to do something else for the meantime.

> Given that the libvirt code isn't ready, why do we need this done and
> declared stable in 4.0?  We're going to tag -rc2 tomorrow...

The next libvirt release will be long before the next QEMU release. If
we don't have it in 4.0, enabling -blockdev will have to wait until the
first libvirt release after the QEMU 4.1 release (so maybe September)
and we'll still be stuck with -drive and unable to start deprecating it
until then.

Also, we don't want to give Peter the excuse that the qemu code isn't
ready. ;-)

Kevin

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-02  8:19                 ` Kevin Wolf
@ 2019-04-02  8:42                   ` Peter Krempa
  2019-04-02  9:09                   ` Markus Armbruster
  1 sibling, 0 replies; 18+ messages in thread
From: Peter Krempa @ 2019-04-02  8:42 UTC (permalink / raw)
  To: Kevin Wolf
  Cc: Markus Armbruster, qemu-devel, Eric Blake, stefanha, qemu-block

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

On Tue, Apr 02, 2019 at 10:19:29 +0200, Kevin Wolf wrote:
> Am 01.04.2019 um 22:02 hat Markus Armbruster geschrieben:
> > Markus Armbruster <armbru@redhat.com> writes:

[...]

> > Given that the libvirt code isn't ready, why do we need this done and
> > declared stable in 4.0?  We're going to tag -rc2 tomorrow...
> 
> The next libvirt release will be long before the next QEMU release. If
> we don't have it in 4.0, enabling -blockdev will have to wait until the
> first libvirt release after the QEMU 4.1 release (so maybe September)
> and we'll still be stuck with -drive and unable to start deprecating it
> until then.
> 
> Also, we don't want to give Peter the excuse that the qemu code isn't
> ready. ;-)

It will not be an excuse. That's because we have plenty of 'prior art'
in adding version-based capabilities if everything else fails. I'm just
always unhappy when I see it.

In this instance I failed to speak up soon enough to get a better fix
ready. I could try to excuse myself, but I will not.

Just please don't drop the work that can be used to detect further
changes like this. It might come in handy sooner or later.

At this point I'd not change anything. I agree that last second fixes
have a big probability to be regretted later.

In the end it may end up being necessary to fix yet another thing for
blockdev or something else and we can at least prevent that from having
a version-based check.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] Options for 4.0
  2019-04-02  8:19                 ` Kevin Wolf
  2019-04-02  8:42                   ` Peter Krempa
@ 2019-04-02  9:09                   ` Markus Armbruster
  1 sibling, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2019-04-02  9:09 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-block, pkrempa, qemu-devel, stefanha

If in a hurry, skip ahead to "bottom line".

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.04.2019 um 22:02 hat Markus Armbruster geschrieben:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Let's review our options for 4.0.
>> >
>> > Please note my analysis is handicapped by incomplete information, in
>> > particular on libvirt's needs.
>> >
>> > Terminology:
>> >
>> > * "Hard read-write" semantics: open read/write.
>> >
>> > * "Hard read-only" semantics: open read-only.
>> >
>> > * "Dynamic read-only" semantics: open read-only, reopen read/write when
>> >   a writer attaches, reopen read-only when the last writer detaches.
>> >
>> > * "Fallback read-only" semantics:. try to open read/write, fall back to
>> >   read-only.
>> >
>> > We have a use case for dynamic read-only: libvirt.  I'm not aware of a
>> > use case for fallback read-only.
>> >
>> > Behavior before 3.1:
>> >
>> > * read-only=on: hard read-only
>> >
>> > * read-only=off: hard read-write
>> >
>> > Behavior in 3.1: new parameter auto-read-only, default off.
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>> >   silently ignored)
>> >
>> > * read-only=off: hard read-write
>> >
>> > * read-only=off,auto-read-only=on: depends on the driver
>> >
>> >   - file-posix.c's drivers: fallback read-only
>> >   - some other drivers: fallback read-only
>> >   - remaining drivers: hard read/write
>> >
>> > Behavior in 4.0 so far:
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (no change)
>> >
>> > * read-only=off: hard read-write (no change)
>> >
>> > * read-only=off,auto-read-only=on: depends on the driver
>> >
>> >   - file-posix.c's drivers: dynamic read-only
>> >   - some other drivers: fallback read-only (no change)
>> >   - remaining drivers: hard read/write (no change)
>> >
>> >
>> > Option 1: Rename @auto-read-only.
>> >
>> > Rationale: we released auto-read-only in v3.1 with unproven fallback
>> > read-only semantics.  It turned out not to be useful.  Admit the
>> > mistake, retract it.  Release our next attempt in 4.0 under a suitable
>> > new name with fallback read-only semantics.
>> >
>> > CON:
>> >
>> > * Compatibility break.  No-go if there are users.  Users seem quite
>> >   unlikely, though.
>> >
>> > * Still unproven, albeit less so: this time we have some unreleased
>> >   (unfinished?) libvirt code.
>> >
>> > * Semantics are still largely left to drivers, and the schema can't tell
>> >   which one does what.  Awkward.
>> >
>> > * Unless we're fairly confident we won't upgrade drivers from "hard" to
>> >   "fallback" to "dynamic", this merely kicks the can down the road:
>> >   we'll face the exact same "how can libvirt find out" problem on every
>> >   upgrade.
>> >
>> > PRO:
>> >
>> > * When libvirt sees the new name, it can rely on file-posix.c's drivers
>> >   providing dynamic read-only semantics.
>> >
>> >
>> > Option 2: Add query-qemu-features command, return
>> > file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
>> >
>> > Rationale: we released auto-read-only in v3.1 with unproven fallback
>> > read-only semantics.  It turned out not to be useful.  Admit the
>> > mistake, and patch it up in 4.0.  Libirt needs to know whether it's
>> > patche up, and this is a simple way to tell it.
>> >
>> > CON:
>> >
>> > * All of option 1's, except for the compatibility break
>> >
>> > * Uses query-qemu-features to expose a property of the build.  I
>> >   consider that a mistake.
>> >
>> > PRO
>> >
>> > * Libvirt can use either query-qmp-schema or query-qemu-features to find
>> >   out whether it can can rely on file-posix.c's drivers providing
>> >   dynamic read-only semantics.  To make query-qemu-features usable, we
>> >   need to promise query-qemu-features never returns false for it.  To
>> >   make query-qemu-features usable, we need to promise the value will
>> >   remain valid for the remainder of the QEMU run (defeats caching) or
>> >   for any future run of the same QEMU binary (enables caching).
>> >
>> >
>> > Option 3: Add @dynamic-read-only to the drivers that provide dynamic
>> > read-only, default to value of auto-read-only, promise we'll never add
>> > it to drivers that don't.
>> >
>> > Rationale: give users explicit control over dynamic vs. fallback for all
>> > drivers that can provide dynamic.  This makes some sense as an
>> > interface, as long as you ignore the fact that no driver implements both
>> > dynamic and fallback.  I can't see why a driver couldn't implement both.
>> > It also makes dynamic support visible in the schema.
>> >
>> > Behavior (three bools -> eight cases):
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> >   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (no change)
>> >
>> >   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
>> >
>> > * read-only=off: hard read-write (no change)
>> >
>> >   Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
>> >
>> > * read-only=off,auto-read-only=on:
>> >
>> >   Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
>> >
>> >   - file-posix.c's drivers: dynamic read-only (no change,
>> >     dynamic-read-only=on is the default)
>> >   - some other drivers: fallback read-only (no change)
>> >   - remaining drivers: hard read/write (no change)
>> >
>> > * read-only=off,auto-read-only=on,dynamic-read-only=off
>> >
>> >   - file-posix.c's drivers: error
>> >   - all other drivers: N/A
>> >
>> > * read-only=off,auto-read-only=off,dynamic-read-only=on
>> >
>> >   - file-posix.c's drivers: error
>> >   - all other drivers: N/A
>> >
>> > * read-only=on,dynamic-read-only=on
>> >
>> >   Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
>> >
>> >   - file-posix.c's drivers: error
>> >   - all other drivers: N/A
>> >
>> > * read-only=on,auto-read-only=on,dynamic-read-only=off
>> >
>> >   Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
>> >
>> >   - file-posix.c's drivers: error
>> >   - all other drivers: N/A
>> >
>> > CON:
>> >
>> > * Two bools (read-only and auto-read-only) to select from three choices
>> >   was already ugly.  Three bools (the same plus dynamic-read-only) to
>> >   select from four choices is even uglier.
>> >
>> > * The explicit control is just a facade so far: since only the default
>> >   setting is implemented, it doesn't actually control anything.
>> >
>> > PRO:
>> >
>> > * When libvirt sees a driver providing a dynamic-read-only parameter, it
>> >   knows it can rely on the driver providing dynamic read-only semantics.
>> >
>> > * Adding dynamic read-only capability to drivers creates no
>> >   introspection problems: we simply add dynamic-read-only to their
>> >   parameters.
>> >
>> > Code sketch appended
>> 
>> Option 4 [Kevin]: Add a dummy option to BlockdevOptionsFile.
>> Message-ID: <20190401115410.GB4935@localhost.localdomain>
>> 
>> A variation of option 3, code is almost identical.  Where option 3 tries
>> to evolve what we have into the least bad permanent interface, option 4
>> only wants to serve as a stop gap until we get a better solution, such
>> as option 5.
>
> I think code-wise, this is actually very different from option 3. We
> need to actually process the dynamic-read-only option with option 3, and
> I can see things go wrong easily, which is not something to do for -rc2.
>
> Option 4, in contrast, is purely a minimal schema change without
> touching any C source file.

While I reject your "is very different" claim, I accept your "option 4
is less risky than option 3" claim.

Common part modulo naming:

    diff --git a/qapi/block-core.json b/qapi/block-core.json
    index 7ccbfff9d0..0cf15477ce 100644
    --- a/qapi/block-core.json
    +++ b/qapi/block-core.json
    @@ -2827,6 +2827,7 @@
     # Driver specific block device options for the file backend.
     #
     # @filename:    path to the image file
    +# @dynamic-read-only: FIXME document (since 4.0)
     # @pr-manager:  the id for the object that will handle persistent reservations
     #               for this device (default: none, forward the commands via SG_IO;
     #               since 2.11)
    @@ -2847,6 +2848,8 @@
     ##
     { 'struct': 'BlockdevOptionsFile',
       'data': { 'filename': 'str',
    +            '*dynamic-read-only': {'type': 'bool',
    +                                   'if': 'defined(CONFIG_POSIX)'},
                 '*pr-manager': 'str',
                 '*locking': 'OnOffAuto',
                 '*aio': 'BlockdevAioOptions',

Option 3 only:

    diff --git a/block/file-posix.c b/block/file-posix.c
    index db4cccbe51..d62681df14 100644
    --- a/block/file-posix.c
    +++ b/block/file-posix.c
    @@ -420,6 +420,11 @@ static QemuOptsList raw_runtime_opts = {
                 .type = QEMU_OPT_STRING,
                 .help = "File name of the image",
             },
    +        {
    +            .name = "dynamic-read-only",
    +            .type = QEMU_OPT_BOOL,
    +            .help = "FIXME",
    +        },
             {
                 .name = "aio",
                 .type = QEMU_OPT_STRING,
    @@ -540,6 +545,13 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         s->open_flags = open_flags;
         raw_parse_flags(bdrv_flags, &s->open_flags, false);

    +    if (qemu_opt_get_bool(opts, "dynamic-read-only",
    +                          bdrv_flags & BDRV_O_AUTO_RDONLY)
    +        == !(bdrv_flags & BDRV_O_AUTO_RDONLY)) {
    +        error_setg(errp, "FIXME");
    +        ret = - EINVAL;
    +    }
    +
         s->fd = -1;
         fd = qemu_open(filename, s->open_flags, 0644);
         ret = fd < 0 ? -errno : 0;

Yes, this is more code, and yes, things could conceivably go wrong here.

>> Option 5 [Kevin]: Add feature flags to the QAPI language, use one to say
>> "this block driver provides dynamic read-only".
>> Message-ID: <20190401140842.GD4935@localhost.localdomain>
>
> More complex than option 4, but the advantage here is that none of the
> new code is actually externally visible, so if the implementation turns
> out buggy, we can fix it without having to maintain compatibility with
> the broken state.
>
> The actually visible change, the query-qmp-schema output, is almost as
> minimal as option 4 and can manually be verified.
>
> A bit more invasive than option 4, but I'd consider it still doable for
> -rc2 if we decide quickly.

It's a good option given the mess we made with auto-read-only in 3.1.
It's just three weeks late.

>> Option 6 [Peter]: Trigger reopen explicitly via QMP.
>> Message-ID: <20190401122054.GA2686@andariel.pipo.sk>
>> [Me] This leaves auto-read-only without a use case; deprecate?
>
> We have made some progress there with the new x-blockdev-reopen command.
> There are a few things that we know must be addressed before we can
> declare it stable, and probably there are more such things that we don't
> know of.
>
> x-blockdev-reopen takes the same options as blockdev-add and as such has
> a similar complexity. The experience with blockdev-add makes me doubt
> that we can mark it stable even for 4.1. So this might be a long-term
> solution, but we'd have to do something else for the meantime.

Understand.

>> Given that the libvirt code isn't ready, why do we need this done and
>> declared stable in 4.0?  We're going to tag -rc2 tomorrow...
>
> The next libvirt release will be long before the next QEMU release. If

Libvirt releases roughly monthly.

QEMU releases roughly April, August, December.

> we don't have it in 4.0, enabling -blockdev will have to wait until the
> first libvirt release after the QEMU 4.1 release (so maybe September)
> and we'll still be stuck with -drive and unable to start deprecating it
> until then.

Unless libvirt does yet another version check.

I talked this over with Peter.

libvirt 5.2 is expected today.  Peter is trying to get his work merged
in time for the next libvirt release (but he was trying that a month
ago, too).  So we're talking about 5.3 (May) or 5.4 (June).  Leaves a
gap of 2-3 month to 4.1 (August).

I offered option 4 "Add a dummy option to BlockdevOptionsFile".  He said
he'd advise against ugly hacks at this point, but going forward, QEMU
really needs a way to let libvirt detect code fixes, such as QAPI schema
feature flags.

> Also, we don't want to give Peter the excuse that the qemu code isn't
> ready. ;-)

Ah, you'd never do that to us, Peter, would you?


Bottom line:

* I can't bring myself to do QAPI schema language changes (option 5)
  this late in the game.  Sorry.

* I'm okay with option 4 if Kevin believes it could be useful.  Even if
  Peter won't actually use it, we're fairly unlikely to regret it.

* A more complete respin of the proposed QAPI schema language changes
  would be fair game for 4.1.

* QEMU and libvirt need to get better at co-developing features.  QEMU
  doesn't want to release stable interfaces without a user, and libvirt
  doesn't want to release a user without QEMU releasing first.  When we
  break this cycle by having QEMU releases stable interfaces completely
  unproven, we unsurprisingly get to regret it later.

Does this make sense?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH for-4.0 0/2] file-posix: query-qemu-features for auto-read-only
       [not found] <20190328182810.21497-1-kwolf@redhat.com>
       [not found] ` <20190328182810.21497-2-kwolf@redhat.com>
@ 2019-04-05 10:32 ` Stefan Hajnoczi
  2019-04-05 10:32   ` Stefan Hajnoczi
  2019-04-05 12:46   ` Kevin Wolf
  1 sibling, 2 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2019-04-05 10:32 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-block, armbru, pkrempa, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Thu, Mar 28, 2019 at 07:28:08PM +0100, Kevin Wolf wrote:
> auto-read-only=on changed its behaviour in file-posix for the 4.0
> release. This change cannot be detected through the usual mechanisms
> like schema introspection.
> 
> I took Stefan's patch 'qmp: add query-qemu-capabilities', removed the
> feature that he originally wanted this for and addressed the review
> comments. The introduction of the file-posix-dynamic-auto-read-only
> feature is a separate patch.
> 
> Stefan, I kept you as the author of the first patch and also your
> Signed-off-by. Is this okay with you or would you prefer a change?

Sorry I was unresponsive.  I'm fine with the authorship and
Signed-off-by: but won't be offended if either or both are removed when
the patch is reworked (my code is trivial).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH for-4.0 0/2] file-posix: query-qemu-features for auto-read-only
  2019-04-05 10:32 ` [Qemu-devel] [PATCH for-4.0 0/2] file-posix: query-qemu-features for auto-read-only Stefan Hajnoczi
@ 2019-04-05 10:32   ` Stefan Hajnoczi
  2019-04-05 12:46   ` Kevin Wolf
  1 sibling, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2019-04-05 10:32 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, armbru, qemu-block, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Thu, Mar 28, 2019 at 07:28:08PM +0100, Kevin Wolf wrote:
> auto-read-only=on changed its behaviour in file-posix for the 4.0
> release. This change cannot be detected through the usual mechanisms
> like schema introspection.
> 
> I took Stefan's patch 'qmp: add query-qemu-capabilities', removed the
> feature that he originally wanted this for and addressed the review
> comments. The introduction of the file-posix-dynamic-auto-read-only
> feature is a separate patch.
> 
> Stefan, I kept you as the author of the first patch and also your
> Signed-off-by. Is this okay with you or would you prefer a change?

Sorry I was unresponsive.  I'm fine with the authorship and
Signed-off-by: but won't be offended if either or both are removed when
the patch is reworked (my code is trivial).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH for-4.0 0/2] file-posix: query-qemu-features for auto-read-only
  2019-04-05 10:32 ` [Qemu-devel] [PATCH for-4.0 0/2] file-posix: query-qemu-features for auto-read-only Stefan Hajnoczi
  2019-04-05 10:32   ` Stefan Hajnoczi
@ 2019-04-05 12:46   ` Kevin Wolf
  2019-04-05 12:46     ` Kevin Wolf
  1 sibling, 1 reply; 18+ messages in thread
From: Kevin Wolf @ 2019-04-05 12:46 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: qemu-block, armbru, pkrempa, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

Am 05.04.2019 um 12:32 hat Stefan Hajnoczi geschrieben:
> On Thu, Mar 28, 2019 at 07:28:08PM +0100, Kevin Wolf wrote:
> > auto-read-only=on changed its behaviour in file-posix for the 4.0
> > release. This change cannot be detected through the usual mechanisms
> > like schema introspection.
> > 
> > I took Stefan's patch 'qmp: add query-qemu-capabilities', removed the
> > feature that he originally wanted this for and addressed the review
> > comments. The introduction of the file-posix-dynamic-auto-read-only
> > feature is a separate patch.
> > 
> > Stefan, I kept you as the author of the first patch and also your
> > Signed-off-by. Is this okay with you or would you prefer a change?
> 
> Sorry I was unresponsive.  I'm fine with the authorship and
> Signed-off-by: but won't be offended if either or both are removed when
> the patch is reworked (my code is trivial).

The plan is now that we integrate features in the QAPI schema (attached
to struct definitions), so they will be available with the normal schema
introspection. Obviously, this will be a completely new patch series
that isn't based on this patch any more.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH for-4.0 0/2] file-posix: query-qemu-features for auto-read-only
  2019-04-05 12:46   ` Kevin Wolf
@ 2019-04-05 12:46     ` Kevin Wolf
  0 siblings, 0 replies; 18+ messages in thread
From: Kevin Wolf @ 2019-04-05 12:46 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: pkrempa, armbru, qemu-block, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

Am 05.04.2019 um 12:32 hat Stefan Hajnoczi geschrieben:
> On Thu, Mar 28, 2019 at 07:28:08PM +0100, Kevin Wolf wrote:
> > auto-read-only=on changed its behaviour in file-posix for the 4.0
> > release. This change cannot be detected through the usual mechanisms
> > like schema introspection.
> > 
> > I took Stefan's patch 'qmp: add query-qemu-capabilities', removed the
> > feature that he originally wanted this for and addressed the review
> > comments. The introduction of the file-posix-dynamic-auto-read-only
> > feature is a separate patch.
> > 
> > Stefan, I kept you as the author of the first patch and also your
> > Signed-off-by. Is this okay with you or would you prefer a change?
> 
> Sorry I was unresponsive.  I'm fine with the authorship and
> Signed-off-by: but won't be offended if either or both are removed when
> the patch is reworked (my code is trivial).

The plan is now that we integrate features in the QAPI schema (attached
to struct definitions), so they will be available with the normal schema
introspection. Obviously, this will be a completely new patch series
that isn't based on this patch any more.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-04-05 12:47 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20190328182810.21497-1-kwolf@redhat.com>
     [not found] ` <20190328182810.21497-2-kwolf@redhat.com>
     [not found]   ` <2fc10494-0f79-8d18-610d-be21cfc6c30b@redhat.com>
     [not found]     ` <87ftr5iy8i.fsf@dusky.pond.sub.org>
     [not found]       ` <c233c2b4-5257-2df7-3c1a-06bcf269d657@redhat.com>
     [not found]         ` <20190329170439.GK5081@localhost.localdomain>
     [not found]           ` <87o95sa40r.fsf@dusky.pond.sub.org>
     [not found]             ` <87y34v42z0.fsf_-_@dusky.pond.sub.org>
2019-04-01 11:54               ` [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features) Kevin Wolf
2019-04-01 13:11                 ` [Qemu-devel] Options for 4.0 Markus Armbruster
2019-04-01 14:08                   ` Kevin Wolf
2019-04-01 15:31                     ` Markus Armbruster
2019-04-01 19:54                     ` Markus Armbruster
2019-04-01 12:20               ` [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features) Peter Krempa
2019-04-01 14:55                 ` [Qemu-devel] Options for 4.0 Markus Armbruster
2019-04-01 16:07                   ` Peter Krempa
2019-04-01 16:22                     ` Markus Armbruster
2019-04-02  8:50                       ` Kevin Wolf
2019-04-01 20:02               ` Markus Armbruster
2019-04-02  8:19                 ` Kevin Wolf
2019-04-02  8:42                   ` Peter Krempa
2019-04-02  9:09                   ` Markus Armbruster
2019-04-05 10:32 ` [Qemu-devel] [PATCH for-4.0 0/2] file-posix: query-qemu-features for auto-read-only Stefan Hajnoczi
2019-04-05 10:32   ` Stefan Hajnoczi
2019-04-05 12:46   ` Kevin Wolf
2019-04-05 12:46     ` Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).