[PATCH] staging: erofs: using switch-case while checking the inode type.

[PATCH] staging: erofs: using switch-case while checking the inode type.

[Pratik Shinde]

while filling the linux inode, using switch-case statement to check
the type of inode.
switch-case statement looks more clean.

Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
---
 drivers/staging/erofs/inode.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index 4c3d8bf..2d2d545 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir)
 	err = read_inode(inode, data + ofs);
 	if (!err) {
 		/* setup the new inode */
-		if (S_ISREG(inode->i_mode)) {
+		switch (inode->i_mode & S_IFMT) {
+		case S_IFREG:
 			inode->i_op = &erofs_generic_iops;
 			inode->i_fop = &generic_ro_fops;
-		} else if (S_ISDIR(inode->i_mode)) {
+			break;
+		case S_IFDIR:
 			inode->i_op = &erofs_dir_iops;
 			inode->i_fop = &erofs_dir_fops;
-		} else if (S_ISLNK(inode->i_mode)) {
+			break;
+		case S_IFLNK:
 			/* by default, page_get_link is used for symlink */
 			inode->i_op = &erofs_symlink_iops;
 			inode_nohighmem(inode);
-		} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
-			S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
+			break;
+		case S_IFCHR:
+		case S_IFBLK:
+		case S_IFIFO:
+		case S_IFSOCK:
 			inode->i_op = &erofs_generic_iops;
 			init_special_inode(inode, inode->i_mode, inode->i_rdev);
 			goto out_unlock;
-		} else {
+		default:
 			err = -EIO;
 			goto out_unlock;
 		}
-- 
2.9.3

Re: [PATCH] staging: erofs: using switch-case while checking the inode type.

[Gao Xiang]

Hi Pratik,

On Thu, Aug 29, 2019 at 06:38:13PM +0530, Pratik Shinde wrote:
> while filling the linux inode, using switch-case statement to check
> the type of inode.
> switch-case statement looks more clean.
> 
> Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>

No, that is not the case, see __ext4_iget() in fs/ext4/inode.c.
and could you write patches based on latest staging tree?
erofs is now in "fs/" rather than "drivers/staging".
and I will review it then.

p.s. if someone argues here or there, there will be endless
issues since there is no standard at all.

Thanks,
Gao Xiang

> ---
>  drivers/staging/erofs/inode.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
> index 4c3d8bf..2d2d545 100644
> --- a/drivers/staging/erofs/inode.c
> +++ b/drivers/staging/erofs/inode.c
> @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir)
>  	err = read_inode(inode, data + ofs);
>  	if (!err) {
>  		/* setup the new inode */
> -		if (S_ISREG(inode->i_mode)) {
> +		switch (inode->i_mode & S_IFMT) {
> +		case S_IFREG:
>  			inode->i_op = &erofs_generic_iops;
>  			inode->i_fop = &generic_ro_fops;
> -		} else if (S_ISDIR(inode->i_mode)) {
> +			break;
> +		case S_IFDIR:
>  			inode->i_op = &erofs_dir_iops;
>  			inode->i_fop = &erofs_dir_fops;
> -		} else if (S_ISLNK(inode->i_mode)) {
> +			break;
> +		case S_IFLNK:
>  			/* by default, page_get_link is used for symlink */
>  			inode->i_op = &erofs_symlink_iops;
>  			inode_nohighmem(inode);
> -		} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
> -			S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
> +			break;
> +		case S_IFCHR:
> +		case S_IFBLK:
> +		case S_IFIFO:
> +		case S_IFSOCK:
>  			inode->i_op = &erofs_generic_iops;
>  			init_special_inode(inode, inode->i_mode, inode->i_rdev);
>  			goto out_unlock;
> -		} else {
> +		default:
>  			err = -EIO;
>  			goto out_unlock;
>  		}
> -- 
> 2.9.3
> 

Re: [PATCH] staging: erofs: using switch-case while checking the inode type.

[Pratik Shinde]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

Hi Gao,

Sorry I didn't pull the latest tree. I will do the necessary.
Anyways, don't you think it will be cleaner to have a switch case statement
rather than if-else statement.

--Pratik



On Thu, 29 Aug, 2019, 7:27 PM Gao Xiang, <gaoxiang25@huawei.com> wrote:

> Hi Pratik,
>
> On Thu, Aug 29, 2019 at 06:38:13PM +0530, Pratik Shinde wrote:
> > while filling the linux inode, using switch-case statement to check
> > the type of inode.
> > switch-case statement looks more clean.
> >
> > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
>
> No, that is not the case, see __ext4_iget() in fs/ext4/inode.c.
> and could you write patches based on latest staging tree?
> erofs is now in "fs/" rather than "drivers/staging".
> and I will review it then.
>
> p.s. if someone argues here or there, there will be endless
> issues since there is no standard at all.
>
> Thanks,
> Gao Xiang
>
> > ---
> >  drivers/staging/erofs/inode.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/erofs/inode.c
> b/drivers/staging/erofs/inode.c
> > index 4c3d8bf..2d2d545 100644
> > --- a/drivers/staging/erofs/inode.c
> > +++ b/drivers/staging/erofs/inode.c
> > @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int
> isdir)
> >       err = read_inode(inode, data + ofs);
> >       if (!err) {
> >               /* setup the new inode */
> > -             if (S_ISREG(inode->i_mode)) {
> > +             switch (inode->i_mode & S_IFMT) {
> > +             case S_IFREG:
> >                       inode->i_op = &erofs_generic_iops;
> >                       inode->i_fop = &generic_ro_fops;
> > -             } else if (S_ISDIR(inode->i_mode)) {
> > +                     break;
> > +             case S_IFDIR:
> >                       inode->i_op = &erofs_dir_iops;
> >                       inode->i_fop = &erofs_dir_fops;
> > -             } else if (S_ISLNK(inode->i_mode)) {
> > +                     break;
> > +             case S_IFLNK:
> >                       /* by default, page_get_link is used for symlink */
> >                       inode->i_op = &erofs_symlink_iops;
> >                       inode_nohighmem(inode);
> > -             } else if (S_ISCHR(inode->i_mode) ||
> S_ISBLK(inode->i_mode) ||
> > -                     S_ISFIFO(inode->i_mode) ||
> S_ISSOCK(inode->i_mode)) {
> > +                     break;
> > +             case S_IFCHR:
> > +             case S_IFBLK:
> > +             case S_IFIFO:
> > +             case S_IFSOCK:
> >                       inode->i_op = &erofs_generic_iops;
> >                       init_special_inode(inode, inode->i_mode,
> inode->i_rdev);
> >                       goto out_unlock;
> > -             } else {
> > +             default:
> >                       err = -EIO;
> >                       goto out_unlock;
> >               }
> > --
> > 2.9.3
> >
>

[-- Attachment #2: Type: text/html, Size: 4221 bytes --]

Re: [PATCH] staging: erofs: using switch-case while checking the inode type.

[Gao Xiang]

On Thu, Aug 29, 2019 at 07:35:01PM +0530, Pratik Shinde wrote:
> Hi Gao,
> 
> Sorry I didn't pull the latest tree. I will do the necessary.
> Anyways, don't you think it will be cleaner to have a switch case statement
> rather than if-else statement.

I think so, but that's another personal choise and no urgent
as well (It is just a cleanup to some extent).

I am very happy that you send a patch about this, but we have
to take care of handling "fall through" properly at least,
and I don't want to introduce some extra compile warnings
instead at this time.

EROFS is sensitive for now and I have no idea what the "real"
point is.

Thanks,
Gao Xiang

> 
> --Pratik
> 
> 
> 
> On Thu, 29 Aug, 2019, 7:27 PM Gao Xiang, <gaoxiang25@huawei.com> wrote:
> 
> > Hi Pratik,
> >
> > On Thu, Aug 29, 2019 at 06:38:13PM +0530, Pratik Shinde wrote:
> > > while filling the linux inode, using switch-case statement to check
> > > the type of inode.
> > > switch-case statement looks more clean.
> > >
> > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> >
> > No, that is not the case, see __ext4_iget() in fs/ext4/inode.c.
> > and could you write patches based on latest staging tree?
> > erofs is now in "fs/" rather than "drivers/staging".
> > and I will review it then.
> >
> > p.s. if someone argues here or there, there will be endless
> > issues since there is no standard at all.
> >
> > Thanks,
> > Gao Xiang
> >
> > > ---
> > >  drivers/staging/erofs/inode.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/erofs/inode.c
> > b/drivers/staging/erofs/inode.c
> > > index 4c3d8bf..2d2d545 100644
> > > --- a/drivers/staging/erofs/inode.c
> > > +++ b/drivers/staging/erofs/inode.c
> > > @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int
> > isdir)
> > >       err = read_inode(inode, data + ofs);
> > >       if (!err) {
> > >               /* setup the new inode */
> > > -             if (S_ISREG(inode->i_mode)) {
> > > +             switch (inode->i_mode & S_IFMT) {
> > > +             case S_IFREG:
> > >                       inode->i_op = &erofs_generic_iops;
> > >                       inode->i_fop = &generic_ro_fops;
> > > -             } else if (S_ISDIR(inode->i_mode)) {
> > > +                     break;
> > > +             case S_IFDIR:
> > >                       inode->i_op = &erofs_dir_iops;
> > >                       inode->i_fop = &erofs_dir_fops;
> > > -             } else if (S_ISLNK(inode->i_mode)) {
> > > +                     break;
> > > +             case S_IFLNK:
> > >                       /* by default, page_get_link is used for symlink */
> > >                       inode->i_op = &erofs_symlink_iops;
> > >                       inode_nohighmem(inode);
> > > -             } else if (S_ISCHR(inode->i_mode) ||
> > S_ISBLK(inode->i_mode) ||
> > > -                     S_ISFIFO(inode->i_mode) ||
> > S_ISSOCK(inode->i_mode)) {
> > > +                     break;
> > > +             case S_IFCHR:
> > > +             case S_IFBLK:
> > > +             case S_IFIFO:
> > > +             case S_IFSOCK:
> > >                       inode->i_op = &erofs_generic_iops;
> > >                       init_special_inode(inode, inode->i_mode,
> > inode->i_rdev);
> > >                       goto out_unlock;
> > > -             } else {
> > > +             default:
> > >                       err = -EIO;
> > >                       goto out_unlock;
> > >               }
> > > --
> > > 2.9.3
> > >
> >

Re: [PATCH] staging: erofs: using switch-case while checking the inode type.

[Dan Carpenter]

On Thu, Aug 29, 2019 at 09:56:07PM +0800, Gao Xiang wrote:
> Hi Pratik,
> 
> On Thu, Aug 29, 2019 at 06:38:13PM +0530, Pratik Shinde wrote:
> > while filling the linux inode, using switch-case statement to check
> > the type of inode.
> > switch-case statement looks more clean.
> > 
> > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> 
> No, that is not the case, see __ext4_iget() in fs/ext4/inode.c.

Unnecessary complaining.

> and could you write patches based on latest staging tree?
> erofs is now in "fs/" rather than "drivers/staging".
> and I will review it then.
> 
> p.s. if someone argues here or there, there will be endless
> issues since there is no standard at all.

More complaining...  It doesn't matter if you can find ext4 that looks
like dog poop, that's irrelevant.

regards,
dan carpenter

Re: [PATCH] staging: erofs: using switch-case while checking the inode type.

[Dan Carpenter]

On Thu, Aug 29, 2019 at 10:15:22PM +0800, Gao Xiang wrote:
> I am very happy that you send a patch about this, but we have
> to take care of handling "fall through" properly at least,
> and I don't want to introduce some extra compile warnings
> instead at this time.

I can't apply the patch so I maybe missed something.  I don't see
a fall through issue.  We have the code so you could use ^^^^^^^^ to
indicate which lines have a fall through problem.

> 
> EROFS is sensitive for now and I have no idea what the "real"
> point is.

What does "sensitive" mean here?  Now that it's out of staging we
aren't applying clean up patches?

regards,
dan carpenter

Re: [PATCH] staging: erofs: using switch-case while checking the inode type.

[Gao Xiang]

Hi Dan,

On Thu, Aug 29, 2019 at 06:04:36PM +0300, Dan Carpenter wrote:
> On Thu, Aug 29, 2019 at 10:15:22PM +0800, Gao Xiang wrote:
> > I am very happy that you send a patch about this, but we have
> > to take care of handling "fall through" properly at least,
> > and I don't want to introduce some extra compile warnings
> > instead at this time.
> 
> I can't apply the patch so I maybe missed something.  I don't see
> a fall through issue.  We have the code so you could use ^^^^^^^^ to
> indicate which lines have a fall through problem.
> 
> > 
> > EROFS is sensitive for now and I have no idea what the "real"
> > point is.
> 
> What does "sensitive" mean here?  Now that it's out of staging we
> aren't applying clean up patches?

Of course not, I mean we should avoid "fall through" problem
but I have no time to verify this patch since I am fixing what
hch said as well.

Thanks,
Gao Xiang

> 
> regards,
> dan carpenter
> 

Re: [PATCH] staging: erofs: using switch-case while checking the inode type.

[Gao Xiang via Linux-erofs]

Hi Dan,

On Thu, Aug 29, 2019 at 11:13:53PM +0800, Gao Xiang wrote:
> Hi Dan,
> 
> On Thu, Aug 29, 2019 at 06:04:36PM +0300, Dan Carpenter wrote:
> > On Thu, Aug 29, 2019 at 10:15:22PM +0800, Gao Xiang wrote:
> > > I am very happy that you send a patch about this, but we have
> > > to take care of handling "fall through" properly at least,
> > > and I don't want to introduce some extra compile warnings
> > > instead at this time.
> > 
> > I can't apply the patch so I maybe missed something.  I don't see
> > a fall through issue.  We have the code so you could use ^^^^^^^^ to
> > indicate which lines have a fall through problem.
> > 
> > > 
> > > EROFS is sensitive for now and I have no idea what the "real"
> > > point is.
> > 
> > What does "sensitive" mean here?  Now that it's out of staging we
> > aren't applying clean up patches?

Again, due to language obstacle, I have to give a detailed explanation
of what "sensitive" meant here.

I meant it seems all topic had no relationship with EROFS at all, but
someone mentioned erofs in different topics, I have no idea what is
wrong with EROFS, and I have no idea where it is like POS.

As your unlikely/likely concern, I think I discussed with you earlier,
in fact, most of EROFS unlikely is due to error handling paths, which are
meaningful in my thought.

If you argue that it has little performance difference, I think unlikely
in IS_ERR can also be killed as well since for most use cases in Linux
it is true that they are little performance difference at all. But I
think totally it will have some impact.

Anyway, I didn't find some formal standard about this, and I remove all
of these as you like.

Thanks,
Gao Xiang

> 
> Of course not, I mean we should avoid "fall through" problem
> but I have no time to verify this patch since I am fixing what
> hch said as well.
> 
> Thanks,
> Gao Xiang
> 
> > 
> > regards,
> > dan carpenter
> >