----- Original Message -----
> +static int vp_alloc_pages(struct vp_ctx *vpx, int start, int end)
> +{
> + int i;
> +
> + for (i = start; i < end; i++) {
> + vpx->vp_pages[i] = alloc_page(GFP_KERNEL | GFP_NOFS);
> + if (vpx->vp_pages[i] == NULL)
> + goto free;
> + }
> + return 0;
> +free:
> + for (i = start; i < end; i++)
> + if (vpx->vp_pages[i]) {
> + __free_page(vpx->vp_pages[i]);
> + vpx->vp_pages[i] = NULL;
> + }
> + return -ENOMEM;
> +}
I'm concerned that if we have a value for vpx->vp_pages[i] that is
nonzero (uninitialized) _after_ an element in which we couldn't
allocate a page, we could try to free the nonzero value. For that
reason, I always prefer to do something like this instead:
free:
while (i > start) {
i--;
__free_page(vpx->vp_pages[i]);
vpx->vp_pages[i] = NULL;
}
return -ENOMEM;
(snip)
> +struct vp_ctx* vp_get_vpx(struct vbuf *vb)
> +{
> + struct vp_ctx *vpx = NULL;
> +
> + if (!vb || !vb->v_opaque)
> + goto out;
> +
> + vpx = vb->v_opaque;
> + if (vpx->vp_magic != VP_MAGIC) {
> + vpx = NULL;
> + goto out;
This goto is unnecessary, and if you eliminate it, you don't need the brackets.
> + }
> +out:
> + return vpx;
> +}
Regards,
Bob Peterson
Red Hat File Systems
