linux-kernel - Re: [PATCH v5 01/10] fs/ntfs3: Add headers and misc files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Sep 2020 19:28:27 -0700
From:   Joe Perches <joe@...ches.com>
To:     Konstantin Komarov <almaz.alexandrovich@...agon-software.com>,
        linux-fsdevel@...r.kernel.org
Cc:     viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org,
        pali@...nel.org, dsterba@...e.cz, aaptel@...e.com,
        willy@...radead.org, rdunlap@...radead.org, mark@...mstone.com,
        nborisov@...e.com
Subject: Re: [PATCH v5 01/10] fs/ntfs3: Add headers and misc files

On Fri, 2020-09-11 at 17:10 +0300, Konstantin Komarov wrote:
> This adds headers and misc files

The code may be ok, but its cosmetics are poor.

> diff --git a/fs/ntfs3/debug.h b/fs/ntfs3/debug.h
[]
> +#define QuadAlign(n) (((n) + 7u) & (~7u))
> +#define IsQuadAligned(n) (!((size_t)(n)&7u))
> +#define Quad2Align(n) (((n) + 15(&u) & (~15u))
> +#define IsQuad2Aligned(n) (!((size_t)(n)&15u))
> +#define Quad4Align(n) (((n) + 31u) & (~31u))
> +#define IsSizeTAligned(n) (!((size_t)(n) & (sizeof(size_t) - 1)))
> +#define DwordAlign(n) (((n) + 3u) & (~3u))
> +#define IsDwordAligned(n) (!((size_t)(n)&3u))
> +#define WordAlign(n) (((n) + 1u) & (~1u))
> +#define IsWordAligned(n) (!((size_t)(n)&1u))

All of these could use column alignment.
IsSizeTAligned could is the kernel's IS_ALIGNED

#define IsQuadAligned(n)	(!((size_t)(n) & 7u))
#define QuadAlign(n)		(((n) + 7u) & (~7u))
#define IsQuadAligned(n)	(!((size_t)(n) & 7u))
#define Quad2Align(n)		(((n) + 15u) & (~15u))
#define IsQuad2Aligned(n)	(!((size_t)(n) & 15u))

Though all of these could also use two macros with
the same form.  Something like:

#define NTFS3_ALIGN(n, at)	(((n) + ((at) - 1)) & (~((at) - 1)))
#define NTFS3_IS_ALIGNED(n, at)	(!((size_t)(n) & ((at) - 1)))

So all of these could be ordered by size and use actual size

#define WordAlign(n)		NTFS3_ALIGN(n, 2)
#define IsWordAligned(n)	NTFS3_IS_ALIGNED(n, 2)
#define DwordAlign(n)		NTFS3_ALIGN(n, 4)
#define IsDwordAligned(n)	
NTFS3_IS_ALIGNED(n, 4)
#define QuadAlign(n)		NTFS3_ALIGN(n, 8)
#define IsQuadAligned(n)	NTFS3_IS_ALIGNED(n, 8)
#define
Quad2Align(n)		NTFS3_ALIGN(n, 16)
#define IsQuad2Aligned(n)	NTFS3_IS_ALIGNED(n, 16)

#define IsSizeTAligned(n)	NTFS3_IS_ALIGNED(n, sizeof(size_t))


> +#ifdef CONFIG_PRINTK
> +__printf(2, 3) void ntfs_printk(const struct super_block *sb, const char *fmt,
> +				...);

Better would be

__printf(2, 3)
void ntfs_printk(const struct super_block *sb, const char *fmt, ...);

etc...

There's a lot of code that could be made more readable for a human.