[LU-12923] replace CLASSERT() with BUILD_BUG_ON()

• Type: Technical task
• Resolution: Fixed
• Priority: Minor
• Fix Version/s: Lustre 2.14.0
• Affects Version/s: Lustre 2.14.0
• Labels: easy
• Assignee: Arshad Hussain
• Reporter: Andreas Dilger

Description

The Lustre-specific compile-time assertion CLASSERT() should be replaced with the standard kernel BUILD_BUG_ON() macro that does the same thing.

NOTE that the logic of CLASSERT() is ensuring that the expression is true while BUILD_BUG_ON() is verifying if the expression is false, so the logic of each check should be reversed.

All of these expressions are evaluated at compile time, so it is possible to mark all patches fixing this with Test-Parameters: trivial.

The first patch in the series should update contrib/scripts/spelling.txt to add CLASSERT||BUILD_BUG_ON() so that new patches get a warning if they are using the old CLASSERT() macro. The last patch in the series should remove the CLASSERT() macro. There are currently 480 instances of CLASSERT() in the master branch, but a majority of them (318) are in the automatically-generated wirecheck.c, pack-generic.c and api-ni.c file, so most could be easily replaced by fixing the wiretest.c scripts.

Issue Links

• is related to: LU-9859 libcfs simplification

Comments

[James A Simmons]

spelling.txt will remain since it tells people not use CLASSERT anymore. The define in libcfs_private.h does need to be removed but we should wait a bit since other patches in flight might land that have a CLASSERT.

[Aurelien Degremont (Inactive)]

What should we do with the last 2 occurrences?

$ git grep CLASSERT
contrib/scripts/spelling.txt:CLASSERT||BUILD_BUG_ON()
libcfs/include/libcfs/libcfs_private.h:#define CLASSERT(cond) do {switch (1) {case (cond): case 0: break; } } while (0)

I think they should also be removed?

[Peter Jones]

Landed for 2.14

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch https://review.whamcloud.com/37113/
Subject: LU-12923 lnet: Replace CLASSERT() with BUILD_BUG_ON()
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 81fff75016dae08332d1a6cdd5137fc4e3097665

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch https://review.whamcloud.com/37111/
Subject: LU-12923 lustre: Replace CLASSERT() with BUILD_BUG_ON()
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: f0fa794d0fc35be8fb0839731d122a73395f2a05

[Arshad Hussain]

Aurelien, Thanks for pointing out.

Updated remaining CLASSERT() under lnet.

[Gerrit Updater]

Arshad Hussain ([e-mail]) uploaded a new patch: https://review.whamcloud.com/37113
Subject: LU-12923 lustre: Replace CLASSERT() with BUILD_BUG_ON()
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 51a21905a4a815a80497b59496ccdeeda9a3f3b8

[Aurelien Degremont (Inactive)]

Looks like lnet also needs one last patch:

$ git grep -c CLASSERT lnet/
lnet/lnet/api-ni.c:96
lnet/utils/wirecheck.c:2

 

[Gerrit Updater]

Arshad Hussain ([e-mail]) uploaded a new patch: https://review.whamcloud.com/37111
Subject: LU-12923 lustre: Replace CLASSERT() with BUILD_BUG_ON()
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: de937279e835c26b61bbb6c248279c53852d631d

[Arshad Hussain]

Hi Peter, I was away traveling. I am catching up on my mails and messages now, sorry for delay response.

Unfortunately, as I double checked it looks like few instances under files which escaped my scrutiny.  So, this LU is almost done but not complete. I will address this quickly.

 

[lustre-release]$ find ./lustre/ -name "*.c" | xargs grep CLASSERT
./lustre/osd-zfs/osd_index.c: CLASSERT(sizeof(zde->lzd_reg) == 8);
./lustre/osd-zfs/osd_index.c: CLASSERT(sizeof(*zde) % 8 == 0);
./lustre/osd-zfs/osd_index.c: CLASSERT(sizeof(za->za_name) <= sizeof(it->ozi_name));
./lustre/ptlrpc/nodemap_storage.c: CLASSERT(sizeof(union nodemap_rec) == 32);
./lustre/mdd/mdd_object.c: CLASSERT(ARRAY_SIZE(info->mti_buf) >= 4);

Where I went wrong was, I was using below command to verify. Which unfortunately was give me false positives.

$ find ./lustre -name "*.c" | xargs ./contrib/scripts/checkpatch.pl -f --no-tree | grep CLASSERT | wc -l

[Peter Jones]

Everything currently in flight has landed - is this task now complete?