[2/2] Add file-posix-dynamic-auto-read-only feature
diff mbox series

Message ID 20190328182810.21497-3-kwolf@redhat.com
State New
Headers show
Series
  • file-posix: query-qemu-features for auto-read-only
Related show

Commit Message

Kevin Wolf March 28, 2019, 6:28 p.m. UTC
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. Add a new feature to query-qemu-features to
allow libvirt to detect the presence of the new behaviour.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/misc.json | 7 ++++++-
 qmp.c          | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Eric Blake March 28, 2019, 6:45 p.m. UTC | #1
On 3/28/19 1:28 PM, 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. Add a new feature to query-qemu-features to
> allow libvirt to detect the presence of the new behaviour.

Oddly enough, introspecting the schema is sufficient to learn about this
particular feature (that is, until we actually have a runtime feature
that requires us to run the command, merely seeing what features the
command supports is useful on its own). But that doesn't change the fact
that we should keep things as a command.

Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf March 29, 2019, 7:52 a.m. UTC | #2
Am 28.03.2019 um 19:45 hat Eric Blake geschrieben:
> On 3/28/19 1:28 PM, 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. Add a new feature to query-qemu-features to
> > allow libvirt to detect the presence of the new behaviour.
> 
> Oddly enough, introspecting the schema is sufficient to learn about this
> particular feature (that is, until we actually have a runtime feature
> that requires us to run the command, merely seeing what features the
> command supports is useful on its own). But that doesn't change the fact
> that we should keep things as a command.

Yes, I thought the same. But without the command, the type would
disapear from query-qmp-schema, so it has to be there either way.

I think, however, that libvirt should actually run the command, not only
test for its presence. Maybe we end up with some case where a new
version can't provide the feature unconditionally for some reason, and
then we want to be able to turn a static feature into a runtime one.

Kevin
Peter Krempa March 29, 2019, 8:38 a.m. UTC | #3
On Fri, Mar 29, 2019 at 08:52:13 +0100, Kevin Wolf wrote:
> Am 28.03.2019 um 19:45 hat Eric Blake geschrieben:
> > On 3/28/19 1:28 PM, 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. Add a new feature to query-qemu-features to
> > > allow libvirt to detect the presence of the new behaviour.
> > 
> > Oddly enough, introspecting the schema is sufficient to learn about this
> > particular feature (that is, until we actually have a runtime feature
> > that requires us to run the command, merely seeing what features the
> > command supports is useful on its own). But that doesn't change the fact
> > that we should keep things as a command.
> 
> Yes, I thought the same. But without the command, the type would
> disapear from query-qmp-schema, so it has to be there either way.
> 
> I think, however, that libvirt should actually run the command, not only
> test for its presence. Maybe we end up with some case where a new
> version can't provide the feature unconditionally for some reason, and
> then we want to be able to turn a static feature into a runtime one.

I'm definitely going to call the command. Actually I'm working on
implementing it right now. One benefit of doing it right away is that we
don't have any capability test data for the new version yet, so they
will not have to be updated once we'd actually need to call the command.
Markus Armbruster March 29, 2019, 9:57 a.m. UTC | #4
Kevin Wolf <kwolf@redhat.com> writes:

> auto-read-only=on changed its behaviour in file-posix for the 4.0
> release.

Commit hash, please.

>          This change cannot be detected through the usual mechanisms
> like schema introspection. Add a new feature to query-qemu-features to
> allow libvirt to detect the presence of the new behaviour.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/misc.json | 7 ++++++-
>  qmp.c          | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index d892f37633..df23c54a65 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -3058,10 +3058,15 @@
>  # Information about support for QEMU features that isn't available through
>  # schema introspection.
>  #
> +# @file-posix-dynamic-auto-read-only:
> +#   true if auto-read-only=on means that the image file is dynamically reopened
> +#   read-only or read-write depending on whether any writers are attached to
> +#   the node.
> +#

The name @file-posix-dynamic-auto-read-only suggests the semantic change
of auto-read-only=on explained in the doc comment applies only to some
BlockDrivers (as a non-variant member of BlockdevOptions, auto-read-only
applies to all).  Which ones exactly?

