diff mbox series

[16/21] erofs: kill magic underscores

Message ID 20190901055130.30572-17-hsiangkao@aol.com (mailing list archive)
State New, archived
Headers show
Series erofs: patchset addressing Christoph's comments | expand

Commit Message

Gao Xiang Sept. 1, 2019, 5:51 a.m. UTC 
From: Gao Xiang <gaoxiang25@huawei.com>

As Christoph said [1], "
> +	vi->datamode = __inode_data_mapping(advise);

What is the deal with these magic underscores here and various
other similar helpers? "

Let avoid magic underscores now...
[1] https://lore.kernel.org/lkml/20190829102426.GE20598@infradead.org/
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 fs/erofs/inode.c    |  8 ++++----
 fs/erofs/internal.h | 14 ++++++--------
 2 files changed, 10 insertions(+), 12 deletions(-)

Comments

Christoph Hellwig Sept. 2, 2019, 12:26 p.m. UTC | #1 
>  
> -	vi->datamode = __inode_data_mapping(advise);
> +	vi->datamode = erofs_inode_data_mapping(advise);
>  
>  	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {

While you are at it can we aim for some naming consistency here?  The
inode member is called is called datamode, the helper is called
inode_data_mapping, and the enum uses layout?  To me data_layout seems
most descriptive, datamode is probably ok, but mapping seems very
misleading given that we've already overloaded that terms for multiple
other uses.

And while we are at naming choices - maybe i_format might be
a better name for the i_advise field in the on-disk inode?

> +	if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) {

I still need to wade through the old thread - but didn't you say this
wasn't really a new vs old version but a compat vs full inode?  Maybe
the names aren't that suitable either?

>  		struct erofs_inode_v2 *v2 = data;
>  
>  		vi->inode_isize = sizeof(struct erofs_inode_v2);
> @@ -58,7 +58,7 @@ static int read_inode(struct inode *inode, void *data)
>  		/* total blocks for compressed files */
>  		if (erofs_inode_is_data_compressed(vi->datamode))
>  			nblks = le32_to_cpu(v2->i_u.compressed_blocks);
> -	} else if (__inode_version(advise) == EROFS_INODE_LAYOUT_V1) {
> +	} else if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V1) {

Also a lot of code would use a switch statements to switch for different
matches on the same value instead of chained if/else if/else if
statements.

> +#define erofs_bitrange(x, bit, bits) (((x) >> (bit)) & ((1 << (bits)) - 1))

> +#define erofs_inode_version(advise)	\
> +	erofs_bitrange(advise, EROFS_I_VERSION_BIT, EROFS_I_VERSION_BITS)
>  
> +#define erofs_inode_data_mapping(advise)	\
> +	erofs_bitrange(advise, EROFS_I_DATA_MAPPING_BIT, \
> +		       EROFS_I_DATA_MAPPING_BITS)

All these should probably be inline functions.
Gao Xiang Sept. 2, 2019, 12:39 p.m. UTC | #2 
Hi Christoph,

On Mon, Sep 02, 2019 at 05:26:27AM -0700, Christoph Hellwig wrote:
> >  
> > -	vi->datamode = __inode_data_mapping(advise);
> > +	vi->datamode = erofs_inode_data_mapping(advise);
> >  
> >  	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {
> 
> While you are at it can we aim for some naming consistency here?  The
> inode member is called is called datamode, the helper is called
> inode_data_mapping, and the enum uses layout?  To me data_layout seems
> most descriptive, datamode is probably ok, but mapping seems very
> misleading given that we've already overloaded that terms for multiple
> other uses.

the naming changed for many times...
Initially, it was called "data_mapping_mode", and I think it was too long
(and as you said confusing, since confusing "mapping" meaning, sorry about
my awkward  English) so I simplified some into datamode....

In a word, I will change all data_mapping_mode into datalayout....

> 
> And while we are at naming choices - maybe i_format might be
> a better name for the i_advise field in the on-disk inode?

That is a good idea, I will resend v2 to address it...

> 
> > +	if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) {
> 
> I still need to wade through the old thread - but didn't you say this
> wasn't really a new vs old version but a compat vs full inode?  Maybe
> the names aren't that suitable either?

Could you kindly give some suggestions for better naming about it?
there are all supported by EROFS... (Actually we only mainly use v1...)

> 
> >  		struct erofs_inode_v2 *v2 = data;
> >  
> >  		vi->inode_isize = sizeof(struct erofs_inode_v2);
> > @@ -58,7 +58,7 @@ static int read_inode(struct inode *inode, void *data)
> >  		/* total blocks for compressed files */
> >  		if (erofs_inode_is_data_compressed(vi->datamode))
> >  			nblks = le32_to_cpu(v2->i_u.compressed_blocks);
> > -	} else if (__inode_version(advise) == EROFS_INODE_LAYOUT_V1) {
> > +	} else if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V1) {
> 
> Also a lot of code would use a switch statements to switch for different
> matches on the same value instead of chained if/else if/else if
> statements.

I will change it as well.

> 
> > +#define erofs_bitrange(x, bit, bits) (((x) >> (bit)) & ((1 << (bits)) - 1))
> 
> > +#define erofs_inode_version(advise)	\
> > +	erofs_bitrange(advise, EROFS_I_VERSION_BIT, EROFS_I_VERSION_BITS)
> >  
> > +#define erofs_inode_data_mapping(advise)	\
> > +	erofs_bitrange(advise, EROFS_I_DATA_MAPPING_BIT, \
> > +		       EROFS_I_DATA_MAPPING_BITS)
> 
> All these should probably be inline functions.

Will fix...

Thanks,
Gao Xiang
Christoph Hellwig Sept. 2, 2019, 12:54 p.m. UTC | #3 
On Mon, Sep 02, 2019 at 08:39:37PM +0800, Gao Xiang wrote:
> > 
> > > +	if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) {
> > 
> > I still need to wade through the old thread - but didn't you say this
> > wasn't really a new vs old version but a compat vs full inode?  Maybe
> > the names aren't that suitable either?
> 
> Could you kindly give some suggestions for better naming about it?
> there are all supported by EROFS... (Actually we only mainly use v1...)

Maybe EROFS_INODE_LAYOUT_COMPACT and EROFS_INODE_LAYOUT_EXTENDED?
Gao Xiang Sept. 2, 2019, 1:38 p.m. UTC | #4 
Hi Christoph,

On Mon, Sep 02, 2019 at 05:54:38AM -0700, Christoph Hellwig wrote:
> On Mon, Sep 02, 2019 at 08:39:37PM +0800, Gao Xiang wrote:
> > > 
> > > > +	if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) {
> > > 
> > > I still need to wade through the old thread - but didn't you say this
> > > wasn't really a new vs old version but a compat vs full inode?  Maybe
> > > the names aren't that suitable either?
> > 
> > Could you kindly give some suggestions for better naming about it?
> > there are all supported by EROFS... (Actually we only mainly use v1...)
> 
> Maybe EROFS_INODE_LAYOUT_COMPACT and EROFS_INODE_LAYOUT_EXTENDED?

Okay, that is fine.

Thanks,
Gao Xiang
diff mbox series

Patch

diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index 19a574ee690b..2ca4eda6e5bf 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -16,7 +16,7 @@  static int read_inode(struct inode *inode, void *data)
 	const unsigned int advise = le16_to_cpu(v1->i_advise);
 	erofs_blk_t nblks = 0;
 
-	vi->datamode = __inode_data_mapping(advise);
+	vi->datamode = erofs_inode_data_mapping(advise);
 
 	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {
 		errln("unsupported data mapping %u of nid %llu",
@@ -25,7 +25,7 @@  static int read_inode(struct inode *inode, void *data)
 		return -EOPNOTSUPP;
 	}
 
-	if (__inode_version(advise) == EROFS_INODE_LAYOUT_V2) {
+	if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) {
 		struct erofs_inode_v2 *v2 = data;
 
 		vi->inode_isize = sizeof(struct erofs_inode_v2);
@@ -58,7 +58,7 @@  static int read_inode(struct inode *inode, void *data)
 		/* total blocks for compressed files */
 		if (erofs_inode_is_data_compressed(vi->datamode))
 			nblks = le32_to_cpu(v2->i_u.compressed_blocks);
-	} else if (__inode_version(advise) == EROFS_INODE_LAYOUT_V1) {
+	} else if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V1) {
 		struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
 
 		vi->inode_isize = sizeof(struct erofs_inode_v1);
@@ -91,7 +91,7 @@  static int read_inode(struct inode *inode, void *data)
 			nblks = le32_to_cpu(v1->i_u.compressed_blocks);
 	} else {
 		errln("unsupported on-disk inode version %u of nid %llu",
-		      __inode_version(advise), vi->nid);
+		      erofs_inode_version(advise), vi->nid);
 		DBG_BUGON(1);
 		return -EOPNOTSUPP;
 	}
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 15545959af92..4a35a31fd454 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -308,16 +308,14 @@  struct erofs_inode {
 #define EROFS_I(ptr)	\
 	container_of(ptr, struct erofs_inode, vfs_inode)
 
-#define __inode_advise(x, bit, bits) \
-	(((x) >> (bit)) & ((1 << (bits)) - 1))
+#define erofs_bitrange(x, bit, bits) (((x) >> (bit)) & ((1 << (bits)) - 1))
 
-#define __inode_version(advise)	\
-	__inode_advise(advise, EROFS_I_VERSION_BIT,	\
-		EROFS_I_VERSION_BITS)
+#define erofs_inode_version(advise)	\
+	erofs_bitrange(advise, EROFS_I_VERSION_BIT, EROFS_I_VERSION_BITS)
 
-#define __inode_data_mapping(advise)	\
-	__inode_advise(advise, EROFS_I_DATA_MAPPING_BIT,\
-		EROFS_I_DATA_MAPPING_BITS)
+#define erofs_inode_data_mapping(advise)	\
+	erofs_bitrange(advise, EROFS_I_DATA_MAPPING_BIT, \
+		       EROFS_I_DATA_MAPPING_BITS)
 
 static inline unsigned long inode_datablocks(struct inode *inode)
 {