Re: [PATCH V2] ubifs: correct UBIFS_DFS_DIR_LEN macro definition and improve code clarity

From: Zhihao Cheng <chengzhihao1@huawei.com>
To: ZhaoLong Wang <wangzhaolong1@huawei.com>, <richard@nod.at>,
	<miquel.raynal@bootlin.com>, <vigneshr@ti.com>
Cc: <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<yi.zhang@huawei.com>, <yangerkun@huawei.com>
Subject: Re: [PATCH V2] ubifs: correct UBIFS_DFS_DIR_LEN macro definition and improve code clarity
Date: Mon, 25 Mar 2024 21:46:35 +0800	[thread overview]
Message-ID: <3fce4fde-0163-4fd4-feb0-2a68aa785e2f@huawei.com> (raw)
In-Reply-To: <20240325125713.95035-1-wangzhaolong1@huawei.com>

在 2024/3/25 20:57, ZhaoLong Wang 写道:
> The UBIFS_DFS_DIR_LEN macro, which defines the maximum length of the UBIFS
> debugfs directory name, has an incorrect formula and misleading comments.
> The current formula is (3 + 1 + 2*2 + 1), which assumes that both UBI device
> number and volume ID are limited to 2 characters. However, UBI device number
> ranges from 0 to 37 (2 characters), and volume ID ranges from 0 to 127 (up
> to 3 characters).
> 
> Although the current code works due to the cancellation of mathematical
> errors (9 + 1 = 10, which matches the correct UBIFS_DFS_DIR_LEN value), it
> can lead to confusion and potential issues in the future.
> 
> This patch aims to improve the code clarity and maintainability by making
> the following changes:
> 
> 1. Corrects the UBIFS_DFS_DIR_LEN macro definition to (3 + 1 + 2 + 3 + 1),
>     accommodating the maximum lengths of both UBI device number and volume ID,
>     plus the separators and null terminator.
> 2. Updates the snprintf calls to use UBIFS_DFS_DIR_LEN instead of
>     UBIFS_DFS_DIR_LEN + 1, removing the unnecessary +1.
> 3. Modifies the error checks to compare against UBIFS_DFS_DIR_LEN using >=
>     instead of >, aligning with the corrected macro definition.
> 4. Removes the redundant +1 in the dfs_dir_name array definitions in ubi.h
>     and debug.h.
> 5. Renames the duplicated UBIFS_DFS_DIR_LEN and UBIFS_DFS_DIR_NAME macros in
>     the sysfs-related code (sysfs.c and ubifs.h) to UBIFS_SYSFS_DIR_LEN and
>     UBIFS_SYSFS_DIR_NAME, respectively, to differentiate their usage from the
>     debugfs constants.
> 
[...]
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 1f3ea879d93a..549e147c5ce5 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -159,10 +159,11 @@
>   
>   /*
>    * The UBIFS sysfs directory name pattern and maximum name length (3 for "ubi"
> - * + 1 for "_" and plus 2x2 for 2 UBI numbers and 1 for the trailing zero byte.
> + * + 1 for "_" and 2 for UBI device numbers and 3 for volume number and 1 for
> + * the trailing zero byte.
>    */
> -#define UBIFS_DFS_DIR_NAME "ubi%d_%d"
> -#define UBIFS_DFS_DIR_LEN  (3 + 1 + 2*2 + 1)
> +#define UBIFS_SYSFS_DIR_NAME "ubi%d_%d"
> +#define UBIFS_SYSFS_DIR_LEN  (3 + 1 + 2 + 3 + 1)

I don't thinks this renaming is necessary, just remove it from "ubifs.h" 
like v1 does.
>   
>   /*
>    * Lockdep classes for UBIFS inode @ui_mutex.
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/