Re: [PATCH] fs: exFAT read-only driver GPL implementation by Paragon Software.

* Re: [PATCH] fs: exFAT read-only driver GPL implementation by Paragon Software.
       [not found] <453A1153-9493-4A04-BF66-CE6A572DEBDB@paragon-software.com>
@ 2019-10-19 23:34 ` Pali Rohár
  2019-10-22 17:13   ` [PATCH] " Konstantin Komarov
  2019-10-20 18:08 ` [PATCH] fs: " Richard Weinberger
  2019-10-21 11:11 ` Pali Rohár
  2 siblings, 1 reply; 11+ messages in thread
From: Pali Rohár @ 2019-10-19 23:34 UTC (permalink / raw)
  To: Konstantin Komarov; +Cc: viro, linux-kernel, linux-fsdevel

[-- Attachment #1: Type: text/plain, Size: 3814 bytes --]

Hello! I have not read deeply whole implementation, just spotted
suspicious options. See below.

On Friday 18 October 2019 15:18:39 Konstantin Komarov wrote:
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> new file mode 100644
> index 000000000000..5f8713fe1b0c
> --- /dev/null
> +++ b/fs/exfat/exfat_fs.h
> @@ -0,0 +1,388 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  linux/fs/exfat/super.c
> + *
> + * Copyright (c) 2010-2019 Paragon Software GmbH, All rights reserved.
> + *
> + */
> +
> +#include <linux/buffer_head.h>
> +#include <linux/hash.h>
> +#include <linux/nls.h>
> +#include <linux/ratelimit.h>
> +
> +struct exfat_mount_options {
> +	kuid_t fs_uid;
> +	kgid_t fs_gid;
> +	u16 fs_fmask;
> +	u16 fs_dmask;
> +	u16 codepage; /* Codepage for shortname conversions */

According to exFAT specification, section 7.7.3 FileName Field there is
no 8.3 shortname support with DOS/OEM codepage.

https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification#773-filename-field

Plus it looks like that this member codepage is only set and never
accessed in whole driver.

So it can be clean it up and removed?

> +	/* minutes bias= UTC - local time. Eastern time zone: +300, */
> +	/*Paris,Berlin: -60, Moscow: -180*/
> +	int bias;
> +	u16 allow_utime; /* permission for setting the [am]time */
> +	unsigned quiet : 1, /* set = fake successful chmods and chowns */
> +		showexec : 1, /* set = only set x bit for com/exe/bat */
> +		sys_immutable : 1, /* set = system files are immutable */
> +		utf8 : 1, /* Use of UTF-8 character set (Default) */
> +		/* create escape sequences for unhandled Unicode */
> +		unicode_xlate : 1, flush : 1, /* write things quickly */
> +		tz_set : 1, /* Filesystem timestamps' offset set */
> +		discard : 1 /* Issue discard requests on deletions */
> +		;
> +};

...

> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> new file mode 100644
> index 000000000000..0705dab3c3fc
> --- /dev/null
> +++ b/fs/exfat/super.c
...
> +enum {
> +	Opt_uid, Opt_gid, Opt_umask, Opt_dmask, Opt_fmask, Opt_allow_utime,
> +	Opt_codepage, Opt_quiet, Opt_showexec, Opt_debug, Opt_immutable,
> +	Opt_utf8_no, Opt_utf8_yes, Opt_uni_xl_no, Opt_uni_xl_yes, Opt_flush,
> +	Opt_tz_utc, Opt_discard, Opt_nfs, Opt_bias, Opt_err,
> +};
> +
> +static const match_table_t fat_tokens = {
> +	{ Opt_uid, "uid=%u" },
> +	{ Opt_gid, "gid=%u" },
> +	{ Opt_umask, "umask=%o" },
> +	{ Opt_dmask, "dmask=%o" },
> +	{ Opt_fmask, "fmask=%o" },
> +	{ Opt_allow_utime, "allow_utime=%o" },
> +	{ Opt_codepage, "codepage=%u" },
> +	{ Opt_quiet, "quiet" },
> +	{ Opt_showexec, "showexec" },
> +	{ Opt_debug, "debug" },
> +	{ Opt_immutable, "sys_immutable" },
> +	{ Opt_flush, "flush" },
> +	{ Opt_tz_utc, "tz=UTC" },
> +	{ Opt_bias, "bias=%d" },
> +	{ Opt_discard, "discard" },
> +	{ Opt_utf8_no, "utf8=0" }, /* 0 or no or false */
> +	{ Opt_utf8_no, "utf8=no" },
> +	{ Opt_utf8_no, "utf8=false" },
> +	{ Opt_utf8_yes, "utf8=1" }, /* empty or 1 or yes or true */
> +	{ Opt_utf8_yes, "utf8=yes" },
> +	{ Opt_utf8_yes, "utf8=true" },
> +	{ Opt_utf8_yes, "utf8" },

There are lot of utf8 mount options. Are they really needed?

Would not it be better to use just one "iocharset" mount option like
other Unicode based filesystem have it (e.g. vfat, jfs, iso9660, udf or
ntfs)?

> +	{ Opt_uni_xl_no, "uni_xlate=0" }, /* 0 or no or false */
> +	{ Opt_uni_xl_no, "uni_xlate=no" },
> +	{ Opt_uni_xl_no, "uni_xlate=false" },
> +	{ Opt_uni_xl_yes, "uni_xlate=1" }, /* empty or 1 or yes or true */
> +	{ Opt_uni_xl_yes, "uni_xlate=yes" },
> +	{ Opt_uni_xl_yes, "uni_xlate=true" },
> +	{ Opt_uni_xl_yes, "uni_xlate" },
> +	{ Opt_err, NULL }
> +};

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread