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

[David Hildenbrand <david@redhat.com>]

On 27.07.23 17:20, ThinerLogoer wrote:
> 
> At 2023-07-27 21:18:44, "David Hildenbrand" <david@redhat.com> wrote:
>> 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.
> 
> Maybe you can notice this edge case? I am not sure whether this
> case is on your todo list?

I guess we would have to check if we opened a directory. Should be easy to add.

As long as QEMU fails reasonably well later, good for now :)

> 
>>>
>>>    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.
> 
> I feel that the retry can be applied in general - for shared mappings,
> it will merely fail on the mmap step and should be ok?

I guess a proper "can't open backing store" message is better for the cases that obviously can't work.

> 
> Though, to retry only on private mapping seems straightforwards -
> this function is called only once, and whether the mapping is private
> can be passed here with a boolean flag as argument. Nonetheless
> it may make the logic of the function more complex and less intuitive.

Quick untested attempt to move retry handling to the caller:

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 3df73542e1..c826bb78fc 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);
+        }
+    }
      if (fd < 0) {
+        error_setg_errno(errp, -fd,
+                         "can't open backing store %s for guest RAM",
+                         mem_path);
          return NULL;
      }
  
-- 
2.41.0




-- 
Cheers,

David / dhildenb