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.

Sign up for GitHub

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

Jump to bottom

epoll_wait syscall doesn't exist on arm64. #116

Merged
merged 1 commit into from
Mar 1, 2017
Merged

epoll_wait syscall doesn't exist on arm64. #116

merged 1 commit into from
Mar 1, 2017

Conversation

harshavardhana
Copy link
Contributor

@harshavardhana harshavardhana commented Feb 28, 2017 •
edited
Loading

Implement it with by calling epoll_pwait(). According to man epoll_pwait,
calling epoll_pwait with sigmask of NULL is identical to
epoll_wait.

Bring the fixes from golang.org/x/sys/unix to support these
transparently on arm64.

This is needed to fix minio/mc#2047

@harshavardhana harshavardhana changed the title epoll_wait syscall doesn't exist on arm64. Implement it with epoll_wait syscall doesn't exist on arm64. Feb 28, 2017
@harshavardhana harshavardhana mentioned this pull request Feb 28, 2017
Closed
rjeczalik
rjeczalik reviewed Feb 28, 2017
View reviewed changes
watcher_inotify.go Outdated
@@ -13,14 +13,16 @@ import (
"runtime"
"sync"
"sync/atomic"
"syscall"

"golang.org/x/sys/unix"
Copy link
Owner

Choose a reason for hiding this comment

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

should go under unsafe import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@rjeczalik
Copy link
Owner

/cc @ppknap

@rjeczalik
Copy link
Owner

rjeczalik commented Feb 28, 2017 •
edited
Loading

@harshavardhana You'd also need to add missing import to event_kqueue.go:

# github.com/rjeczalik/notify
./event_kqueue.go:29: undefined: unix in unix.NOTE_DELETE

Goimports seems to not like build tags.

Thanks for the PR! 😄

ppknap
ppknap approved these changes Feb 28, 2017
View reviewed changes
Copy link
Collaborator

@ppknap ppknap left a comment

Choose a reason for hiding this comment

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

LGTM but please fix missing import in event_kqueue.go

@rjeczalik
Copy link
Owner

One more:

# github.com/rjeczalik/notify
./watcher_kqueue.go:53: undefined: unix in unix.Write
./watcher_kqueue.go:59: undefined: unix in unix.Close
./watcher_kqueue.go:64: undefined: unix in unix.Open
./watcher_kqueue.go:78: undefined: unix in unix.Close

You can test build with:

$ go build -tag kqueue ./...
$ go test -run none -tag kqueue ./...

@ghost
Copy link

ghost commented Feb 28, 2017

With 465c9e8 fen does not work anymore. Tests hang:

$ go test -v ./...
=== RUN   TestEventString
--- PASS: TestEventString (0.00s)
=== RUN   TestNotifyExample

@rjeczalik
Copy link
Owner

rjeczalik commented Feb 28, 2017 via email

@harshavardhana
Copy link
Contributor Author

With 465c9e8 fen does not work anymore. Tests hang:

$ go test -v ./...
=== RUN TestEventString
--- PASS: TestEventString (0.00s)
=== RUN TestNotifyExample

What is the operating system you are using? @pblaszczyk

ghost
ghost suggested changes Feb 28, 2017
View reviewed changes
watcher_fen.go Outdated
@@ -97,9 +98,9 @@ func (f *fen) Init() (err error) {
}

func fi2fo(fi os.FileInfo, p string) FileObj {
st, ok := fi.Sys().(*syscall.Stat_t)
st, ok := fi.Sys().(*unix.Stat_t)
Copy link

Choose a reason for hiding this comment

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

fi is a regular os.FileInfo from os.Stat. As such fi.Sys() returns standard *syscall.Stat_t.

watcher_fen.go Outdated
if !ok {
panic(fmt.Sprintf("fen: type should be *syscall.Stat_t, %T instead", st))
panic(fmt.Sprintf("fen: type should be *unix.Stat_t, %T instead", st))
Copy link

Choose a reason for hiding this comment

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

Here should be syscall instead of unix.
If we are here already you could change st to fi.Sys().

@ghost
Copy link

ghost commented Feb 28, 2017

@harshavardhana I'm using OmniOS.

@harshavardhana
Copy link
Contributor Author

@harshavardhana I'm using OmniOS.

@pblaszczyk can you try the latest push?

@ghost
Copy link

ghost commented Feb 28, 2017

Yes, reverting all changes to fen makes it work.

@harshavardhana
Copy link
Contributor Author

Yes, reverting all changes to fen makes it work.

yeah that's what i expected sys/unix perhaps has bugs for Solaris.

ppknap
ppknap reviewed Feb 28, 2017
View reviewed changes
Copy link
Collaborator

@ppknap ppknap left a comment

Choose a reason for hiding this comment

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

Just two little things more

watcher_readdcw.go Outdated
@@ -12,6 +12,7 @@ import (
"sync"
"sync/atomic"
"syscall"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove this empty line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

event_readdcw.go Outdated
@@ -45,7 +45,7 @@ const (
)

// according to: http://msdn.microsoft.com/en-us/library/windows/desktop/aa365465(v=vs.85).aspx
// this flag should be declared in: http://golang.org/src/pkg/syscall/ztypes_windows.go
// this flag should be declared in: http://golang.org/src/pkg/syscall.ztypes_windows.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a valid URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@harshavardhana
Copy link
Contributor Author

S3 is down guys :-) so Travis not conclusive.

@harshavardhana
epoll_wait syscall doesn't exist on arm64.
e80522e 
Implement it with by calling epoll_pwait().
According to man epoll_pwait, calling
epoll_pwait with sigmask of NULL is identical
to epoll_wait.

Bring the fixes from golang.org/x/sys/unix to support these
transparently on arm64.

This is needed to fix minio/mc#2047
@harshavardhana
Copy link
Contributor Author

Anything else pending on this @rjeczalik @pblaszczyk @ppknap ?

@rjeczalik rjeczalik merged commit 41a8645 into rjeczalik:master Mar 1, 2017
@rjeczalik
Copy link
Owner

@harshavardhana Nope, merged. Thanks for the PR!

@ppknap
Copy link
Collaborator

ppknap commented Mar 1, 2017

Thanks @harshavardhana 💯

@harshavardhana harshavardhana deleted the unix branch March 1, 2017 21:28
@decathorpe decathorpe mentioned this pull request Mar 13, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjeczalik rjeczalik rjeczalik approved these changes

@ppknap ppknap ppknap approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

arm64: panic: notify: epoll_wait(2) error function not implemented
3 participants
@harshavardhana @rjeczalik @ppknap