| Message ID | 20230906120503.359863-8-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-backend-file related improvements and VM templating support | expand |
This series has been successfully tested by QE. Specify a directory for the mem-path of a memory-backend-file object. Check the error message has been improved without referring to a device, which can lead to confusion. Tested-by: Mario Casquero <mcasquer@redhat.com> On Wed, Sep 6, 2023 at 2:07 PM David Hildenbrand <david@redhat.com> wrote: > > open() does not fail on directories when opening them readonly (O_RDONLY). > > Currently, we succeed opening such directories and fail later during > mmap(), resulting in a misleading error message. > > $ ./qemu-system-x86_64 \ > -object memory-backend-file,id=ram0,mem-path=tmp,readonly=true,size=1g > qemu-system-x86_64: unable to map backing store for guest RAM: No such device > > To identify directories and handle them accordingly in file_ram_open() > also when readonly=true was specified, detect if we just opened a directory > using fstat() instead. Then, fail file_ram_open() right away, similarly > to how we now fail if the file does not exist and we want to open the > file readonly. > > With this change, we get a nicer error message: > qemu-system-x86_64: can't open backing store tmp for guest RAM: Is a directory > > 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(). > > Reported-by: Thiner Logoer <logoerthiner1@163.com> > Reviewed-by: Peter Xu <peterx@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > softmmu/physmem.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index 138402b6cf..f1cd3ec28a 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -1299,6 +1299,25 @@ static int file_ram_open(const char *path, > for (;;) { > fd = open(path, readonly ? O_RDONLY : O_RDWR); > if (fd >= 0) { > + /* > + * open(O_RDONLY) won't fail with EISDIR. Check manually if we > + * opened a directory and fail similarly to how we fail ENOENT > + * in readonly mode. Note that mkstemp() would imply O_RDWR. > + */ > + if (readonly) { > + struct stat file_stat; > + > + if (fstat(fd, &file_stat)) { > + close(fd); > + if (errno == EINTR) { > + continue; > + } > + return -errno; > + } else if (S_ISDIR(file_stat.st_mode)) { > + close(fd); > + return -EISDIR; > + } > + } > /* @path names an existing file, use it */ > break; > } > -- > 2.41.0 > >
On 07.09.23 12:31, Mario Casquero wrote:
> Tested-by: Mario Casquero<mcasquer@redhat.com>
Thanks Mario!
diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 138402b6cf..f1cd3ec28a 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -1299,6 +1299,25 @@ static int file_ram_open(const char *path, for (;;) { fd = open(path, readonly ? O_RDONLY : O_RDWR); if (fd >= 0) { + /* + * open(O_RDONLY) won't fail with EISDIR. Check manually if we + * opened a directory and fail similarly to how we fail ENOENT + * in readonly mode. Note that mkstemp() would imply O_RDWR. + */ + if (readonly) { + struct stat file_stat; + + if (fstat(fd, &file_stat)) { + close(fd); + if (errno == EINTR) { + continue; + } + return -errno; + } else if (S_ISDIR(file_stat.st_mode)) { + close(fd); + return -EISDIR; + } + } /* @path names an existing file, use it */ break; }
open() does not fail on directories when opening them readonly (O_RDONLY). Currently, we succeed opening such directories and fail later during mmap(), resulting in a misleading error message. $ ./qemu-system-x86_64 \ -object memory-backend-file,id=ram0,mem-path=tmp,readonly=true,size=1g qemu-system-x86_64: unable to map backing store for guest RAM: No such device To identify directories and handle them accordingly in file_ram_open() also when readonly=true was specified, detect if we just opened a directory using fstat() instead. Then, fail file_ram_open() right away, similarly to how we now fail if the file does not exist and we want to open the file readonly. With this change, we get a nicer error message: qemu-system-x86_64: can't open backing store tmp for guest RAM: Is a directory 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(). Reported-by: Thiner Logoer <logoerthiner1@163.com> Reviewed-by: Peter Xu <peterx@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- softmmu/physmem.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)