Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: replace syscall with unix #141

Merged
merged 6 commits into from
Jul 20, 2017
Merged

*: replace syscall with unix #141

merged 6 commits into from
Jul 20, 2017

Conversation

vrothberg
Copy link
Contributor

@vrothberg vrothberg commented Jul 12, 2017

Some parts of the code required some more work than pure substitutions
as the unix interfaces expect a unix.Stat_t, so we couldn't just use
os.FileInfo's Sys() interface to get actual metadata.

Note: this PR bases on and extends https://github.com/openSUSE/umoci/pull/128
Closes #128

@cyphar
Copy link
Member

cyphar commented Jul 12, 2017

@@ -315,6 +320,7 @@ func Remove(path string) error {
// currently have enough access bits.
func RemoveAll(path string) error {
return errors.Wrap(Wrap(path, func(path string) error {
// XXX(VR): os.RemoveAll(path) works, but I am not sure if that's right
// If remove works, we're done.
err := os.Remove(path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, if we use os.RemoveAll(path) here, the CI succeeds. Would this be an alternative here, @cyphar?

@@ -58,20 +58,16 @@ func splitpath(path string) []string {
// isNotExist tells you if err is an error that implies that either the path
// accessed does not exist (or path components don't exist).
func isNotExist(err error) bool {
Copy link
Member

Choose a reason for hiding this comment

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

The failure happens because of this change. In particular, err == unix.ENOTDIR will always be false. This is what I was referring to:

// isNotExist tells you if err is an error that implies that either the path
// accessed does not exist (or path components don't exist).
func isNotExist(err error) bool {
	if os.IsNotExist(errors.Cause(err)) {
		return true
	}
	// Check that it's not actually an ENOTDIR.
	perr, ok := errors.Cause(err).(*os.PathError)
	if !ok {
		return false
	}
	errno := perr.Err
	return errno == syscall.ENOTDIR || errno == syscall.ENOENT
}

(And then just swap syscall for unix). However, I was thinking about this some more and maybe we should do this (we keep os.IsNotExist to avoid possible cases where the stdlib changes in some way).

// isNotExist tells you if err is an error that implies that either the path
// accessed does not exist (or path components don't exist).
func isNotExist(err error) bool {
	if os.IsNotExist(errors.Cause(err)) {
		return true
	}

	// Check that it's not actually an ENOTDIR in disguise. This is technically
	// a dup of doing os.IsNotExist, but it doesn't hurt to be thorough.
	var errno error
	switch err := errors.Cause(err).(type) {
	case *os.PathError:
		errno = err.Err
	case *os.LinkError:
		errno = err.Err
	case *os.SyscallError:
		errno = err.Err
	}
	return errno == syscall.ENOTDIR || errno == syscall.ENOENT
}

Also, make sure you don't remove errors.Causes. It's a no-op for non-wrapped error messages, but for wrapper error messages it makes sure that things like os.IsNotExist actually do the right thing.

@vrothberg vrothberg changed the title [WIP] Switch to sysunix *: replace syscall with unix Jul 13, 2017
@vrothberg
Copy link
Contributor Author

After some CI hiccups, it has finally passed \o/

@cyphar
Copy link
Member

cyphar commented Jul 14, 2017

Yeah, the CI hiccups were due to openSUSE repos being broken yesterday. I'll go through a review later today.

@vrothberg
Copy link
Contributor Author

I rebased and also fixed a copy-paste error in utime_linux_test.go that caused random fails since the the nano seconds of atime has been used for the mtime timestamp.

@cyphar cyphar added this to the 0.3.0 milestone Jul 17, 2017
cyphar and others added 6 commits July 21, 2017 00:21
This is necessary for the repeal and replacement of pkg/system with
golang.org/x/sys. Believe me, it's going to be HUGE.

Signed-off-by: Aleksa Sarai <[email protected]>
This is a fairly trivial switch to unix.Flock, and removal of our own
pkg/system bindings for it.

Signed-off-by: Aleksa Sarai <[email protected]>
fseval was fairly ugly in the global github.com/openSUSE/umoci package
(which should be empty), so move it to pkg/fseval. In the future, we
might want to move it to separate project entirely. This also makes the
import graph slightly less hectic.

Signed-off-by: Aleksa Sarai <[email protected]>
Unfortunately, I've found that Go's archive/tar doesn't appear to
consistently handle empty-value xattrs. On the one hand they claim it's
against the PAX standard (and so ignore such entries when reading
archives) but will also happily generate said non-compliant headers.

In addition, these wrappers aren't significantly more useful than the
stock "syscall" ones (forcing us to keep our pkg/system wrappers).

Ref: https://golang.org/issues/20698
Signed-off-by: Aleksa Sarai <[email protected]>
[ comment out new test code until go-mtree gets fixed ]
Signed-off-by: Valentin Rothberg <[email protected]>
Some parts of the code required some more work than pure substitutions
as the unix interfaces expect a unix.Stat_t, so we couldn't just use
os.FileInfo's Sys() interface to get actual metadata.

Signed-off-by: Valentin Rothberg <[email protected]>
[ small style cleanups ]
Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar
Copy link
Member

cyphar commented Jul 20, 2017

LGTM

@cyphar cyphar merged commit d4b86b8 into opencontainers:master Jul 20, 2017
cyphar added a commit that referenced this pull request Jul 20, 2017
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.

2 participants