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,
>- 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
