Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [RFC PATCH 11/20] hfs: Explicitly set hsb->nls_disk when
 hsb->nls_io is set
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <20210808162453.1653-12-pali@kernel.org>
Date:   Mon, 9 Aug 2021 10:31:55 -0700
Cc:     Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        linux-ntfs-dev@lists.sourceforge.net, linux-cifs@vger.kernel.org,
        jfs-discussion@lists.sourceforge.net,
        LKML <linux-kernel@vger.kernel.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Jan Kara <jack@suse.cz>,
        OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>,
        "Theodore Y . Ts'o" <tytso@mit.edu>,
        Luis de Bethencourt <luisbg@kernel.org>,
        Salah Triki <salah.triki@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Dave Kleikamp <shaggy@kernel.org>,
        Anton Altaparmakov <anton@tuxera.com>,
        Pavel Machek <pavel@ucw.cz>,
        =?utf-8?Q?Marek_Beh=C3=BAn?= <marek.behun@nic.cz>,
        Christoph Hellwig <hch@infradead.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <D0302F93-BAE5-48F0-87D0-B68B10D7757B@dubeyko.com>
References: <20210808162453.1653-1-pali@kernel.org>
 <20210808162453.1653-12-pali@kernel.org>
To:     =?utf-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>
Precedence: bulk


> On Aug 8, 2021, at 9:24 AM, Pali Roh=C3=A1r <pali@kernel.org> wrote:
>=20
> It does not make any sense to set hsb->nls_io (NLS iocharset used =
between
> VFS and hfs driver) when hsb->nls_disk (NLS codepage used between hfs
> driver and disk) is not set.
>=20
> Reverse engineering driver code shown what is doing in this special =
case:
>=20
>    When codepage was not defined but iocharset was then
>    hfs driver copied 8bit character from disk directly to
>    16bit unicode wchar_t type. Which means it did conversion
>    from Latin1 (ISO-8859-1) to Unicode because first 256
>    Unicode code points matches 8bit ISO-8859-1 codepage table.
>    So when iocharset was specified and codepage not, then
>    codepage used implicit value "iso8859-1".
>=20
> So when hsb->nls_disk is not set and hsb->nls_io is then explicitly =
set
> hsb->nls_disk to "iso8859-1".
>=20
> Such setup is obviously incompatible with Mac OS systems as they do =
not
> support iso8859-1 encoding for hfs. So print warning into dmesg about =
this
> fact.
>=20
> After this change hsb->nls_disk is always set, so remove code paths =
for
> case when hsb->nls_disk was not set as they are not needed anymore.
>=20


Sounds reasonable. But it will be great to know that the change has been =
tested reasonably well.

Thanks,
Slava.


> Signed-off-by: Pali Roh=C3=A1r <pali@kernel.org>
> ---
> fs/hfs/super.c | 31 +++++++++++++++++++++++++++++++
> fs/hfs/trans.c | 38 ++++++++++++++------------------------
> 2 files changed, 45 insertions(+), 24 deletions(-)
>=20
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index 12d9bae39363..86bc46746c7f 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -351,6 +351,37 @@ static int parse_options(char *options, struct =
hfs_sb_info *hsb)
> 		}
> 	}
>=20
> +	if (hsb->nls_io && !hsb->nls_disk) {
> +		/*
> +		 * Previous version of hfs driver did something =
unexpected:
> +		 * When codepage was not defined but iocharset was then
> +		 * hfs driver copied 8bit character from disk directly =
to
> +		 * 16bit unicode wchar_t type. Which means it did =
conversion
> +		 * from Latin1 (ISO-8859-1) to Unicode because first 256
> +		 * Unicode code points matches 8bit ISO-8859-1 codepage =
table.
> +		 * So when iocharset was specified and codepage not, =
then
> +		 * codepage used implicit value "iso8859-1".
> +		 *
> +		 * To not change this previous default behavior as some =
users
> +		 * may depend on it, we load iso8859-1 NLS table =
explicitly
> +		 * to simplify code and make it more reable what =
happens.
> +		 *
> +		 * In context of hfs driver it is really strange to use
> +		 * ISO-8859-1 codepage table for storing data to disk, =
but
> +		 * nothing forbids it. Just it is highly incompatible =
with
> +		 * Mac OS systems. So via pr_warn() inform user that =
this
> +		 * is not probably what he wants.
> +		 */
> +		pr_warn("iocharset was specified but codepage not, "
> +			"using default codepage=3Diso8859-1\n");
> +		pr_warn("this default codepage=3Diso8859-1 is =
incompatible with "
> +			"Mac OS systems and may be changed in the =
future");
> +		hsb->nls_disk =3D load_nls("iso8859-1");
> +		if (!hsb->nls_disk) {
> +			pr_err("unable to load iso8859-1 codepage\n");
> +			return 0;
> +		}
> +	}
> 	if (hsb->nls_disk && !hsb->nls_io) {
> 		hsb->nls_io =3D load_nls_default();
> 		if (!hsb->nls_io) {
> diff --git a/fs/hfs/trans.c b/fs/hfs/trans.c
> index 39f5e343bf4d..c75682c61b06 100644
> --- a/fs/hfs/trans.c
> +++ b/fs/hfs/trans.c
> @@ -48,18 +48,13 @@ int hfs_mac2asc(struct super_block *sb, char *out, =
const struct hfs_name *in)
> 		wchar_t ch;
>=20
> 		while (srclen > 0) {
> -			if (nls_disk) {
> -				size =3D nls_disk->char2uni(src, srclen, =
&ch);
> -				if (size <=3D 0) {
> -					ch =3D '?';
> -					size =3D 1;
> -				}
> -				src +=3D size;
> -				srclen -=3D size;
> -			} else {
> -				ch =3D *src++;
> -				srclen--;
> +			size =3D nls_disk->char2uni(src, srclen, &ch);
> +			if (size <=3D 0) {
> +				ch =3D '?';
> +				size =3D 1;
> 			}
> +			src +=3D size;
> +			srclen -=3D size;
> 			if (ch =3D=3D '/')
> 				ch =3D ':';
> 			size =3D nls_io->uni2char(ch, dst, dstlen);
> @@ -119,20 +114,15 @@ void hfs_asc2mac(struct super_block *sb, struct =
hfs_name *out, const struct qstr
> 			srclen -=3D size;
> 			if (ch =3D=3D ':')
> 				ch =3D '/';
> -			if (nls_disk) {
> -				size =3D nls_disk->uni2char(ch, dst, =
dstlen);
> -				if (size < 0) {
> -					if (size =3D=3D -ENAMETOOLONG)
> -						goto out;
> -					*dst =3D '?';
> -					size =3D 1;
> -				}
> -				dst +=3D size;
> -				dstlen -=3D size;
> -			} else {
> -				*dst++ =3D ch > 0xff ? '?' : ch;
> -				dstlen--;
> +			size =3D nls_disk->uni2char(ch, dst, dstlen);
> +			if (size < 0) {
> +				if (size =3D=3D -ENAMETOOLONG)
> +					goto out;
> +				*dst =3D '?';
> +				size =3D 1;
> 			}
> +			dst +=3D size;
> +			dstlen -=3D size;
> 		}
> 	} else {
> 		char ch;
> --=20
> 2.20.1
>=20