Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open

[David Hildenbrand <david@redhat.com>]

On 26.07.23 16:59, Thiner Logoer wrote:
> Users may give "-mem-path" a read only file and expect the file
> to be mapped read-write privately. Allow this but give a warning
> since other users may surprise when the ram file is readonly and
> qemu suddenly aborts elsewhere.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Thiner Logoer <logoerthiner1@163.com>
> ---
> 
> See the previous version at:
> https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432ca4@redhat.com/T/
> 
> verified, this patch works for my setup, both functionality and the warning
> are expected behavior.
> 
> Also another problem when I look at the file_ram_open
> 
> When readonly is true and the path is a directory, the open will succeed but
> any later operations will fail since it is a directory fd. This may require
> additional commits which is out of my scope. Merely record the question here.
> 
>   softmmu/physmem.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 3df73542e1..e8279d69d4 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1296,6 +1296,7 @@ static int file_ram_open(const char *path,
>       char *sanitized_name;
>       char *c;
>       int fd = -1;
> +    bool first_trial = true;
>   
>       *created = false;
>       for (;;) {
> @@ -1332,6 +1333,18 @@ static int file_ram_open(const char *path,
>                   break;
>               }
>               g_free(filename);
> +        } else if (first_trial && !readonly && errno == EACCES) {

I guess it's better to only retry on private mappings, for shared 
mappings that cannot possibly work.

> +            /* @path may be a read only file */
> +            fd = open(path, O_RDONLY);
> +            if (fd >= 0) {
> +                /*
> +                 * Sometimes this behavior is not desired. Fire a warning but
> +                 * continue.
> +                 */
> +                warn_report("backing store %s is opened readonly because the"
> +                            "file is not writable", path);


Makes sense, this used to not work in the past, now it works with a 
warning. Now it will work in many cases (except when ftruncate/fallocate 
would be required, and at least most fallocate callers can handle errors 
gracefully).


-- 
Cheers,

David / dhildenb