Jason Ekstrand <[email protected]> writes:

>  static PixmapPtr
> +drmmode_create_pixmap_header(ScreenPtr pScreen, int width, int height,
> +                             int depth, int bitsPerPixel, int devKind,
> +                             void *pPixData)
> +{
> +    PixmapPtr pixmap;
> +
> +    /* width and height of 0 means don't allocate any pixmap data */
> +    pixmap = (*pScreen->CreatePixmap)(pScreen, 0, 0, depth, 0);
> +
> +    if (pixmap) {
> +        if ((*pScreen->ModifyPixmapHeader)(pixmap, width, height, depth,
> +                                           bitsPerPixel, devKind, pPixData))
> +            return pixmap;
> +        (*pScreen->DestroyPixmap)(pixmap);
> +    }
> +    return NullPixmap;
> +}

This one looks correct, but seems like it's structured backwards from
the usual code when checking for errors. I think this is easier to
follow:

    /* width and height of 0 means don't allocate any pixmap data */
    pixmap = (*pScreen->CreatePixmap)(pScreen, 0, 0, depth, 0);
    if (!pixmap)
        return NullPixmap;

    if (!(*pScreen->ModifyPixmapHeader)(pixmap, width, height, depth,
                                        bitsPerPixel, devKind,
                                        pPixData)) {
        (*pScreen->DestroyPixmap)(pixmap);
        return NullPixmap;
    }
    return pixmap;

> +    if (shadow_pixmap == NULL) {
> +        xf86DrvMsg(scrn->scrnIndex, X_ERROR,
> +                   "Couldn't allocate shadow pixmap for rotated CRTC\n");
> +        return NULL;

This error path will need to free the shadow data, if that was allocated
as a part of this function.

The rest of this patch has been

Reviewed-by: Keith Packard <[email protected]>

-- 
-keith

Attachment: signature.asc
Description: PGP signature

_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to