Re: [PATCH] staging: exfat: add millisecond support
On Sun, Sep 08, 2019 at 02:47:35PM +, Valentin Vidić wrote:
> On Sun, Sep 08, 2019 at 02:03:37PM +0100, Greg Kroah-Hartman wrote:
> > Please run checkpatch on your patches so that we don't have to go and
> > fix up those issues later on.
>
> Strange, it did not report anything for me:
>
> total: 0 errors, 0 warnings, 0 checks, 439 lines checked
> 0001-staging-exfat-add-millisecond-support.patch has no obvious style
> problems and is ready for submission.
See my response to the broken out patch as to where it should have
complained.
> > Also, can you break this up into smaller patches please? You are doing
> > multiple things all at once.
>
> Sure, I was just trying to improve the code a bit :)
No problem, it's much appreciated.
> > And, are you sure about the millisecond field for access time stuff? It
> > was obviously added for some reason (there are lots in the spec that the
> > code does not yet cover, this seems odd being the other way around).
> > Did you test it against any other operating system exfat images to
> > ensure that it really is not being used at all? If so, which ones?
>
> Don't really have access to another OS, but here is what exfat-fuse has:
>
> struct exfat_entry_meta1/* file or directory info
> (part 1) */
> {
> uint8_t type; /* EXFAT_ENTRY_FILE */
> uint8_t continuations;
> le16_t checksum;
> le16_t attrib; /* combination of
> EXFAT_ATTRIB_xxx */
> le16_t __unknown1;
> le16_t crtime, crdate; /* creation date and time */
> le16_t mtime, mdate;/* latest modification date
> and time */
> le16_t atime, adate;/* latest access date and
> time */
> uint8_t crtime_cs; /* creation time in
> cs (centiseconds) */
> uint8_t mtime_cs; /* latest
> modification time in cs */
> uint8_t __unknown2[10];
> }
>
> The spec matches this and defines 3 additional UtcOffset fields that we don't
> use:
I would only go off of the spec, who knows where exfat-fuse got its
information from :)
> EntryType
> SecondaryCount
> SetChecksum
> FileAttributes
> Reserved1
> CreateTimestamp
> LastModifiedTimestamp
> LastAccessedTimestamp
> Create10msIncrement
> LastModified10msIncrement
>
> CreateUtcOffset (1 byte)
> LastModifiedUtcOffset (1 byte)
> LastAccessedUtcOffset (1 byte)
> Reserved2 (7 bytes)
>
> So I'm not sure where access_time_ms came from. In any case it was always set
> to
> 0 so it should not matter much?
If it really is CreateUtcOffset, we should use that.
For things like messing with fields, testing on another operating system
to ensure we got this right is going to be very essential. Virtual
machines of osx or windows might be a good way to do that.
thanks,
greg k-h
Re: [PATCH] staging: exfat: add millisecond support
On Sun, Sep 08, 2019 at 02:03:37PM +0100, Greg Kroah-Hartman wrote:
> Please run checkpatch on your patches so that we don't have to go and
> fix up those issues later on.
Strange, it did not report anything for me:
total: 0 errors, 0 warnings, 0 checks, 439 lines checked
0001-staging-exfat-add-millisecond-support.patch has no obvious style problems
and is ready for submission.
> Also, can you break this up into smaller patches please? You are doing
> multiple things all at once.
Sure, I was just trying to improve the code a bit :)
> And, are you sure about the millisecond field for access time stuff? It
> was obviously added for some reason (there are lots in the spec that the
> code does not yet cover, this seems odd being the other way around).
> Did you test it against any other operating system exfat images to
> ensure that it really is not being used at all? If so, which ones?
Don't really have access to another OS, but here is what exfat-fuse has:
struct exfat_entry_meta1/* file or directory info (part
1) */
{
uint8_t type; /* EXFAT_ENTRY_FILE */
uint8_t continuations;
le16_t checksum;
le16_t attrib; /* combination of
EXFAT_ATTRIB_xxx */
le16_t __unknown1;
le16_t crtime, crdate; /* creation date and time */
le16_t mtime, mdate;/* latest modification date and
time */
le16_t atime, adate;/* latest access date and time
*/
uint8_t crtime_cs; /* creation time in cs
(centiseconds) */
uint8_t mtime_cs; /* latest modification
time in cs */
uint8_t __unknown2[10];
}
The spec matches this and defines 3 additional UtcOffset fields that we don't
use:
EntryType
SecondaryCount
SetChecksum
FileAttributes
Reserved1
CreateTimestamp
LastModifiedTimestamp
LastAccessedTimestamp
Create10msIncrement
LastModified10msIncrement
CreateUtcOffset (1 byte)
LastModifiedUtcOffset (1 byte)
LastAccessedUtcOffset (1 byte)
Reserved2 (7 bytes)
So I'm not sure where access_time_ms came from. In any case it was always set to
0 so it should not matter much?
--
Valentin
Re: [PATCH] staging: exfat: add millisecond support
On Sun, Sep 08, 2019 at 12:48:08PM +, Valentin Vidic wrote: > Drop duplicated date_time_t struct and add millisecond handling for > create and modify time. Also drop millisecond field for access time > since it is not defined in the the spec. > > Signed-off-by: Valentin Vidic > --- > drivers/staging/exfat/exfat.h | 38 +++--- > drivers/staging/exfat/exfat_core.c | 31 - > drivers/staging/exfat/exfat_super.c | 174 > 3 files changed, 92 insertions(+), 151 deletions(-) Please run checkpatch on your patches so that we don't have to go and fix up those issues later on. Also, can you break this up into smaller patches please? You are doing multiple things all at once. And, are you sure about the millisecond field for access time stuff? It was obviously added for some reason (there are lots in the spec that the code does not yet cover, this seems odd being the other way around). Did you test it against any other operating system exfat images to ensure that it really is not being used at all? If so, which ones? thanks, greg k-h
[PATCH] staging: exfat: add millisecond support
Drop duplicated date_time_t struct and add millisecond handling for
create and modify time. Also drop millisecond field for access time
since it is not defined in the the spec.
Signed-off-by: Valentin Vidic
---
drivers/staging/exfat/exfat.h | 38 +++---
drivers/staging/exfat/exfat_core.c | 31 -
drivers/staging/exfat/exfat_super.c | 174
3 files changed, 92 insertions(+), 151 deletions(-)
diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
index 0aa14dea4e09..6491ea034928 100644
--- a/drivers/staging/exfat/exfat.h
+++ b/drivers/staging/exfat/exfat.h
@@ -241,16 +241,6 @@ static inline u16 get_row_index(u16 i)
#define UNI_PAR_DIR_NAME"\0.\0."
#endif
-struct date_time_t {
- u16 Year;
- u16 Month;
- u16 Day;
- u16 Hour;
- u16 Minute;
- u16 Second;
- u16 MilliSecond;
-};
-
struct part_info_t {
u32 Offset;/* start sector number of the partition */
u32 Size; /* in sectors */
@@ -289,6 +279,16 @@ struct file_id_t {
u32 hint_last_clu;
};
+struct timestamp_t {
+ u16 millisec; /* 0 ~ 999 */
+ u16 sec;/* 0 ~ 59 */
+ u16 min;/* 0 ~ 59 */
+ u16 hour; /* 0 ~ 23 */
+ u16 day;/* 1 ~ 31 */
+ u16 mon;/* 1 ~ 12 */
+ u16 year; /* 0 ~ 127 (since 1980) */
+};
+
struct dir_entry_t {
char Name[MAX_NAME_LENGTH * MAX_CHARSET_SIZE];
@@ -298,18 +298,9 @@ struct dir_entry_t {
u32 Attr;
u64 Size;
u32 NumSubdirs;
- struct date_time_t CreateTimestamp;
- struct date_time_t ModifyTimestamp;
- struct date_time_t AccessTimestamp;
-};
-
-struct timestamp_t {
- u16 sec;/* 0 ~ 59 */
- u16 min;/* 0 ~ 59 */
- u16 hour; /* 0 ~ 23 */
- u16 day;/* 1 ~ 31 */
- u16 mon;/* 1 ~ 12 */
- u16 year; /* 0 ~ 127 (since 1980) */
+ struct timestamp_t CreateTimestamp;
+ struct timestamp_t ModifyTimestamp;
+ struct timestamp_t AccessTimestamp;
};
/* MS_DOS FAT partition boot record (512 bytes) */
@@ -453,8 +444,7 @@ struct file_dentry_t {
u8 access_date[2];
u8 create_time_ms;
u8 modify_time_ms;
- u8 access_time_ms;
- u8 reserved2[9];
+ u8 reserved2[10];
};
/* MS-DOS EXFAT stream extension directory entry (32 bytes) */
diff --git a/drivers/staging/exfat/exfat_core.c
b/drivers/staging/exfat/exfat_core.c
index 995358cc7c79..e87119fa8c0a 100644
--- a/drivers/staging/exfat/exfat_core.c
+++ b/drivers/staging/exfat/exfat_core.c
@@ -1139,6 +1139,7 @@ void exfat_set_entry_size(struct dentry_t *p_entry, u64
size)
void fat_get_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
u8 mode)
{
+ u8 ms = 0;
u16 t = 0x00, d = 0x21;
struct dos_dentry_t *ep = (struct dos_dentry_t *)p_entry;
@@ -1146,6 +1147,7 @@ void fat_get_entry_time(struct dentry_t *p_entry, struct
timestamp_t *tp,
case TM_CREATE:
t = GET16_A(ep->create_time);
d = GET16_A(ep->create_date);
+ ms = ep->create_time_ms * 10;
break;
case TM_MODIFY:
t = GET16_A(ep->modify_time);
@@ -1159,11 +1161,17 @@ void fat_get_entry_time(struct dentry_t *p_entry,
struct timestamp_t *tp,
tp->day = (d & 0x001F);
tp->mon = (d >> 5) & 0x000F;
tp->year = (d >> 9);
+
+ if (ms >= 1000) {
+ ms -= 1000;
+ tp->sec++;
+ }
}
void exfat_get_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
u8 mode)
{
+ u8 ms = 0;
u16 t = 0x00, d = 0x21;
struct file_dentry_t *ep = (struct file_dentry_t *)p_entry;
@@ -1171,10 +1179,12 @@ void exfat_get_entry_time(struct dentry_t *p_entry,
struct timestamp_t *tp,
case TM_CREATE:
t = GET16_A(ep->create_time);
d = GET16_A(ep->create_date);
+ ms = ep->create_time_ms * 10;
break;
case TM_MODIFY:
t = GET16_A(ep->modify_time);
d = GET16_A(ep->modify_date);
+ ms = ep->modify_time_ms * 10;
break;
case TM_ACCESS:
t = GET16_A(ep->access_time);
@@ -1188,21 +1198,33 @@ void exfat_get_entry_time(struct dentry_t *p_entry,
struct timestamp_t *tp,
tp->day = (d & 0x001F);
tp->mon = (d >> 5) & 0x000F;
tp->year = (d >> 9);
+
+ if (ms >= 1000) {
+ ms -= 1000;
+ tp->sec++;
+ }
}
