All of lore.kernel.org
 help / color / mirror / Atom feed
From: brookxu <brookxu.cn@gmail.com>
To: Andreas Dilger <adilger@dilger.ca>
Cc: Ted Tso <tytso@mit.edu>,
	Ext4 Developers List <linux-ext4@vger.kernel.org>
Subject: Re: [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT()
Date: Tue, 20 Oct 2020 13:03:41 +0800	[thread overview]
Message-ID: <b9c6851c-7b0f-6b22-fa4c-5e620df55a41@gmail.com> (raw)
In-Reply-To: <849873AE-1880-45D6-B987-C5DD42967D4D@dilger.ca>

Thanks for your reply.

Andreas Dilger wrote on 2020/10/20 11:37:
> On Oct 19, 2020, at 3:02 AM, Chunguang Xu <brookxu.cn@gmail.com> wrote:
>>
>> There are currently multiple forms of assertion, such as J_ASSERT().
>> J_ASEERT is provided for the jbd module, which is a public module.
> 
> (typo) "J_ASSERT()"
Thanks, I  will Fixed that.

>> Maybe we should use custom ASSERT() like other file systems, such as
>> xfs, which would be better.
> 
> My one minor complaint is that "ASSERT()" is a very generic name and is
> likely to cause conflicts with ASSERT in other headers.  That said, I
> also see many other filesystems using their own ASSERT() macro, so I
> guess they are all in private headers only?
I also thought about this before, but even if we define it in a private
header file, because we still include several header files in a certain
file, it seems that the conflict cannot be resolved. However, maybe it
is safest to use a name with ext4 prefix. I will try to fix it in next
version. Thanks.

> Some minor comments/questions below, but not worth changing the patch
> unless you think they are important...
> 
> You can add:
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> 
>> @@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb,
>> 	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> 	ext4_fsblk_t start, tmp;
>>
>> -	J_ASSERT_BH(bh, buffer_locked(bh));
>> +	ASSERT(buffer_locked(bh));
> 
> I thought J_ASSERT_BH() did something useful, but J_ASSERT_BH() just maps
> to J_ASSERT() internally anyway.
> 
>> +#define ASSERT(assert)							\
>> +do {									\
>> +	if (unlikely(!(assert))) {					\
>> +		printk(KERN_EMERG					\
>> +		       "Assertion failure in %s() at %s:%d: \"%s\"\n",	\
> 
> (style) better to use single quotes '%s' to avoid the need to escape \".
Thanks, this is a good suggestion, I will fix it next version.

> Cheers, Andreas
> 
> 
> 
> 
> 

      reply	other threads:[~2020-10-20  5:03 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 2/8] ext4: remove redundant mb_regenerate_buddy() Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 3/8] ext4: simplify the code of mb_find_order_for_block Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 4/8] ext4: add the gdt block of meta_bg to system_zone Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 5/8] ext4: update ext4_data_block_valid related comments Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 6/8] ext4: add a helper function to validate metadata block Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 7/8] ext4: delete invalid code inside ext4_xattr_block_set() Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 8/8] ext4: fix a memory leak of ext4_free_data Chunguang Xu
2020-10-20  3:37 ` [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Andreas Dilger
2020-10-20  5:03   ` brookxu [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b9c6851c-7b0f-6b22-fa4c-5e620df55a41@gmail.com \
    --to=brookxu.cn@gmail.com \
    --cc=adilger@dilger.ca \
    --cc=linux-ext4@vger.kernel.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.