| Message ID | 20200302095716.64155-1-Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] staging: exfat: clean up d_entry rebuilding. | expand |
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...)
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>
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>
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);
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(-)