Re: [PATCH] staging: exfat: cleanup braces for if/else statements
On Tue, Sep 03, 2019 at 06:32:49PM +0100, Al Viro wrote:
> On Tue, Sep 03, 2019 at 06:47:32PM +0200, Valentin Vidic wrote:
> > + } else if (uni == 0x) {
> > skip = TRUE;
>
> While we are at it, could you get rid of that 'TRUE' macro?
Sure, but maybe in a separate patch since TRUE/FALSE has more
calls than just this.
--
Valentin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: cleanup braces for if/else statements
On Wed, Sep 04, 2019 at 09:38:55AM +, David Laight wrote:
> From: Valentin Vidic
> > Sent: 03 September 2019 19:12
> > On Tue, Sep 03, 2019 at 06:32:49PM +0100, Al Viro wrote:
> > > On Tue, Sep 03, 2019 at 06:47:32PM +0200, Valentin Vidic wrote:
> > > > + } else if (uni == 0x) {
> > > > skip = TRUE;
> > >
> > > While we are at it, could you get rid of that 'TRUE' macro?
> >
> > Sure, but maybe in a separate patch since TRUE/FALSE has more
> > calls than just this.
>
> I bet you can get rid of the 'skip' variable and simplify the code
> by using continue/break/return (or maybe goto).
Seems it is not so simple - the value of skip is used to control the
behavior in the next loop iteration based on the current uni value.
So maybe just replace skip with a less confusing name.
--
Valentin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/3] staging: exfat: add millisecond support
On Sun, Sep 08, 2019 at 05:40:40PM +0100, Greg Kroah-Hartman wrote:
> On Sun, Sep 08, 2019 at 04:10:15PM +, Valentin Vidic wrote:
> > void fat_set_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
> > u8 mode)
> > {
> > + u8 ms;
> > u16 t, d;
> > struct dos_dentry_t *ep = (struct dos_dentry_t *)p_entry;
> >
> > t = (tp->hour << 11) | (tp->min << 5) | (tp->sec >> 1);
> > d = (tp->year << 9) | (tp->mon << 5) | tp->day;
> >
> > + ms = tp->millisec;
> > + if (tp->sec & 1) {
> > + ms += 1000;
> > + }
>
> checkpatch didn't complain about this { } not being needed?
>
> Same in other parts of this patch, please fix up.
No warnings from checkpatch here, will update the code.
--
Valentin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/3] staging: exfat: drop unused field access_time_ms
On Sun, Sep 08, 2019 at 08:19:21PM -0400, Valdis Klētnieks wrote: > In that case, rather than removing it, shouldn't we be *adding* > code to properly set it instead? Right, setting the UtcOffset fields to 0 is the first step marking them as invalid for now. This is also why access_time_ms did not do any harm here - it was always set to 0 too. 7.4.10.2 OffsetValid Field The OffsetValid field shall describe whether the contents of the OffsetFromUtc field are valid or not, as follows: 0, which means the contents of the OffsetFromUtc field are invalid and shall be 00h 1, which means the contents of the OffsetFromUtc field are valid -- Valentin ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
