Re: [PATCH v11 00/10] NTFS read-write driver GPL implementation by Paragon Software

[Pali Rohár]

Hello!

On Friday 30 October 2020 15:51:10 Konstantin Komarov wrote:
> From: Pali Rohár <pali@xxxxxxxxxx>
> Sent: Friday, October 30, 2020 6:25 PM
> > To: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>
> > Cc: linux-fsdevel@xxxxxxxxxxxxxxx; viro@xxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dsterba@xxxxxxx; aaptel@xxxxxxxx;
> > willy@xxxxxxxxxxxxx; rdunlap@xxxxxxxxxxxxx; joe@xxxxxxxxxxx; mark@xxxxxxxxxxxxx; nborisov@xxxxxxxx; linux-ntfs-
> > dev@xxxxxxxxxxxxxxxxxxxxx; anton@xxxxxxxxxx
> > Subject: Re: [PATCH v11 00/10] NTFS read-write driver GPL implementation by Paragon Software
> > 
> > Hello and thanks for update!
> > 
> > I have just two comments for the last v11 version.
> > 
> > I really do not like nls_alt mount option and I do not think we should
> > merge this mount option into ntfs kernel driver. Details I described in:
> > https://lore.kernel.org/linux-fsdevel/20201009154734.andv4es3azkkskm5@pali/
> > 
> > tl;dr it is not systematic solution and is incompatible with existing
> > in-kernel ntfs driver, also incompatible with in-kernel vfat, udf and
> > ext4 (with UNICODE support) drivers. In my opinion, all kernel fs
> > drivers which deals with UNICODE should handle it in similar way.
> > 
> 
> Hello Pali! First of all, apologies for not providing a feedback on your previous
> message regarding the 'nls_alt'. We had internal discussions on the topic and
> overall conclusion is that: we do not want to compromise Kernel standards with
> our submission. So we will remove the 'nls_alt' option in the next version.
> 
> However, there are still few points we have on the topic, please read below.
> 
> > It would be really bad if userspace application need to behave
> > differently for this new ntfs driver and differently for all other
> > UNICODE drivers.
> > 
> 
> The option does not anyhow affect userspace applications. For the "default" example
> of unzip/tar:
> 1 - if this option is not applied (e.g. "vfat case"), trying to unzip an archive with, e.g. CP-1251,
> names inside to the target fs volume, will return error, and issued file(s) won't be unzipped;
> 2 - if this option is applied and "nls_alt" is set, the above case will result in unzipping all the files;

I understand what is the point and I'm not against discussion how to fix
it. But it should be implemented for all filesystems with UNICODE
semantic, so behavior would be same.

For user application point of view, behavior of vfat, ntfs, udf and ext4
(with UNICODE support; see below) in handling file names should be very
similar (or exactly same if fs tech details allows it).

> Also, this issue in general only applies to "non-native" filesystems. I.e. ext4 is not affected by it
> in any case, as it just stores the name as bytes, no matter what those bytes are. The above case
> won't give an unzip error on ext4. The only symptom of this would be, maybe, "incorrect encoding"
> marking within the listing of such files (in File Manager or Terminal, e.g. in Ubuntu), but there won't
> be an unzip process termination with incomplete unarchived fileset, unlike it is for vfat/exfat/ntfs
> without "nls_alt".

When using ext4 in default mode then it really does not apply here. But
I wrote that it applies for ext4 with UNICODE support. This mode needs
to be first enabled for directory, it is relatively new feature and I do
not know if there are users of it and how many people tried different
crazy test scenarios with normalization, etc...

> > Second comment is simplification of usage nls_load() with UTF-8 parameter
> > which I described in older email:
> > https://lore.kernel.org/linux-fsdevel/948ac894450d494ea15496c2e5b8c906@xxxxxxxxxxxxxxxxxxxx/
> > 
> > You wrote that you have applied it, but seems it was lost (maybe during
> > rebase?) as it is not present in the last v11 version.
> > 
> > I suggested to not use nls_load() with UTF-8 at all. Your version of
> > ntfs driver does not use kernel's nls utf8 module for UTF-8 support, so
> > trying to load it should be avoided. Also kernel can be compiled without
> > utf8 nls module (which is moreover broken) and with my above suggestion,
> > ntfs driver would work correctly. Without that suggestion, mounting
> > would fail.
> 
> Thanks for pointing that out. It is likely the "nls_load()" fixes were lost during rebase.
> Will recheck it and return them to the v12.

OK!