| Message ID | 20230823153412.832081-7-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory-backend-file related improvements and VM templating support | expand |
On Wed, Aug 23, 2023 at 05:34:06PM +0200, David Hildenbrand wrote: > Currently, if a file does not exist yet, file_ram_open() will create new > empty file and open it writable. However, it even does that when > readonly=true was specified. > > Specifying O_RDONLY instead to create a new readonly file would > theoretically work, however, ftruncate() will refuse to resize the new > empty file and we'll get a warning: > ftruncate: Invalid argument > And later eventually more problems when actually mmap'ing that file and > accessing it. > > If someone intends to let QEMU open+mmap a file read-only, better > create+resize+fill that file ahead of time outside of QEMU context. > > We'll now fail with: > ./qemu-system-x86_64 \ > -object memory-backend-file,id=ram0,mem-path=tmp,readonly=true,size=1g > qemu-system-x86_64: can't open backing store tmp for guest RAM: No such file or directory > > All use cases of readonly files (R/O NVDIMMs, VM templating) work on > existing files, so silently creating new files might just hide user > errors when accidentally specifying a non-existent file. > > Note that the only memory-backend-file will end up calling > memory_region_init_ram_from_file() -> qemu_ram_alloc_from_file() -> > file_ram_open(). > > Move error reporting to the single caller. > > Signed-off-by: David Hildenbrand <david@redhat.com> Acked-by: Peter Xu <peterx@redhat.com>
diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 32d641754b..f8c47ecb85 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; @@ -1305,6 +1304,10 @@ static int file_ram_open(const char *path, break; } if (errno == ENOENT) { + if (readonly) { + /* Refuse to create new, readonly files. */ + return -ENOENT; + } /* @path names a file that doesn't exist, create it */ fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0644); if (fd >= 0) { @@ -1334,10 +1337,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 @@ -1947,8 +1947,10 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, RAMBlock *block; fd = file_ram_open(mem_path, memory_region_name(mr), - !!(ram_flags & RAM_READONLY_FD), &created, errp); + !!(ram_flags & RAM_READONLY_FD), &created); if (fd < 0) { + error_setg_errno(errp, -fd, "can't open backing store %s for guest RAM", + mem_path); return NULL; }
Currently, if a file does not exist yet, file_ram_open() will create new empty file and open it writable. However, it even does that when readonly=true was specified. Specifying O_RDONLY instead to create a new readonly file would theoretically work, however, ftruncate() will refuse to resize the new empty file and we'll get a warning: ftruncate: Invalid argument And later eventually more problems when actually mmap'ing that file and accessing it. If someone intends to let QEMU open+mmap a file read-only, better create+resize+fill that file ahead of time outside of QEMU context. We'll now fail with: ./qemu-system-x86_64 \ -object memory-backend-file,id=ram0,mem-path=tmp,readonly=true,size=1g qemu-system-x86_64: can't open backing store tmp for guest RAM: No such file or directory All use cases of readonly files (R/O NVDIMMs, VM templating) work on existing files, so silently creating new files might just hide user errors when accidentally specifying a non-existent file. Note that the only memory-backend-file will end up calling memory_region_init_ram_from_file() -> qemu_ram_alloc_from_file() -> file_ram_open(). Move error reporting to the single caller. Signed-off-by: David Hildenbrand <david@redhat.com> --- softmmu/physmem.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)