On 28.07.23 07:46, ThinerLogoer wrote:
Sorry my mail agent just have a bug
No worries :)
At 2023-07-28 02:30:09, "David Hildenbrand" <da...@redhat.com> wrote:
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,
For some reason this prereq part of patch has one additional space,
which causes my `patch` to reject the patch. I have to manually
fix it to test later.
Yes, to be expected. Pasting a "git show" diff always messes up
whitespace for me. It was only meant as a POC.
- 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) {
"readonly" should be "!readonly" here? The readonly variable in this function is
about whether the mapping is readonly. In our case the mapping is private
writable, so readonly will be false.
Yes, indeed!
After manually fix this up, this patch also works in my environment, both
functionality and the warning.
Maybe you can directly format the patch and start a new thread there?
Whatever you prefer! If I resend the patch, I would keep you the author
and only add my Co-authored-by: Signed-off-by:.
Just let me know.
--
Cheers,
David / dhildenb
