On Thu, Jan 12, 2012 at 12:03:44PM +0100, Lukas Czerner wrote: > + /* We do not support data journalling with delayed allocation */ > + if (!S_ISREG(inode->i_mode) || > + (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) || > + (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA) && > + !test_opt(inode->i_sb, DELALLOC))) > + return EXT4_INODE_JOURNAL_DATA_MODE; /* journal data */ It's probably clearer to make avoid the complex boolean expression: if (!S_ISREG(inode->i_mode)) return EXT4_INODE_JOURNAL_DATA_MODE; if (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)) return EXT4_INODE_JOURNAL_DATA_MODE; if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA) && !test_opt(inode->i_sb, DELALLOC))) return EXT4_INODE_JOURNAL_DATA_MODE; You could combine the first two condiionals: if (!S_ISREG(inode->i_mode) || test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) return EXT4_INODE_JOURNAL_DATA_MODE; if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA) && !test_opt(inode->i_sb, DELALLOC))) return EXT4_INODE_JOURNAL_DATA_MODE; ... but combining || and && where someone has to squint and count parenthesis can be error-prone. I can look at either of the above two and understand quickly what's going on. With what you had (especially since the identation of !test_opt(...) wasn't quite right, I had to squint and think for a bit before I convince dmyself it was correct. Otherwise, looks good! - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
