| Message ID | TYCP286MB2066D19A68A9176E289BB4FDC0709@TYCP286MB2066.JPNP286.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: account for name and fsid in new device spec | expand |
On 5/8/23 01:55, Hu Weiwen wrote: > From: Hu Weiwen <sehuww@mail.scut.edu.cn> > > These are present in the device spec of cephfs. So they should be > treated as immutable. Also reject `mount()' calls where options and > device spec are inconsistent. > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > --- > net/ceph/ceph_common.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c > index 4c6441536d55..c59c5ccc23a8 100644 > --- a/net/ceph/ceph_common.c > +++ b/net/ceph/ceph_common.c > @@ -440,17 +440,33 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt, > break; > > case Opt_fsid: > - err = ceph_parse_fsid(param->string, &opt->fsid); > + { BTW, do we need the '{}' here ? > + struct ceph_fsid fsid; > + > + err = ceph_parse_fsid(param->string, &fsid); > if (err) { > error_plog(&log, "Failed to parse fsid: %d", err); > return err; > } > - opt->flags |= CEPH_OPT_FSID; > + > + if (!(opt->flags & CEPH_OPT_FSID)) { > + opt->fsid = fsid; > + opt->flags |= CEPH_OPT_FSID; > + } else if (ceph_fsid_compare(&opt->fsid, &fsid)) { > + error_plog(&log, "fsid already set to %pU", > + &opt->fsid); > + return -EINVAL; > + } > break; > + } > case Opt_name: > - kfree(opt->name); > - opt->name = param->string; > - param->string = NULL; > + if (!opt->name) { > + opt->name = param->string; > + param->string = NULL; > + } else if (strcmp(opt->name, param->string)) { > + error_plog(&log, "name already set to %s", opt->name); > + return -EINVAL; > + } > break; > case Opt_secret: > ceph_crypto_key_destroy(opt->key);
On Wed, May 10, 2023 at 03:02:09PM +0800, Xiubo Li wrote: > > On 5/8/23 01:55, Hu Weiwen wrote: > > From: Hu Weiwen <sehuww@mail.scut.edu.cn> > > > > These are present in the device spec of cephfs. So they should be > > treated as immutable. Also reject `mount()' calls where options and > > device spec are inconsistent. > > > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > > --- > > net/ceph/ceph_common.c | 26 +++++++++++++++++++++----- > > 1 file changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c > > index 4c6441536d55..c59c5ccc23a8 100644 > > --- a/net/ceph/ceph_common.c > > +++ b/net/ceph/ceph_common.c > > @@ -440,17 +440,33 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt, > > break; > > case Opt_fsid: > > - err = ceph_parse_fsid(param->string, &opt->fsid); > > + { > > BTW, do we need the '{}' here ? I want to declare 'fsid' variable closer to its usage. But a declaration cannot follow a case label: error: a label can only be part of a statement and a declaration is not a statement searching for 'case \w+:\n\s+\{' in the source tree reveals about 1400 such usage. Should be pretty common. > > + struct ceph_fsid fsid; > > + > > + err = ceph_parse_fsid(param->string, &fsid); > > if (err) { > > error_plog(&log, "Failed to parse fsid: %d", err); > > return err; > > } > > - opt->flags |= CEPH_OPT_FSID; > > + > > + if (!(opt->flags & CEPH_OPT_FSID)) { > > + opt->fsid = fsid; > > + opt->flags |= CEPH_OPT_FSID; > > + } else if (ceph_fsid_compare(&opt->fsid, &fsid)) { > > + error_plog(&log, "fsid already set to %pU", > > + &opt->fsid); > > + return -EINVAL; > > + } > > break; > > + } > > case Opt_name: > > - kfree(opt->name); > > - opt->name = param->string; > > - param->string = NULL; > > + if (!opt->name) { > > + opt->name = param->string; > > + param->string = NULL; > > + } else if (strcmp(opt->name, param->string)) { > > + error_plog(&log, "name already set to %s", opt->name); > > + return -EINVAL; > > + } > > break; > > case Opt_secret: > > ceph_crypto_key_destroy(opt->key); >
On 5/10/23 16:44, 胡玮文 wrote: > On Wed, May 10, 2023 at 03:02:09PM +0800, Xiubo Li wrote: >> On 5/8/23 01:55, Hu Weiwen wrote: >>> From: Hu Weiwen <sehuww@mail.scut.edu.cn> >>> >>> These are present in the device spec of cephfs. So they should be >>> treated as immutable. Also reject `mount()' calls where options and >>> device spec are inconsistent. >>> >>> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> >>> --- >>> net/ceph/ceph_common.c | 26 +++++++++++++++++++++----- >>> 1 file changed, 21 insertions(+), 5 deletions(-) >>> >>> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c >>> index 4c6441536d55..c59c5ccc23a8 100644 >>> --- a/net/ceph/ceph_common.c >>> +++ b/net/ceph/ceph_common.c >>> @@ -440,17 +440,33 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt, >>> break; >>> case Opt_fsid: >>> - err = ceph_parse_fsid(param->string, &opt->fsid); >>> + { >> BTW, do we need the '{}' here ? > I want to declare 'fsid' variable closer to its usage. But a declaration > cannot follow a case label: > > error: a label can only be part of a statement and a declaration is not a statement > > searching for 'case \w+:\n\s+\{' in the source tree reveals about 1400 > such usage. Should be pretty common. Did you see this when compiling ? So odd I jsut remove them and it worked for me. >>> + struct ceph_fsid fsid; >>> + >>> + err = ceph_parse_fsid(param->string, &fsid); >>> if (err) { >>> error_plog(&log, "Failed to parse fsid: %d", err); >>> return err; >>> } >>> - opt->flags |= CEPH_OPT_FSID; >>> + >>> + if (!(opt->flags & CEPH_OPT_FSID)) { >>> + opt->fsid = fsid; >>> + opt->flags |= CEPH_OPT_FSID; >>> + } else if (ceph_fsid_compare(&opt->fsid, &fsid)) { >>> + error_plog(&log, "fsid already set to %pU", >>> + &opt->fsid); >>> + return -EINVAL; >>> + } >>> break; >>> + } >>> case Opt_name: >>> - kfree(opt->name); >>> - opt->name = param->string; >>> - param->string = NULL; >>> + if (!opt->name) { >>> + opt->name = param->string; >>> + param->string = NULL; >>> + } else if (strcmp(opt->name, param->string)) { >>> + error_plog(&log, "name already set to %s", opt->name); >>> + return -EINVAL; >>> + } >>> break; >>> case Opt_secret: >>> ceph_crypto_key_destroy(opt->key);
On Wed, May 10, 2023 at 06:44:22PM +0800, Xiubo Li wrote: > > On 5/10/23 16:44, 胡玮文 wrote: > > On Wed, May 10, 2023 at 03:02:09PM +0800, Xiubo Li wrote: > > > On 5/8/23 01:55, Hu Weiwen wrote: > > > > From: Hu Weiwen <sehuww@mail.scut.edu.cn> > > > > > > > > These are present in the device spec of cephfs. So they should be > > > > treated as immutable. Also reject `mount()' calls where options and > > > > device spec are inconsistent. > > > > > > > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > > > > --- > > > > net/ceph/ceph_common.c | 26 +++++++++++++++++++++----- > > > > 1 file changed, 21 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c > > > > index 4c6441536d55..c59c5ccc23a8 100644 > > > > --- a/net/ceph/ceph_common.c > > > > +++ b/net/ceph/ceph_common.c > > > > @@ -440,17 +440,33 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt, > > > > break; > > > > case Opt_fsid: > > > > - err = ceph_parse_fsid(param->string, &opt->fsid); > > > > + { > > > BTW, do we need the '{}' here ? > > I want to declare 'fsid' variable closer to its usage. But a declaration > > cannot follow a case label: > > error: a label can only be part of a statement and a declaration is not a statement > > > > searching for 'case \w+:\n\s+\{' in the source tree reveals about 1400 > > such usage. Should be pretty common. > > Did you see this when compiling ? So odd I jsut remove them and it worked > for me. Yes, my compiler is gcc version 9.4.0 from ubuntu 20.04. clangd 16.0.2 also gives error. Console output: CC net/ceph/ceph_common.o net/ceph/ceph_common.c: In function ‘ceph_parse_param’: net/ceph/ceph_common.c:443:3: error: a label can only be part of a statement and a declaration is not a statement 443 | struct ceph_fsid fsid; | ^~~~~~ > > > > + struct ceph_fsid fsid; > > > > + > > > > + err = ceph_parse_fsid(param->string, &fsid); > > > > if (err) { > > > > error_plog(&log, "Failed to parse fsid: %d", err); > > > > return err; > > > > } > > > > - opt->flags |= CEPH_OPT_FSID; > > > > + > > > > + if (!(opt->flags & CEPH_OPT_FSID)) { > > > > + opt->fsid = fsid; > > > > + opt->flags |= CEPH_OPT_FSID; > > > > + } else if (ceph_fsid_compare(&opt->fsid, &fsid)) { > > > > + error_plog(&log, "fsid already set to %pU", > > > > + &opt->fsid); > > > > + return -EINVAL; > > > > + } > > > > break; > > > > + } > > > > case Opt_name: > > > > - kfree(opt->name); > > > > - opt->name = param->string; > > > > - param->string = NULL; > > > > + if (!opt->name) { > > > > + opt->name = param->string; > > > > + param->string = NULL; > > > > + } else if (strcmp(opt->name, param->string)) { > > > > + error_plog(&log, "name already set to %s", opt->name); > > > > + return -EINVAL; > > > > + } > > > > break; > > > > case Opt_secret: > > > > ceph_crypto_key_destroy(opt->key); >
On Sun, May 7, 2023 at 7:56 PM Hu Weiwen <huww98@outlook.com> wrote: > > From: Hu Weiwen <sehuww@mail.scut.edu.cn> > > These are present in the device spec of cephfs. So they should be > treated as immutable. Also reject `mount()' calls where options and > device spec are inconsistent. > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > --- > net/ceph/ceph_common.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c > index 4c6441536d55..c59c5ccc23a8 100644 > --- a/net/ceph/ceph_common.c > +++ b/net/ceph/ceph_common.c > @@ -440,17 +440,33 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt, > break; > > case Opt_fsid: > - err = ceph_parse_fsid(param->string, &opt->fsid); > + { > + struct ceph_fsid fsid; > + > + err = ceph_parse_fsid(param->string, &fsid); > if (err) { > error_plog(&log, "Failed to parse fsid: %d", err); > return err; > } > - opt->flags |= CEPH_OPT_FSID; > + > + if (!(opt->flags & CEPH_OPT_FSID)) { > + opt->fsid = fsid; > + opt->flags |= CEPH_OPT_FSID; > + } else if (ceph_fsid_compare(&opt->fsid, &fsid)) { > + error_plog(&log, "fsid already set to %pU", > + &opt->fsid); > + return -EINVAL; > + } > break; > + } > case Opt_name: > - kfree(opt->name); > - opt->name = param->string; > - param->string = NULL; > + if (!opt->name) { > + opt->name = param->string; > + param->string = NULL; > + } else if (strcmp(opt->name, param->string)) { > + error_plog(&log, "name already set to %s", opt->name); > + return -EINVAL; > + } > break; > case Opt_secret: > ceph_crypto_key_destroy(opt->key); > -- > 2.25.1 > Hi Hu, I'm not following the reason for singling out "fsid" and "name" in ceph_parse_param() in net/ceph. All options are overridable in the sense that the last setting wins on purpose, this is a long-standing behavior. Allowing "key" or "secret" to be overridden while rejecting the corresponding override for "name" is weird. If there is a desire to treat "fsid" and "name" specially in CephFS because they are coming from the device spec and aren't considered to be mount options in the usual sense, I would suggest enforcing this behavior in fs/ceph (and only when the device spec syntax is used). Thanks, Ilya
On Tue, Jun 27, 2023 at 01:47:53PM +0200, Ilya Dryomov wrote: > On Sun, May 7, 2023 at 7:56 PM Hu Weiwen <huww98@outlook.com> wrote: > > > > From: Hu Weiwen <sehuww@mail.scut.edu.cn> > > > > These are present in the device spec of cephfs. So they should be > > treated as immutable. Also reject `mount()' calls where options and > > device spec are inconsistent. > > > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > > --- > > net/ceph/ceph_common.c | 26 +++++++++++++++++++++----- > > 1 file changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c > > index 4c6441536d55..c59c5ccc23a8 100644 > > --- a/net/ceph/ceph_common.c > > +++ b/net/ceph/ceph_common.c > > @@ -440,17 +440,33 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt, > > break; > > > > case Opt_fsid: > > - err = ceph_parse_fsid(param->string, &opt->fsid); > > + { > > + struct ceph_fsid fsid; > > + > > + err = ceph_parse_fsid(param->string, &fsid); > > if (err) { > > error_plog(&log, "Failed to parse fsid: %d", err); > > return err; > > } > > - opt->flags |= CEPH_OPT_FSID; > > + > > + if (!(opt->flags & CEPH_OPT_FSID)) { > > + opt->fsid = fsid; > > + opt->flags |= CEPH_OPT_FSID; > > + } else if (ceph_fsid_compare(&opt->fsid, &fsid)) { > > + error_plog(&log, "fsid already set to %pU", > > + &opt->fsid); > > + return -EINVAL; > > + } > > break; > > + } > > case Opt_name: > > - kfree(opt->name); > > - opt->name = param->string; > > - param->string = NULL; > > + if (!opt->name) { > > + opt->name = param->string; > > + param->string = NULL; > > + } else if (strcmp(opt->name, param->string)) { > > + error_plog(&log, "name already set to %s", opt->name); > > + return -EINVAL; > > + } > > break; > > case Opt_secret: > > ceph_crypto_key_destroy(opt->key); > > -- > > 2.25.1 > > > > Hi Hu, > > I'm not following the reason for singling out "fsid" and "name" in > ceph_parse_param() in net/ceph. All options are overridable in the > sense that the last setting wins on purpose, this is a long-standing > behavior. Allowing "key" or "secret" to be overridden while rejecting > the corresponding override for "name" is weird. > > If there is a desire to treat "fsid" and "name" specially in CephFS > because they are coming from the device spec and aren't considered to > be mount options in the usual sense, I would suggest enforcing this > behavior in fs/ceph (and only when the device spec syntax is used). > > Thanks, > > Ilya Hi Ilya, I agree. Thank you for your advise. Please see my patch v2. Hu Weiwen
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c index 4c6441536d55..c59c5ccc23a8 100644 --- a/net/ceph/ceph_common.c +++ b/net/ceph/ceph_common.c @@ -440,17 +440,33 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt, break; case Opt_fsid: - err = ceph_parse_fsid(param->string, &opt->fsid); + { + struct ceph_fsid fsid; + + err = ceph_parse_fsid(param->string, &fsid); if (err) { error_plog(&log, "Failed to parse fsid: %d", err); return err; } - opt->flags |= CEPH_OPT_FSID; + + if (!(opt->flags & CEPH_OPT_FSID)) { + opt->fsid = fsid; + opt->flags |= CEPH_OPT_FSID; + } else if (ceph_fsid_compare(&opt->fsid, &fsid)) { + error_plog(&log, "fsid already set to %pU", + &opt->fsid); + return -EINVAL; + } break; + } case Opt_name: - kfree(opt->name); - opt->name = param->string; - param->string = NULL; + if (!opt->name) { + opt->name = param->string; + param->string = NULL; + } else if (strcmp(opt->name, param->string)) { + error_plog(&log, "name already set to %s", opt->name); + return -EINVAL; + } break; case Opt_secret: ceph_crypto_key_destroy(opt->key);