Re: [PATCH 09/16] ui/pixman: generalize shared_image_destroy

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

Hi

On Sat, Oct 5, 2024 at 12:50 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Learn to free memfd-allocated shared memory.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   include/ui/qemu-pixman.h |  2 +-
> >   hw/display/virtio-gpu.c  |  4 ++--
> >   ui/console.c             |  2 +-
> >   ui/qemu-pixman.c         | 20 ++++++++++++--------
> >   4 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
> > index e3dd72b9e3..a97f56d09a 100644
> > --- a/include/ui/qemu-pixman.h
> > +++ b/include/ui/qemu-pixman.h
> > @@ -97,7 +97,7 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
> >
> >   void qemu_pixman_image_unref(pixman_image_t *image);
> >
> > -void qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data);
> > +void qemu_pixman_shared_image_destroy(pixman_image_t *image, void *data);
> >
> >   G_DEFINE_AUTOPTR_CLEANUP_FUNC(pixman_image_t, qemu_pixman_image_unref)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 017a0f170c..77f6e76f23 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -298,7 +298,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
> >               bits, c2d.height ? res->hostmem / c2d.height : 0);
> >   #ifdef WIN32
> >           if (res->image) {
> > -            pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
> > +            pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
> >           }
> >   #endif
> >       }
> > @@ -1317,7 +1317,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> >               return -EINVAL;
> >           }
> >   #ifdef WIN32
> > -        pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
> > +        pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
> >   #endif
> >
> >           res->addrs = g_new(uint64_t, res->iov_cnt);
> > diff --git a/ui/console.c b/ui/console.c
> > index 8f416ff0b9..fdd76c2be4 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -487,7 +487,7 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
> >   #ifdef WIN32
> >       qemu_displaysurface_win32_set_handle(surface, handle, 0);
> >       pixman_image_set_destroy_function(surface->image,
> > -                                      qemu_pixman_win32_image_destroy, handle);
> > +                                      qemu_pixman_shared_image_destroy, handle);
> >   #endif
> >       return surface;
> >   }
> > diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
> > index 3870e1a215..43050ab7c5 100644
> > --- a/ui/qemu-pixman.c
> > +++ b/ui/qemu-pixman.c
> > @@ -6,6 +6,7 @@
> >   #include "qemu/osdep.h"
> >   #include "qapi/error.h"
> >   #include "ui/console.h"
> > +#include "qemu/memfd.h"
> >   #include "standard-headers/drm/drm_fourcc.h"
> >   #include "trace.h"
> >
> > @@ -269,16 +270,19 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
> >   }
> >   #endif /* CONFIG_PIXMAN */
> >
> > -#ifdef WIN32
> >   void
> > -qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data)
> > +qemu_pixman_shared_image_destroy(pixman_image_t *image, void *data)
> >   {
> > +    void *ptr = pixman_image_get_data(image);
> > +
> > +#ifndef WIN32
>
> I think it is better to have #ifdef instead of #ifndef. It is a common
> pattern to have #ifdef and #elif defined() for the platform-specific
> code and follow them with #else that implements the generic code. This
> allows supporting multiple platform conditions.

indeed! thanks

>
> > +    int shmfd = GPOINTER_TO_INT(data);
> > +    size_t size = pixman_image_get_height(image) * pixman_image_get_stride(image);
> > +
> > +    qemu_memfd_free(ptr, size, shmfd);
> > +#else
> >       HANDLE handle = data;
> >
> > -    qemu_win32_map_free(
> > -        pixman_image_get_data(image),
> > -        handle,
> > -        &error_warn
> > -    );
> > -}
> > +    qemu_win32_map_free(ptr, handle, &error_warn);
> >   #endif
> > +}
>