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