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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
