Re: [PATCH v2 3/9] backends/hostmem-file: Add "rom" property to support VM templating with R/O files

[David Hildenbrand]

On 23.08.23 16:47, ThinerLogoer wrote:
> At 2023-08-23 20:43:48, "David Hildenbrand" <david@redhat.com> wrote:
> > > > +        The ``rom`` option specifies whether to create Read Only Memory (ROM)
> > > > +        that cannot be modified by the VM. If set to ``on``, the VM cannot
> > > > +        modify the memory. If set to ``off``, the VM can modify the memory.
> > > > +        If set to ``auto`` (default), the value of the ``readonly`` property
> > > > +        is used. This option is primarily helpful for VM templating, where we
> > > > +        want to open a file readonly (``readonly=on``) and allow private
> > > > +        modifications of the memory by the VM (``share=off``, ``rom=off``).
> > > > +
> > > >       ``-object 
> > > > memory-backend-ram,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave``
> > > >           Creates a memory backend object, which can be used to back the
> > > >           guest RAM. Memory backend objects offer more control than the
> > >
> > > In one word, I'd suggest advertising the existence of "rom" option more 
> > > invasively, whenever
> > > private file mapping is used.
> > >
> > > IMHO you should probably be more invasive here to warn unconditionally when
> > > the memory backend file is going to be opened readwrite but is mapped non 
> > > shared.
> >
> > As Daniel said, we should not add new warnings for sane use cases. But we can 
> > indeed give a hint when opening the file failed, see below.
>
> Well I don't think it's completely sane use cases though we can keep it for 
> backward
> compatibility.

Well, yes, but these are sane use cases that have been using that way of 
dealing with files forever. We cannot simply change their usage, 
unfortunately.

Having that said, your important use case is currently VM templating. 
Note that that's not what the many other QEMU users actually do.

So I can understand why you want a different behavior and more hints; 
but we have to balance a bit here (after all, I'm writing documentation 
how to do VM templating for a reason ;) ).

[...]
> > > When the file is readonly, the error message is:
> > > ```
> > > qemu-system-x86_64: can't open backing store pc.ram for guest RAM: Permission 
> > > denied
> > > ```
> > >
> > > This should be probably helpful if qemu found that the file exists as a regular 
> > > file and
> > > is mapped private, to say something like
> > >
> > > ```
> > > qemu-system-x86_64: can't open backing store pc.ram for guest RAM: Permission 
> > > denied
> > > tip: mapping is private and ram file is probably readonly, maybe you should append 
> > > "readonly=on,rom=off"
> > > to "-object memory-backend-file,..." option list. see documentation xxx for 
> > > details
> > > ```
> >
> > What about the following, if we can indeed open the file R/O and we're dealing 
> > witha  private mapping:
> >
> > qemu-system-x86_64: can't open backing store tmp-file for guest RAM: Permission 
> > denied
> > Consider opening the backing store read-only using '-object 
> > memory-backend-file,readonly=on,rom=off,...' (see "VM templating" documentation)
> >
> > ?
>
> This is good. Wording might need improvement. (Are qemu error messages always 
> this cold?)

Maybe. Maybe just my writing style :P

Looking at most error_append_hint(), they are fairly neutral/cold.

--
Cheers,

David / dhildenb