Re: [PATCH v3 8/9] fs/ntfs3: Rename mount option no_acl_rules > (no)acl_rules

[Kari Argillander]

On Sun, Aug 29, 2021 at 12:16:37PM +0200, Pali Rohár wrote:
> Hello!
> 
> On Sunday 29 August 2021 12:56:13 Kari Argillander wrote:
> > Rename mount option no_acl_rules to noacl_rules. This allow us to use
> > possibility to mount with options noacl_rules or acl_rules.
> 
> $commit_message =~ s/acl/acs/g;

Thanks.

> 
> Anyway, for me "noacs_rules" name looks strange. Underline is used as a
> word separator and so original name "no_acs_rules" looks better. But if
> you are going to remove first underline, why not then remove also the
> second one? So name would be "noacsrules" and better matches naming
> convention?

I agree. Now that you wrote it like that I see that is definitely
better.

> And I see that other filesystems have option 'mode' (e.g. iso9660, udf)
> whicha is basically superset of this no_acs_rules as it supports to set
> permission to also any other mode than 0777.

We also have umask, fmask and dmask. Isn't fmask=mode and dmask=dmode?

I have not tested these really, but my impression is that noacsrules
will kinda overwrite everything else. It can also lie to user because
user can change file permission, but it will not change in reality.

I'm not even sure when do we need this option. Konstantin can probably
enlighten us or at least me.

> Maybe this could be a good thing to unify across all filesystems in
> future...

Hopefully.

> 
> > Acked-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > Signed-off-by: Kari Argillander <kari.argillander@xxxxxxxxx>
> > ---
> >  Documentation/filesystems/ntfs3.rst |  2 +-
> >  fs/ntfs3/file.c                     |  2 +-
> >  fs/ntfs3/ntfs_fs.h                  |  2 +-
> >  fs/ntfs3/super.c                    | 12 ++++++------
> >  fs/ntfs3/xattr.c                    |  2 +-
> >  5 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/filesystems/ntfs3.rst b/Documentation/filesystems/ntfs3.rst
> > index ded706474825..bdc9dd5a9185 100644
> > --- a/Documentation/filesystems/ntfs3.rst
> > +++ b/Documentation/filesystems/ntfs3.rst
> > @@ -73,7 +73,7 @@ prealloc		Preallocate space for files excessively when file size is
> >  			increasing on writes. Decreases fragmentation in case of
> >  			parallel write operations to different files.
> >  
> > -no_acs_rules		"No access rules" mount option sets access rights for
> > +noacs_rules		"No access rules" mount option sets access rights for
> >  			files/folders to 777 and owner/group to root. This mount
> >  			option absorbs all other permissions:
> >  			- permissions change for files/folders will be reported
> > diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
> > index c79e4aff7a19..4c9ff7fcf0b1 100644
> > --- a/fs/ntfs3/file.c
> > +++ b/fs/ntfs3/file.c
> > @@ -743,7 +743,7 @@ int ntfs3_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
> >  	umode_t mode = inode->i_mode;
> >  	int err;
> >  
> > -	if (sbi->options->no_acs_rules) {
> > +	if (sbi->options->noacs_rules) {
> >  		/* "no access rules" - force any changes of time etc. */
> >  		attr->ia_valid |= ATTR_FORCE;
> >  		/* and disable for editing some attributes */
> > diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> > index 45d6f4f91222..5df55bc733bd 100644
> > --- a/fs/ntfs3/ntfs_fs.h
> > +++ b/fs/ntfs3/ntfs_fs.h
> > @@ -70,7 +70,7 @@ struct ntfs_mount_options {
> >  		showmeta : 1, /*show meta files*/
> >  		nohidden : 1, /*do not show hidden files*/
> >  		force : 1, /*rw mount dirty volume*/
> > -		no_acs_rules : 1, /*exclude acs rules*/
> > +		noacs_rules : 1, /*exclude acs rules*/
> >  		prealloc : 1 /*preallocate space when file is growing*/
> >  		;
> >  };
> > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> > index e5c319604c4d..d7408b4f6813 100644
> > --- a/fs/ntfs3/super.c
> > +++ b/fs/ntfs3/super.c
> > @@ -221,7 +221,7 @@ enum Opt {
> >  	Opt_acl,
> >  	Opt_iocharset,
> >  	Opt_prealloc,
> > -	Opt_no_acs_rules,
> > +	Opt_noacs_rules,
> >  	Opt_err,
> >  };
> >  
> > @@ -239,7 +239,7 @@ static const struct fs_parameter_spec ntfs_fs_parameters[] = {
> >  	fsparam_flag_no("acl",			Opt_acl),
> >  	fsparam_flag_no("showmeta",		Opt_showmeta),
> >  	fsparam_flag_no("prealloc",		Opt_prealloc),
> > -	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
> > +	fsparam_flag_no("acs_rules",		Opt_noacs_rules),
> >  	fsparam_string("iocharset",		Opt_iocharset),
> >  
> >  	__fsparam(fs_param_is_string,
> > @@ -351,8 +351,8 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
> >  	case Opt_prealloc:
> >  		opts->prealloc = result.negated ? 0 : 1;
> >  		break;
> > -	case Opt_no_acs_rules:
> > -		opts->no_acs_rules = 1;
> > +	case Opt_noacs_rules:
> > +		opts->noacs_rules = result.negated ? 1 : 0;
> >  		break;
> >  	default:
> >  		/* Should not be here unless we forget add case. */
> > @@ -538,8 +538,8 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
> >  		seq_puts(m, ",nohidden");
> >  	if (opts->force)
> >  		seq_puts(m, ",force");
> > -	if (opts->no_acs_rules)
> > -		seq_puts(m, ",no_acs_rules");
> > +	if (opts->noacs_rules)
> > +		seq_puts(m, ",noacs_rules");
> >  	if (opts->prealloc)
> >  		seq_puts(m, ",prealloc");
> >  	if (sb->s_flags & SB_POSIXACL)
> > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> > index a17d48735b99..4b37ed239579 100644
> > --- a/fs/ntfs3/xattr.c
> > +++ b/fs/ntfs3/xattr.c
> > @@ -774,7 +774,7 @@ int ntfs_acl_chmod(struct user_namespace *mnt_userns, struct inode *inode)
> >  int ntfs_permission(struct user_namespace *mnt_userns, struct inode *inode,
> >  		    int mask)
> >  {
> > -	if (ntfs_sb(inode->i_sb)->options->no_acs_rules) {
> > +	if (ntfs_sb(inode->i_sb)->options->noacs_rules) {
> >  		/* "no access rules" mode - allow all changes */
> >  		return 0;
> >  	}
> > -- 
> > 2.25.1
> > 
>