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;
Date:   Sun, 17 Oct 2021 11:10:27 +0300
From:   Kari Argillander <kari.argillander@gmail.com>
To:     Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc:     ntfs3@lists.linux.dev, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/5] fs/ntfs3: Rework ntfs_utf16_to_nls
Message-ID: <20211017081027.zxh2hbbxdk7ffwqn@kari-VirtualBox>
References: <98a166e4-f894-8bff-9479-05ef5435f1ed@paragon-software.com>
 <5f5a0b13-90bd-b97c-aa50-c646bb243bde@paragon-software.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5f5a0b13-90bd-b97c-aa50-c646bb243bde@paragon-software.com>
Precedence: bulk

On Tue, Oct 05, 2021 at 07:47:03PM +0300, Konstantin Komarov wrote:
> Now ntfs_utf16_to_nls takes length as one of arguments.
> If length of symlink > 255, then we tried to convert
> length of symlink +- some random number.
> Now 255 symbols limit was removed.
> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/dir.c     | 19 ++++++++-----------
>  fs/ntfs3/ntfs_fs.h |  2 +-
>  2 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ntfs3/dir.c b/fs/ntfs3/dir.c
> index 785e72d4392e..fb438d604040 100644
> --- a/fs/ntfs3/dir.c
> +++ b/fs/ntfs3/dir.c
> @@ -15,11 +15,10 @@
>  #include "ntfs_fs.h"
>  
>  /* Convert little endian UTF-16 to NLS string. */
> -int ntfs_utf16_to_nls(struct ntfs_sb_info *sbi, const struct le_str *uni,
> +int ntfs_utf16_to_nls(struct ntfs_sb_info *sbi, const __le16 *name, u32 len,
>  		      u8 *buf, int buf_len)

I just like to point out that this patch break the build. We cannot
break build in any commit. If you make change you need to make sure
everything still works. This is of course now late, but I just bring it
up if you did not know it already.

Problem here is that you change callers to match this in patch 2/5. You
need to make it in same patch. Basically every patch should build it is
own and even all tests should work.

Reason here is git bisecting. We really want to easy way to bisect bugs
and if some commit break something then it make bisecting more
difficult.

I also wonder why 0day bot did not report this. Maybe because there was
no base or something, but I have to look into that as for now that is
only CI system we have. When we get ntfs3 to kernel.org then we also can
use patchwork and then my first thing to do is to start CI system that
reports results to patchwork.

I will use Snowpatch to fetch patches from patchwork and Jenkins to CI.

>  {
> -	int ret, uni_len, warn;
> -	const __le16 *ip;
> +	int ret, warn;
>  	u8 *op;
>  	struct nls_table *nls = sbi->options->nls;
>  
> @@ -27,18 +26,16 @@ int ntfs_utf16_to_nls(struct ntfs_sb_info *sbi, const struct le_str *uni,
>  
>  	if (!nls) {
>  		/* UTF-16 -> UTF-8 */
> -		ret = utf16s_to_utf8s((wchar_t *)uni->name, uni->len,
> -				      UTF16_LITTLE_ENDIAN, buf, buf_len);
> +		ret = utf16s_to_utf8s(name, len, UTF16_LITTLE_ENDIAN, buf,
> +				      buf_len);
>  		buf[ret] = '\0';
>  		return ret;
>  	}
>  
> -	ip = uni->name;
>  	op = buf;
> -	uni_len = uni->len;
>  	warn = 0;
>  
> -	while (uni_len--) {
> +	while (len--) {
>  		u16 ec;
>  		int charlen;
>  		char dump[5];
> @@ -49,7 +46,7 @@ int ntfs_utf16_to_nls(struct ntfs_sb_info *sbi, const struct le_str *uni,
>  			break;
>  		}
>  
> -		ec = le16_to_cpu(*ip++);
> +		ec = le16_to_cpu(*name++);
>  		charlen = nls->uni2char(ec, op, buf_len);
>  
>  		if (charlen > 0) {
> @@ -304,8 +301,8 @@ static inline int ntfs_filldir(struct ntfs_sb_info *sbi, struct ntfs_inode *ni,
>  	if (sbi->options->nohidden && (fname->dup.fa & FILE_ATTRIBUTE_HIDDEN))
>  		return 0;
>  
> -	name_len = ntfs_utf16_to_nls(sbi, (struct le_str *)&fname->name_len,
> -				     name, PATH_MAX);
> +	name_len = ntfs_utf16_to_nls(sbi, fname->name, fname->name_len, name,
> +				     PATH_MAX);
>  	if (name_len <= 0) {
>  		ntfs_warn(sbi->sb, "failed to convert name for inode %lx.",
>  			  ino);
> diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> index 38b7c1a9dc52..9277b552f257 100644
> --- a/fs/ntfs3/ntfs_fs.h
> +++ b/fs/ntfs3/ntfs_fs.h
> @@ -475,7 +475,7 @@ bool are_bits_set(const ulong *map, size_t bit, size_t nbits);
>  size_t get_set_bits_ex(const ulong *map, size_t bit, size_t nbits);
>  
>  /* Globals from dir.c */
> -int ntfs_utf16_to_nls(struct ntfs_sb_info *sbi, const struct le_str *uni,
> +int ntfs_utf16_to_nls(struct ntfs_sb_info *sbi, const __le16 *name, u32 len,
>  		      u8 *buf, int buf_len);
>  int ntfs_nls_to_utf16(struct ntfs_sb_info *sbi, const u8 *name, u32 name_len,
>  		      struct cpu_str *uni, u32 max_ulen,
> -- 
> 2.33.0
> 
>