>  # Since: 4.0
>  ##
>  { 'struct': 'QemuFeatures',
> -  'data': { } }
> +  'data': { 'file-posix-dynamic-auto-read-only': 'bool' } }
>  
>  ##
>  # @query-qemu-features:
> diff --git a/qmp.c b/qmp.c
> index 0aad533eca..2a887c1e7d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -723,6 +723,7 @@ QemuFeatures *qmp_query_qemu_features(Error **errp)
>      QemuFeatures *caps = g_new(QemuFeatures, 1);
>  
>      *caps = (QemuFeatures) {
> +        .file_posix_dynamic_auto_read_only = true,
>      };
>  
>      return caps;

Running the command provides no additional information over schema
introspection.  Kind of contradicts "Information about support for QEMU
features that isn't available through schema introspection" :)
Peter Krempa March 29, 2019, 11:20 a.m. UTC | #5
On Fri, Mar 29, 2019 at 10:57:55 +0100, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > auto-read-only=on changed its behaviour in file-posix for the 4.0
> > release.
> 
> Commit hash, please.
> 
> >          This change cannot be detected through the usual mechanisms
> > like schema introspection. Add a new feature to query-qemu-features to
> > allow libvirt to detect the presence of the new behaviour.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/misc.json | 7 ++++++-
> >  qmp.c          | 1 +
> >  2 files changed, 7 insertions(+), 1 deletion(-)

[...]

> > diff --git a/qmp.c b/qmp.c
> > index 0aad533eca..2a887c1e7d 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -723,6 +723,7 @@ QemuFeatures *qmp_query_qemu_features(Error **errp)
> >      QemuFeatures *caps = g_new(QemuFeatures, 1);
> >  
> >      *caps = (QemuFeatures) {
> > +        .file_posix_dynamic_auto_read_only = true,
> >      };
> >  
> >      return caps;
> 
> Running the command provides no additional information over schema
> introspection.  Kind of contradicts "Information about support for QEMU
> features that isn't available through schema introspection" :)

Honestly I don't care whether it provides information on top of the
schema. There is a long term defficiency which does not allow us to
detect changes in the code as they are invisible to libvirt's means of
capability detection. Then it results in monstrosities like:

https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_capabilities.c;h=56228e7a36e78ad2ff52a5e83d4af144deea9528;hb=HEAD#l4309

Or you get ad-hoc hacks which add a useless command just to have
something exposed in the schema:

commit e1ca8f7e1915496148f6e0ce1f7c2309af013312
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Thu Nov 22 08:16:13 2018 +0100

    qapi: add query-display-options command
    
    Add query-display-options command, which allows querying the qemu
    display configuration.  This isn't particularly useful, except it
    exposes QAPI type DisplayOptions in query-qmp-schema, so that libvirt
    can discover recently added -display parameter rendernode (commit
    d4dc4ab133b).  Works around lack of sufficiently powerful command line
    introspection.

Whether there is a static entry in the QMP schema (which feels wrong
btw as it does not actually expose something which is regarding the
interaction with QMP) or something like this, we need it yesterday. I
don't want to add version checks any more.
Markus Armbruster March 29, 2019, 1:05 p.m. UTC | #6
Peter Krempa <pkrempa@redhat.com> writes:

[...]
> Whether there is a static entry in the QMP schema (which feels wrong
> btw as it does not actually expose something which is regarding the
> interaction with QMP) or something like this, we need it yesterday. I
> don't want to add version checks any more.

I do understand the pressing need for a solution.  But beware of "we
need to do something, $this is something, therefore we need to do
$this".
Markus Armbruster March 29, 2019, 1:14 p.m. UTC | #7
Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> auto-read-only=on changed its behaviour in file-posix for the 4.0
>> release.
>
> Commit hash, please.

I guess it's commit 23dece19da4 "file-posix: Make auto-read-only
dynamic".

>>          This change cannot be detected through the usual mechanisms
>> like schema introspection. Add a new feature to query-qemu-features to
>> allow libvirt to detect the presence of the new behaviour.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  qapi/misc.json | 7 ++++++-
>>  qmp.c          | 1 +
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index d892f37633..df23c54a65 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -3058,10 +3058,15 @@
>>  # Information about support for QEMU features that isn't available through
>>  # schema introspection.
>>  #
>> +# @file-posix-dynamic-auto-read-only:
>> +#   true if auto-read-only=on means that the image file is dynamically reopened
>> +#   read-only or read-write depending on whether any writers are attached to
>> +#   the node.
>> +#
>
> The name @file-posix-dynamic-auto-read-only suggests the semantic change
> of auto-read-only=on explained in the doc comment applies only to some
> BlockDrivers (as a non-variant member of BlockdevOptions, auto-read-only
> applies to all).  Which ones exactly?

