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:   Mon, 4 Jan 2021 01:17:55 +0200
From:   Kari Argillander <kari.argillander@gmail.com>
To:     Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc:     linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
        linux-kernel@vger.kernel.org, pali@kernel.org, dsterba@suse.cz,
        aaptel@suse.com, willy@infradead.org, rdunlap@infradead.org,
        joe@perches.com, mark@harmstone.com, nborisov@suse.com,
        linux-ntfs-dev@lists.sourceforge.net, anton@tuxera.com,
        dan.carpenter@oracle.com, hch@lst.de, ebiggers@kernel.org,
        andy.lavr@gmail.com
Subject: Re: [PATCH v17 01/10] fs/ntfs3: Add headers and misc files
Message-ID: <20210103231755.bcmyalz3maq4ama2@kari-VirtualBox>
References: <20201231152401.3162425-1-almaz.alexandrovich@paragon-software.com>
 <20201231152401.3162425-2-almaz.alexandrovich@paragon-software.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201231152401.3162425-2-almaz.alexandrovich@paragon-software.com>
Precedence: bulk

On Thu, Dec 31, 2020 at 06:23:52PM +0300, Konstantin Komarov wrote:

> diff --git a/fs/ntfs3/debug.h b/fs/ntfs3/debug.h

> +/*
> + * Logging macros ( thanks Joe Perches <joe@perches.com> for implementation )
> + */
> +
> +#define ntfs_err(sb, fmt, ...)  ntfs_printk(sb, KERN_ERR fmt, ##__VA_ARGS__)
> +#define ntfs_warn(sb, fmt, ...) ntfs_printk(sb, KERN_WARNING fmt, ##__VA_ARGS__)
> +#define ntfs_info(sb, fmt, ...) ntfs_printk(sb, KERN_INFO fmt, ##__VA_ARGS__)
> +#define ntfs_notice(sb, fmt, ...)                                              \
> +	ntfs_printk(sb, KERN_NOTICE fmt, ##__VA_ARGS__)
> +
> +#define ntfs_inode_err(inode, fmt, ...)                                        \
> +	ntfs_inode_printk(inode, KERN_ERR fmt, ##__VA_ARGS__)
> +#define ntfs_inode_warn(inode, fmt, ...)                                       \
> +	ntfs_inode_printk(inode, KERN_WARNING fmt, ##__VA_ARGS__)
> +
> +#define ntfs_alloc(s, z)	kmalloc(s, (z) ? (GFP_NOFS | __GFP_ZERO) : GFP_NOFS)

kmalloc with __GFP_ZERO is just kzalloc. So why we even need ntfs_alloc(). We
will be much happier if we straight away see

kzalloc( , GFP_NOFS) or kmalloc( , GFP_NOFS)

That way it will be easier to remove GFP_NOFS flag when not needed.
I have not knowledge but I have read that even with filesystems it
is not good pratice to always use that flag. Another point is that
we will get these defines deleted from debug.h. Atleast to me this
is strange place for them. And also this not even save line space
much.

kzalloc( , GFP_NOFS)
ntfs_alloc( , 0)

ntfs_free()
kree()

I can send patch fror this if you prefer this way. And nobady not
nack about it.

> +#define ntfs_free(p)		kfree(p)
> +#define ntfs_memdup(src, len)	kmemdup(src, len, GFP_NOFS)

> diff --git a/fs/ntfs3/upcase.c b/fs/ntfs3/upcase.c

> +static inline u16 upcase_unicode_char(const u16 *upcase, u16 chr)
> +{
> +	if (chr < 'a')
> +		return chr;
> +
> +	if (chr <= 'z')
> +		return chr - ('a' - 'A');
> +
> +	return upcase[chr];
> +}
> +
> +int ntfs_cmp_names(const __le16 *s1, size_t l1, const __le16 *s2, size_t l2,
> +		   const u16 *upcase)
> +{
> +	int diff;
> +	size_t len = l1 < l2 ? l1 : l2;
> +
> +	if (upcase) {
> +		while (len--) {
> +			diff = upcase_unicode_char(upcase, le16_to_cpu(*s1++)) -
> +			       upcase_unicode_char(upcase, le16_to_cpu(*s2++));
> +			if (diff)
> +				return diff;
> +		}
> +	} else {
> +		while (len--) {
> +			diff = le16_to_cpu(*s1++) - le16_to_cpu(*s2++);
> +			if (diff)
> +				return diff;
> +		}
> +	}
> +
> +	return (int)(l1 - l2);
> +}

I notice that these functions might call both ignore case and upcase in a row.
record.c - compare_attr()
index.c - cmp_fnames()

So maybe we can add bool bothcases.

int ntfs_cmp_names(const __le16 *s1, size_t l1, const __le16 *s2, size_t l2,
		   const u16 *upcase, bool bothcase)
{
	int diff1 = 0;
	int diff2;
	size_t len = l1 < l2 ? l1 : l2;

	if (!bothcase && upcase)
		goto case_insentive;

	for (; len; s1++, s2++, len--) {
		diff1 = le16_to_cpu(*s1) - le16_to_cpu(*s2);
		if (diff1) {
			if (bothcase && upcase)
				goto case_insentive;

			return diff1;
		}
	}
	return l1 - l2;

case_insentive:
	for (; len; s1++, s2++, len--) {
		diff2 = upcase_unicode_char(upcase, le16_to_cpu(*s1)) -
			 upcase_unicode_char(upcase, le16_to_cpu(*s2));
		if (diff2)
			return diff2;
	}

	if (bothcase && diff1)
		return diff1;

	return l1 - l2;
}

This is not tested. I can send patch for this also if you like idea.
cmp_fnames() and compare_attr() will clean up alot with this.

> +
> +int ntfs_cmp_names_cpu(const struct cpu_str *uni1, const struct le_str *uni2,
> +		       const u16 *upcase)
> +{
> +	const u16 *s1 = uni1->name;
> +	const __le16 *s2 = uni2->name;
> +	size_t l1 = uni1->len;
> +	size_t l2 = uni2->len;
> +	size_t len = l1 < l2 ? l1 : l2;
> +	int diff;
> +
> +	if (upcase) {
> +		while (len--) {
> +			diff = upcase_unicode_char(upcase, *s1++) -
> +			       upcase_unicode_char(upcase, le16_to_cpu(*s2++));
> +			if (diff)
> +				return diff;
> +		}
> +	} else {
> +		while (len--) {
> +			diff = *s1++ - le16_to_cpu(*s2++);
> +			if (diff)
> +				return diff;
> +		}
> +	}
> +
> +	return l1 - l2;
> +}