softmmu/physmem.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
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) {
+ /* @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);
+ break;
+ }
}
if (errno != EEXIST && errno != EINTR) {
error_setg_errno(errp, errno,
@@ -1343,6 +1356,7 @@ static int file_ram_open(const char *path,
* Try again on EINTR and EEXIST. The latter happens when
* something else creates the file between our two open().
*/
+ first_trial = false;
}
return fd;
--
2.40.1On 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
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?
>>
>> 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?
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.
---
Regards,
logoerthiner
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
Sorry my mail agent just have a bug
At 2023-07-28 02:30:09, "David Hildenbrand" <david@redhat.com> wrote:
>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,
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.
>- 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.
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?
>+ /*
>+ * 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;
> }
>
---
Regards,
logoerthiner
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" <david@redhat.com> wrote:
>> 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,
>
> 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
At 2023-07-28 18:45:20, "David Hildenbrand" <david@redhat.com> wrote: > > >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. > Hello, I wonder whether you have planned to resubmit the current patch anytime soon, or is it already inside the patch queue? --- Regards, logoerthiner
On 03.08.23 17:43, ThinerLogoer wrote:
> At 2023-07-28 18:45:20, "David Hildenbrand" <david@redhat.com> wrote:
>>
>>
>> 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.
>>
>
> Hello,
>
> I wonder whether you have planned to resubmit the current patch anytime soon, or is it already
> inside the patch queue?
I'm currently testing the following change on top, not compile-tested under
Windows, though.
From b1abec2fe024ea90860ecf600c381e4a25e22ed8 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Thu, 3 Aug 2023 19:14:34 +0200
Subject: [PATCH] softmmu/physmem: Always detect and handle directories in
file_ram_open()
open() does not fail on directories when opening readonly -- O_RDONLY.
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.
Before this change:
$ ./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
With this change, it works as expected: we create a temporary hidden
file in that directory, just like when specifying readonly=false.
Reported-by: Thiner Logoer <logoerthiner1@163.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
softmmu/physmem.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index d1ae694b20..32b51fd54d 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1300,9 +1300,29 @@ static int file_ram_open(const char *path,
for (;;) {
fd = open(path, readonly ? O_RDONLY : O_RDWR);
if (fd >= 0) {
- /* @path names an existing file, use it */
- break;
+ /*
+ * open() won't fail when passing O_RDONLY on directories. So
+ * check manually if we're given a directory, and convert to
+ * EISDIR.
+ */
+ if (readonly) {
+ struct stat file_stat;
+
+ if (fstat(fd, &file_stat)) {
+ close(fd);
+ return -errno;
+ } else if (S_ISDIR(file_stat.st_mode)) {
+ close(fd);
+ fd = -1;
+ errno = EISDIR;
+ }
+ }
+ if (fd >= 0) {
+ /* @path names an existing file, use it */
+ break;
+ }
}
+
if (errno == ENOENT) {
/* @path names a file that doesn't exist, create it */
fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0644);
--
2.41.0
--
Cheers,
David / dhildenb
At 2023-07-28 18:45:20, "David Hildenbrand" <david@redhat.com> wrote:
>>> 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.
Everything is good and clear now. I think it is better that you do the patch
here. Waiting for the final version of patch.
>
>--
>Cheers,
>
>David / dhildenb
At 2023-07-28 02:30:09, "David Hildenbrand" <david@redhat.com> wrote:
>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
© 2016 - 2024 Red Hat, Inc.