Kevin answered this on IRC.  In my words (and possibly with my
mistakes):

@auto-read-only is advisory, as documented:

# @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
#                  (default: false, since 3.1)

Note the "QEMU may".

The file-posix drivers always honor it.  These are "file",
"host_device", "host_cdrom" on a POSIX host, but not on a Windows host.
No other driver does.

The semantic change we're trying to convey is the following.  Before
commit 23dece19da4, these drivers did not make use of "(and switch
between the modes later)".  Since then, they do: they switch from
read-only to read/write only when the first writer appears, and switch
back to read-only when the last writer disappears.


The "posix" in @file-posix-dynamic-auto-read-only feels weird.  To make
sense of it, you need to know on what kind of host QEMU runs.  If we
ever implement it for Windows hosts, we'd get do add
@file-windows-posix-dynamic-auto-read-only.

What about instead adding @file-dynamic-auto-read-only with a suitable
compile-time conditional?

[...]
Peter Krempa March 29, 2019, 1:28 p.m. UTC | #8
On Fri, Mar 29, 2019 at 14:05:46 +0100, Markus Armbruster wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
> [...]
> > Whether there is a static entry in the QMP schema (which feels wrong
> > btw as it does not actually expose something which is regarding the
> > interaction with QMP) or something like this, we need it yesterday. I
> > don't want to add version checks any more.
> 
> I do understand the pressing need for a solution.  But beware of "we
> need to do something, $this is something, therefore we need to do
> $this".

Well, then that tends to result in a perpetual search for the optimal
solution while libvirt is stuck with adding version checks or false
witnesses (checking that something not entirely relevant, which was
added at the same time, is present).

So, is the issue that fixes to code which are always unconditionally
present would show up both in the new command and in the schema? In the
case of fixes this will always happen. As I've tried to say in the
first reply a solution like this feels much better than trying to stuff
it somehow into the QMP schema without an command. If a fix to the code
is advertised it does not have much to do with QMP itself, thus adding
anything to QMP schema only would feel partially odd.
Markus Armbruster March 29, 2019, 2:29 p.m. UTC | #9
Peter Krempa <pkrempa@redhat.com> writes:

> On Fri, Mar 29, 2019 at 14:05:46 +0100, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>> 
>> [...]
>> > Whether there is a static entry in the QMP schema (which feels wrong
>> > btw as it does not actually expose something which is regarding the
>> > interaction with QMP) or something like this, we need it yesterday. I
>> > don't want to add version checks any more.
>> 
>> I do understand the pressing need for a solution.  But beware of "we
>> need to do something, $this is something, therefore we need to do
>> $this".
>
> Well, then that tends to result in a perpetual search for the optimal
> solution while libvirt is stuck with adding version checks or false
> witnesses (checking that something not entirely relevant, which was
> added at the same time, is present).

"Tends to" yes.  "Does" only if we fail to pick a solution.

All I wanted to say was "beware of first past the post".  I did not mean
to say "all that matters is getting to the post prettily, not when".

> So, is the issue that fixes to code which are always unconditionally
> present would show up both in the new command and in the schema? In the
> case of fixes this will always happen. As I've tried to say in the
> first reply a solution like this feels much better than trying to stuff
> it somehow into the QMP schema without an command. If a fix to the code
> is advertised it does not have much to do with QMP itself, thus adding
> anything to QMP schema only would feel partially odd.

I'm not sure I understand.

The proposed solution puts the information in the schema.  It also makes
it available in the result of the new command.  But why bother running
the new command when you already run query-qmp-schema?
Markus Armbruster March 29, 2019, 4:11 p.m. UTC | #10
Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> auto-read-only=on changed its behaviour in file-posix for the 4.0
>>> release.
>>
>> Commit hash, please.
>
> I guess it's commit 23dece19da4 "file-posix: Make auto-read-only
> dynamic".
>
>>>          This change cannot be detected through the usual mechanisms
>>> like schema introspection. Add a new feature to query-qemu-features to
>>> allow libvirt to detect the presence of the new behaviour.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
[...]
> The "posix" in @file-posix-dynamic-auto-read-only feels weird.  To make
> sense of it, you need to know on what kind of host QEMU runs.  If we
> ever implement it for Windows hosts, we'd get do add
> @file-windows-posix-dynamic-auto-read-only.
>
> What about instead adding @file-dynamic-auto-read-only with a suitable
> compile-time conditional?
>
> [...]

