[PATCH v4 0/3] staging: erofs: option validation for remount and some code cleanups

[PATCH v4 0/3] staging: erofs: option validation for remount and some code cleanups

[Chengguang Xu]

Hi Greg, Xiang

I rebased code on latest erofs-master branch and that branch
has already merged the first patch in my previous patchset,
so this time I only post rest 3 patches.

Thanks,

--

This patch set mainly does option validation for remount and at
the same time does related code cleanups. Currently when we call
fault injection related code we have to surround it with macro
CONFIG_EROFS_FAULT_INJECTION in every calling place, after this
patch set we don't have to do that, so the code looks clean and
more understandable.

v3->v4:
- Rebase code on latest erofs-master branch in Chao's linux tree.
- Fix checkpatch complains.

v2->v3:
- Fold related patches to one patch.
- Fix building issue.

v1->v2:
Address Chao's comments:
- Allow to set fault_injection=0.
- Keep flag bit when setting fault_injection=0.
- Show injection info in original place.
- Rebase code on latest erofs branch in Chao's linux tree.
- Fix building issue.

Chengguang Xu (3):
  staging: erofs: code cleanup for option parsing of fault_injection
  staging: erofs: code cleanup for erofs_show_options()
  staging: erofs: option validation in remount

 drivers/staging/erofs/super.c | 73 +++++++++++++++++++++++++++--------
 1 file changed, 57 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH v4 1/3] staging: erofs: code cleanup for option parsing of fault_injection

[Chengguang Xu]

Define a dummpy function of erofs_build_fault_attr() when macro
CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
check the macro in calling place. Based on above adjustment,
do proper code cleanup for option parsing of fault_injection.

Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Reviewed-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/super.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 802202ca7213..d6c6ccc4936d 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -145,10 +145,14 @@ char *erofs_fault_name[FAULT_MAX] = {
 	[FAULT_KMALLOC]		= "kmalloc",
 };
 
-static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
-						unsigned int rate)
+static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
+				  substring_t *args)
 {
 	struct erofs_fault_info *ffi = &sbi->fault_info;
+	int rate = 0;
+
+	if (args->from && match_int(args, &rate))
+		return -EINVAL;
 
 	if (rate) {
 		atomic_set(&ffi->inject_ops, 0);
@@ -157,6 +161,16 @@ static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
 	} else {
 		memset(ffi, 0, sizeof(struct erofs_fault_info));
 	}
+
+	set_opt(sbi, FAULT_INJECTION);
+	return 0;
+}
+#else
+static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
+				  substring_t *args)
+{
+	infoln("fault_injection options not supported");
+	return 0;
 }
 #endif
 
@@ -193,7 +207,7 @@ static int parse_options(struct super_block *sb, char *options)
 {
 	substring_t args[MAX_OPT_ARGS];
 	char *p;
-	int arg = 0;
+	int err;
 
 	if (!options)
 		return 0;
@@ -238,18 +252,12 @@ static int parse_options(struct super_block *sb, char *options)
 			infoln("noacl options not supported");
 			break;
 #endif
-#ifdef CONFIG_EROFS_FAULT_INJECTION
 		case Opt_fault_injection:
-			if (args->from && match_int(args, &arg))
-				return -EINVAL;
-			erofs_build_fault_attr(EROFS_SB(sb), arg);
-			set_opt(EROFS_SB(sb), FAULT_INJECTION);
+			err = erofs_build_fault_attr(EROFS_SB(sb), args);
+			if (err)
+				return err;
 			break;
-#else
-		case Opt_fault_injection:
-			infoln("fault_injection options not supported");
-			break;
-#endif
+
 		default:
 			errln("Unrecognized mount option \"%s\" "
 					"or missing value", p);
-- 
2.17.1

[PATCH v4 2/3] staging: erofs: code cleanup for erofs_show_options()

[Chengguang Xu]

Add new helper erofs_get_fault_rate() to get fault rate instead of
directly getting it from sbi, so we can remove the macro check
surrounding it.

Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Reviewed-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/super.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index d6c6ccc4936d..720436d082f7 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -165,6 +165,11 @@ static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
 	set_opt(sbi, FAULT_INJECTION);
 	return 0;
 }
