On Mon, Aug 07, 2023 at 09:07:32PM +0200, David Hildenbrand wrote:
> From: Thiner Logoer <logoerthin...@163.com>
>
> Users may specify
> * "-mem-path" or
> * "-object memory-backend-file,share=off,readonly=off"
> and expect such COW (MAP_PRIVATE) mappings to work, even if the user
> does not have write permissions to open the file.
>
> For now, we would always fail in that case, always requiring file write
> permissions. Let's detect when that failure happens and fallback to opening
> the file readonly.
>
> Warn the user, since there are other use cases where we want the file to
> be mapped writable: ftruncate() and fallocate() will fail if the file
> was not opened with write permissions.
>
> Signed-off-by: Thiner Logoer <logoerthin...@163.com>
> Co-developed-by: David Hildenbrand <da...@redhat.com>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
> ---
> softmmu/physmem.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 3df73542e1..d1ae694b20 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd)
> static int file_ram_open(const char *path,
> const char *region_name,
> bool readonly,
> - bool *created,
> - Error **errp)
> + bool *created)
> {
> char *filename;
> char *sanitized_name;
> @@ -1334,10 +1333,7 @@ static int file_ram_open(const char *path,
> g_free(filename);
> }
> if (errno != EEXIST && errno != EINTR) {
> - error_setg_errno(errp, errno,
> - "can't open backing store %s for guest RAM",
> - path);
> - return -1;
> + return -errno;
> }
> /*
> * Try again on EINTR and EEXIST. The latter happens when
> @@ -1946,9 +1942,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size,
> MemoryRegion *mr,
> bool created;
> RAMBlock *block;
>
> - fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
> - errp);
> + fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created);
> + if (fd == -EACCES && !(ram_flags & RAM_SHARED) && !readonly) {
> + /*
> + * We can have a writable MAP_PRIVATE mapping of a readonly file.
> + * However, some operations like ftruncate() or fallocate() might
> fail
> + * later, let's warn the user.
> + */
> + fd = file_ram_open(mem_path, memory_region_name(mr), true, &created);
> + if (fd >= 0) {
> + warn_report("backing store %s for guest RAM (MAP_PRIVATE) opened"
> + " readonly because the file is not writable",
> mem_path);
IIUC, from the description, the goal is that usage of a readonly
backing store is intented to be an explicitly supported deployment
configuration. At the time time though, this scenario could also be
a deployment mistake that we want to diagnose
It is inappropriate to issue warn_report() for things that are
supported usage.
It is also undesirable to continue execution in the case of things
which are a deployment mistake.
These two scenarios are mutually incompatible, so I understand why
you choose to fudge it with a warn_report().
I wonder if this is pointing to the need for another configuration
knob for the memory backend, to express the different desired usage
models.
We want O_WRONLY when opening the file, either if we want to file
shared, or so that we can ftruncate it to the right size, if it
does not exist. If shared=off and the file is pre-created at the
right size, we should be able to use O_RDONLY even if the file is
writable.
So what if we added a 'create=yes|no' option to memory-backend-file
-object memory-backend-file,share=off,readonly=off,create=yes
would imply need for O_WRONLY|O_RDONLY, so that ftruncate() can
do its work.
With share=off,create=no, we could unconditionally open O_RDONLY,
even if the file is writable.
This would let us support read-only backing files, without any
warn_reports() for this usage, while also stopping execution
with deployment mistakes
This doesn't help -mem-path, since it doesn't take options, but
IMHO it would be acceptable to say users need to use the more
verbose '-object memory-backend-file' instead.
> + }
> + }
> if (fd < 0) {
> + error_setg_errno(errp, -fd,
> + "can't open backing store %s for guest RAM",
> + mem_path);
> return NULL;
> }
>
> --
> 2.41.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