Incremental patch:

diff --git a/qapi/misc.json b/qapi/misc.json
index df23c54a65..36b5ceb595 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3058,7 +3058,7 @@
 # Information about support for QEMU features that isn't available through
 # schema introspection.
 #
-# @file-posix-dynamic-auto-read-only:
+# @file-dynamic-auto-read-only:
 #   true if auto-read-only=on means that the image file is dynamically reopened
 #   read-only or read-write depending on whether any writers are attached to
 #   the node.
@@ -3066,7 +3066,9 @@
 # Since: 4.0
 ##
 { 'struct': 'QemuFeatures',
-  'data': { 'file-posix-dynamic-auto-read-only': 'bool' } }
+  'data': {
+      'file-dynamic-auto-read-only': { 'type': 'bool',
+                                       'if': 'defined(CONFIG_POSIX)' } } }
 
 ##
 # @query-qemu-features:
diff --git a/qmp.c b/qmp.c
index 2a887c1e7d..7a4e8d5935 100644
--- a/qmp.c
+++ b/qmp.c
@@ -723,7 +723,7 @@ QemuFeatures *qmp_query_qemu_features(Error **errp)
     QemuFeatures *caps = g_new(QemuFeatures, 1);
 
     *caps = (QemuFeatures) {
-        .file_posix_dynamic_auto_read_only = true,
+        .file_dynamic_auto_read_only = true,
     };
 
     return caps;
Markus Armbruster March 29, 2019, 4:19 p.m. UTC | #11
Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> auto-read-only=on changed its behaviour in file-posix for the 4.0
>>>> release.
>>>
>>> Commit hash, please.
>>
>> I guess it's commit 23dece19da4 "file-posix: Make auto-read-only
>> dynamic".
>>
>>>>          This change cannot be detected through the usual mechanisms
>>>> like schema introspection. Add a new feature to query-qemu-features to
>>>> allow libvirt to detect the presence of the new behaviour.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> [...]
>> The "posix" in @file-posix-dynamic-auto-read-only feels weird.  To make
>> sense of it, you need to know on what kind of host QEMU runs.  If we
>> ever implement it for Windows hosts, we'd get do add
>> @file-windows-posix-dynamic-auto-read-only.
>>
>> What about instead adding @file-dynamic-auto-read-only with a suitable
>> compile-time conditional?
>>
>> [...]
>
> Incremental patch:

Too fast, use this one.

diff --git a/qapi/misc.json b/qapi/misc.json
index df23c54a65..36b5ceb595 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3058,7 +3058,7 @@
 # Information about support for QEMU features that isn't available through
 # schema introspection.
 #
-# @file-posix-dynamic-auto-read-only:
+# @file-dynamic-auto-read-only:
 #   true if auto-read-only=on means that the image file is dynamically reopened
 #   read-only or read-write depending on whether any writers are attached to
 #   the node.
@@ -3066,7 +3066,9 @@
 # Since: 4.0
 ##
 { 'struct': 'QemuFeatures',
-  'data': { 'file-posix-dynamic-auto-read-only': 'bool' } }
+  'data': {
+      'file-dynamic-auto-read-only': { 'type': 'bool',
+                                       'if': 'defined(CONFIG_POSIX)' } } }
 
 ##
 # @query-qemu-features:
diff --git a/qmp.c b/qmp.c
index 2a887c1e7d..2f5a7d7a2e 100644
--- a/qmp.c
+++ b/qmp.c
@@ -723,7 +723,9 @@ QemuFeatures *qmp_query_qemu_features(Error **errp)
     QemuFeatures *caps = g_new(QemuFeatures, 1);
 
     *caps = (QemuFeatures) {
-        .file_posix_dynamic_auto_read_only = true,
+#ifdef CONFIG_POSIX
+        .file_dynamic_auto_read_only = true,
+#endif
     };
 
     return caps;
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 826b8066e1..ad14fadf54 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -110,14 +110,9 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
     }
 ''')
 
-    # 'goto out' produced for base, for each member, and if variants were
-    # present
-    if base or members or variants:
-        ret += mcgen('''
-
-out:
-''')
     ret += mcgen('''
