LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/5] fs/ntfs3: Fix various styling issues
@ 2021-08-31 18:15 Kari Argillander
  2021-08-31 18:15 ` [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply Kari Argillander
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Kari Argillander @ 2021-08-31 18:15 UTC (permalink / raw)
  To: Konstantin Komarov, ntfs3; +Cc: Kari Argillander, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply
  2021-08-31 18:15 [PATCH 0/5] fs/ntfs3: Fix various styling issues Kari Argillander
@ 2021-08-31 18:15 ` Kari Argillander
  2021-09-01  2:40   ` Joe Perches
  2021-08-31 18:15 ` [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+' Kari Argillander
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Kari Argillander @ 2021-08-31 18:15 UTC (permalink / raw)
  To: Konstantin Komarov, ntfs3; +Cc: Kari Argillander, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'
  2021-08-31 18:15 [PATCH 0/5] fs/ntfs3: Fix various styling issues Kari Argillander
  2021-08-31 18:15 ` [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply Kari Argillander
@ 2021-08-31 18:15 ` Kari Argillander
  2021-09-02  4:22   ` Joe Perches
  2021-08-31 18:15 ` [PATCH 3/5] fs/ntfs3: Place Comparisons constant right side of the test Kari Argillander
  2021-08-31 18:15 ` [PATCH 4/5] fs/ntfs3: Remove braces from single statment block Kari Argillander
  3 siblings, 1 reply; 12+ messages in thread
From: Kari Argillander @ 2021-08-31 18:15 UTC (permalink / raw)
  To: Konstantin Komarov, ntfs3; +Cc: Kari Argillander, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/5] fs/ntfs3: Place Comparisons constant right side of the test
  2021-08-31 18:15 [PATCH 0/5] fs/ntfs3: Fix various styling issues Kari Argillander
  2021-08-31 18:15 ` [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply Kari Argillander
  2021-08-31 18:15 ` [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+' Kari Argillander
@ 2021-08-31 18:15 ` Kari Argillander
  2021-08-31 18:15 ` [PATCH 4/5] fs/ntfs3: Remove braces from single statment block Kari Argillander
  3 siblings, 0 replies; 12+ messages in thread
From: Kari Argillander @ 2021-08-31 18:15 UTC (permalink / raw)
  To: Konstantin Komarov, ntfs3; +Cc: Kari Argillander, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 4/5] fs/ntfs3: Remove braces from single statment block
  2021-08-31 18:15 [PATCH 0/5] fs/ntfs3: Fix various styling issues Kari Argillander
                   ` (2 preceding siblings ...)
  2021-08-31 18:15 ` [PATCH 3/5] fs/ntfs3: Place Comparisons constant right side of the test Kari Argillander
@ 2021-08-31 18:15 ` Kari Argillander
  3 siblings, 0 replies; 12+ messages in thread
From: Kari Argillander @ 2021-08-31 18:15 UTC (permalink / raw)
  To: Konstantin Komarov, ntfs3; +Cc: Kari Argillander, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply
  2021-08-31 18:15 ` [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply Kari Argillander
@ 2021-09-01  2:40   ` Joe Perches
  2021-09-01 13:24     ` Kari Argillander
  2021-09-02 14:03     ` David Laight
  0 siblings, 2 replies; 12+ messages in thread
From: Joe Perches @ 2021-09-01  2:40 UTC (permalink / raw)
  To: Kari Argillander, Konstantin Komarov, ntfs3; +Cc: linux-kernel

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?



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply
  2021-09-01  2:40   ` Joe Perches
@ 2021-09-01 13:24     ` Kari Argillander
  2021-09-02 14:03     ` David Laight
  1 sibling, 0 replies; 12+ messages in thread
From: Kari Argillander @ 2021-09-01 13:24 UTC (permalink / raw)
  To: Joe Perches; +Cc: Konstantin Komarov, ntfs3, linux-kernel

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'
  2021-08-31 18:15 ` [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+' Kari Argillander
@ 2021-09-02  4:22   ` Joe Perches
  2021-09-02  8:10     ` Kari Argillander
  0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2021-09-02  4:22 UTC (permalink / raw)
  To: Kari Argillander, Konstantin Komarov, ntfs3; +Cc: linux-kernel

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);



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'
  2021-09-02  4:22   ` Joe Perches
@ 2021-09-02  8:10     ` Kari Argillander
  2021-09-07 16:23       ` Konstantin Komarov
  0 siblings, 1 reply; 12+ messages in thread
From: Kari Argillander @ 2021-09-02  8:10 UTC (permalink / raw)
  To: Konstantin Komarov; +Cc: Joe Perches, ntfs3, linux-kernel

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);
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply
  2021-09-01  2:40   ` Joe Perches
  2021-09-01 13:24     ` Kari Argillander
@ 2021-09-02 14:03     ` David Laight
  1 sibling, 0 replies; 12+ messages in thread
From: David Laight @ 2021-09-02 14:03 UTC (permalink / raw)
  To: 'Joe Perches', Kari Argillander, Konstantin Komarov, ntfs3
  Cc: linux-kernel

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)


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'
  2021-09-02  8:10     ` Kari Argillander
@ 2021-09-07 16:23       ` Konstantin Komarov
  2021-09-07 16:25         ` Kari Argillander
  0 siblings, 1 reply; 12+ messages in thread
From: Konstantin Komarov @ 2021-09-07 16:23 UTC (permalink / raw)
  To: Kari Argillander; +Cc: Joe Perches, ntfs3, linux-kernel



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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+'
  2021-09-07 16:23       ` Konstantin Komarov
@ 2021-09-07 16:25         ` Kari Argillander
  0 siblings, 0 replies; 12+ messages in thread
From: Kari Argillander @ 2021-09-07 16:25 UTC (permalink / raw)
  To: Konstantin Komarov; +Cc: Joe Perches, ntfs3, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-09-07 16:25 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 18:15 [PATCH 0/5] fs/ntfs3: Fix various styling issues Kari Argillander
2021-08-31 18:15 ` [PATCH 1/5] fs/ntfs3: Use kmalloc_array over kmalloc with multiply Kari Argillander
2021-09-01  2:40   ` Joe Perches
2021-09-01 13:24     ` Kari Argillander
2021-09-02 14:03     ` David Laight
2021-08-31 18:15 ` [PATCH 2/5] fs/ntfs3: Use consistent spacing around '+' Kari Argillander
2021-09-02  4:22   ` Joe Perches
2021-09-02  8:10     ` Kari Argillander
2021-09-07 16:23       ` Konstantin Komarov
2021-09-07 16:25         ` Kari Argillander
2021-08-31 18:15 ` [PATCH 3/5] fs/ntfs3: Place Comparisons constant right side of the test Kari Argillander
2021-08-31 18:15 ` [PATCH 4/5] fs/ntfs3: Remove braces from single statment block Kari Argillander

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).