On Thu, 19 Jan 2012, Ted Ts'o wrote: > 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 Thanks Ted, you're right it is much clearer this way. I'll resend the patch. -Lukas -- 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
