diff mbox series

[1/3] ceph: refactor mds_namespace comparing

Message ID TYCP286MB2066C89FCBD9FB30AFDF2D70C0709@TYCP286MB2066.JPNP286.PROD.OUTLOOK.COM (mailing list archive)
State New, archived
Headers show
Series ceph: account for name and fsid in new device spec | expand

Commit Message

胡玮文 May 7, 2023, 5:55 p.m. UTC 
From: Hu Weiwen <sehuww@mail.scut.edu.cn>

Same logic, slightly less code.  Make the following changes easier.

Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
---
 fs/ceph/super.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

Comments

Xiubo Li May 9, 2023, 1:04 a.m. UTC | #1 
On 5/8/23 01:55, Hu Weiwen wrote:
> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
>
> Same logic, slightly less code.  Make the following changes easier.
>
> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> ---
>   fs/ceph/super.c | 34 ++++++++++++++--------------------
>   1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 3fc48b43cab0..4e1f4031e888 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -235,18 +235,10 @@ static void canonicalize_path(char *path)
>   	path[j] = '\0';
>   }
>   
> -/*
> - * Check if the mds namespace in ceph_mount_options matches
> - * the passed in namespace string. First time match (when
> - * ->mds_namespace is NULL) is treated specially, since
> - * ->mds_namespace needs to be initialized by the caller.
> - */
> -static int namespace_equals(struct ceph_mount_options *fsopt,
> -			    const char *namespace, size_t len)
> +/* check if s1 (null terminated) equals to s2 (with length len2) */
> +static int strstrn_equals(const char *s1, const char *s2, size_t len2)
>   {
> -	return !(fsopt->mds_namespace &&
> -		 (strlen(fsopt->mds_namespace) != len ||
> -		  strncmp(fsopt->mds_namespace, namespace, len)));
> +	return !strncmp(s1, s2, len2) && strlen(s1) == len2;
>   }

Could this helper be defined as inline explicitly ?

