[PATCH 1/2] staging: exfat: clean up d_entry rebuilding.

[PATCH 1/2] staging: exfat: clean up d_entry rebuilding.

[Tetsuhiro Kohada]

Clean up d_entry rebuilding in exfat_rename_file() and move_file().

-Replace memcpy of d_entry with structure copy.
-Change to use the value already stored in fid.

Signed-off-by: Tetsuhiro Kohada <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>
---
 drivers/staging/exfat/exfat_core.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c
index ceaea1ba1a83..374a4fe183f5 100644
--- a/drivers/staging/exfat/exfat_core.c
+++ b/drivers/staging/exfat/exfat_core.c
@@ -2285,12 +2285,10 @@ s32 exfat_rename_file(struct inode *inode, struct chain_t *p_dir, s32 oldentry,
 			return -ENOENT;
 		}
 
-		memcpy((void *)epnew, (void *)epold, DENTRY_SIZE);
-		if (exfat_get_entry_type(epnew) == TYPE_FILE) {
-			exfat_set_entry_attr(epnew,
-					     exfat_get_entry_attr(epnew) |
-					     ATTR_ARCHIVE);
+		*epnew = *epold;
+		if (fid->type == TYPE_FILE) {
 			fid->attr |= ATTR_ARCHIVE;
+			exfat_set_entry_attr(epnew, fid->attr);
 		}
 		exfat_buf_modify(sb, sector_new);
 		exfat_buf_unlock(sb, sector_old);
@@ -2306,7 +2304,7 @@ s32 exfat_rename_file(struct inode *inode, struct chain_t *p_dir, s32 oldentry,
 			return -ENOENT;
 		}
 
-		memcpy((void *)epnew, (void *)epold, DENTRY_SIZE);
+		*epnew = *epold;
 		exfat_buf_modify(sb, sector_new);
 		exfat_buf_unlock(sb, sector_old);
 
@@ -2319,11 +2317,9 @@ s32 exfat_rename_file(struct inode *inode, struct chain_t *p_dir, s32 oldentry,
 				       num_old_entries);
 		fid->entry = newentry;
 	} else {
-		if (exfat_get_entry_type(epold) == TYPE_FILE) {
-			exfat_set_entry_attr(epold,
-					     exfat_get_entry_attr(epold) |
-					     ATTR_ARCHIVE);
+		if (fid->type == TYPE_FILE) {
 			fid->attr |= ATTR_ARCHIVE;
+			exfat_set_entry_attr(epold, fid->attr);
 		}
 		exfat_buf_modify(sb, sector_old);
 		exfat_buf_unlock(sb, sector_old);
@@ -2387,11 +2383,10 @@ s32 move_file(struct inode *inode, struct chain_t *p_olddir, s32 oldentry,
 		return -ENOENT;
 	}
 
-	memcpy((void *)epnew, (void *)epmov, DENTRY_SIZE);
-	if (exfat_get_entry_type(epnew) == TYPE_FILE) {
-		exfat_set_entry_attr(epnew, exfat_get_entry_attr(epnew) |
-				     ATTR_ARCHIVE);
+	*epnew = *epmov;
+	if (fid->type == TYPE_FILE) {
 		fid->attr |= ATTR_ARCHIVE;
+		exfat_set_entry_attr(epnew, fid->attr);
 	}
 	exfat_buf_modify(sb, sector_new);
 	exfat_buf_unlock(sb, sector_mov);
@@ -2406,7 +2401,7 @@ s32 move_file(struct inode *inode, struct chain_t *p_olddir, s32 oldentry,
 		return -ENOENT;
 	}
 
-	memcpy((void *)epnew, (void *)epmov, DENTRY_SIZE);
+	*epnew = *epmov;
 	exfat_buf_modify(sb, sector_new);
 	exfat_buf_unlock(sb, sector_mov);
 
-- 
2.25.1

[PATCH 2/2] staging: exfat: remove redundant if statements

[Tetsuhiro Kohada]

If statement does not affect results when updating directory entry in
ffsMapCluster().

Signed-off-by: Tetsuhiro Kohada <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>
---
 drivers/staging/exfat/exfat_super.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/exfat/exfat_super.c b/drivers/staging/exfat/exfat_super.c
index 708398265828..75813d0fe7a7 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -1361,11 +1361,8 @@ static int ffsMapCluster(struct inode *inode, s32 clu_offset, u32 *clu)
 
 		/* (3) update directory entry */
 		if (modified) {
-			if (exfat_get_entry_flag(ep) != fid->flags)
-				exfat_set_entry_flag(ep, fid->flags);
-
-			if (exfat_get_entry_clu0(ep) != fid->start_clu)
-				exfat_set_entry_clu0(ep, fid->start_clu);
+			exfat_set_entry_flag(ep, fid->flags);
+			exfat_set_entry_clu0(ep, fid->start_clu);
 		}
 
 		update_dir_checksum_with_entry_set(sb, es);
-- 
2.25.1

Re: [PATCH 1/2] staging: exfat: clean up d_entry rebuilding.

[Valdis Klētnieks]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

On Mon, 02 Mar 2020 18:57:15 +0900, Tetsuhiro Kohada said:
> Clean up d_entry rebuilding in exfat_rename_file() and move_file().
>
> -Replace memcpy of d_entry with structure copy.

Those look OK.

> -Change to use the value already stored in fid.

> -		if (exfat_get_entry_type(epnew) == TYPE_FILE) {

> +		if (fid->type == TYPE_FILE) {

Are you sure this is OK to do? exfat_get_entry_type() does a lot of
mapping between values, using a file_dentry_t->type, while
fid->type is a file_id_t->type. and at first read it's not obvious to me whether
fid->type is guaranteed to have the correct value already.

(The abundant use of 0xNN constants in exfat_get_entry_type()  doesn't
inspire confidence that it's looking at what you think it's looking at...)



[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

RE: [PATCH 1/2] staging: exfat: clean up d_entry rebuilding.

[Kohada.Tetsuhiro]

Thanks for your comment.

> Are you sure this is OK to do? exfat_get_entry_type() does a lot of 
> mapping between values, using a file_dentry_t->type, while
> fid->type is a file_id_t->type.

The fid argument of exfat_rename_file()/move_file()from old_dentry->fid of exfat_rename().
 * exfat_rename_file() <- ffsMoveFile() <- exfat_rename()
 * move_file() <- ffsMoveFile() <- exfat_rename()

The value that vfs sets to the old_dentry of exfat_rename() is the dentry value returned by exfat_lookup(), exfat_create(), and create_dir().
In each function, the value of dentry->fid is initialized to fid->type at create_file(), ffsLookupFile(), and create_dir().

 * create_file() <- ffsCreateFile() <-exfat_create()
 * ffsLookupFile() <- exfat_find() <-exfat_lookup()
 * exfat_mkdir() <- ffsCreateDir() <-create_dir()

> and at first read it's not obvious to 
> fid->me whether type is guaranteed to have the correct value already.

A valid value is set in fid->type for all paths.
What do you think?

--
Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>

Re: [PATCH 1/2] staging: exfat: clean up d_entry rebuilding.

[Valdis Klētnieks]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Tue, 03 Mar 2020 03:07:51 +0000, "Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp" said:

> > Are you sure this is OK to do? exfat_get_entry_type() does a lot of
> > mapping between values, using a file_dentry_t->type, while
> > fid->type is a file_id_t->type.

> The value that vfs sets to the old_dentry of exfat_rename() is the dentry value returned by exfat_lookup(), exfat_create(), and create_dir().
> In each function, the value of dentry->fid is initialized to fid->type at create_file(), ffsLookupFile(), and create_dir().
>
>  * create_file() <- ffsCreateFile() <-exfat_create()
>  * ffsLookupFile() <- exfat_find() <-exfat_lookup()
>  * exfat_mkdir() <- ffsCreateDir() <-create_dir()
>
> > and at first read it's not obvious to
> > me whether type is guaranteed to have the correct value already.
>
> A valid value is set in fid->type for all paths.
> What do you think?

OK, that's the part I was worried about, but I hadn't had enough caffeine
to do that analysis.  Thanks.

Acked-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>


[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]