libasyncd icon indicating copy to clipboard operation
libasyncd copied to clipboard

Uses Linux-specific APIs

Open codebrainz opened this issue 10 years ago • 15 comments

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.

codebrainz avatar Sep 19 '15 03:09 codebrainz

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?

wolkykim avatar Sep 22 '15 10:09 wolkykim

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!

codebrainz avatar Sep 23 '15 02:09 codebrainz

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.

dascandy avatar Dec 11 '15 18:12 dascandy

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

wolkykim avatar Dec 11 '15 22:12 wolkykim

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).

codebrainz avatar Dec 12 '15 00:12 codebrainz

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...

spbruner avatar Jun 01 '16 22:06 spbruner

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

wolkykim avatar Jun 03 '16 00:06 wolkykim

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.

spbruner avatar Jun 03 '16 02:06 spbruner

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.

codebrainz avatar Jun 03 '16 04:06 codebrainz

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

wolkykim avatar Jun 03 '16 07:06 wolkykim

@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 avatar Jun 03 '16 10:06 spbruner

@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.

spbruner avatar Jun 03 '16 11:06 spbruner

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

codebrainz avatar Jun 03 '16 16:06 codebrainz

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.

spbruner avatar Jun 05 '16 15:06 spbruner

@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?

manjunathkokhle avatar Dec 10 '18 14:12 manjunathkokhle