Re: [Qemu-devel] [PATCH v2 2/8] block: Add auto-read-only option

[Eric Blake <eblake@redhat.com>]

On 10/15/18 4:37 AM, Kevin Wolf wrote:
> Am 12.10.2018 um 18:47 hat Eric Blake geschrieben:
>> On 10/12/18 6:55 AM, Kevin Wolf wrote:
>>> If a management application builds the block graph node by node, the
>>> protocol layer doesn't inherit its read-only option from the format
>>> layer any more, so it must be set explicitly.
>>>
>>

>>
>> Bike-shedding: Do we really want to ignore @read-only? Here's the table of 9
>> combinations ('t'rue, 'f'alse, 'o'mitted), with '*' on the rows that must be
>> preserved for back-compat:
>>
>> RO   Auto   effect
>> o    o      *open for write, fail if not possible
>> f    o      *open for write, fail if not possible
>> t    o      *open for read, no conversion to write
>> o    f      open for write, fail if not possible
>> f    f      open for write, fail if not possible
>> t    f      open for read, no conversion to write
>> o    t      attempt write but graceful fall back to read
>> f    t      attempt write but graceful fall back to read
>> t    t      ignore RO flag, attempt write anyway
>>
>> That last row is weird, why not make it an explicit error instead of
>> ignoring the implied difference in semantics between the two?
> 
> You're right that the description allows this. In practice,
> auto-read-only can only make a node go from rw to ro, not the other way
> round.
> 
> So our options are to document the current behaviour (auto-read-only has
> no effect when the image is already read-only) or to make it an error.

Ah, that's different. I was reading it as "auto-read-only true lets you 
write if possible, overriding an explicit readonly request", while you 
are reading it as "auto-read-only true allows graceful fallback to 
read-only, and is thus a no-op if you already requested readonly"

I like yours better, so it's just a matter of coming up with the correct 
documentation wording.

> 
> One thought I had is that for convenience options like -hda (or in fact
> -drive), auto-read-only=on could be the default, and only -blockdev and
> blockdev-add would disable it by default. That would suggest that we
> don't want to make it an error.

Yes, having convenience options set auto-read-only would not be too 
terrible (since those are already magic and designed for short-hand 
human use), as long as the low-level QMP commands don't add the magic 
(explicit control is better at the low levels).

> 
>> Or, another idea: is it worth trying to support a single tri-state member
>> (via an alternative between bool and enum, since the existing code uses a
>> JSON bool):
>>
>> "read-only": false (open for write, fail if not possible)
>> "read-only": true (open read-only, no later switching)
>> "read-only": "auto" (switch as needed; or for initial implementation attempt
>> for write with graceful fallback to read)
>> omitting read-only: same as "read-only":false for back-compat
> 
> If read-only were new, I would probably make it an enum, but adding it
> now isn't very practical. I did actually start with an alternate and it
> just wasn't very nice. One thing I remember is places that directly
> accessed the options QDict, for which you could now have either a bool, a
> string, an int or not present. It becomes a bit too much.

Fair enough. Maybe it's worth a commit message note that we at least 
considered and rejected alternate implementations.

> 
> As read-only is optional, we could make it true/false/absent without
> introducing an alternate and the additional int/string options, but I
> don't like that very much either.

No, that way is not introspectible.  Adding auto-read-only is much 
friendlier.

> 
> 
> While we're talking about the schema, another thing I considered was
> making auto-read-only an option only for the specific drivers that
> support it so introspection could tell the management tool whether the
> functionality is available. However, if we do this, we can't parse it in
> block.c code and use a flag any more, but need to parse it in each
> driver individually. Maybe it would be a better design anyway?

Which drivers do you have in mind? Ones like file-posix, gluster, and 
NBD that actually have a notion of opening either read-write or 
read-only, or others that are read-only no matter what?

I'm still not convinced that a per-driver option is smart, and am 
reasonably happy with you adding it globally.

> 
>>> @@ -1328,6 +1338,11 @@ QemuOptsList bdrv_runtime_opts = {
>>>                .type = QEMU_OPT_BOOL,
>>>                .help = "Node is opened in read-only mode",
>>>            },
>>> +        {
>>> +            .name = BDRV_OPT_AUTO_READ_ONLY,
>>> +            .type = QEMU_OPT_BOOL,
>>> +            .help = "Node can become read-only if opening read-write fails",
>>> +        },
>>
>> If we keep your current approach, is it worth mentioning that
>> auto-read-only true overrides read-only true?
> 
> This help text is never printed anywhere anyway... Maybe we should just
> delete it. What we refer to is the QAPI documentation anyway.

Are you sure it never gets printed, with some of the recent patches 
around trying to improve help output?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org