+    goto out;                   /* suppress unused label warning */
+out:
     error_propagate(errp, err);
 }
 ''')
Kevin Wolf March 29, 2019, 4:48 p.m. UTC | #12
Am 29.03.2019 um 17:19 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> Markus Armbruster <armbru@redhat.com> writes:
> >>
> >>> Kevin Wolf <kwolf@redhat.com> writes:
> >>>
> >>>> auto-read-only=on changed its behaviour in file-posix for the 4.0
> >>>> release.
> >>>
> >>> Commit hash, please.
> >>
> >> I guess it's commit 23dece19da4 "file-posix: Make auto-read-only
> >> dynamic".
> >>
> >>>>          This change cannot be detected through the usual mechanisms
> >>>> like schema introspection. Add a new feature to query-qemu-features to
> >>>> allow libvirt to detect the presence of the new behaviour.
> >>>>
> >>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > [...]
> >> The "posix" in @file-posix-dynamic-auto-read-only feels weird.  To make
> >> sense of it, you need to know on what kind of host QEMU runs.  If we
> >> ever implement it for Windows hosts, we'd get do add
> >> @file-windows-posix-dynamic-auto-read-only.
> >>
> >> What about instead adding @file-dynamic-auto-read-only with a suitable
> >> compile-time conditional?
> >>
> >> [...]
> >
> > Incremental patch:
> 
> Too fast, use this one.
> 
> diff --git a/qapi/misc.json b/qapi/misc.json
> index df23c54a65..36b5ceb595 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -3058,7 +3058,7 @@
>  # Information about support for QEMU features that isn't available through
>  # schema introspection.
>  #
> -# @file-posix-dynamic-auto-read-only:
> +# @file-dynamic-auto-read-only:
>  #   true if auto-read-only=on means that the image file is dynamically reopened
>  #   read-only or read-write depending on whether any writers are attached to
>  #   the node.
> @@ -3066,7 +3066,9 @@
>  # Since: 4.0
>  ##
>  { 'struct': 'QemuFeatures',
> -  'data': { 'file-posix-dynamic-auto-read-only': 'bool' } }
> +  'data': {
> +      'file-dynamic-auto-read-only': { 'type': 'bool',
> +                                       'if': 'defined(CONFIG_POSIX)' } } }

I don't think there is a good reason to make the field conditional. What
should be conditional is its value:

diff --git a/qapi/misc.json b/qapi/misc.json
index df23c54a65..a460fd2acc 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3058,7 +3058,7 @@
 # Information about support for QEMU features that isn't available through
 # schema introspection.
 #
-# @file-posix-dynamic-auto-read-only:
+# @file-dynamic-auto-read-only:
 #   true if auto-read-only=on means that the image file is dynamically reopened
 #   read-only or read-write depending on whether any writers are attached to
 #   the node.
@@ -3066,7 +3066,7 @@
 # Since: 4.0
 ##
 { 'struct': 'QemuFeatures',
-  'data': { 'file-posix-dynamic-auto-read-only': 'bool' } }
+  'data': { 'file-dynamic-auto-read-only': 'bool' } }
 
 ##
 # @query-qemu-features:
diff --git a/qmp.c b/qmp.c
index 2a887c1e7d..3d3c300dc6 100644
--- a/qmp.c
+++ b/qmp.c
@@ -723,7 +723,11 @@ QemuFeatures *qmp_query_qemu_features(Error **errp)
     QemuFeatures *caps = g_new(QemuFeatures, 1);
 
     *caps = (QemuFeatures) {
-        .file_posix_dynamic_auto_read_only = true,
+#ifdef CONFIG_POSIX
+        .file_dynamic_auto_read_only = true,
+#else
+        .file_dynamic_auto_read_only = false,
+#endif
     };
 
     return caps;
Kevin Wolf March 29, 2019, 5:15 p.m. UTC | #13
Am 29.03.2019 um 14:14 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Kevin Wolf <kwolf@redhat.com> writes:
> >
> >> auto-read-only=on changed its behaviour in file-posix for the 4.0
> >> release.
> >
> > Commit hash, please.
> 
> I guess it's commit 23dece19da4 "file-posix: Make auto-read-only
> dynamic".

Yes.

> >>          This change cannot be detected through the usual mechanisms
> >> like schema introspection. Add a new feature to query-qemu-features to
> >> allow libvirt to detect the presence of the new behaviour.
> >>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> ---
> >>  qapi/misc.json | 7 ++++++-
> >>  qmp.c          | 1 +
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/qapi/misc.json b/qapi/misc.json
> >> index d892f37633..df23c54a65 100644
> >> --- a/qapi/misc.json
> >> +++ b/qapi/misc.json
> >> @@ -3058,10 +3058,15 @@
> >>  # Information about support for QEMU features that isn't available through
> >>  # schema introspection.
> >>  #
> >> +# @file-posix-dynamic-auto-read-only:
> >> +#   true if auto-read-only=on means that the image file is dynamically reopened
> >> +#   read-only or read-write depending on whether any writers are attached to
> >> +#   the node.
> >> +#
> >
> > The name @file-posix-dynamic-auto-read-only suggests the semantic change
> > of auto-read-only=on explained in the doc comment applies only to some
> > BlockDrivers (as a non-variant member of BlockdevOptions, auto-read-only
> > applies to all).  Which ones exactly?
> 
> Kevin answered this on IRC.  In my words (and possibly with my
> mistakes):
> 
> @auto-read-only is advisory, as documented:
> 
> # @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
> #                  (default: false, since 3.1)
> 
> Note the "QEMU may".
> 
> The file-posix drivers always honor it.  These are "file",
> "host_device", "host_cdrom" on a POSIX host, but not on a Windows host.

I actually noticed that host_cdrom doesn't implement the permission
system callbacks (they would be the same as for the other two drivers,
but they aren't hooked up in host_cdrom).

First I though this might mean that I broke host_cdrom, but actually
such backends should always be read-only, so it might be okay.

Does anyone have a physical CD drive available to test it?

> No other driver does.

This is actually not true.

I already mentioned on IRC that some read-only format drivers support
it. Specifically, these are:

    bochs, cloop, dmg

But I also gave you wrong information regarding protocol drivers, sorry
about that. I grepped for the wrong thing and missed that there are more
protocol drivers that support auto-read-only (with the old semantics,
fallback in .bdrv_open):

    curl, gluster, iscsi, nbd, rbd, vvfat

> The semantic change we're trying to convey is the following.  Before
> commit 23dece19da4, these drivers did not make use of "(and switch
> between the modes later)".  Since then, they do: they switch from
> read-only to read/write only when the first writer appears, and switch
> back to read-only when the last writer disappears.
> 
> 
> The "posix" in @file-posix-dynamic-auto-read-only feels weird.  To make
> sense of it, you need to know on what kind of host QEMU runs.  If we
> ever implement it for Windows hosts, we'd get do add
> @file-windows-posix-dynamic-auto-read-only.
> 
> What about instead adding @file-dynamic-auto-read-only with a suitable
> compile-time conditional?

I think a management tool ought to know what kind of host it's running
its VM on, but I'm fine with file-dynamic-auto-read-only.

Kevin

Patch
diff mbox series

diff --git a/qapi/misc.json b/qapi/misc.json
index d892f37633..df23c54a65 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3058,10 +3058,15 @@ 
 # Information about support for QEMU features that isn't available through
 # schema introspection.
 #
+# @file-posix-dynamic-auto-read-only:
+#   true if auto-read-only=on means that the image file is dynamically reopened
+#   read-only or read-write depending on whether any writers are attached to
+#   the node.
+#
 # Since: 4.0
 ##
 { 'struct': 'QemuFeatures',
-  'data': { } }
+  'data': { 'file-posix-dynamic-auto-read-only': 'bool' } }
 
 ##
 # @query-qemu-features:
diff --git a/qmp.c b/qmp.c
index 0aad533eca..2a887c1e7d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -723,6 +723,7 @@  QemuFeatures *qmp_query_qemu_features(Error **errp)
     QemuFeatures *caps = g_new(QemuFeatures, 1);
 
     *caps = (QemuFeatures) {
+        .file_posix_dynamic_auto_read_only = true,
     };
 
     return caps;