From: Bob Peterson <rpeterso@redhat.com> To: Abhi Das <adas@redhat.com> Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, cluster-devel@redhat.com Subject: Re: [Cluster-devel] [RFC PATCH 3/5] gfs2: Add a dynamic buffer backed by a vector of pages Date: Fri, 25 Jul 2014 14:42:49 -0400 (EDT) [thread overview] Message-ID: <742068374.13487420.1406313769169.JavaMail.zimbra@redhat.com> (raw) In-Reply-To: <1406309888-10749-4-git-send-email-adas@redhat.com> ----- 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
WARNING: multiple messages have this Message-ID (diff)
From: Bob Peterson <rpeterso@redhat.com> To: cluster-devel.redhat.com Subject: [Cluster-devel] [RFC PATCH 3/5] gfs2: Add a dynamic buffer backed by a vector of pages Date: Fri, 25 Jul 2014 14:42:49 -0400 (EDT) [thread overview] Message-ID: <742068374.13487420.1406313769169.JavaMail.zimbra@redhat.com> (raw) In-Reply-To: <1406309888-10749-4-git-send-email-adas@redhat.com> ----- 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
next prev parent reply other threads:[~2014-07-25 18:42 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-07-25 17:38 [RFC PATCH 0/5] xgetdents system call Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` [RFC PATCH 1/5] fs: xstat system call VFS bits Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` Abhi Das 2014-07-25 18:17 ` [Cluster-devel] " Bob Peterson 2014-07-25 18:17 ` Bob Peterson 2014-07-25 17:38 ` [RFC PATCH 2/5] fs: Add xgetdents system call and xreaddir file operation Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` Abhi Das 2014-07-29 8:20 ` Michael Kerrisk 2014-07-29 8:20 ` [Cluster-devel] " Michael Kerrisk 2014-07-29 8:20 ` Michael Kerrisk 2014-07-25 17:38 ` [RFC PATCH 3/5] gfs2: Add a dynamic buffer backed by a vector of pages Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` Abhi Das 2014-07-25 18:42 ` Bob Peterson [this message] 2014-07-25 18:42 ` [Cluster-devel] " Bob Peterson 2014-07-25 17:38 ` [RFC PATCH 4/5] gfs2: Add sort functionality with extra parameter Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-29 18:58 ` Jonathan Corbet 2014-07-29 18:58 ` [Cluster-devel] " Jonathan Corbet 2014-07-29 22:25 ` Abhijith Das 2014-07-29 22:25 ` [Cluster-devel] " Abhijith Das 2014-07-30 9:06 ` Steven Whitehouse 2014-07-30 13:57 ` Jonathan Corbet 2014-07-30 13:57 ` [Cluster-devel] " Jonathan Corbet 2014-07-29 8:18 ` [RFC PATCH 0/5] xgetdents system call Michael Kerrisk 2014-07-29 8:18 ` [Cluster-devel] " Michael Kerrisk 2014-07-29 8:18 ` Michael Kerrisk
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=742068374.13487420.1406313769169.JavaMail.zimbra@redhat.com \ --to=rpeterso@redhat.com \ --cc=adas@redhat.com \ --cc=cluster-devel@redhat.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.