Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[David Hildenbrand <david@redhat.com>]

On 17.08.23 15:46, Daniel P. Berrangé wrote:
> On Fri, Aug 11, 2023 at 09:00:54PM +0200, David Hildenbrand wrote:
>> On 11.08.23 07:49, ThinerLogoer wrote:
>>> At 2023-08-11 05:24:43, "Peter Xu" <peterx@redhat.com> wrote:
>>>> On Fri, Aug 11, 2023 at 01:06:12AM +0800, ThinerLogoer wrote:
>>>>>> I think we have the following options (there might be more)
>>>>>>
>>>>>> 1) This patch.
>>>>>>
>>>>>> 2) New flag for memory-backend-file. We already have "readonly" and
>>>>>> "share=". I'm having a hard time coming up with a good name that really
>>>>>> describes the subtle difference.
>>>>>>
>>>>>> 3) Glue behavior to the QEMU machine
>>>>>>
>>>>>
>>>>> 4) '-deny-private-discard' argv, or environment variable, or both
>>>>
>>>> I'd personally vote for (2).  How about "fdperm"?  To describe when we want
>>>> to use different rw permissions on the file (besides the access permission
>>>> of the memory we already provided with "readonly"=XXX).  IIUC the only sane
>>>> value will be ro/rw/default, where "default" should just use the same rw
>>>> permission as the memory ("readonly"=XXX).
>>>>
>>>> Would that be relatively clean and also work in this use case?
>>>>
>>>> (the other thing I'd wish we don't have that fallback is, as long as we
>>>> have any of that "fallback" we'll need to be compatible with it since
>>>> then, and for ever...)
>>>
>>> If it must be (2), I would vote (2) + (4), with (4) adjust the default behavior of said `fdperm`.
>>> Mainly because (private+discard) is itself not a good practice and (4) serves
>>> as a good tool to help catch existing (private+discard) problems.
>>
>> Instead of fdperm, maybe we could find a better name.
>>
>> The man page of "open" says: The argument flags must include one of the
>> following access modes: O_RDONLY, O_WRONLY, or O_RDWR.  These request
>> opening the file read-only, write-only, or read/write, respectively.
>>
>> So maybe something a bit more mouthful like "file-access-mode" would be
>> better.
> 
> I don't think we should directly express the config in terms
> of file-access-mode, as that's a low level impl detail. The
> required file access mode is an artifact of the higher level
> goal, or whether the RAM should be process private vs shared,
> and whether we want QEMU to be able to create the backing
> file or use pre-create one.

See my other mails "readonly" already expresses exactly that. So no need 
for "file-access-mode".

(and as far as I can see, no need for any other flags)

-- 
Cheers,

David / dhildenb