proto: support timestamps #88
Conversation
Signed-off-by: Akihiro Suda <[email protected]>
| @@ -65,6 +67,13 @@ message Resource { | |||
| // Ads stores one or more alternate data streams for the target resource. | |||
| repeated ADSEntry ads = 13; | |||
|
|
|||
| // Timestamps. | |||
| // btime (birthtime) is available on Darwin, FreeBSD, NetBSD, and Windows. | |||
| // When btime is unknown, it SHOULD be set to zero. | |||
There was a problem hiding this comment.
Why not omit the field instead of setting it to zero? I believe these fields are nullable.
| ATime() time.Time | ||
| MTime() time.Time | ||
| CTime() time.Time | ||
| BTime() time.Time |
There was a problem hiding this comment.
I think it's fine to use these abbreviated forms - they're pretty widely used. But it would be useful to have a comment explaining what each one means.
There was a problem hiding this comment.
Well, it might be okay to follow the times library and just have AccessTime, ModTime and ChangeTime.
There was a problem hiding this comment.
Document these. Please change to the expanded versions to match golang os package.
| @@ -108,6 +116,22 @@ func Merge(fs ...Resource) (Resource, error) { | |||
| return nil, fmt.Errorf("gid does not match: %v != %v", f.GID(), prototype.GID()) | |||
| } | |||
|
|
|||
| if f.ATime() != prototype.ATime() { | |||
| return nil, fmt.Errorf("atime does not match: %v != %v", f.ATime(), prototype.ATime()) | |||
There was a problem hiding this comment.
I'm a little worried this will be racy. If a hardlinked file is accessed in between the Stats, atime wouldn't match. The existing checks for things like mode and UID/GID are much less likely to change in the narrow window between two Stats.
Should we avoid considering atime when merging hardlinks?
| @@ -503,13 +560,25 @@ func toProto(resource Resource) *pb.Resource { | |||
| return b | |||
| } | |||
|
|
|||
| func timestampFromProto(ts *tspb.Timestamp) time.Time { | |||
There was a problem hiding this comment.
Let's return time.Time{} when ts == nil. This will clearly distinguish missing timestamps, for example from old manifests that don't implement timestamping, or ones generated on OSs that don't support every field. Otherwise we would return the unix epoch here.
There was a problem hiding this comment.
Just use the protobuf helper function here.
| @@ -190,6 +194,26 @@ func (c *context) Resource(p string, fi os.FileInfo) (Resource, error) { | |||
| return nil, ErrNotFound | |||
| } | |||
|
|
|||
| func (c *context) verifyTimestamps(resource, target Resource) error { | |||
| unixZero := func(t time.Time) bool { | |||
There was a problem hiding this comment.
More interestingly, why do we care about unix time?
| base.btime = ts.BirthTime() | ||
| } else { | ||
| // Note: time.Time{} stands for 0001-01-01 00:00:00 UTC | ||
| base.btime = time.Unix(0, 0) |
There was a problem hiding this comment.
I'm very worried about the redefinition of the zero time. This should really be time.Time{}.
There was a problem hiding this comment.
Does that get serialized correctly?
|
I'm not sure whether we should use 0001-epoch timestamp. i.e. According to Wikipedia, most filesystems except FAT support Unix(1970)-epoch zero but not 0001-epoch zero:
So my initial opinion is to consistently use 1970 epoch, but no strong opinion. |
|
Any thought for timestamp epoch? |
Use proper time types, ie |
| // Timestamps. | ||
| // btime (birthtime) is available on Darwin, FreeBSD, NetBSD, and Windows. | ||
| // When btime is unknown, it SHOULD be set to zero. | ||
| google.protobuf.Timestamp atime = 14; |
There was a problem hiding this comment.
Make sure to document the fields.
How to |
Fix #16 #30
Applier is not implemented yet
@stevvooe @dmcgowan @aaronlehmann @tonistiigi
Signed-off-by: Akihiro Suda [email protected]