Skip to content

Add time type conversion for 32 bit linix platforms - #105

Merged
stevvooe merged 1 commit into
containerd:masterfrom
tkporter:master
Feb 16, 2018
Merged

stevvooe merged 1 commit into
containerd:masterfrom
tkporter:master

Conversation

@tkporter

Copy link
Copy Markdown

Ensures the time.Unix function is given int64 arguments. I'm running 32 bit Debian 9.1-- this fixes the following error when building containerd after this PR added the continuity fs package to containerd:

# github.com/containerd/containerd/vendor/github.com/containerd/continuity/fs
../../containerd/containerd/vendor/github.com/containerd/continuity/fs/stat_linux.go:25:26: cannot use st.Atim.Sec (type int32) as type int64 in argument to time.Unix
../../containerd/containerd/vendor/github.com/containerd/continuity/fs/stat_linux.go:25:39: cannot use st.Atim.Nsec (type int32) as type int64 in argument to time.Unix

I recently had a PR merged for pretty much the same fix in the containerd repo

Ensures the time.Unix function is given int64 arguments

Signed-off-by: Trevor Porter <trkporter@ucdavis.edu>

@dmcgowan dmcgowan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dnephin dnephin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread fs/stat_linux.go
// StatATimeAsTime returns st.Atim as a time.Time
func StatATimeAsTime(st *syscall.Stat_t) time.Time {
return time.Unix(st.Atim.Sec, st.Atim.Nsec)
return time.Unix(int64(st.Atim.Sec), int64(st.Atim.Nsec)) // nolint: unconvert

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include a comment here why these conversions are necessary. Otherwise, they may get removed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevvooe I'm willing to address this in a follow up PR, but if you don't want it, I'm okay opening a carry-PR, just let me know.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

@stevvooe

Copy link
Copy Markdown
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants