[Spice-devel] [PATCH] Simplify memory management for the primary screen.

[Alon Levy]

> Passing NULL for pbits into fbScreenInit is a treacherous
> thing to do.  It works, because we rapidly resize
> the screen, and because the normal path never uses
> the screen pixmap.
> 
> However, having NULL for pbits has side effects that derive
> from the fact that NULL prevents ModifyPixmapHeader
> from doing anything, which leaves the internal Screen Pixmap
> pointer pointing to the end of an allocation, where
> careless use can corrupt memory willy nilly.
> 
> We didn't really have a compelling reason to do this;
> the qxl->fb memory can be dedicated to just screen use,
> and the host_image can manage it's own memory.
> ---
>  src/qxl_driver.c  |    2 +-
>  src/qxl_surface.c |    8 +-------
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> index dbb8ceb..dbb66b3 100644
> --- a/src/qxl_driver.c
> +++ b/src/qxl_driver.c
> @@ -1717,7 +1717,7 @@ qxl_fb_init (qxl_screen_t *qxl, ScreenPtr
> pScreen)
>      ErrorF ("allocated %d x %d  %p\n", pScrn->virtualX,
>      pScrn->virtualY, qxl->fb);
>  #endif
>      
> -    if (!fbScreenInit (pScreen, NULL,
> +    if (!fbScreenInit (pScreen, qxl->fb,
>                         pScrn->virtualX, pScrn->virtualY,
>                         pScrn->xDpi, pScrn->yDpi,
>                         pScrn->displayWidth,
>                         pScrn->bitsPerPixel))
> diff --git a/src/qxl_surface.c b/src/qxl_surface.c
> index e88675f..76ec7d1 100644
> --- a/src/qxl_surface.c
> +++ b/src/qxl_surface.c
> @@ -404,15 +404,9 @@ qxl_surface_cache_create_primary
> (surface_cache_t	*cache,
>      dev_image = pixman_image_create_bits (format, mode->x_res,
>      mode->y_res,
>  					  (uint32_t *)dev_addr, -mode->stride);
>  
> -    if (qxl->fb != NULL)
> -        free(qxl->fb);
> -    qxl->fb = calloc (qxl->virtual_x * qxl->virtual_y, 4);
> -    if (!qxl->fb)
> -        return NULL;
> -
>      host_image = pixman_image_create_bits (format,
>  					   qxl->virtual_x, qxl->virtual_y,
> -					   qxl->fb, mode->stride);
> +					   NULL, mode->stride);

So we allocate another framebuffer here. Hmm, it is just another 1-16 MB of memory ;)

Do you have an example how to produce corruption?

Can you reuse the same buffer and still pass non NULL? I think it might not be trivial though, so in that case I'm ok with this since I don't see any other option.

>  #if 0
>      xf86DrvMsg(cache->qxl->pScrn->scrnIndex, X_ERROR,
>                 "testing dev_image memory (%d x %d)\n",
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>