-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Introduce POSIX socket driver microlibrary #65
Conversation
I need to remove the uk ring commits from this series. I will force push an update. |
d19d5b6
to
81d290c
Compare
81d290c
to
1c977d5
Compare
This PR is ready for review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello.
I looked two times over the commits during the past week and there wasn't much to say, I just left some comments (mostly nitpicks). It looks good, although there is a compilation error. I fetched the branch and tried to build it, but header sys/socket.h
is missing.
In file included from /home/ubuntu/unikraft/lib/posix-socket/driver.c:38:
/home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49:10: fatal error: sys/socket.h: No such file or directory
49 | #include <sys/socket.h>
| ^~~~~~~~~~~~~~
compilation terminated.
This header can be found in lwip
library, but that is an external library. It doesn't make sense to me to have an internal library dependant on an external one. Even if we were to compile with lwip
, there are another errors and warning (such as conflicting types for a couple of functions):
/home/ubuntu/unikraft/lib/posix-socket/driver.c: In function ‘posix_socket_get_family’:
/home/ubuntu/unikraft/lib/posix-socket/driver.c:149:29: warning: unused parameter ‘sock’ [-Wunused-parameter]
149 | posix_socket_get_family(int sock)
| ~~~~^~~~
CC libposix_socket: socket_vnops.o
In file included from /home/ubuntu/unikraft/lib/nolibc/include/fcntl.h:19,
from /home/ubuntu/unikraft/lib/vfscore/include/vfscore/fs.h:4,
from /home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:38:
/home/ubuntu/unikraft/lib/nolibc/arch/x86_64/bits/fcntl.h:6: warning: "O_NONBLOCK" redefined
6 | #define O_NONBLOCK 04000
|
In file included from /home/ubuntu/libs/lwip/include/sys/socket.h:46,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:35:
/home/ubuntu/apps/app-httpreply/build/liblwip/origin/lwip-2.1.2/src/include/lwip/sockets.h:478: note: this is the location of the previous definition
478 | #define O_NONBLOCK 1 /* nonblocking I/O */
|
In file included from /home/ubuntu/unikraft/lib/vfscore/include/vfscore/fs.h:4,
from /home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:38:
/home/ubuntu/unikraft/lib/nolibc/include/fcntl.h:40: warning: "O_RDONLY" redefined
40 | #define O_RDONLY 00
|
In file included from /home/ubuntu/libs/lwip/include/sys/socket.h:46,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:35:
/home/ubuntu/apps/app-httpreply/build/liblwip/origin/lwip-2.1.2/src/include/lwip/sockets.h:484: note: this is the location of the previous definition
484 | #define O_RDONLY 2
|
In file included from /home/ubuntu/unikraft/lib/vfscore/include/vfscore/fs.h:4,
from /home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:38:
/home/ubuntu/unikraft/lib/nolibc/include/fcntl.h:41: warning: "O_WRONLY" redefined
41 | #define O_WRONLY 01
|
In file included from /home/ubuntu/libs/lwip/include/sys/socket.h:46,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:35:
/home/ubuntu/apps/app-httpreply/build/liblwip/origin/lwip-2.1.2/src/include/lwip/sockets.h:487: note: this is the location of the previous definition
487 | #define O_WRONLY 4
|
In file included from /home/ubuntu/unikraft/lib/vfscore/include/vfscore/fs.h:4,
from /home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:38:
/home/ubuntu/unikraft/lib/nolibc/include/fcntl.h:42: warning: "O_RDWR" redefined
42 | #define O_RDWR 02
|
In file included from /home/ubuntu/libs/lwip/include/sys/socket.h:46,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:35:
/home/ubuntu/apps/app-httpreply/build/liblwip/origin/lwip-2.1.2/src/include/lwip/sockets.h:490: note: this is the location of the previous definition
490 | #define O_RDWR (O_RDONLY|O_WRONLY)
|
/home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:45:31: warning: cast between incompatible function types from ‘int (*)(void)’ to ‘int (*)(struct vnode *, struct vattr *)’ [-Wcast-function-type]
45 | #define posix_socket_getattr ((vnop_getattr_t) vfscore_vop_einval)
| ^
/home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:54:17: note: in expansion of macro ‘posix_socket_getattr’
54 | .vop_getattr = posix_socket_getattr,
| ^~~~~~~~~~~~~~~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:46:32: warning: cast between incompatible function types from ‘int (*)(void)’ to ‘int (*)(struct vnode *)’ [-Wcast-function-type]
46 | #define posix_socket_inactive ((vnop_inactive_t) vfscore_vop_nullop)
| ^
/home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:55:18: note: in expansion of macro ‘posix_socket_inactive’
55 | .vop_inactive = posix_socket_inactive,
| ^~~~~~~~~~~~~~~~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:58:28: warning: cast between incompatible function types from ‘int (*)(void)’ to ‘int (*)(struct mount *, struct vnode *)’ [-Wcast-function-type]
58 | #define posix_socket_vget ((vfsop_vget_t) vfscore_nullop)
| ^
/home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:62:14: note: in expansion of macro ‘posix_socket_vget’
62 | .vfs_vget = posix_socket_vget,
| ^~~~~~~~~~~~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c: In function ‘posix_socket_vfscore_close’:
/home/ubuntu/unikraft/lib/posix-socket/socket_vnops.c:205:24: warning: unused parameter ‘vfscore_file’ [-Wunused-parameter]
205 | struct vfscore_file *vfscore_file)
| ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
CC libposix_socket: socket.o
In file included from /home/ubuntu/unikraft/lib/posix-socket/socket.c:35:
/home/ubuntu/unikraft/lib/posix-socket/include/uk/socket.h:43:2: error: unknown type name ‘uint32_t’
43 | uint32_t type;
| ^~~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c: In function ‘getnameinfo’:
/home/ubuntu/unikraft/lib/posix-socket/socket.c:378:45: warning: unused parameter ‘sa’ [-Wunused-parameter]
378 | getnameinfo(const struct sockaddr *restrict sa, socklen_t sl,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:378:59: warning: unused parameter ‘sl’ [-Wunused-parameter]
378 | getnameinfo(const struct sockaddr *restrict sa, socklen_t sl,
| ~~~~~~~~~~^~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:379:18: warning: unused parameter ‘node’ [-Wunused-parameter]
379 | char *restrict node, socklen_t nodelen, char *restrict serv,
| ~~~~~~~~~~~~~~~^~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:379:34: warning: unused parameter ‘nodelen’ [-Wunused-parameter]
379 | char *restrict node, socklen_t nodelen, char *restrict serv,
| ~~~~~~~~~~^~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:379:58: warning: unused parameter ‘serv’ [-Wunused-parameter]
379 | char *restrict node, socklen_t nodelen, char *restrict serv,
| ~~~~~~~~~~~~~~~^~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:380:13: warning: unused parameter ‘servlen’ [-Wunused-parameter]
380 | socklen_t servlen, int flags)
| ~~~~~~~~~~^~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:380:26: warning: unused parameter ‘flags’ [-Wunused-parameter]
380 | socklen_t servlen, int flags)
| ~~~~^~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c: At top level:
/home/ubuntu/unikraft/lib/posix-socket/socket.c:467:1: error: conflicting types for ‘recv’
467 | recv(int sock, void *buf, size_t len, int flags)
| ^~~~
In file included from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket.h:56,
from /home/ubuntu/unikraft/lib/posix-socket/socket.c:35:
/home/ubuntu/libs/lwip/include/sys/socket.h:95:5: note: previous declaration of ‘recv’ was here
95 | int recv(int s, void *mem, size_t len, int flags);
| ^~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:505:1: error: conflicting types for ‘recvfrom’
505 | recvfrom(int sock, void *buf, size_t len, int flags,
| ^~~~~~~~
In file included from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket.h:56,
from /home/ubuntu/unikraft/lib/posix-socket/socket.c:35:
/home/ubuntu/libs/lwip/include/sys/socket.h:96:5: note: previous declaration of ‘recvfrom’ was here
96 | int recvfrom(int s, void *mem, size_t len, int flags,
| ^~~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:544:1: error: conflicting types for ‘recvmsg’
544 | recvmsg(int sock, struct msghdr *msg, int flags)
| ^~~~~~~
In file included from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket.h:56,
from /home/ubuntu/unikraft/lib/posix-socket/socket.c:35:
/home/ubuntu/libs/lwip/include/sys/socket.h:98:5: note: previous declaration of ‘recvmsg’ was here
98 | int recvmsg(int s, struct msghdr *msg, int flags);
| ^~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:582:1: error: conflicting types for ‘send’
582 | send(int sock, const void *buf, size_t len, int flags)
| ^~~~
In file included from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket.h:56,
from /home/ubuntu/unikraft/lib/posix-socket/socket.c:35:
/home/ubuntu/libs/lwip/include/sys/socket.h:99:5: note: previous declaration of ‘send’ was here
99 | int send(int s, const void *dataptr, size_t size, int flags);
| ^~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:620:1: error: conflicting types for ‘sendmsg’
620 | sendmsg(int sock, const struct msghdr *msg, int flags)
| ^~~~~~~
In file included from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket.h:56,
from /home/ubuntu/unikraft/lib/posix-socket/socket.c:35:
/home/ubuntu/libs/lwip/include/sys/socket.h:100:5: note: previous declaration of ‘sendmsg’ was here
100 | int sendmsg(int s, const struct msghdr *message, int flags);
| ^~~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:658:1: error: conflicting types for ‘sendto’
658 | sendto(int sock, const void *buf, size_t len, int flags,
| ^~~~~~
In file included from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket_driver.h:49,
from /home/ubuntu/unikraft/lib/posix-socket/include/uk/socket.h:56,
from /home/ubuntu/unikraft/lib/posix-socket/socket.c:35:
/home/ubuntu/libs/lwip/include/sys/socket.h:101:5: note: previous declaration of ‘sendto’ was here
101 | int sendto(int s, const void *dataptr, size_t size, int flags,
| ^~~~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c: In function ‘socketpair’:
/home/ubuntu/unikraft/lib/posix-socket/socket.c:741:1: warning: label ‘EXIT’ defined but not used [-Wunused-label]
741 | EXIT:
| ^~~~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:704:33: warning: unused variable ‘u2’ [-Wunused-variable]
704 | struct posix_socket_file *u1, *u2;
| ^~
/home/ubuntu/unikraft/lib/posix-socket/socket.c:704:28: warning: unused variable ‘u1’ [-Wunused-variable]
704 | struct posix_socket_file *u1, *u2;
| ^~
make[3]: *** [/home/ubuntu/unikraft/support/build/Makefile.build:27: /home/ubuntu/apps/app-httpreply/build/libposix_socket/socket.o] Error 1
make[2]: *** [Makefile:990: sub-make] Error 2
make[1]: *** [Makefile:32: _all] Error 2
make[1]: Leaving directory '/home/ubuntu/unikraft'
make: *** [Makefile:6: all] Error 2
lib/posix-socket/driver.c
Outdated
static int | ||
posix_socket_family_lib_init(void) | ||
{ | ||
uk_pr_info("Initializing socket handlers...\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nitpick: I would move this message after the variable declarations. Again, it's just a nitpick, in the worst case scenario it will cause a warning.
lib/posix-socket/socket_vnops.c
Outdated
vfs_file->f_dentry = s_dentry; | ||
vfs_file->f_vfs_flags = UK_VFSCORE_NOPOS; | ||
|
||
// s_vnode->v_data = file; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nitpick, but I would drop this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should not be commented out, turns out it is very important and an artifact of previous debugging :)
Hey @marcrittinghaus, I have a few updates to this series out of this PR. I will incorporate your suggestions (thanks btw!) when i force-push to this PR. |
I already did a ton of changes to this library internally. I think it makes sense to update it before making any reviews. It also needs some more clean up. |
@marcrittinghaus , maybe it makes sense that you update the PR contents (I think you can push to @nderjung 's brach ( |
Thanks for the feedback regarding |
5fa8243
to
47eba4f
Compare
@craciunoiuc I changed the |
All good on my side now I'll do one more pass/test just in case to make sure nothing pops up. Your turn now @skuenzer ;) |
47eba4f
to
8e2863a
Compare
@craciunoiuc, @razvand Just noticed that if the socket error message is off, the single-line Other than that, I tested this again with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @marcrittinghaus . Thanks for the work. I used it before for binary compatibility and it did the trick. I will test it again.
I did a first read pass. See my suggestions as inline comments.
Apart from this, some other comments below.
There is an inconsistency between macros in sys/socket.h
(as I understand, this is imported from OpenBSD) and the macros in Linux for address families. Some examples:
AF_BLUETOOTH
is 32 - in Linux: 31AF_ROUTE
is 17 - in Linux: 16AF_INET6
is 24 - in Linux: 10AF_IPX
is 23 - in Linux: 4
This may be an issue when using binary compatibility mode.
For Doxygen comments, there is an inconsistency around the use of dot (.
) at the end of a comment line or not. Either should be OK, as long as it's consistent.
trace_posix_socket_socketpair_ret(ret); | ||
return ret; | ||
EXIT_ERR_FREE_FD1: | ||
/* TODO: Broken error handling. Leaking vfs_fd1! */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't vfs_fd1
be freed? Isn't there a free()
equivalent to the posix_socket_alloc_fd()
function?
This commit adds a reduced version of the <sys/socket.h> header for use by the posix-socket library. Signed-off-by: Alexander Jung <alex@unikraft.io> Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
I redefined all of the address families by taking the values from musl. This should increase compatibility. |
8e2863a
to
41eb0dd
Compare
41eb0dd
to
4b01753
Compare
This library allows for the registration of socket drivers that implement the posix_socket_ops interface and thereby expose a POSIX-compliant socket API for a certain AF address family. Drivers may register as many AF numbers as desired or specify new families which are not listed in the <sys/socket.h> header. To access the relevant implementation, the application simply creates a socket with the required AF family number. Signed-off-by: Alexander Jung <alex@unikraft.io> Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
4b01753
to
1a74365
Compare
@marcrittinghaus , all is OK. @craciunoiuc , please approve this PR and then I'll approve it as well. I'll be careful to do the approval in the order signaled by @marcrittinghaus . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good on my side!
Reviewed-by: Cezar Craciunoiu cezar.craciunoiu@gmail.com
Beep boop! I ran Unikraft's
Truncated logs starting from first warning 1a74365:
View complete logs | Learn more about Unikraft's coding style and contribution guidelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @marcrittinghaus, @nderjung . This works.
Reviewed-by: Razvan Deaconescu razvan.deaconescu@cs.pub.ro
Approved-by: Razvan Deaconescu razvan.deaconescu@cs.pub.ro
This commit adds a reduced version of the <sys/socket.h> header for use by the posix-socket library. Signed-off-by: Alexander Jung <alex@unikraft.io> Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu> Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com> Reviewed-by: Razvan Deaconescu <razvan.deaconescu@cs.pub.ro> Approved-by: Razvan Deaconescu <razvan.deaconescu@cs.pub.ro> Tested-by: Unikraft CI <monkey@unikraft.io> GitHub-Closes: #65
This library allows for the registration of socket drivers that implement the posix_socket_ops interface and thereby expose a POSIX-compliant socket API for a certain AF address family. Drivers may register as many AF numbers as desired or specify new families which are not listed in the <sys/socket.h> header. To access the relevant implementation, the application simply creates a socket with the required AF family number. Signed-off-by: Alexander Jung <alex@unikraft.io> Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu> Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com> Reviewed-by: Razvan Deaconescu <razvan.deaconescu@cs.pub.ro> Approved-by: Razvan Deaconescu <razvan.deaconescu@cs.pub.ro> Tested-by: Unikraft CI <monkey@unikraft.io> GitHub-Closes: #65
This patch series introduces the posix-socket microlibrary within the Unikraft core in order to facilitate the use of multiple POSIX-compliant socket implementations which target specific AF family numbers.
This library allows for the registration of socket interfaces, listed within
struct posix_socket_ops
, and exposes unikernel-wide prototypes forsocket()
,accept()
,bind()
,listen()
,connect()
,send()
,recv()
and friends in order to access them via a corresponding AF family number. The implementing library simply needs to register against the desired interfaces with a glue function:Each glue function accepts traditional parameters for the POSIX interface as side from the file descriptor, traditionally
int fd
orint sock
, which has instead been cast asvoid *
. This allows the implementing library to use internal socket file descriptor or reference. The creation of a socket viasocket()
andaccept()
have also been cast to returnvoid *
in line with internal needs of the implementing library. This makes sense for implementing libraries which use a structure instead of a file descriptor identification integer. An additional parameter,struct posix_socket_driver
, is passed to each interface which can store private data as well as a preferred memory allocator. The glue for each interface simply needs to call the appropriate method internally, for example:The library selects the appropriate implementation based on the AF family number and registers the required interfaces using a newly exposed macro:
Implementing libraries can register as many AF numbers as desired or specify new families which are not listed in the
<sys/socket.h>
header. To access the relevant implementation, the application simply needs to call thesocket()
method with the relevant AF family number.This PR is now part of a larger group of dependent PRs which must be merged in the following order: