On 9 May 2014 21:05, Matthew Wilcox <willy@xxxxxxxxxxxxxxx> wrote:
> On Wed, May 07, 2014 at 06:56:56PM +0200, Sergei Antonov wrote:
>> On 7 May 2014 18:25, Vyacheslav Dubeyko <slava@xxxxxxxxxxx> wrote:
>> > I believe that such example is good solution too:
>> >
>> > static unsigned char
>> > nilfs_filetype_table[NILFS_FT_MAX] = {
>> > [NILFS_FT_UNKNOWN] = DT_UNKNOWN,
>> > [NILFS_FT_REG_FILE] = DT_REG,
>> > [NILFS_FT_DIR] = DT_DIR,
>> > [NILFS_FT_CHRDEV] = DT_CHR,
>> > [NILFS_FT_BLKDEV] = DT_BLK,
>> > [NILFS_FT_FIFO] = DT_FIFO,
>> > [NILFS_FT_SOCK] = DT_SOCK,
>> > [NILFS_FT_SYMLINK] = DT_LNK,
>> > };
>> >
>> > What about likewise way?
>>
>> Using an array means calculating an index to it using such expression:
>> (mode & S_IFMT) >> 12.
>> I prefer a more straightforward "if/else if".
>
> This should all be wrapped up neatly like so:
>
> } else if (type == HFSPLUS_FILE) {
> + unsigned type;
> +
> if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
> pr_err("small file entry\n");
> err = -EIO;
> goto out;
> }
> +
> + type = hfsplus_de_type(&entry);
> +
A dedicated function is a good idea. I regret it didn't occur to me at
the moment of writing the patch.
Furthermore, hfsplus_readdir() is 125 lines long which exceeds
Documentation/CodingStyle recommended limit of two screenfuls of text
(i.e. around 50 lines). It makes sense to submit a patch breaking
hfsplus_readdir() into several (not only hfsplus_de_type()) smaller
functions.
> if (!dir_emit(ctx, strbuf, len,
> - be32_to_cpu(entry.file.id), DT_REG))
> + be32_to_cpu(entry.file.id), type))
> break;
>
> and then the array, case, series of else ifs, whatever can all be confined
> to a single function.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