+
+static unsigned int erofs_get_fault_rate(struct erofs_sb_info *sbi)
+{
+	return sbi->fault_info.inject_rate;
+}
 #else
 static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
 				  substring_t *args)
@@ -172,6 +177,11 @@ static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
 	infoln("fault_injection options not supported");
 	return 0;
 }
+
+static unsigned int erofs_get_fault_rate(struct erofs_sb_info *sbi)
+{
+	return 0;
+}
 #endif
 
 static void default_options(struct erofs_sb_info *sbi)
@@ -626,11 +636,9 @@ static int erofs_show_options(struct seq_file *seq, struct dentry *root)
 	else
 		seq_puts(seq, ",noacl");
 #endif
-#ifdef CONFIG_EROFS_FAULT_INJECTION
 	if (test_opt(sbi, FAULT_INJECTION))
 		seq_printf(seq, ",fault_injection=%u",
-				sbi->fault_info.inject_rate);
-#endif
+			erofs_get_fault_rate(sbi));
 	return 0;
 }
 
-- 
2.17.1

[PATCH v4 3/3] staging: erofs: option validation in remount

[Chengguang Xu]

Add option validation in remount. After this patch, remount
can change recognized options, and for unknown options remount
will fail and report error.

Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Reviewed-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/super.c | 37 +++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 720436d082f7..880d01f857ca 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -145,14 +145,10 @@ char *erofs_fault_name[FAULT_MAX] = {
 	[FAULT_KMALLOC]		= "kmalloc",
 };
 
-static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
-				  substring_t *args)
+static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
+				     unsigned int rate)
 {
 	struct erofs_fault_info *ffi = &sbi->fault_info;
-	int rate = 0;
-
-	if (args->from && match_int(args, &rate))
-		return -EINVAL;
 
 	if (rate) {
 		atomic_set(&ffi->inject_ops, 0);
@@ -163,6 +159,17 @@ static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
 	}
 
 	set_opt(sbi, FAULT_INJECTION);
+}
+
+static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
+				  substring_t *args)
+{
+	int rate = 0;
+
+	if (args->from && match_int(args, &rate))
+		return -EINVAL;
+
+	__erofs_build_fault_attr(sbi, rate);
 	return 0;
 }
 
@@ -171,6 +178,11 @@ static unsigned int erofs_get_fault_rate(struct erofs_sb_info *sbi)
 	return sbi->fault_info.inject_rate;
 }
 #else
+static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
+				     unsigned int rate)
+{
+}
+
 static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
 				  substring_t *args)
 {
@@ -644,10 +656,23 @@ static int erofs_show_options(struct seq_file *seq, struct dentry *root)
 
 static int erofs_remount(struct super_block *sb, int *flags, char *data)
 {
+	struct erofs_sb_info *sbi = EROFS_SB(sb);
+	unsigned int org_mnt_opt = sbi->mount_opt;
+	unsigned int org_inject_rate = erofs_get_fault_rate(sbi);
+	int err;
+
 	BUG_ON(!sb_rdonly(sb));
+	err = parse_options(sb, data);
+	if (err)
+		goto out;
 
 	*flags |= SB_RDONLY;
 	return 0;
+out:
+	__erofs_build_fault_attr(sbi, org_inject_rate);
+	sbi->mount_opt = org_mnt_opt;
+
+	return err;
 }
 
 const struct super_operations erofs_sops = {
-- 
2.17.1

[PATCH v4 0/3] staging: erofs: option validation for remount and some code cleanups

[Gao Xiang]

Hi Chengguang,

On 2018/9/19 22:53, Chengguang Xu wrote:
> Hi Greg, Xiang
> 
> I rebased code on latest erofs-master branch and that branch
> has already merged the first patch in my previous patchset,
> so this time I only post rest 3 patches.

Great, at the most time Chao's erofs-master is the same as Greg's
staging tree (currently it is), but I personally think all patches
could be better directly based on the final staging tree if these
patches has no real/effective dependency with some submitted patches
but haven't been applied by Greg (p.s. it should be avoided as
much as possible because community guys could find something
important like the yesterday patches).

Once again, the detail information of branches is described in
  https://lists.ozlabs.org/listinfo/linux-erofs/

I have nothing more to say, good luck :)

