Hi, Rover,
This patch seems to violate the exFAT specification below.
Please refer to the description for ATTR_ARCHIVE in FAT32 Spec.
* Archive
This field is mandatory and conforms to the MS-DOS definition.
* ATTR_ARCHIVE
This attribute supports backup utilities. This bit is set by the FAT file
system driver when a file is created, renamed, or written to. Backup
utilities may use this attribute to indicate which files on the volume
have been modified since the last time that a backup was performed.
Thanks.
B.R.
Sungjong Seo
> When renaming, FileAttributes remain unchanged, do not need to be
> set, so the code that sets FileAttributes is unneeded, remove it.
>
> Signed-off-by: rover <[email protected]>
> ---
> fs/exfat/namei.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
> diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
> index b617bebc3d0f..5ffaf553155e 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -1031,10 +1031,6 @@ static int exfat_rename_file(struct inode *inode,
> struct exfat_chain *p_dir,
> return -EIO;
>
> *epnew = *epold;
> - if (exfat_get_entry_type(epnew) == TYPE_FILE) {
> - epnew->dentry.file.attr |=
> cpu_to_le16(ATTR_ARCHIVE);
> - ei->attr |= ATTR_ARCHIVE;
> - }
> exfat_update_bh(new_bh, sync);
> brelse(old_bh);
> brelse(new_bh);
> @@ -1063,10 +1059,6 @@ static int exfat_rename_file(struct inode *inode,
> struct exfat_chain *p_dir,
> ei->dir = *p_dir;
> ei->entry = newentry;
> } else {
> - if (exfat_get_entry_type(epold) == TYPE_FILE) {
> - epold->dentry.file.attr |=
> cpu_to_le16(ATTR_ARCHIVE);
> - ei->attr |= ATTR_ARCHIVE;
> - }
> exfat_update_bh(old_bh, sync);
> brelse(old_bh);
> ret = exfat_init_ext_entry(inode, p_dir, oldentry,
> @@ -1112,10 +1104,6 @@ static int exfat_move_file(struct inode *inode,
> struct exfat_chain *p_olddir,
> return -EIO;
>
> *epnew = *epmov;
> - if (exfat_get_entry_type(epnew) == TYPE_FILE) {
> - epnew->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE);
> - ei->attr |= ATTR_ARCHIVE;
> - }
> exfat_update_bh(new_bh, IS_DIRSYNC(inode));
> brelse(mov_bh);
> brelse(new_bh);
> --
> 2.25.1
> Hi Sungjong,
>
> This patch is to remove the duplicate setting ATTR_ARCHIVE when renaming.
>
> We know that a file will not become a directory after being renamed, and a
> directory will not become a file after being renamed.
Sure, rename-op does not change any types of files.
> >>
> >> *epnew = *epold;
> So ATTR_ARCHIVE is already set here if rename file.
Do you think ATTR_ARCHIVE represents the file type? If so, you probably
are misunderstanding ATTR_ARCHIVE. It's just like a flag indicating that
there was a change. For example, in case of rename, it's just like a flag
indicating that its name has been changed.
The current linux exfat implementation does not yet provide an IOCTL
to control this attribute, but keep in mind that the ATTR_ARCHIVE may
be removed even if it is a file via another exfat driver.
B.R.
Sungjong Seo
>
> >> - if (exfat_get_entry_type(epnew) == TYPE_FILE) {
> >> - epnew->dentry.file.attr |=
> cpu_to_le16(ATTR_ARCHIVE);
> No need to set again here.
>
> >> - ei->attr |= ATTR_ARCHIVE;
> ei->attr doesn't change and already set ATTR_ARCHIVE in
> exfat_fill_inode()
>
> >> - }
>
> Best regards,
> Rover
>
> ------------------ Original ------------------
> From: "Sungjong Seo" <[email protected]>;
> Date: Wed, Sep 14, 2022 04:48 PM
> To: "Rover"<[email protected]>;"'linkinjeon'"<[email protected]>;
> Cc: "'linux-kernel'"<[email protected]>;"'linux-
> fsdevel'"<linux-
> [email protected]>;"sj1557.seo"<[email protected]>;
> Subject: RE: [PATCH v1] exfat: remove the code that sets FileAttributes
> when renaming
>
>
> Hi, Rover,
>
> This patch seems to violate the exFAT specification below.
> Please refer to the description for ATTR_ARCHIVE in FAT32 Spec.
>
> * Archive
> This field is mandatory and conforms to the MS-DOS definition.
>
> * ATTR_ARCHIVE
> This attribute supports backup utilities. This bit is set by the FAT file
> system driver when a file is created, renamed, or written to. Backup
> utilities may use this attribute to indicate which files on the volume
> have been modified since the last time that a backup was performed.
>
> Thanks.
> B.R.
>
> Sungjong Seo
>
> > When renaming, FileAttributes remain unchanged, do not need to be
>
> set, so the code that sets FileAttributes is unneeded, remove it.
> >
> > Signed-off-by: rover &lt;[email protected]&gt; > ---
> > fs/exfat/namei.c | 12 ------------ > 1 file changed,
> 12 deletions(-) > > diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
> > index b617bebc3d0f..5ffaf553155e 100644 > --- a/fs/exfat/namei.c
> > +++ b/fs/exfat/namei.c > @@ -1031,10 +1031,6 @@ static int
> exfat_rename_file(struct inode *inode, > struct exfat_chain *p_dir,
> > return -EIO; > > *epnew = *epold; > -if
> (exfat_get_entry_type(epnew) == TYPE_FILE) { > - epnew-
> &gt;dentry.file.attr |= > cpu_to_le16(ATTR_ARCHIVE); > - ei-
> &gt;attr |= ATTR_ARCHIVE; > -} > exfat_update_bh(new_bh,
> sync); > brelse(old_bh); > brelse(new_bh); > @@ -
> 1063,10 +1059,6 @@ static int exfat_rename_file(struct inode *inode, >
> struct exfat_chain *p_dir, > ei-&gt;dir = *p_dir; >
> ei-&gt;entry = newentry; > } else { > -if
> (exfat_get_entry_type(epold) == TYPE_FILE) { > - epold-
> &gt;dentry.file.attr |= > cpu_to_le16(ATTR_ARCHIVE); > - ei-
> &gt;attr |= ATTR_ARCHIVE; > -} > exfat_update_bh(old_bh,
> sync); > brelse(old_bh); > ret =
> exfat_init_ext_entry(inode, p_dir, oldentry, > @@ -1112,10 +1104,6 @@
> static int exfat_move_file(struct inode *inode, > struct exfat_chain
> *p_olddir, > return -EIO; > > *epnew = *epmov; > -
> if (exfat_get_entry_type(epnew) == TYPE_FILE) { > -epnew-
> &gt;dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); > -ei-
> &gt;attr |= ATTR_ARCHIVE; > - } > exfat_update_bh(new_bh,
> IS_DIRSYNC(inode)); > brelse(mov_bh); > brelse(new_bh);
> > -- > 2.25.1</[email protected]></linux-
> [email protected]></linux-
> [email protected]></[email protected]></[email protected]>