[OPW,kernel,2/2] staging: lustre: lustre: llite: Use IS_ERR_OR_NULL
diff mbox

Message ID 34725cab53dec6b03aa726a16e5b16bf0df68e97.1414675234.git.tapaswenipathak@gmail.com
State New, archived
Headers show

Commit Message

Tapasweni Pathak Oct. 30, 2014, 1:22 p.m. UTC
This patch introduces the use of the macro IS_ERR_OR_NULL in place of
tests for NULL and IS_ERR.

The following Coccinelle semantic patch was used for making the change:

@@
expression e;
@@

- e == NULL || IS_ERR(e)
+ IS_ERR_OR_NULL(e)
 || ...

Signed-off-by: Tapasweni Pathak <tapaswenipathak@gmail.com>
---
 drivers/staging/lustre/lustre/llite/llite_lib.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
1.7.9.5

Comments

Arnd Bergmann Oct. 30, 2014, 1:35 p.m. UTC | #1
On Thursday 30 October 2014 18:52:25 Tapasweni Pathak wrote:
> This patch introduces the use of the macro IS_ERR_OR_NULL in place of
> tests for NULL and IS_ERR.
> 
> The following Coccinelle semantic patch was used for making the change:
> 
> @@
> expression e;
> @@
> 
> - e == NULL || IS_ERR(e)
> + IS_ERR_OR_NULL(e)
>  || ...
> 
> Signed-off-by: Tapasweni Pathak <tapaswenipathak@gmail.com>

I think this is not a useful semantic patch.

> diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
> index 74cd451..b3999ca 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> @@ -533,7 +533,7 @@ static int client_common_fill_super(struct super_block *sb, char *md, char *dt,
>  	md_free_lustre_md(sbi->ll_md_exp, &lmd);
>  	ptlrpc_req_finished(request);
> 
> -	if (root == NULL || IS_ERR(root)) {
> +	if (IS_ERR_OR_NULL(root)) {
>  		if (lmd.lsm)
>  			obd_free_memmd(sbi->ll_dt_exp, &lmd.lsm);
>  #ifdef CONFIG_FS_POSIX_ACL


While the conversion is clearly correct, it does not fix the underlying problem
of having an interface that is not well defined. Each function that can
return a valid pointer on success should either return NULL on failure
consistently, or should return an error pointer consistently.

A better fix for this would be to change ll_iget to return an error
pointer when iget5_locked() returns NULL.

	Arnd
Josh Triplett Oct. 30, 2014, 1:37 p.m. UTC | #2
On Thu, Oct 30, 2014 at 06:52:25PM +0530, Tapasweni Pathak wrote:
> This patch introduces the use of the macro IS_ERR_OR_NULL in place of
> tests for NULL and IS_ERR.
> 
> The following Coccinelle semantic patch was used for making the change:
> 
> @@
> expression e;
> @@
> 
> - e == NULL || IS_ERR(e)
> + IS_ERR_OR_NULL(e)
>  || ...
> 
> Signed-off-by: Tapasweni Pathak <tapaswenipathak@gmail.com>

"lustre:" shouldn't be duplicated in the subject.  Apart from that:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  drivers/staging/lustre/lustre/llite/llite_lib.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
> index 74cd451..b3999ca 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> @@ -533,7 +533,7 @@ static int client_common_fill_super(struct super_block *sb, char *md, char *dt,
>  	md_free_lustre_md(sbi->ll_md_exp, &lmd);
>  	ptlrpc_req_finished(request);
> 
> -	if (root == NULL || IS_ERR(root)) {
> +	if (IS_ERR_OR_NULL(root)) {
>  		if (lmd.lsm)
>  			obd_free_memmd(sbi->ll_dt_exp, &lmd.lsm);
>  #ifdef CONFIG_FS_POSIX_ACL
> @@ -2110,7 +2110,7 @@ int ll_prep_inode(struct inode **inode, struct ptlrpc_request *req,
>  		*inode = ll_iget(sb, cl_fid_build_ino(&md.body->fid1,
>  					     sbi->ll_flags & LL_SBI_32BIT_API),
>  				 &md);
> -		if (*inode == NULL || IS_ERR(*inode)) {
> +		if (IS_ERR_OR_NULL(*inode)) {
>  #ifdef CONFIG_FS_POSIX_ACL
>  			if (md.posix_acl) {
>  				posix_acl_release(md.posix_acl);
> --
> 1.7.9.5
> 
> -- 
> You received this message because you are subscribed to the Google Groups "opw-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to opw-kernel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
Julia Lawall Oct. 30, 2014, 1:38 p.m. UTC | #3
On Thu, 30 Oct 2014, Arnd Bergmann wrote:

> On Thursday 30 October 2014 18:52:25 Tapasweni Pathak wrote:
> > This patch introduces the use of the macro IS_ERR_OR_NULL in place of
> > tests for NULL and IS_ERR.
> >
> > The following Coccinelle semantic patch was used for making the change:
> >
> > @@
> > expression e;
> > @@
> >
> > - e == NULL || IS_ERR(e)
> > + IS_ERR_OR_NULL(e)
> >  || ...
> >
> > Signed-off-by: Tapasweni Pathak <tapaswenipathak@gmail.com>
>
> I think this is not a useful semantic patch.
>
> > diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
> > index 74cd451..b3999ca 100644
> > --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> > +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> > @@ -533,7 +533,7 @@ static int client_common_fill_super(struct super_block *sb, char *md, char *dt,
> >  	md_free_lustre_md(sbi->ll_md_exp, &lmd);
> >  	ptlrpc_req_finished(request);
> >
> > -	if (root == NULL || IS_ERR(root)) {
> > +	if (IS_ERR_OR_NULL(root)) {
> >  		if (lmd.lsm)
> >  			obd_free_memmd(sbi->ll_dt_exp, &lmd.lsm);
> >  #ifdef CONFIG_FS_POSIX_ACL
>
>
> While the conversion is clearly correct, it does not fix the underlying problem
> of having an interface that is not well defined. Each function that can
> return a valid pointer on success should either return NULL on failure
> consistently, or should return an error pointer consistently.
>
> A better fix for this would be to change ll_iget to return an error
> pointer when iget5_locked() returns NULL.

Not particular to this case, but I believe that there are some functions
that return NULL to indicate that they didn't find anything (ie in a list)
and an ERR_PTR value when something went wrong.

julia
Arnd Bergmann Oct. 30, 2014, 1:49 p.m. UTC | #4
On Thursday 30 October 2014 14:38:38 Julia Lawall wrote:
> On Thu, 30 Oct 2014, Arnd Bergmann wrote:
> 
> > On Thursday 30 October 2014 18:52:25 Tapasweni Pathak wrote:
> > > This patch introduces the use of the macro IS_ERR_OR_NULL in place of
> > > tests for NULL and IS_ERR.
> > >
> > > The following Coccinelle semantic patch was used for making the change:
> > >
> > > @@
> > > expression e;
> > > @@
> > >
> > > - e == NULL || IS_ERR(e)
> > > + IS_ERR_OR_NULL(e)
> > >  || ...
> > >
> > > Signed-off-by: Tapasweni Pathak <tapaswenipathak@gmail.com>
> >
> > I think this is not a useful semantic patch.
> >
> > > diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
> > > index 74cd451..b3999ca 100644
> > > --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> > > +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> > > @@ -533,7 +533,7 @@ static int client_common_fill_super(struct super_block *sb, char *md, char *dt,
> > >     md_free_lustre_md(sbi->ll_md_exp, &lmd);
> > >     ptlrpc_req_finished(request);
> > >
> > > -   if (root == NULL || IS_ERR(root)) {
> > > +   if (IS_ERR_OR_NULL(root)) {
> > >             if (lmd.lsm)
> > >                     obd_free_memmd(sbi->ll_dt_exp, &lmd.lsm);
> > >  #ifdef CONFIG_FS_POSIX_ACL
> >
> >
> > While the conversion is clearly correct, it does not fix the underlying problem
> > of having an interface that is not well defined. Each function that can
> > return a valid pointer on success should either return NULL on failure
> > consistently, or should return an error pointer consistently.
> >
> > A better fix for this would be to change ll_iget to return an error
> > pointer when iget5_locked() returns NULL.
> 
> Not particular to this case, but I believe that there are some functions
> that return NULL to indicate that they didn't find anything (ie in a list)
> and an ERR_PTR value when something went wrong.

Yes, and those functions tend to be used incorrectly ;-)

Maybe we could use Coccinelle to check for this kind of function instead:
anything that can return NULL in the function but return ERR_PTR(...)
or a pointer that has been tested with IS_ERR() is creating a hard-to-use
interface that we should try to avoid.

	Arnd

Patch
diff mbox

diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
index 74cd451..b3999ca 100644
--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
@@ -533,7 +533,7 @@  static int client_common_fill_super(struct super_block *sb, char *md, char *dt,
 	md_free_lustre_md(sbi->ll_md_exp, &lmd);
 	ptlrpc_req_finished(request);

-	if (root == NULL || IS_ERR(root)) {
+	if (IS_ERR_OR_NULL(root)) {
 		if (lmd.lsm)
 			obd_free_memmd(sbi->ll_dt_exp, &lmd.lsm);
 #ifdef CONFIG_FS_POSIX_ACL
@@ -2110,7 +2110,7 @@  int ll_prep_inode(struct inode **inode, struct ptlrpc_request *req,
 		*inode = ll_iget(sb, cl_fid_build_ino(&md.body->fid1,
 					     sbi->ll_flags & LL_SBI_32BIT_API),
 				 &md);
-		if (*inode == NULL || IS_ERR(*inode)) {
+		if (IS_ERR_OR_NULL(*inode)) {
 #ifdef CONFIG_FS_POSIX_ACL
 			if (md.posix_acl) {
 				posix_acl_release(md.posix_acl);