[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/3] hostmem-file: add readonly=on|off option
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 2/3] hostmem-file: add readonly=on|off option |
Date: |
Wed, 16 Sep 2020 10:31:33 +0100 |
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).
signature.asc
Description: PGP signature
- Re: [PATCH 2/3] hostmem-file: add readonly=on|off option,
Stefan Hajnoczi <=