Re: [PATCH for-9.1 v2 09/11] hostmem: add a new memory backend based on POSIX shm_open()

[Stefano Garzarella]

On Wed, Mar 27, 2024 at 12:51:51PM +0100, David Hildenbrand wrote:
> On 27.03.24 11:23, Stefano Garzarella wrote:
> > On Tue, Mar 26, 2024 at 03:45:52PM +0100, David Hildenbrand wrote:
> > > > +    mode = 0;
> > > > +    oflag = O_RDWR | O_CREAT | O_EXCL;
> > > > +    backend_name = host_memory_backend_get_name(backend);
> > > > +
> > > > +    /*
> > > > +     * Some operating systems allow creating anonymous POSIX shared memory
> > > > +     * objects (e.g. FreeBSD provides the SHM_ANON constant), but this is not
> > > > +     * defined by POSIX, so let's create a unique name.
> > > > +     *
> > > > +     * From Linux's shm_open(3) man-page:
> > > > +     *   For  portable  use,  a shared  memory  object should be identified
> > > > +     *   by a name of the form /somename;"
> > > > +     */
> > > > +    g_string_printf(shm_name, "/qemu-" FMT_pid "-shm-%s", getpid(),
> > > > +                    backend_name);
> > >
> > > Hm, shouldn't we just let the user specify a name, and if no name was
> > > specified, generate one ourselves?
> >
> > I thought about it and initially did it that way, but then some problems
> > came up so I tried to keep it as simple as possible for the user and for
> > our use case (having an fd associated with memory and sharing it with
> > other processes).
> >
> > The problems I had were:
> >
> > - what mode_t to use if the object does not exist and needs to be
> >    created? >
> > - exclude O_EXCL if the user passes the name since they may have already
> >    created it?
>
> I'd handle both like we handle files. But I understand now that you 
> really just want to "simulate" memfd.

Right, maybe I should write that in the commit message and 
documentation.

> > - call shm_unlink() only at object deallocation?
>
> Right, we don't really do that for ordinary files, they keep existing. 
> We only have the "discard-data" property to free up memory.
>
> For memfd, it just happens "automatically". They cannot be looked up 
> by name, and once the last user closes the fd, it gets destroyed.

Yep, I see.

> > So I thought that for now we only support the "anonymous" mode, and if
> > in the future we have a use case where the user wants to specify the
> > name, we can add it later.
>
> Okay, so for now you really only want an anonymous fd that behaves like 
> a memfd, got it.
>
> Likely we should somehow fail if the "POSIX shared memory object" 
> already exists. Hmm.

`O_CREAT | O_EXCL` should provide just this behavior.
From shm_open(3) manpage:

    O_EXCL  If O_CREAT was also specified, and a shared memory object
            with the given name already exists, return an error.  The
            check for the existence of the object, and its creation if
            it does not exist, are performed atomically.

> > That said, if you think it's already useful from the beginning, I can
> > add the name as an optional parameter.
> >
> > > I'm also not quite sure if "host_memory_backend_get_name()" should be
> > > used for the purpose here.
> >
> > What problem do you see? As an alternative I thought of a static
> > counter.
>
> If it's really just an "internal UUID", as we'll remove the file using 
> shm_unlink(), then the name in fact shouldn't matter and this would be 
> fine. Correct?

Right. It's a name that will only "appear" in the system for a very 
short time since we call shm_unlink() right after shm_open(). So I just 
need the unique name to prevent several QEMUs launched at the same time 
from colliding with the name.

> So I assume if we ever want to have non-anonymous fds here, we'd pass 
> in a new property "name", and change the logic how we open/unlink.  
> Makes sense.

Exactly! I can clarify this in the commit description as well.

Thanks for the helpful comments!
If there is anything I need to change for v3, please tell me ;-)

Stefano