2009-06-06 10:51:58

by Figo.zhang

Subject: [PATCH] no need for checking it

vfree() does it's own NULL checking,so no need for check before
calling it.

Signed-off-by: Figo.zhang <[email protected]>
---
drivers/staging/comedi/drivers.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers.c
b/drivers/staging/comedi/drivers.c
index 6e13e45..ce50e85 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -495,9 +495,9 @@ int comedi_buf_alloc(struct comedi_device *dev,
struct comedi_subdevice *s,
vmap(pages, n_pages, VM_MAP,
PAGE_KERNEL_NOCACHE);
}
- if (pages) {
- vfree(pages);
- }
+ vfree(pages);
+ pages = NULL;
+
if (async->prealloc_buf == NULL) {
/* Some allocation failed above. */
if (async->buf_page_list) {

2009-06-06 10:56:18

by Pekka Enberg

Subject: Re: [PATCH] no need for checking it

On Sat, Jun 6, 2009 at 1:51 PM, Figo.zhang <[email protected]> wrote:
> vfree() does it's own NULL checking,so no need for check before
> calling it.
>
> Signed-off-by: Figo.zhang <[email protected]>
> ---
> ?drivers/staging/comedi/drivers.c | ? ?6 +++---
> ?1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers.c
> b/drivers/staging/comedi/drivers.c
> index 6e13e45..ce50e85 100644
> --- a/drivers/staging/comedi/drivers.c
> +++ b/drivers/staging/comedi/drivers.c
> @@ -495,9 +495,9 @@ int comedi_buf_alloc(struct comedi_device *dev,
> struct comedi_subdevice *s,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?vmap(pages, n_pages, VM_MAP,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?PAGE_KERNEL_NOCACHE);
> ? ? ? ? ? ? ? ?}
> - ? ? ? ? ? ? ? if (pages) {
> - ? ? ? ? ? ? ? ? ? ? ? vfree(pages);
> - ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? vfree(pages);
> + ? ? ? ? ? ? ? pages = NULL;

Why the assignment to NULL? It looks redundant to me.

> +
> ? ? ? ? ? ? ? ?if (async->prealloc_buf == NULL) {
> ? ? ? ? ? ? ? ? ? ? ? ?/* Some allocation failed above. */
> ? ? ? ? ? ? ? ? ? ? ? ?if (async->buf_page_list) {
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>

2009-06-06 11:09:05

by Figo.zhang

Subject: Re: [PATCH] no need for checking it

On Sat, 2009-06-06 at 13:56 +0300, Pekka Enberg wrote:
> On Sat, Jun 6, 2009 at 1:51 PM, Figo.zhang <[email protected]> wrote:
> > vfree() does it's own NULL checking,so no need for check before
> > calling it.
> >
> > Signed-off-by: Figo.zhang <[email protected]>
> > ---
> > drivers/staging/comedi/drivers.c | 6 +++---
> > 1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers.c
> > b/drivers/staging/comedi/drivers.c
> > index 6e13e45..ce50e85 100644
> > --- a/drivers/staging/comedi/drivers.c
> > +++ b/drivers/staging/comedi/drivers.c
> > @@ -495,9 +495,9 @@ int comedi_buf_alloc(struct comedi_device *dev,
> > struct comedi_subdevice *s,
> > vmap(pages, n_pages, VM_MAP,
> > PAGE_KERNEL_NOCACHE);
> > }
> > - if (pages) {
> > - vfree(pages);
> > - }
> > + vfree(pages);
> > + pages = NULL;
>
> Why the assignment to NULL? It looks redundant to me.
>
> > +
> > if (async->prealloc_buf == NULL) {
> > /* Some allocation failed above. */
> > if (async->buf_page_list) {
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >

yes, 'pages' is local variable argment,it is no need assignment to NULL.

Best Regards,

Figo.zhang

2009-06-06 11:12:22

by Figo.zhang

Subject: [PATCH V2] no need for checking it

vfree() does it's own NULL checking,so no need for check before
calling it.

'pages' is local variable argment,so in v2, it is no need assignment
to NULL.

Signed-off-by: Figo.zhang <[email protected]>
---
drivers/staging/comedi/drivers.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 6e13e45..587283a 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -495,9 +495,8 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
vmap(pages, n_pages, VM_MAP,
PAGE_KERNEL_NOCACHE);
}
- if (pages) {
- vfree(pages);
- }
+ vfree(pages);
+
if (async->prealloc_buf == NULL) {
/* Some allocation failed above. */
if (async->buf_page_list) {

2009-06-06 11:30:09

by Pekka Enberg

Subject: Re: [PATCH V2] no need for checking it

On Sat, Jun 6, 2009 at 2:11 PM, Figo.zhang <[email protected]> wrote:
> vfree() does it's own NULL checking,so no need for check before
> calling it.
>
> 'pages' is local variable argment,so in v2, it is no need assignment
> ?to NULL.
>
> Signed-off-by: Figo.zhang <[email protected]>

Looks good to me.

Acked-by: Pekka Enberg <[email protected]>