Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] Add file-posix-dynamic-auto-read-only feature

[Kevin Wolf]

Am 29.03.2019 um 14:14 hat Markus Armbruster geschrieben:
> Markus Armbruster <address@hidden> writes:
> 
> > Kevin Wolf <address@hidden> 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 <address@hidden>
> >> ---
> >>  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