Re: [PATCH 2/3] hostmem-file: add readonly=on|off option

[Philippe Mathieu-Daudé]

On 9/16/20 11:31 AM, Stefan Hajnoczi wrote:
> On Fri, Aug 21, 2020 at 02:50:42PM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
>>> Let -object memory-backend-file work on read-only files when the
>>> readonly=on option is given. This can be used to share the contents of a
>>> file between multiple guests while preventing them from consuming
>>> Copy-on-Write memory if guests dirty the pages, for example.
>>>
[...]

>>> +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
>>> +{
>>> +    return MEMORY_BACKEND_FILE(o)->readonly;
>>> +}
>>> +
>>> +static void file_memory_backend_set_readonly(Object *o, bool value,
>>> +                                             Error **errp)
>>> +{
>>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>>> +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
>>> +
>>> +    if (host_memory_backend_mr_inited(backend)) {
>>> +        error_setg(errp, "cannot change property 'readonly' of %s.",
>>> +                   object_get_typename(o));
>>
>>
>> The 'host_memory_backend_mr_inited()' function is not documented;
>> my understanding is a backend is considered initialized once it has
>> a MemoryRegion assigned to it.
>>
>> So this error message is not very helpful, it doesn't explain the
>> reason. I see all other setters in this file use the same error,
>> so it is almost a predating issue.
>>
>> Still I'd rather use a different message, something like:
>> "'%s' already initialized, can not set it 'readonly'".
>>
>> Preferably with the error message reworded:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> I haven't reworded the error message because it's used in
> hostmem-file.c, hostmem-memfd.c, and hostmem.c. A separate patch would
> need to change the error messages across these files.
> 
> There is no time when users can actually change these QOM properties, so
> "cannot change FOO" is a reasonable wording form the user perspective.
> Telling the user that there is a pre-initialization state when the
> property can be change isn't useful because they cannot observe that
> state (the object is created and ->complete is called in a single step).

OK, understood.