add aio.d for linux x64 by Darredevil · Pull Request #2074 · dlang/druntime

Darredevil · 2018-02-03T17:24:03Z

Added interface for posix aio in D (only for linux x64 for now).
The aiocb struct can have various implementations, for linux x64 I tested the following in C and created the equivalent in D:

C aiocb
Sizeof aiocb: 168 bytes
Offsets:
aiocb.aio_fildes:	 0
aiocb.aio_lio_opcode:	 4
aiocb.aio_reqprio:	 8
aiocb.aio_buf:		 16
aiocb.aio_nbytes:	 24
aiocb.aio_sigevent:	 32
aiocb.aio_offset:	 128

dlang-bot · 2018-02-03T17:24:04Z

Thanks for your pull request and interest in making D better, @Darredevil! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

wilzbach · 2018-02-03T17:25:48Z

src/core/sys/posix/aio.d

+ * D header file to interface with the Linux aio API (http://man7.org/linux/man-pages/man7/aio.7.html).
+ * Available since Linux 2.6
+ *
+ * Copyright: Copyright Alexandru Razvan Caciulescu 2018.


D Language Foundation.

I took core.sys.linux.epoll as an example for the copyright, is that file not respecting the convention?

That file was written long before the DFL existed. We can't easily change the copyright for existing modules, but we can for new additions.

joakim-noah · 2018-02-04T05:07:58Z

src/core/sys/posix/aio.d

+
+private import core.sys.posix.signal;
+
+version (linux):


CRuntime_Glibc, not linux.

ibuclaw · 2018-02-06T07:51:33Z

src/core/sys/posix/aio.d

+    ubyte[24] internal_members_padding;
+    off_t aio_offset;
+    ubyte[32] __glibc_reserved;
+


Extra space

ibuclaw · 2018-02-06T07:52:44Z

src/core/sys/posix/aio.d

@@ -0,0 +1,66 @@
+/**
+ * D header file to interface with the Linux aio API (http://man7.org/linux/man-pages/man7/aio.7.html).
+ * Available since Linux 2.6


So this should be in core/sys/linux then, not posix.

Actually no, there are two instance of AIO, the POSIX aio (aio_* family) and Linux libaio.
I believe this is in the correct place.

This is POSIX AIO interface except for aio_init I think.

andralex · 2018-02-13T03:01:29Z

@ibuclaw good to go?

@CyberShadow can you please advise regarding the merge error in circleci?

wilzbach · 2018-02-13T03:08:06Z

Can you please advise regarding the merge error in circleci?

@Darredevil's fork was pretty outdated and we upgraded to CircleCi 2.0.
tl;dr: rebasing helps. I just did so.

Darredevil · 2018-02-14T00:43:49Z

@ibuclaw all looks good now

ibuclaw · 2018-02-15T08:15:19Z

src/core/sys/posix/aio.d

@@ -0,0 +1,65 @@
+/**
+ * D header file to interface with the Linux aio API (http://man7.org/linux/man-pages/man7/aio.7.html).


Any link to the posix standard that is not a Linux manpage?

Will this suffice?
https://www.freebsd.org/cgi/man.cgi?query=aio&sektion=4&apropos=0&manpath=FreeBSD+11.1-RELEASE+and+Ports

Also http://www2.fisica.unlp.edu.ar/electro/temas/p7/POSIX4.pdf , see page 277

This one seems best: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/aio.h.html

I made the change myself. All, feel free to operate noncontroversial changes (spacing, better links) right on the spot - it takes only a bit longer than making the request and accelerates the cycle greatly.

andralex

@ibuclaw good to go?

andralex · 2018-02-15T17:42:25Z

src/core/sys/posix/aio.d

@@ -0,0 +1,65 @@
+/**
+ * D header file to interface with the Linux aio API (http://man7.org/linux/man-pages/man7/aio.7.html).


I made the change myself. All, feel free to operate noncontroversial changes (spacing, better links) right on the spot - it takes only a bit longer than making the request and accelerates the cycle greatly.

ibuclaw

Nits and C compatibly problems.

ibuclaw · 2018-02-15T22:39:02Z

src/core/sys/posix/aio.d

+int aio_read(aiocb *aiocbp);
+int aio_write(aiocb *aiocbp);
+int aio_fsync(int op, aiocb *aiocbp);
+int aio_error(aiocb *aiocbp);


const(aiocb)*?

ibuclaw · 2018-02-15T22:59:26Z

src/core/sys/posix/aio.d

+int aio_fsync(int op, aiocb *aiocbp);
+int aio_error(aiocb *aiocbp);
+ssize_t aio_return(aiocb *aiocbp);
+int aio_suspend(aiocb*[] aiocb_list, int nitems, timespec *timeout);


First parameter would not be the equivalent D type you're looking for. The closest approximation would be aiocb**.

Also could have const(aiocb*)* and const(timespec)*

ibuclaw · 2018-02-15T23:04:46Z

src/core/sys/posix/aio.d

+ssize_t aio_return(aiocb *aiocbp);
+int aio_suspend(aiocb*[] aiocb_list, int nitems, timespec *timeout);
+int aio_cancel(int fd, aiocb *aiocbp);
+int lio_listio(int mode, aiocb*[] aiocb_list, int nitems, sigevent *sevp);


aiocb*[] would be a dynamic array, so this should be instead const(aiocb*)*

ibuclaw · 2018-02-15T23:15:09Z

src/core/sys/posix/aio.d

+
+private import core.sys.posix.signal;
+
+version (CRuntime_Glibc):


Top of file should be guarded with version (Posix), not glibc.

I think the idea is that this has only been tested on linux/Glibc so far, of all Posix platforms.

Doesn't mean we should drop the convention used in every other sys/posix module. New modules that require porting should come with big alarm bells to notify those working on said unsupported platforms to check this code very carefully.

Sure, your edit suggestions look good.

ibuclaw · 2018-02-15T23:16:10Z

src/core/sys/posix/aio.d

+private import core.sys.posix.signal;
+
+version (CRuntime_Glibc):
+version (X86_64):


This should not be here, rather it should be moved to where x86_64-specific definitions are.

ibuclaw · 2018-02-15T23:25:36Z

src/core/sys/posix/aio.d

+
+    ubyte[24] internal_members_padding;
+    off_t aio_offset;
+    ubyte[32] __glibc_reserved;


This looks like the only platform specific part of this module here. Make a porters life easy and guard it properly with appropriate version blocks and static asserts.

i.e:

version (CRuntime_Glibc) { version (X86_64) { struct aiocb { //... } } else static assert(0); } else static assert(false, "Unsupported platform");

ibuclaw · 2018-02-15T23:42:35Z

src/core/sys/posix/aio.d

+/**
+ * D header file to interface with the
+ * $(HTTP pubs.opengroup.org/onlinepubs/9699919799/basedefs/aio.h.html, Linux aio API).
+ * Available since Linux 2.6


Shouldn't you have dropped the Linux references here?

Take a look any of the other core.sys.posix module headers, eg: https://github.com/dlang/druntime/blob/master/src/core/sys/posix/pwd.d

Darredevil · 2018-03-03T04:55:21Z

@ibuclaw thanks for the suggestions, updated accordingly

ibuclaw · 2018-03-03T11:32:04Z

src/core/sys/posix/aio.d

@@ -0,0 +1,74 @@
+/**
+ * D header file to interface with the
+ * $(HTTP pubs.opengroup.org/onlinepubs/9699919799/basedefs/aio.h.html, Linux aio API).


I'd still say Posix AIO here, as I've repeatedly said, this is not a linux module. ;-)

ibuclaw · 2018-03-03T11:33:01Z

@Darredevil - I think just one last nit and then I'll shut up.

Darredevil · 2018-03-03T13:02:18Z

@ibuclaw should be good to go now

ibuclaw · 2018-03-03T15:14:50Z

Ping @joakim-noah @redstar @yshui @dkgroot @rracariu

FYI this module is in dire need of porting. ALL PLATFORMS WILL FTBFS until this is fixed.

joakim-noah · 2018-03-03T15:23:33Z

Not a big deal, none of our platforms are tested with this master branch, if at all. Once it moves downstream, we'll test and fix it.

ibuclaw · 2018-03-03T15:29:07Z

@joakim-noah - Yeah, depends how you build druntime also. As a litmus test, all druntime modules are built in gdc's tree regardless of compiler target. This ensures that at least -fsyntax-only passes on all platforms.

dkgroot · 2018-03-03T16:52:18Z

How about adding some unittests, that would ease porting to other platforms.
example posix aio conformance tests (in c):
http://repo.or.cz/ltp-debian.git/tree/HEAD:/testcases/open_posix_testsuite/conformance/interfaces.

The linux implementation of posix_aio is (mostly) oriented toward async non-blocking (raw) file i/o. I am not 100% sure if i remember correctly, but using it with sockets was not supported on linux (?). On other platforms posix_aio can also be used to deal with sockets (though not often used as far as i know). Should the orientation toward file i/o be noted, or should using a socket FD not be permitted for example ?

ibuclaw · 2018-03-03T17:29:14Z

How about adding some unittests, that would ease porting to other platforms.

That's never really been done for other modules, and I can think of a few where there's good reason for that as well. Be it math functions that return a different result depending on the platform, or format functions where format specifiers may not exist on older systems.

In any case, I don't think its our role to test the C runtime library here, and there's too many variations between them to really be useful, and these libraries have their own testsuite anyways. ;-)

Of course, if there's something we implement, then that needs a unittest.

dkgroot · 2018-03-03T17:35:09Z

@ibuclaw i was not looking to test the c-implementation, to to check that we present the same results on all platforms. But ok, if that is not something we normally do. I assume that once there is a phobos library using this, it would be unittested there.
I will look into porting to freebsd and dragonfly when this part is merged. Should be pretty straight forward.

Darredevil requested review from CyberShadow, MartinNowak, andralex and wilzbach as code owners February 3, 2018 17:24

wilzbach reviewed Feb 3, 2018

View reviewed changes

Darredevil force-pushed the aio branch from 1cddffd to fe547a9 Compare February 3, 2018 17:35

joakim-noah reviewed Feb 4, 2018

View reviewed changes

src/core/sys/posix/aio.d Outdated

private import core.sys.posix.signal;

version (linux):

Copy link

Contributor

joakim-noah Feb 4, 2018

Choose a reason for hiding this comment

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

CRuntime_Glibc, not linux.

ibuclaw suggested changes Feb 6, 2018

View reviewed changes

Darredevil force-pushed the aio branch from 8addc24 to 5ec83ba Compare February 9, 2018 11:54

andralex approved these changes Feb 13, 2018

View reviewed changes

Darredevil and others added 3 commits February 13, 2018 04:07

add aio.d

541b999

Update aio.d

6bc992e

Update aio.d

6d1f5fc

wilzbach force-pushed the aio branch from ea8795d to 6d1f5fc Compare February 13, 2018 03:07

add aio.d to win*.mak

ba2157f

ibuclaw reviewed Feb 15, 2018

View reviewed changes

Update aio.d

6c21ed7

andralex approved these changes Feb 15, 2018

View reviewed changes

ibuclaw suggested changes Feb 15, 2018

View reviewed changes

ibuclaw reviewed Feb 15, 2018

View reviewed changes

Darredevil force-pushed the aio branch from 8d8e9a7 to 39757cb Compare March 3, 2018 04:54

ibuclaw reviewed Mar 3, 2018

View reviewed changes

aio.d nitpicking

53ba9b1

Darredevil force-pushed the aio branch from 39757cb to 53ba9b1 Compare March 3, 2018 13:01

ibuclaw approved these changes Mar 3, 2018

View reviewed changes

ibuclaw added the auto-merge label Mar 3, 2018

ibuclaw closed this Mar 5, 2018

ibuclaw reopened this Mar 5, 2018

dlang-bot merged commit 64cb6b4 into dlang:master Mar 5, 2018

dkgroot mentioned this pull request Mar 6, 2018

Added aio.h implementations for FreeBSD, NetBSD and DragonFly #2133

Merged

		@@ -0,0 +1,65 @@
		/**
		* D header file to interface with the Linux aio API (http://man7.org/linux/man-pages/man7/aio.7.html).


		private import core.sys.posix.signal;

		version (CRuntime_Glibc):

Uh oh!

Comments

Conversation

Darredevil commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Feb 13, 2018

Uh oh!

wilzbach commented Feb 13, 2018

Uh oh!

Darredevil commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Darredevil commented Mar 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Mar 3, 2018

Uh oh!

Darredevil commented Feb 3, 2018 •

edited

Loading

dlang-bot commented Feb 3, 2018 •

edited

Loading

ibuclaw Feb 15, 2018 •

edited

Loading

ibuclaw commented Mar 3, 2018 •

edited

Loading