Thanks,
Gao Xiang

> 
> Thanks,
> 
> --
> 
> This patch set mainly does option validation for remount and at
> the same time does related code cleanups. Currently when we call
> fault injection related code we have to surround it with macro
> CONFIG_EROFS_FAULT_INJECTION in every calling place, after this
> patch set we don't have to do that, so the code looks clean and
> more understandable.
> 
> v3->v4:
> - Rebase code on latest erofs-master branch in Chao's linux tree.
> - Fix checkpatch complains.
> 
> v2->v3:
> - Fold related patches to one patch.
> - Fix building issue.
> 
> v1->v2:
> Address Chao's comments:
> - Allow to set fault_injection=0.
> - Keep flag bit when setting fault_injection=0.
> - Show injection info in original place.
> - Rebase code on latest erofs branch in Chao's linux tree.
> - Fix building issue.
> 
> Chengguang Xu (3):
>   staging: erofs: code cleanup for option parsing of fault_injection
>   staging: erofs: code cleanup for erofs_show_options()
>   staging: erofs: option validation in remount
> 
>  drivers/staging/erofs/super.c | 73 +++++++++++++++++++++++++++--------
>  1 file changed, 57 insertions(+), 16 deletions(-)
> 

[PATCH v4 0/3] staging: erofs: option validation for remount and some code cleanups

[cgxu519]

On 9/19/18 11:22 PM, Gao Xiang wrote:
> Hi Chengguang,
>
> On 2018/9/19 22:53, Chengguang Xu wrote:
>> Hi Greg, Xiang
>>
>> I rebased code on latest erofs-master branch and that branch
>> has already merged the first patch in my previous patchset,
>> so this time I only post rest 3 patches.
> Great, at the most time Chao's erofs-master is the same as Greg's
> staging tree (currently it is), but I personally think all patches
> could be better directly based on the final staging tree if these
> patches has no real/effective dependency with some submitted patches
> but haven't been applied by Greg (p.s. it should be avoided as
> much as possible because community guys could find something
> important like the yesterday patches).
>
> Once again, the detail information of branches is described in
>    https://lists.ozlabs.org/listinfo/linux-erofs/
>
> I have nothing more to say, good luck :)
>
Thanks for the guide!

Chengguang

[PATCH v4 0/3] staging: erofs: option validation for remount and some code cleanups

[Chao Yu]

On 2018/9/20 7:34, cgxu519 wrote:
> On 9/19/18 11:22 PM, Gao Xiang wrote:
>> Hi Chengguang,
>>
>> On 2018/9/19 22:53, Chengguang Xu wrote:
>>> Hi Greg, Xiang
>>>
>>> I rebased code on latest erofs-master branch and that branch
>>> has already merged the first patch in my previous patchset,
>>> so this time I only post rest 3 patches.
>> Great, at the most time Chao's erofs-master is the same as Greg's
>> staging tree (currently it is), but I personally think all patches
>> could be better directly based on the final staging tree if these
>> patches has no real/effective dependency with some submitted patches
>> but haven't been applied by Greg (p.s. it should be avoided as
>> much as possible because community guys could find something
>> important like the yesterday patches).
>>
>> Once again, the detail information of branches is described in
>>    https://lists.ozlabs.org/listinfo/linux-erofs/

Yeah, I can confirm that's all true. :)

>>
>> I have nothing more to say, good luck :)
>>
> Thanks for the guide!
> 
> Chengguang
> 
> .
>