Re: [PATCH] staging: exfat: add millisecond support

[Greg Kroah-Hartman]

On Sun, Sep 08, 2019 at 02:47:35PM +0000, 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