Re: [PATCH v3 2/4] staging: exfat: drop duplicate date_time_t struct
On Sun, Sep 08, 2019 at 07:50:31PM +0100, Greg Kroah-Hartman wrote:
> Wait, how are these "duplicate"? The fields are in different order,
> don't these refer to things on-disk?
On-disk combines the values from these structures in a different form:
offset bits
DoubleSeconds 0 5
Minute 5 6
Hour 11 5
Day 16 5
Month 21 4
Year 25 7
> Did you test this?
Just compile tested for now.
> > -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) */
> > +};
>
> They really look "backwards" to me, how are these the same? What am I
> missing?
date_time_t was only used in a few functions and there was a lot of
copying of the same fields between the two structs. Also some code was
duplicated to do the same thing for each of the structs.
--
Valentin
Re: [PATCH v3 2/4] staging: exfat: drop duplicate date_time_t struct
On Sun, Sep 08, 2019 at 07:54:24PM +0100, Greg Kroah-Hartman wrote:
> On Sun, Sep 08, 2019 at 05:35:37PM +, Valentin Vidic wrote:
> > +struct timestamp_t {
> > + u16 millisec; /* 0 ~ 999 */
>
> You added this field to this structure, why? You did not document that
> in the changelog text above. Are you _sure_ you can do this and that
> this does not refer to an on-disk layout?
Both date_time_t and timestamp_t were used in memory only, but
date_time_t had the additional MilliSecond field. To keep the
functionality I added the millisec field to timestamp_t and
replaced all usages of date_time_t with timestamp_t.
For storing on disk the values from timestamp_t get shifted
and combined (exfat_set_entry_time).
--
Valentin
Re: [PATCH v3 2/4] staging: exfat: drop duplicate date_time_t struct
On Sun, Sep 08, 2019 at 05:35:37PM +, Valentin Vidic wrote:
> Use timestamp_t for everything and cleanup duplicate code.
>
> Signed-off-by: Valentin Vidic
> ---
> drivers/staging/exfat/exfat.h | 35 +++---
> drivers/staging/exfat/exfat_super.c | 158
> 2 files changed, 55 insertions(+), 138 deletions(-)
>
> diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
> index 0aa14dea4e09..58e1e889779f 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 */
You added this field to this structure, why? You did not document that
in the changelog text above. Are you _sure_ you can do this and that
this does not refer to an on-disk layout?
thanks,
greg k-h
Re: [PATCH v3 2/4] staging: exfat: drop duplicate date_time_t struct
On Sun, Sep 08, 2019 at 05:35:37PM +, Valentin Vidic wrote:
> Use timestamp_t for everything and cleanup duplicate code.
Wait, how are these "duplicate"? The fields are in different order,
don't these refer to things on-disk?
Did you test this?
> -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) */
> +};
They really look "backwards" to me, how are these the same? What am I
missing?
thanks,
greg k-h
[PATCH v3 2/4] staging: exfat: drop duplicate date_time_t struct
Use timestamp_t for everything and cleanup duplicate code.
Signed-off-by: Valentin Vidic
---
drivers/staging/exfat/exfat.h | 35 +++---
drivers/staging/exfat/exfat_super.c | 158
2 files changed, 55 insertions(+), 138 deletions(-)
diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
index 0aa14dea4e09..58e1e889779f 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) */
diff --git a/drivers/staging/exfat/exfat_super.c
b/drivers/staging/exfat/exfat_super.c
index 3a58907caf7c..54b6c2ff3c96 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -56,16 +56,16 @@ static void exfat_write_super(struct super_block *sb);
#define UNIX_SECS_21084354819200L
/* Convert a FAT time/date pair to a UNIX date (seconds since 1 1 70). */
-static void exfat_time_fat2unix(struct timespec64 *ts, struct date_time_t *tp)
+static void exfat_time_fat2unix(struct timespec64 *ts, struct timestamp_t *tp)
{
- ts->tv_sec = mktime64(tp->Year + 1980, tp->Month + 1, tp->Day,
- tp->Hour, tp->Minute, tp->Second);
+ ts->tv_sec = mktime64(tp->year + 1980, tp->mon + 1, tp->day,
+ tp->hour, tp->min, tp->sec);
- ts->tv_nsec = tp->MilliSecond * NSEC_PER_MSEC;
+ ts->tv_nsec = tp->millisec * NSEC_PER_MSEC;
}
/* Convert linear UNIX date to a FAT time/date pair. */
-static void exfat_time_unix2fat(struct timespec64 *ts, struct date_time_t *tp)
+static void exfat_time_unix2fat(struct timespec64 *ts, struct timestamp_t *tp)
{
time64_t second = ts->tv_sec;
struct tm tm;
@@ -73,69 +73,42 @@ static void exfat_time_unix2fat(struct timespec64 *ts,
struct date_time_t *tp)
time64_to_tm(second, 0, );
if (second < UNIX_SECS_1980) {
- tp->MilliSecond = 0;
- tp->Second = 0;
- tp->Minute = 0;
- tp->Hour= 0;
- tp->Day = 1;
- tp->Month = 1;
- tp->Year= 0;
+ tp->millisec= 0;
+ tp->sec = 0;
+ tp->min = 0;
+ tp->hour= 0;
+ tp->day = 1;
+ tp->mon = 1;
+ tp->year= 0;
return;
}
if (second >= UNIX_SECS_2108) {
- tp->MilliSecond = 999;
- tp->Second = 59;
- tp->Minute = 59;
- tp->Hour= 23;
- tp->Day = 31;
- tp->Month = 12;
- tp->Year= 127;
+ tp->millisec= 999;
+ tp->sec = 59;
+ tp->min = 59;
+ tp->hour= 23;
+ tp->day = 31;
+ tp->mon = 12;
+ tp->year= 127;
return;
}
- tp->MilliSecond = ts->tv_nsec / NSEC_PER_MSEC;
- tp->Second = tm.tm_sec;
- tp->Minute = tm.tm_min;
- tp->Hour=
