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

Uses Linux-specific APIs #14

Open
codebrainz opened this issue Sep 19, 2015 · 15 comments
Open

Uses Linux-specific APIs #14

codebrainz opened this issue Sep 19, 2015 · 15 comments

Comments

@codebrainz
Copy link

Looking at the source, it unconditionally includes <sys/eventfd.h> and uses the (really nice) eventfd API. This means libasyncd will only run on Linux (and possibly some BSDs if they can emulate it). I'm not very familiar with libevent, but it might provide some portable replacement for the use of eventfd.

@wolkykim
Copy link
Owner

That is right. What O/S are you using? It's written to work best on Linux but if there's needs we can always do the porting work. I guess the question is are there the needs?

@codebrainz
Copy link
Author

It's fine for now to support only Linux, I just didn't realize it at first, and I assumed it used some semi-portable (ie. POSIX) stuff. It might be worth to add a note in the README.md like "Currently only supports Linux but cross-platform improvements welcome" or something.

I probably won't use this library at the moment due to my project not being very important and kind of wanting cross-platform support out-of-the box, but it seems to be pretty interesting still, so I might try to contribute a bit, at least with the Autotools.

Nice work!

@dascandy
Copy link

I was going to create an issue that it does not cleanly compile on OS X but this is pretty much exactly that issue. The concrete things I see is that you use "-Wl,-soname" which does not work on OSX's linker, and you include sys/eventfd.h which does not exist.

Also, there's the potential for Windows support but I am not sure how many libraries would need porting before that would work - I stopped counting when libevent didn't work.

@wolkykim
Copy link
Owner

Sorry about that, yes the implementation is pretty much optimized to linux for now. But I'm very open to any porting ideas.

@codebrainz
Copy link
Author

Maybe using a pipe or semaphore to replace eventfd. Windows has a whole different mechanism for async IO than Unix, but I think if libasyncd could stick to POSIX APIs, it could support most Unix out-of-the-box, and would make porting to Windows somewhat easier (Win32 has select() and WSAPoll() but I'm not sure if they work on non-socket handles).

@spbruner
Copy link
Contributor

spbruner commented Jun 1, 2016

I'd originally written a comment here that said it appears you can safely remove the eventfd.h include, but I was mistaken. A second read through of ad_server.c does indeed indicate usage of the eventfd API.

So, sadly, no-go for now.

Now to find a way to generalize for the sake of portability...

@wolkykim
Copy link
Owner

wolkykim commented Jun 3, 2016

is it only eventfd? if we make the eventfd part portable, will it be a go for the FreeBSD? we could make it portable.

@spbruner
Copy link
Contributor

spbruner commented Jun 3, 2016

Well, most of your library is built on libevent, so yes I think its just the eventfd function. At least that I've seen so far. In my testing on FreeBSD, if I replace the single call to eventfd(0,0) with kqueue() the library builds and runs as you'd expect.

As to your question whether it could be made more portable, I don't really see you getting around the fact that different OS's have different event notification interfaces. I don't really see a problem with that either. In my own repository, I added a quick and dirty #ifdef to use eventfd on Linux, and kqueue otherwise (which covers the "big three" BSD's as well as Mac OS). Long term, I'd recommend adding an ad_platform (a name I pulled out of thin air) header with a get_descriptor (or similar) function that abstracts away the platform-dependent interface. The idea being you have a single place where you can place all those dirty #ifdef's rather than polluting the core C files with them.

That's just my two cents.

@codebrainz
Copy link
Author

If it uses libevent, there's probably not really a need to use any platform specific stuff. AFAIK libevent already contains the needed platform abstractions. A quick skim of the API docs, maybe the eventfd notification channel part could be replaced with event_new passing -1 for the file descriptor and 0 for the flags.

@wolkykim
Copy link
Owner

wolkykim commented Jun 3, 2016

@spbruner your pull request was merged. Thank you. Please add yourself on the contributor list in the readme and send PR please.

@spbruner
Copy link
Contributor

spbruner commented Jun 3, 2016

@codebrainz I believe you're probably correct in that assessment. I'll spend a little time this weekend experimenting with a libevent-only approach.

@spbruner
Copy link
Contributor

spbruner commented Jun 3, 2016

@wolkykim You can probably go ahead and close this issue. While we may continue to search for a better way or just cleaner code, we have at least one working mechanism that directly addresses this issue.

If anyone on thread is using Mac OS X, I'd appreciate if you could test it and drop me a quick note with your findings.

@codebrainz
Copy link
Author

Windows doesn't have eventfd or kqueue, so I think this issue is probably still valid.

@spbruner
Copy link
Contributor

spbruner commented Jun 5, 2016

Ok. Well I really couldn't find a way to create a file descriptor using libevent alone. So I went with my original plan and refactored my previous addition into the afore-mentioned ad_platform(.h/.c) and swapped out the call in ad_server.c to call the new ad_patform_getfd() function. I'm testing this change now and will likely upload with a PR later today.

@codebrainz Windows has IO Completion Ports, which is about as close as you'll get. But since its been 10+ years since I've done any Win32 programming, I'll leave that to someone else to experiment with and add as needed.

@manjunathkokhle
Copy link

@codebrainz / @wolkykim
i am compiling the code for the embedded environment where i dont have eventfd. though i have managed to get through the error by commenting the EVENT__HAVE_EVENTFD flag but i am stuck in eventfd function call in ad_server_start(). is there a alternate method to call eventfd for linux environment using pipe?

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

No branches or pull requests

5 participants