Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open

[David Hildenbrand <david@redhat.com>]

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