>   
>   static int ceph_parse_old_source(const char *dev_name, const char *dev_name_end,
> @@ -297,12 +289,13 @@ static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
>   	++fs_name_start; /* start of file system name */
>   	len = dev_name_end - fs_name_start;
>   
> -	if (!namespace_equals(fsopt, fs_name_start, len))
> +	if (!fsopt->mds_namespace) {
> +		fsopt->mds_namespace = kstrndup(fs_name_start, len, GFP_KERNEL);
> +		if (!fsopt->mds_namespace)
> +			return -ENOMEM;
> +	} else if (!strstrn_equals(fsopt->mds_namespace, fs_name_start, len)) {
>   		return invalfc(fc, "Mismatching mds_namespace");
> -	kfree(fsopt->mds_namespace);
> -	fsopt->mds_namespace = kstrndup(fs_name_start, len, GFP_KERNEL);
> -	if (!fsopt->mds_namespace)
> -		return -ENOMEM;
> +	}
>   	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
>   
>   	fsopt->new_dev_syntax = true;
> @@ -417,11 +410,12 @@ static int ceph_parse_mount_param(struct fs_context *fc,
>   		param->string = NULL;
>   		break;
>   	case Opt_mds_namespace:
> -		if (!namespace_equals(fsopt, param->string, strlen(param->string)))
> +		if (!fsopt->mds_namespace) {
> +			fsopt->mds_namespace = param->string;
> +			param->string = NULL;
> +		} else if (strcmp(fsopt->mds_namespace, param->string)) {
>   			return invalfc(fc, "Mismatching mds_namespace");
> -		kfree(fsopt->mds_namespace);
> -		fsopt->mds_namespace = param->string;
> -		param->string = NULL;
> +		}
>   		break;
>   	case Opt_recover_session:
>   		mode = result.uint_32;
Xiubo Li May 9, 2023, 1:40 a.m. UTC | #2 
On 5/9/23 09:04, Xiubo Li wrote:
>
> On 5/8/23 01:55, Hu Weiwen wrote:
>> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
>>
>> Same logic, slightly less code.  Make the following changes easier.
>>
>> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
>> ---
>>   fs/ceph/super.c | 34 ++++++++++++++--------------------
>>   1 file changed, 14 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>> index 3fc48b43cab0..4e1f4031e888 100644
>> --- a/fs/ceph/super.c
>> +++ b/fs/ceph/super.c
>> @@ -235,18 +235,10 @@ static void canonicalize_path(char *path)
>>       path[j] = '\0';
>>   }
>>   -/*
>> - * Check if the mds namespace in ceph_mount_options matches
>> - * the passed in namespace string. First time match (when
>> - * ->mds_namespace is NULL) is treated specially, since
>> - * ->mds_namespace needs to be initialized by the caller.
>> - */
>> -static int namespace_equals(struct ceph_mount_options *fsopt,
>> -                const char *namespace, size_t len)
>> +/* check if s1 (null terminated) equals to s2 (with length len2) */
>> +static int strstrn_equals(const char *s1, const char *s2, size_t len2)
>>   {
>> -    return !(fsopt->mds_namespace &&
>> -         (strlen(fsopt->mds_namespace) != len ||
>> -          strncmp(fsopt->mds_namespace, namespace, len)));
>> +    return !strncmp(s1, s2, len2) && strlen(s1) == len2;
>>   }
>
> Could this helper be defined as inline explicitly ?
>
Please ignore this, I misreaded and it's not in the header file.


>>     static int ceph_parse_old_source(const char *dev_name, const char 
>> *dev_name_end,
>> @@ -297,12 +289,13 @@ static int ceph_parse_new_source(const char 
>> *dev_name, const char *dev_name_end,
>>       ++fs_name_start; /* start of file system name */
>>       len = dev_name_end - fs_name_start;
>>   -    if (!namespace_equals(fsopt, fs_name_start, len))
>> +    if (!fsopt->mds_namespace) {
>> +        fsopt->mds_namespace = kstrndup(fs_name_start, len, 
>> GFP_KERNEL);
>> +        if (!fsopt->mds_namespace)
>> +            return -ENOMEM;
>> +    } else if (!strstrn_equals(fsopt->mds_namespace, fs_name_start, 
>> len)) {
>>           return invalfc(fc, "Mismatching mds_namespace");
>> -    kfree(fsopt->mds_namespace);
>> -    fsopt->mds_namespace = kstrndup(fs_name_start, len, GFP_KERNEL);
>> -    if (!fsopt->mds_namespace)
>> -        return -ENOMEM;
>> +    }
>>       dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
>>         fsopt->new_dev_syntax = true;
>> @@ -417,11 +410,12 @@ static int ceph_parse_mount_param(struct 
>> fs_context *fc,
>>           param->string = NULL;
>>           break;
>>       case Opt_mds_namespace:
>> -        if (!namespace_equals(fsopt, param->string, 
>> strlen(param->string)))
>> +        if (!fsopt->mds_namespace) {
>> +            fsopt->mds_namespace = param->string;
>> +            param->string = NULL;
>> +        } else if (strcmp(fsopt->mds_namespace, param->string)) {
>>               return invalfc(fc, "Mismatching mds_namespace");
>> -        kfree(fsopt->mds_namespace);
>> -        fsopt->mds_namespace = param->string;
>> -        param->string = NULL;
>> +        }
>>           break;
>>       case Opt_recover_session:
>>           mode = result.uint_32;
Milind Changire June 5, 2023, 11:03 a.m. UTC | #3 
Looks good to me.

Reviewed-by: Milind Changire <mchangir@redhat.com>


On Tue, May 9, 2023 at 7:14 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 5/9/23 09:04, Xiubo Li wrote:
> >
> > On 5/8/23 01:55, Hu Weiwen wrote:
> >> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
> >>
> >> Same logic, slightly less code.  Make the following changes easier.
> >>
> >> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> >> ---
> >>   fs/ceph/super.c | 34 ++++++++++++++--------------------
> >>   1 file changed, 14 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> >> index 3fc48b43cab0..4e1f4031e888 100644
> >> --- a/fs/ceph/super.c
> >> +++ b/fs/ceph/super.c
> >> @@ -235,18 +235,10 @@ static void canonicalize_path(char *path)
> >>       path[j] = '\0';
> >>   }
> >>   -/*
> >> - * Check if the mds namespace in ceph_mount_options matches
> >> - * the passed in namespace string. First time match (when
> >> - * ->mds_namespace is NULL) is treated specially, since
> >> - * ->mds_namespace needs to be initialized by the caller.
> >> - */
> >> -static int namespace_equals(struct ceph_mount_options *fsopt,
> >> -                const char *namespace, size_t len)
> >> +/* check if s1 (null terminated) equals to s2 (with length len2) */
> >> +static int strstrn_equals(const char *s1, const char *s2, size_t len2)
> >>   {
> >> -    return !(fsopt->mds_namespace &&
> >> -         (strlen(fsopt->mds_namespace) != len ||
> >> -          strncmp(fsopt->mds_namespace, namespace, len)));
> >> +    return !strncmp(s1, s2, len2) && strlen(s1) == len2;
> >>   }
> >
> > Could this helper be defined as inline explicitly ?
> >
> Please ignore this, I misreaded and it's not in the header file.
>
>
> >>     static int ceph_parse_old_source(const char *dev_name, const char
> >> *dev_name_end,
> >> @@ -297,12 +289,13 @@ static int ceph_parse_new_source(const char
> >> *dev_name, const char *dev_name_end,
> >>       ++fs_name_start; /* start of file system name */
> >>       len = dev_name_end - fs_name_start;
> >>   -    if (!namespace_equals(fsopt, fs_name_start, len))
> >> +    if (!fsopt->mds_namespace) {
> >> +        fsopt->mds_namespace = kstrndup(fs_name_start, len,
> >> GFP_KERNEL);
> >> +        if (!fsopt->mds_namespace)
> >> +            return -ENOMEM;
> >> +    } else if (!strstrn_equals(fsopt->mds_namespace, fs_name_start,
> >> len)) {
> >>           return invalfc(fc, "Mismatching mds_namespace");
> >> -    kfree(fsopt->mds_namespace);
> >> -    fsopt->mds_namespace = kstrndup(fs_name_start, len, GFP_KERNEL);
> >> -    if (!fsopt->mds_namespace)
> >> -        return -ENOMEM;
> >> +    }
> >>       dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
> >>         fsopt->new_dev_syntax = true;
> >> @@ -417,11 +410,12 @@ static int ceph_parse_mount_param(struct
> >> fs_context *fc,
> >>           param->string = NULL;
> >>           break;
> >>       case Opt_mds_namespace:
> >> -        if (!namespace_equals(fsopt, param->string,
> >> strlen(param->string)))
> >> +        if (!fsopt->mds_namespace) {
> >> +            fsopt->mds_namespace = param->string;
> >> +            param->string = NULL;
> >> +        } else if (strcmp(fsopt->mds_namespace, param->string)) {
> >>               return invalfc(fc, "Mismatching mds_namespace");
> >> -        kfree(fsopt->mds_namespace);
> >> -        fsopt->mds_namespace = param->string;
> >> -        param->string = NULL;
> >> +        }
> >>           break;
> >>       case Opt_recover_session:
> >>           mode = result.uint_32;
>
diff mbox series

Patch

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 3fc48b43cab0..4e1f4031e888 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -235,18 +235,10 @@  static void canonicalize_path(char *path)
 	path[j] = '\0';
 }
 
-/*
- * Check if the mds namespace in ceph_mount_options matches
- * the passed in namespace string. First time match (when
- * ->mds_namespace is NULL) is treated specially, since
- * ->mds_namespace needs to be initialized by the caller.
- */
-static int namespace_equals(struct ceph_mount_options *fsopt,
-			    const char *namespace, size_t len)
+/* check if s1 (null terminated) equals to s2 (with length len2) */
+static int strstrn_equals(const char *s1, const char *s2, size_t len2)
 {
-	return !(fsopt->mds_namespace &&
-		 (strlen(fsopt->mds_namespace) != len ||
-		  strncmp(fsopt->mds_namespace, namespace, len)));
+	return !strncmp(s1, s2, len2) && strlen(s1) == len2;
 }
 
 static int ceph_parse_old_source(const char *dev_name, const char *dev_name_end,
@@ -297,12 +289,13 @@  static int ceph_parse_new_source(const char *dev_name, const char *dev_name_end,
 	++fs_name_start; /* start of file system name */
 	len = dev_name_end - fs_name_start;
 
-	if (!namespace_equals(fsopt, fs_name_start, len))
+	if (!fsopt->mds_namespace) {
+		fsopt->mds_namespace = kstrndup(fs_name_start, len, GFP_KERNEL);
+		if (!fsopt->mds_namespace)
+			return -ENOMEM;
+	} else if (!strstrn_equals(fsopt->mds_namespace, fs_name_start, len)) {
 		return invalfc(fc, "Mismatching mds_namespace");
-	kfree(fsopt->mds_namespace);
-	fsopt->mds_namespace = kstrndup(fs_name_start, len, GFP_KERNEL);
-	if (!fsopt->mds_namespace)
-		return -ENOMEM;
+	}
 	dout("file system (mds namespace) '%s'\n", fsopt->mds_namespace);
 
 	fsopt->new_dev_syntax = true;
@@ -417,11 +410,12 @@  static int ceph_parse_mount_param(struct fs_context *fc,
 		param->string = NULL;
 		break;
 	case Opt_mds_namespace:
-		if (!namespace_equals(fsopt, param->string, strlen(param->string)))
+		if (!fsopt->mds_namespace) {
+			fsopt->mds_namespace = param->string;
+			param->string = NULL;
+		} else if (strcmp(fsopt->mds_namespace, param->string)) {
 			return invalfc(fc, "Mismatching mds_namespace");
-		kfree(fsopt->mds_namespace);
-		fsopt->mds_namespace = param->string;
-		param->string = NULL;
+		}
 		break;
 	case Opt_recover_session:
 		mode = result.uint_32;