Re: [Xen-devel] [PATCH v5 06/10] xen: introduce a generic framebuffer driver

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 15 Jan 2013, Jan Beulich wrote:
> >>> On 14.01.13 at 19:16, Stefano Stabellini 
> >>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > +int __init lfb_init(struct lfb_prop *lfbp)
> > +{
> > +    if ( lfbp->width > MAX_XRES || lfbp->height > MAX_YRES )
> > +    {
> > +        printk(XENLOG_WARNING "Couldn't initialize a %ux%u framebuffer 
> > early.\n",
> > +               lfbp->width, lfbp->height);
> > +        return -EINVAL;
> > +    }
> > +
> > +    lfb.lfbp = *lfbp;
> > +    lfb.lbuf = NULL;
> > +    lfb.text_buf = NULL;
> > +    lfb.line_len = NULL;
> > +
> > +    lfb.lbuf = xmalloc_bytes(lfb.lfbp.bytes_per_line);
> > +    if ( !lfb.lbuf )
> > +        goto fail;
> > +
> > +    lfb.text_buf = xzalloc_bytes(lfb.lfbp.text_columns * 
> > lfb.lfbp.text_rows);
> > +    if ( !lfb.text_buf )
> > +        goto fail;
> > +
> > +    lfb.line_len = xzalloc_array(unsigned int, lfb.lfbp.text_columns);
> > +    if ( !lfb.line_len )
> > +        goto fail;
> 
> While minor, this is inefficient (and needlessly growing the source
> size): The initialization to NULL above could be dropped, the allocs
> all done in a row, and the results could be checked in one go.

You are right, I'll make the changes

> > +
> > +    return 0;
> > +
> > +fail:
> > +    printk(XENLOG_ERR "Couldn't allocate enough memory to drive the 
> > framebuffer\n");
> > +    lfb_free();
> > +
> > +    return -ENOMEM;
> > +}
> 
> Irrespective of the comment above, but provided there's no hidden
> change in the code that got moved around, feel free to stick my ack
> on it.

Thanks!

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel