Sorry my mail agent just have a bug
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.
>- 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
