> On Aug 8, 2021, at 9:24 AM, Pali Rohár <pali@xxxxxxxxxx> wrote:
>
> 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.
>
> Reverse engineering driver code shown what is doing in this special case:
>
> 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".
>
> So when hsb->nls_disk is not set and hsb->nls_io is then explicitly set
> hsb->nls_disk to "iso8859-1".
>
> 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.
>
> 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.
>
Sounds reasonable. But it will be great to know that the change has been tested reasonably well.
Thanks,
Slava.
> Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> ---
> fs/hfs/super.c | 31 +++++++++++++++++++++++++++++++
> fs/hfs/trans.c | 38 ++++++++++++++------------------------
> 2 files changed, 45 insertions(+), 24 deletions(-)
>
> 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)
> }
> }
>
> + 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=iso8859-1\n");
> + pr_warn("this default codepage=iso8859-1 is incompatible with "
> + "Mac OS systems and may be changed in the future");
> + hsb->nls_disk = 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 = 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;
>
> while (srclen > 0) {
> - if (nls_disk) {
> - size = nls_disk->char2uni(src, srclen, &ch);
> - if (size <= 0) {
> - ch = '?';
> - size = 1;
> - }
> - src += size;
> - srclen -= size;
> - } else {
> - ch = *src++;
> - srclen--;
> + size = nls_disk->char2uni(src, srclen, &ch);
> + if (size <= 0) {
> + ch = '?';
> + size = 1;
> }
> + src += size;
> + srclen -= size;
> if (ch == '/')
> ch = ':';
> size = 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 -= size;
> if (ch == ':')
> ch = '/';
> - if (nls_disk) {
> - size = nls_disk->uni2char(ch, dst, dstlen);
> - if (size < 0) {
> - if (size == -ENAMETOOLONG)
> - goto out;
> - *dst = '?';
> - size = 1;
> - }
> - dst += size;
> - dstlen -= size;
> - } else {
> - *dst++ = ch > 0xff ? '?' : ch;
> - dstlen--;
> + size = nls_disk->uni2char(ch, dst, dstlen);
> + if (size < 0) {
> + if (size == -ENAMETOOLONG)
> + goto out;
> + *dst = '?';
> + size = 1;
> }
> + dst += size;
> + dstlen -= size;
> }
> } else {
> char ch;
> --
> 2.20.1
>
