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

From: "Pali Rohár" <pali@kernel.org>
To: Kari Argillander <kari.argillander@gmail.com>
Cc: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>,
	ntfs3@lists.linux.dev, Christoph Hellwig <hch@lst.de>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Matthew Wilcox <willy@infradead.org>,
	Christian Brauner <christian.brauner@ubuntu.com>
Subject: Re: [PATCH v3 8/9] fs/ntfs3: Rename mount option no_acl_rules > (no)acl_rules
Date: Sun, 29 Aug 2021 12:16:37 +0200	[thread overview]
Message-ID: <20210829101637.2w2cxrhsdlv44z5x@pali> (raw)
In-Reply-To: <20210829095614.50021-9-kari.argillander@gmail.com>

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;

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?

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.

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

> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> ---
>  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
> 