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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
> >  qemu-options.hx         |  5 ++++-
> >  2 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > index 37c70acfe2..6bd5bf9b91 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
> >      uint64_t align;
> >      bool discard_data;
> >      bool is_pmem;
> > +    bool readonly;
> >  };
> >  
> >  static void
> > @@ -57,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >                                       backend->size, fb->align,
> >                                       (backend->share ? RAM_SHARED : 0) |
> >                                       (fb->is_pmem ? RAM_PMEM : 0),
> > -                                     fb->mem_path, false, errp);
> > +                                     fb->mem_path, fb->readonly, errp);
> >      g_free(name);
> >  #endif
> >  }
> > @@ -152,6 +153,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> >      fb->is_pmem = value;
> >  }
> >  
> > +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).

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