Re: [PATCH v4 07/11] softmmu/physmem: Never return directories from file_ram_open()

[Mario Casquero]

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
>
>