linux-ext4 - Re: [PATCH 2/2] ext4: clean up group state test macros with predicate functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <6112C6BB-1356-45D8-8435-A93FC2AD4C2D@dilger.ca>
Date:   Tue, 18 Dec 2018 21:55:49 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     "zhangyi (F)" <yi.zhang@...wei.com>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Theodore Ts'o <tytso@....edu>, Wang Shilong <wshilong@....com>,
        Miao Xie <miaoxie@...wei.com>
Subject: Re: [PATCH 2/2] ext4: clean up group state test macros with predicate
 functions

On Dec 18, 2018, at 9:47 PM, zhangyi (F) <yi.zhang@...wei.com> wrote:
> 
> On 2018/12/19 3:51, Andreas Dilger Wrote:
>> On Dec 18, 2018, at 5:00 AM, zhangyi (F) <yi.zhang@...wei.com> wrote:
>>> 
>>> Create separate predicate functions to test/set/clear/test_and_set
>>> bb_state flags in ext4_group_info like features testing, and then
>>> replace all old macros and the places where we use
>>> EXT4_GROUP_INFO_XXX_BIT directly.
>>> 
>>> Signed-off-by: zhangyi (F) <yi.zhang@...wei.com>
>>> ---
>>> 
>>> +EXT4_MB_GROUP_STATE_FUNCS(need_init, NEED_INIT)
>>> +EXT4_MB_GROUP_STATE_FUNCS(trimmed, WAS_TRIMMED)
>>> +EXT4_MB_GROUP_STATE_FUNCS(bbitmap_corrupt, BBITMAP_CORRUPT)
>>> +EXT4_MB_GROUP_STATE_FUNCS(ibitmap_corrupt, IBITMAP_CORRUPT)
>> 
>> One problem with macros like this that internally expand to multiple
>> functions is that there is now nowhere in this code where, for example,
>> the declaration of ext4_mb_grp_test_and_set_bbitmap_corrupt() can be
>> found.  That makes it hard to understand the code, because tags for this
>> function name will not work, and even a grep through the entire code for
>> this string will not show the function implementation, only users.  One
>> would have to search for only the "ext4_mb_grp_test_and_set" part, or
>> "ext4_mb_grp_clear" to find the above macros.
>> 
>> If such macros-that-generate-functions are being used, my preference is
>> that at least a comment block is added that spells out the full function
>> names, so that at least a grep will find them, like:
>> 
>> /*
>> * These macros implement the following functions:
>> * - ext4_mb_grp_need_init(), ext4_mb_grp_test_and_set_need_init(),
>> *   ext4_mb_grp_set_need_init(), ext4_mb_grp_clear_need_init()
>> * - ...
>> * - ...
>> */
>> 
>> Yes, this is a bit cumbersome the rare times a new function is added, but
>> it really makes the code easier to understand in the future, without forcing
>> a cut-and-paste of the body of each function.  I don't know how many times
>> I've had to search for commonly-used functions like buffer_uptodate() or
>> buffer_dirty() in the code without being able to find them easily.
>> 
> 
> Thanks for your comments. Indeed, I also had the same hard time as you said.
> I am not so sure why we have been using these maco functions for ext4 features
> and ext4_inode_info bit flags. But I think it's still worth to unify them.
> 
> I will add the comment block as your suggested and post the second version,
> BTW, I read the commit 3f61c0cc706 "ext4: add prototypes for macro-generated
> functions" you posted, it's also a good choice.

Indeed, adding static function prototypes is even better than putting the
function names in a comment, since tags/ctags/etags will find them for you.
I'd forgotten about that patch.

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)