[PATCH 0/5] fs/ntfs3: Fix various styling issues

[PATCH 0/5] fs/ntfs3: Fix various styling issues

[Kari Argillander]

Many new checkpatch warnings have been introduce to ntfs3. These could
have been prevent if checkpatch is used. One thing that worrys me is
that Konstantin puts new code without code reviewing process to ntfs3.
Patch commit message says one thing, but one huge patch address that and
lot of just refactoring code.  Also with review process we can prevent
these kind of silly checkpatch mistakes.

Kmalloc_array was my fault for some reason checkpatch did not show
those. I have no idea how, but I just fix it now and be very ashamed.
You should also Konstantin use checkpatch always before push so you can
spot these things before hand. I will try to get CI going for patches.

Kari Argillander (5):
  fs/ntfs3: Use kmalloc_array over kmalloc with multiply
  fs/ntfs3: Use consistent spacing around '+'
  fs/ntfs3: Place Comparisons constant right side of the test
  fs/ntfs3: Remove braces from single statment block
  fs/ntfs3: Remove tabs before spaces from comment

 fs/ntfs3/bitmap.c  | 2 +-
 fs/ntfs3/frecord.c | 8 ++++----
 fs/ntfs3/index.c   | 4 ++--
 fs/ntfs3/lznt.c    | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)


base-commit: d3624466b56dd5b1886c1dff500525b544c19c83
-- 
2.25.1

[PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply

[Kari Argillander]

If we do not use kmalloc_array we get checkpatch warning. It is also
little safer if something goes wrong with coding.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 fs/ntfs3/bitmap.c | 2 +-
 fs/ntfs3/index.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ntfs3/bitmap.c b/fs/ntfs3/bitmap.c
index 831501555009..aa0b1ea66cd0 100644
--- a/fs/ntfs3/bitmap.c
+++ b/fs/ntfs3/bitmap.c
@@ -1331,7 +1331,7 @@ int wnd_extend(struct wnd_bitmap *wnd, size_t new_bits)
 		new_last = wbits;
 
 	if (new_wnd != wnd->nwnd) {
-		new_free = kmalloc(new_wnd * sizeof(u16), GFP_NOFS);
+		new_free = kmalloc_array(new_wnd, sizeof(u16), GFP_NOFS);
 		if (!new_free)
 			return -ENOMEM;
 
diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
index 0daca9adc54c..7676a4a657d5 100644
--- a/fs/ntfs3/index.c
+++ b/fs/ntfs3/index.c
@@ -685,7 +685,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
 	if (end > 0x10000)
 		goto next;
 
-	offs = kmalloc(sizeof(u16) * nslots, GFP_NOFS);
+	offs = kmalloc_array(nslots, sizeof(u16), GFP_NOFS);
 	if (!offs)
 		goto next;
 
@@ -707,7 +707,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
 		u16 *ptr;
 		int new_slots = ALIGN(2 * nslots, 8);
 
-		ptr = kmalloc(sizeof(u16) * new_slots, GFP_NOFS);
+		ptr = kmalloc_array(new_slots, sizeof(u16), GFP_NOFS);
 		if (ptr)
 			memcpy(ptr, offs, sizeof(u16) * max_idx);
 		kfree(offs);
-- 
2.25.1

Re: [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply

[Joe Perches]

On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> If we do not use kmalloc_array we get checkpatch warning. It is also
> little safer if something goes wrong with coding.
[]
> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
[]
> @@ -707,7 +707,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
>  		u16 *ptr;
>  		int new_slots = ALIGN(2 * nslots, 8);
>  
> 
> -		ptr = kmalloc(sizeof(u16) * new_slots, GFP_NOFS);
> +		ptr = kmalloc_array(new_slots, sizeof(u16), GFP_NOFS);
>  		if (ptr)
>  			memcpy(ptr, offs, sizeof(u16) * max_idx);

This multiplication could also overflow.

Maybe use krealloc?

Re: [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply

[Kari Argillander]

On Tue, Aug 31, 2021 at 07:40:58PM -0700, Joe Perches wrote:
> On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> > If we do not use kmalloc_array we get checkpatch warning. It is also
> > little safer if something goes wrong with coding.
> []
> > diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> []
> > @@ -707,7 +707,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
> >  		u16 *ptr;
> >  		int new_slots = ALIGN(2 * nslots, 8);
> >  
> > 
> > -		ptr = kmalloc(sizeof(u16) * new_slots, GFP_NOFS);
> > +		ptr = kmalloc_array(new_slots, sizeof(u16), GFP_NOFS);
> >  		if (ptr)
> >  			memcpy(ptr, offs, sizeof(u16) * max_idx);
> 
> This multiplication could also overflow.
> 
> Maybe use krealloc?

Seems to fit lot better here. But as I was watching this it seems that
we do not even need to resize this array. It is quite costly operation
compared to what entry compare cost.

We just need to compare it and if entry diff > 0 then we start entry
table again from zero without need resize array. It may be that we do
not even need to allocate memory. We can probably survive with stack,
but let's think that later.

This can also speed up search a lot. It is quite odd that we always fill
whole table and will not never check if we are over. Worst case is very
very bad. With this change this will be more like jump search but I
think it will be good in this case because we won't need memory allocation.

Thanks Joe.

RE: [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply

[David Laight]

From: Joe Perches
> Sent: 01 September 2021 03:41
> 
> On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> > If we do not use kmalloc_array we get checkpatch warning. It is also
> > little safer if something goes wrong with coding.
> []
> > diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> []
> > @@ -707,7 +707,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
> >  		u16 *ptr;
> >  		int new_slots = ALIGN(2 * nslots, 8);
> >
> >
> > -		ptr = kmalloc(sizeof(u16) * new_slots, GFP_NOFS);
> > +		ptr = kmalloc_array(new_slots, sizeof(u16), GFP_NOFS);
> >  		if (ptr)
> >  			memcpy(ptr, offs, sizeof(u16) * max_idx);
> 
> This multiplication could also overflow.

Not if kmalloc_array() has suceeded.
OTOH the ALIGN(2 * nslots, 8) can also go wrong.
(But probably not if the previous kmalloc() for 1/2 the size worked.)

But there really ought to be some kind of bound check earlier.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'

[Kari Argillander]

Use consistent spacing around '+' for better code reading. Checkpatch
will also be happy.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 fs/ntfs3/frecord.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index 938b12d56ca6..b207d260ac06 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -1451,7 +1451,7 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size,
 		attr->res.flags = RESIDENT_FLAG_INDEXED;
 
 		/* is_attr_indexed(attr)) == true */
-		le16_add_cpu(&ni->mi.mrec->hard_links, +1);
+		le16_add_cpu(&ni->mi.mrec->hard_links, + 1);
 		ni->mi.dirty = true;
 	}
 	attr->res.res = 0;
-- 
2.25.1

Re: [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'

[Joe Perches]

On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> Use consistent spacing around '+' for better code reading. Checkpatch
> will also be happy.

I think you should remove the + instead

> diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
[]
> @@ -1451,7 +1451,7 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size,
>  		attr->res.flags = RESIDENT_FLAG_INDEXED;
>  
> 
>  		/* is_attr_indexed(attr)) == true */
> -		le16_add_cpu(&ni->mi.mrec->hard_links, +1);
> +		le16_add_cpu(&ni->mi.mrec->hard_links, + 1);

		le16_add_cpu(&ni->mi.mrec->hard_links, 1);

Re: [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'

[Kari Argillander]

On Wed, Sep 01, 2021 at 09:22:19PM -0700, Joe Perches wrote:
> On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> > Use consistent spacing around '+' for better code reading. Checkpatch
> > will also be happy.
> 
> I think you should remove the + instead

Konstantin can you look this as this was introduce by you just couple
days ago? 

> 
> > diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
> []
> > @@ -1451,7 +1451,7 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size,
> >  		attr->res.flags = RESIDENT_FLAG_INDEXED;
> >  
> > 
> >  		/* is_attr_indexed(attr)) == true */
> > -		le16_add_cpu(&ni->mi.mrec->hard_links, +1);
> > +		le16_add_cpu(&ni->mi.mrec->hard_links, + 1);
> 
> 		le16_add_cpu(&ni->mi.mrec->hard_links, 1);
> 
> 

Re: [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'

[Konstantin Komarov]

On 02.09.2021 11:10, Kari Argillander wrote:
> On Wed, Sep 01, 2021 at 09:22:19PM -0700, Joe Perches wrote:
>> On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
>>> Use consistent spacing around '+' for better code reading. Checkpatch
>>> will also be happy.
>>
>> I think you should remove the + instead
> 
> Konstantin can you look this as this was introduce by you just couple
> days ago? 
> 
>>
>>> diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
>> []
>>> @@ -1451,7 +1451,7 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size,
>>>  		attr->res.flags = RESIDENT_FLAG_INDEXED;
>>>  
>>>
>>>  		/* is_attr_indexed(attr)) == true */
>>> -		le16_add_cpu(&ni->mi.mrec->hard_links, +1);
>>> +		le16_add_cpu(&ni->mi.mrec->hard_links, + 1);
>>
>> 		le16_add_cpu(&ni->mi.mrec->hard_links, 1);
>>
>>

Hi, Kari, Joe!
Yes, '+' can be removed, it was added for better visibility,
but it's subjective. 
After checking code - it's the only place with '+1', so let's remove it.

Re: [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'

[Kari Argillander]

On Tue, Sep 07, 2021 at 07:23:13PM +0300, Konstantin Komarov wrote:
> 
> 
> On 02.09.2021 11:10, Kari Argillander wrote:
> > On Wed, Sep 01, 2021 at 09:22:19PM -0700, Joe Perches wrote:
> >> On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> >>> Use consistent spacing around '+' for better code reading. Checkpatch
> >>> will also be happy.
> >>
> >> I think you should remove the + instead
> > 
> > Konstantin can you look this as this was introduce by you just couple
> > days ago? 
> > 
> >>
> >>> diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
> >> []
> >>> @@ -1451,7 +1451,7 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size,
> >>>  		attr->res.flags = RESIDENT_FLAG_INDEXED;
> >>>  
> >>>
> >>>  		/* is_attr_indexed(attr)) == true */
> >>> -		le16_add_cpu(&ni->mi.mrec->hard_links, +1);
> >>> +		le16_add_cpu(&ni->mi.mrec->hard_links, + 1);
> >>
> >> 		le16_add_cpu(&ni->mi.mrec->hard_links, 1);
> >>
> >>
> 
> Hi, Kari, Joe!
> Yes, '+' can be removed, it was added for better visibility,
> but it's subjective. 
> After checking code - it's the only place with '+1', so let's remove it.

Already done in v2 [1].

[1]: lore.kernel.org/ntfs3/20210907083441.3212-1-kari.argillander@gmail.com

[PATCH 3/5] fs/ntfs3: Place Comparisons constant right side of the test

[Kari Argillander]

For better code readability place constant always right side of the
test. This will also address checkpatch warning.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 fs/ntfs3/frecord.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index b207d260ac06..bc3887635012 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -1606,7 +1606,7 @@ struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type,
 
 	*le = NULL;
 
-	if (FILE_NAME_POSIX == name_type)
+	if (name_type == FILE_NAME_POSIX)
 		return NULL;
 
 	/* Enumerate all names. */
-- 
2.25.1

[PATCH 4/5] fs/ntfs3: Remove braces from single statment block

[Kari Argillander]

Remove braces from single statment block as they are not needed. Also
Linux kernel coding style guide recommend this and checkpatch warn about
this.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 fs/ntfs3/frecord.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index bc3887635012..61448325c4d1 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -2906,9 +2906,9 @@ bool ni_remove_name_undo(struct ntfs_inode *dir_ni, struct ntfs_inode *ni,
 		memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de + 1, de_key_size);
 		mi_get_ref(&ni->mi, &de->ref);
 
-		if (indx_insert_entry(&dir_ni->dir, dir_ni, de, sbi, NULL, 1)) {
+		if (indx_insert_entry(&dir_ni->dir, dir_ni, de, sbi, NULL, 1))
 			return false;
-		}
+
 	}
 
 	return true;
-- 
2.25.1