On 27.07.23 17:20, ThinerLogoer wrote:
At 2023-07-27 21:18:44, "David Hildenbrand" <da...@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 <da...@redhat.com>
Signed-off-by: Thiner Logoer <logoerthin...@163.com>
---
See the previous version at:
https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432...@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
