On Mon, Jan 9, 2017 at 11:17 PM, Darrick J. Wong
<darrick.wong@xxxxxxxxxx> wrote:
> On Mon, Jan 09, 2017 at 07:21:25PM +0200, Amir Goldstein wrote:
>> On Mon, Jan 9, 2017 at 5:51 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>> > On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
>> >> Fix the size of the xfs_mode_to_ftype conversion table,
>> >> which was too small to handle an invalid value of mode=S_IFMT.
>> >>
>> >> Use a convenience macro S_DT(mode) to convert from
>> >> mode to dirent file type and change the name of the table
>> >> to xfs_dtype_to_ftype to correctly describe its index values.
>> >
>> > This looks like an awful lot of magic. Would a switch statement
>> > generate so much worse code?
>>
>> I doubt it really matters that much, so it's down to a matter of taste.
>> I personally like the existing map table better than switch if anyone cares ;-)
>> but I think that the strongest argument in favor of the existing code
>> is that it works, so no reason to change it.
>> IMHO, this minor fix and cleanup do not justify switching the code over to
>> switch statement.
>
> I can't feed a garbage index to a switch statement and have it fly
> off the end of an array; plus we can throw asserts in a default: case.
>
I think we are in agreement that ASSERT for malformed on-disk data
is a bad idea, as patch 2 demonstrates, and that we are better off
returning -EFSCORRUPTED is such cases.
OK, so just that we are all clear on what I think you are suggesting:
1. retire xfs_mode_to_ftype[] table
2. create helper xfs_mode_to_ftype() that uses a switch statement
and returns XFS_DIR3_FT_UNKNOWN for invalid modes
3. change xfs_dentry_to_name() to return -EFSCORRUPTED for unknown ftype
and check return value on call sites where mode come from disk
(e.g. xfs_generic_create(), xfs_vn_link(), xfs_vn_rename())
4. convert call sites xfs_dentry_to_name(n,d,0) (e.g.
xfs_cleanup_inode()) to use
a helper xfs_dentry_to_name_unknown(n,d) which sets ftype to unknown
instead of calling xfs_mode_to_ftype()
5. in xfs_dinode_verify(), sanity check that di_mode is a valid ftype
Should I cook this patch?
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
