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

[David Hildenbrand]

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.

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

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

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


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.

--
Cheers,

David / dhildenb