Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Shreeya Patel <shreeya.patel@collabora.com>
Cc: tytso@mit.edu, adilger.kernel@dilger.ca, jaegeuk@kernel.org,
	chao@kernel.org, krisman@collabora.com, drosen@google.com,
	yuchao0@huawei.com, linux-ext4@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-fsdevel@vger.kernel.org, kernel@collabora.com,
	andre.almeida@collabora.com
Subject: Re: [PATCH v7 4/4] fs: unicode: Add utf8 module and a unicode layer
Date: Wed, 7 Apr 2021 18:01:26 -0700
Message-ID: <YG5V5l2pD3DCiyVA@gmail.com> (raw)
In-Reply-To: <20210407144845.53266-5-shreeya.patel@collabora.com>

On Wed, Apr 07, 2021 at 08:18:45PM +0530, Shreeya Patel wrote:
> diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig
> index 2c27b9a5cd6c..0c69800a2a37 100644
> --- a/fs/unicode/Kconfig
> +++ b/fs/unicode/Kconfig
> @@ -2,13 +2,31 @@
>  #
>  # UTF-8 normalization
>  #
> +# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8
> +# is enabled. This config option adds the unicode subsystem layer which loads
> +# the UTF-8 module whenever any filesystem needs it.
>  config UNICODE
> -	bool "UTF-8 normalization and casefolding support"
> +	bool
> +
> +config UNICODE_UTF8
> +	tristate "UTF-8 module"
> +	select UNICODE
>  	help
> -	  Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding
> -	  support.
> +	  Say M here to enable UTF-8 NFD normalization and NFD+CF casefolding
> +	  support as a loadable module or say Y for building it into the kernel.
> +
> +	  utf8data.h_shipped has a large database table which is an
> +	  auto-generated decodification trie for the unicode normalization
> +	  functions and it is not necessary to carry this large table in the
> +	  kernel. Hence, enabling UNICODE_UTF8 as M will allow you to avoid
> +	  carrying this large table into the kernel and module will only be
> +	  loaded whenever required by any filesystem.
> +	  Please note, in this case utf8 module will only be available after
> +	  booting into the compiled kernel. If your filesystem requires it to
> +	  have utf8 during boot time then you should have it built into the
> +	  kernel by saying Y here to avoid boot failure.

This help text seems to contradict itself; it says "it is not necessary to carry
this large table in the kernel", and then later it says that in some cases it is
in fact necessary.

It would also be helpful for the help text to mention which filesystems actually
support this feature.

> diff --git a/fs/unicode/unicode-core.c b/fs/unicode/unicode-core.c
> index 730dbaedf593..d9e9e410893d 100644
> --- a/fs/unicode/unicode-core.c
> +++ b/fs/unicode/unicode-core.c
> @@ -1,228 +1,132 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> -#include <linux/string.h>
>  #include <linux/slab.h>
> -#include <linux/parser.h>
>  #include <linux/errno.h>
>  #include <linux/unicode.h>
> -#include <linux/stringhash.h>
> +#include <linux/spinlock.h>
>  
> -#include "utf8n.h"
> +DEFINE_SPINLOCK(utf8mod_lock);

This spinlock should be 'static'.

- Eric

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-07 14:48 [PATCH v7 0/4] Make UTF-8 encoding loadable Shreeya Patel
2021-04-07 14:48 ` [PATCH v7 1/4] fs: unicode: Use strscpy() instead of strncpy() Shreeya Patel
2021-04-08  0:52   ` Eric Biggers
2021-04-07 14:48 ` [PATCH v7 2/4] fs: unicode: Rename function names from utf8 to unicode Shreeya Patel
2021-04-07 14:48 ` [PATCH v7 3/4] fs: unicode: Rename utf8-core file to unicode-core Shreeya Patel
2021-04-07 14:48 ` [PATCH v7 4/4] fs: unicode: Add utf8 module and a unicode layer Shreeya Patel
2021-04-08  1:01   ` Eric Biggers [this message]
2021-04-08 19:10   ` Gabriel Krisman Bertazi
2021-04-14 11:56     ` Shreeya Patel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YG5V5l2pD3DCiyVA@gmail.com \
    --to=ebiggers@kernel.org \
    --cc=adilger.kernel@dilger.ca \
    --cc=andre.almeida@collabora.com \
    --cc=chao@kernel.org \
    --cc=drosen@google.com \
    --cc=jaegeuk@kernel.org \
    --cc=kernel@collabora.com \
    --cc=krisman@collabora.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=shreeya.patel@collabora.com \
    --cc=tytso@mit.edu \
    --cc=yuchao0@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git