diff mbox series

[RFC,05/20] ntfs: Undeprecate iocharset= mount option

Message ID 20210808162453.1653-6-pali@kernel.org (mailing list archive)
State New, archived
Headers show
Series fs: Remove usage of broken nls_utf8 and drop it | expand

Commit Message

Pali Rohár Aug. 8, 2021, 4:24 p.m. UTC
Other fs drivers are using iocharset= mount option for specifying charset.
So mark iocharset= mount option as preferred and deprecate nls= mount
option.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/ntfs/inode.c  |  2 +-
 fs/ntfs/super.c  | 13 ++++---------
 fs/ntfs/unistr.c |  3 ++-
 3 files changed, 7 insertions(+), 11 deletions(-)

Comments

Kari Argillander Aug. 9, 2021, 8:52 p.m. UTC | #1
On Sun, Aug 08, 2021 at 06:24:38PM +0200, Pali Rohár wrote:
> Other fs drivers are using iocharset= mount option for specifying charset.
> So mark iocharset= mount option as preferred and deprecate nls= mount
> option.

Documentation needs to also be updated here.
Kari Argillander Aug. 19, 2021, 1:21 a.m. UTC | #2
On Sun, Aug 08, 2021 at 06:24:38PM +0200, Pali Rohár wrote:
> Other fs drivers are using iocharset= mount option for specifying charset.
> So mark iocharset= mount option as preferred and deprecate nls= mount
> option.
 
One idea is also make this change to fs/fc_parser.c and then when we
want we can drop support from all filesystem same time. This way we
can get more deprecated code off the fs drivers. Draw back is that
then every filesstem has this deprecated nls= option if it support
iocharsets option. But that should imo be ok.
Pali Rohár Aug. 19, 2021, 8:12 a.m. UTC | #3
On Thursday 19 August 2021 04:21:08 Kari Argillander wrote:
> On Sun, Aug 08, 2021 at 06:24:38PM +0200, Pali Rohár wrote:
> > Other fs drivers are using iocharset= mount option for specifying charset.
> > So mark iocharset= mount option as preferred and deprecate nls= mount
> > option.
>  
> One idea is also make this change to fs/fc_parser.c and then when we
> want we can drop support from all filesystem same time. This way we
> can get more deprecated code off the fs drivers. Draw back is that
> then every filesstem has this deprecated nls= option if it support
> iocharsets option. But that should imo be ok.

Beware that iocharset= is required only for fs which store filenames in
some specific encoding (in this case extension to UTF-16). For fs which
store filenames in raw bytes this option should not be parsed at all.

Therefore I'm not sure if this parsing should be in global
fs/fc_parser.c file...
Kari Argillander Aug. 19, 2021, 10:23 a.m. UTC | #4
On Thu, Aug 19, 2021 at 10:12:22AM +0200, Pali Rohár wrote:
> On Thursday 19 August 2021 04:21:08 Kari Argillander wrote:
> > On Sun, Aug 08, 2021 at 06:24:38PM +0200, Pali Rohár wrote:
> > > Other fs drivers are using iocharset= mount option for specifying charset.
> > > So mark iocharset= mount option as preferred and deprecate nls= mount
> > > option.
> >  
> > One idea is also make this change to fs/fc_parser.c and then when we
> > want we can drop support from all filesystem same time. This way we
> > can get more deprecated code off the fs drivers. Draw back is that
> > then every filesstem has this deprecated nls= option if it support
> > iocharsets option. But that should imo be ok.
> 
> Beware that iocharset= is required only for fs which store filenames in
> some specific encoding (in this case extension to UTF-16). For fs which
> store filenames in raw bytes this option should not be parsed at all.

Yeah of course. I was thinking that what we do is that if key is nls=
we change key to iocharset, print deprecated and then send it to driver
parser as usual. This way driver parser will never know that user
specifie nls= because it just get iocharset. But this is probebly too
fancy way to think simple problem. Just idea. 

> Therefore I'm not sure if this parsing should be in global
> fs/fc_parser.c file...
Pali Rohár Aug. 19, 2021, 10:04 p.m. UTC | #5
On Thursday 19 August 2021 13:23:42 Kari Argillander wrote:
> On Thu, Aug 19, 2021 at 10:12:22AM +0200, Pali Rohár wrote:
> > On Thursday 19 August 2021 04:21:08 Kari Argillander wrote:
> > > On Sun, Aug 08, 2021 at 06:24:38PM +0200, Pali Rohár wrote:
> > > > Other fs drivers are using iocharset= mount option for specifying charset.
> > > > So mark iocharset= mount option as preferred and deprecate nls= mount
> > > > option.
> > >  
> > > One idea is also make this change to fs/fc_parser.c and then when we
> > > want we can drop support from all filesystem same time. This way we
> > > can get more deprecated code off the fs drivers. Draw back is that
> > > then every filesstem has this deprecated nls= option if it support
> > > iocharsets option. But that should imo be ok.
> > 
> > Beware that iocharset= is required only for fs which store filenames in
> > some specific encoding (in this case extension to UTF-16). For fs which
> > store filenames in raw bytes this option should not be parsed at all.
> 
> Yeah of course. I was thinking that what we do is that if key is nls=
> we change key to iocharset, print deprecated and then send it to driver
> parser as usual. This way driver parser will never know that user
> specifie nls= because it just get iocharset. But this is probebly too
> fancy way to think simple problem. Just idea. 

This has an issue that when you use nls= option for e.g. ext4 fs then
kernel starts reporting that nls= for ext4 is deprecated. But there is
no nls= option and neither iocharset= option for ext4. So kernel should
not start reporting such warnings for ext4.

> > Therefore I'm not sure if this parsing should be in global
> > fs/fc_parser.c file...
>
Kari Argillander Aug. 19, 2021, 11:18 p.m. UTC | #6
On Fri, Aug 20, 2021 at 12:04:12AM +0200, Pali Rohár wrote:
> On Thursday 19 August 2021 13:23:42 Kari Argillander wrote:
> > On Thu, Aug 19, 2021 at 10:12:22AM +0200, Pali Rohár wrote:
> > > On Thursday 19 August 2021 04:21:08 Kari Argillander wrote:
> > > > On Sun, Aug 08, 2021 at 06:24:38PM +0200, Pali Rohár wrote:
> > > > > Other fs drivers are using iocharset= mount option for specifying charset.
> > > > > So mark iocharset= mount option as preferred and deprecate nls= mount
> > > > > option.
> > > >  
> > > > One idea is also make this change to fs/fc_parser.c and then when we
> > > > want we can drop support from all filesystem same time. This way we
> > > > can get more deprecated code off the fs drivers. Draw back is that
> > > > then every filesstem has this deprecated nls= option if it support
> > > > iocharsets option. But that should imo be ok.
> > > 
> > > Beware that iocharset= is required only for fs which store filenames in
> > > some specific encoding (in this case extension to UTF-16). For fs which
> > > store filenames in raw bytes this option should not be parsed at all.
> > 
> > Yeah of course. I was thinking that what we do is that if key is nls=
> > we change key to iocharset, print deprecated and then send it to driver
> > parser as usual. This way driver parser will never know that user
> > specifie nls= because it just get iocharset. But this is probebly too
> > fancy way to think simple problem. Just idea. 
> 
> This has an issue that when you use nls= option for e.g. ext4 fs then
> kernel starts reporting that nls= for ext4 is deprecated. But there is
> no nls= option and neither iocharset= option for ext4. So kernel should
> not start reporting such warnings for ext4.

It gets kinda messy. I was also thinking that but if that was
implemented then we could first send iocharset to driver and after that
we print deprecated if it succeeded. If it not succeed then we print
error messages same as always.

I have not look how easily this is can be done in parser.

> 
> > > Therefore I'm not sure if this parsing should be in global
> > > fs/fc_parser.c file...
> >
diff mbox series

Patch

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 4474adb393ca..3676f185b4a0 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -2303,7 +2303,7 @@  int ntfs_show_options(struct seq_file *sf, struct dentry *root)
 		seq_printf(sf, ",fmask=0%o", vol->fmask);
 		seq_printf(sf, ",dmask=0%o", vol->dmask);
 	}
-	seq_printf(sf, ",nls=%s", vol->nls_map->charset);
+	seq_printf(sf, ",iocharset=%s", vol->nls_map->charset);
 	if (NVolCaseSensitive(vol))
 		seq_printf(sf, ",case_sensitive");
 	if (NVolShowSystemFiles(vol))
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 0d7e948cb29c..02de1aa05b7c 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -192,11 +192,6 @@  static bool parse_options(ntfs_volume *vol, char *opt)
 			ntfs_warning(vol->sb, "Ignoring obsolete option %s.",
 					p);
 		else if (!strcmp(p, "nls") || !strcmp(p, "iocharset")) {
-			if (!strcmp(p, "iocharset"))
-				ntfs_warning(vol->sb, "Option iocharset is "
-						"deprecated. Please use "
-						"option nls=<charsetname> in "
-						"the future.");
 			if (!v || !*v)
 				goto needs_arg;
 use_utf8:
@@ -218,10 +213,10 @@  static bool parse_options(ntfs_volume *vol, char *opt)
 		} else if (!strcmp(p, "utf8")) {
 			bool val = false;
 			ntfs_warning(vol->sb, "Option utf8 is no longer "
-				   "supported, using option nls=utf8. Please "
-				   "use option nls=utf8 in the future and "
-				   "make sure utf8 is compiled either as a "
-				   "module or into the kernel.");
+				   "supported, using option iocharset=utf8. "
+				   "Please use option iocharset=utf8 in the "
+				   "future and make sure utf8 is compiled "
+				   "either as a module or into the kernel.");
 			if (!v || !*v)
 				val = true;
 			else if (!simple_getbool(v, &val))
diff --git a/fs/ntfs/unistr.c b/fs/ntfs/unistr.c
index a6b6c64f14a9..75a7f73bccdd 100644
--- a/fs/ntfs/unistr.c
+++ b/fs/ntfs/unistr.c
@@ -372,7 +372,8 @@  retry:			wc = nls->uni2char(le16_to_cpu(ins[i]), ns + o,
 conversion_err:
 	ntfs_error(vol->sb, "Unicode name contains characters that cannot be "
 			"converted to character set %s.  You might want to "
-			"try to use the mount option nls=utf8.", nls->charset);
+			"try to use the mount option iocharset=utf8.",
+			nls->charset);
 	if (ns != *outs)
 		kfree(ns);
 	if (wc != -ENAMETOOLONG)