* [PATCH] staging: erofs: using switch-case while checking the inode type. @ 2019-08-29 13:08 Pratik Shinde 2019-08-29 13:56 ` Gao Xiang 0 siblings, 1 reply; 8+ messages in thread From: Pratik Shinde @ 2019-08-29 13:08 UTC (permalink / raw) To: linux-erofs, gaoxiang25, yuchao0; +Cc: devel, gregkh 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: erofs: using switch-case while checking the inode type. 2019-08-29 13:08 [PATCH] staging: erofs: using switch-case while checking the inode type Pratik Shinde @ 2019-08-29 13:56 ` Gao Xiang 2019-08-29 14:05 ` Pratik Shinde 2019-08-29 15:00 ` Dan Carpenter 0 siblings, 2 replies; 8+ messages in thread From: Gao Xiang @ 2019-08-29 13:56 UTC (permalink / raw) To: Pratik Shinde; +Cc: devel, gregkh, linux-erofs 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: erofs: using switch-case while checking the inode type. 2019-08-29 13:56 ` Gao Xiang @ 2019-08-29 14:05 ` Pratik Shinde 2019-08-29 14:15 ` Gao Xiang 2019-08-29 15:00 ` Dan Carpenter 1 sibling, 1 reply; 8+ messages in thread From: Pratik Shinde @ 2019-08-29 14:05 UTC (permalink / raw) To: Gao Xiang; +Cc: devel, gregkh, linux-erofs [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: erofs: using switch-case while checking the inode type. 2019-08-29 14:05 ` Pratik Shinde @ 2019-08-29 14:15 ` Gao Xiang 2019-08-29 15:04 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Gao Xiang @ 2019-08-29 14:15 UTC (permalink / raw) To: Pratik Shinde; +Cc: devel, gregkh, linux-erofs 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 > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: erofs: using switch-case while checking the inode type. 2019-08-29 14:15 ` Gao Xiang @ 2019-08-29 15:04 ` Dan Carpenter 2019-08-29 15:13 ` Gao Xiang 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2019-08-29 15:04 UTC (permalink / raw) To: Gao Xiang; +Cc: devel, gregkh, linux-erofs 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: erofs: using switch-case while checking the inode type. 2019-08-29 15:04 ` Dan Carpenter @ 2019-08-29 15:13 ` Gao Xiang 2019-08-29 18:57 ` Gao Xiang via Linux-erofs 0 siblings, 1 reply; 8+ messages in thread From: Gao Xiang @ 2019-08-29 15:13 UTC (permalink / raw) To: Dan Carpenter; +Cc: devel, gregkh, linux-erofs 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: erofs: using switch-case while checking the inode type. 2019-08-29 15:13 ` Gao Xiang @ 2019-08-29 18:57 ` Gao Xiang via Linux-erofs 0 siblings, 0 replies; 8+ messages in thread From: Gao Xiang via Linux-erofs @ 2019-08-29 18:57 UTC (permalink / raw) To: Dan Carpenter; +Cc: devel, gregkh, 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: erofs: using switch-case while checking the inode type. 2019-08-29 13:56 ` Gao Xiang 2019-08-29 14:05 ` Pratik Shinde @ 2019-08-29 15:00 ` Dan Carpenter 1 sibling, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2019-08-29 15:00 UTC (permalink / raw) To: Gao Xiang; +Cc: devel, gregkh, linux-erofs 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-08-29 18:58 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-29 13:08 [PATCH] staging: erofs: using switch-case while checking the inode type Pratik Shinde 2019-08-29 13:56 ` Gao Xiang 2019-08-29 14:05 ` Pratik Shinde 2019-08-29 14:15 ` Gao Xiang 2019-08-29 15:04 ` Dan Carpenter 2019-08-29 15:13 ` Gao Xiang 2019-08-29 18:57 ` Gao Xiang via Linux-erofs 2019-08-29 15:00 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).