Status: | Merged |
---|---|
Merged at revision: | 1594 |
Proposed branch: | lp:~cameronnemo/upstart/ipv6 |
Merge into: | lp:upstart |
Diff against target: |
202 lines (+92/-25) 3 files modified
ChangeLog (+16/-0) extra/man/socket-event.7 (+10/-1) extra/upstart-socket-bridge.c (+66/-24) |
To merge this branch: | bzr merge lp:~cameronnemo/upstart/ipv6 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
James Hunt | Approve | ||
Review via email: mp+204599@code.launchpad.net |
Commit message
Description of the change
Adds the inet6 option for the socket event parameter PROTO and the corresponding man page examples.
- 1595. By Cameron Norman
-
remerge from trunk
- 1596. By Cameron Norman
-
whoopsie in ChangeLog
- 1597. By Cameron Norman
-
upstart-
socket- bridge. c: epoll_watcher() and job_add_socket() fixes for ipv6 - 1598. By Cameron Norman
-
upstart-
socket- bridge. c: job_add_socket(): pton returns 1 on success, not 0
James Hunt (jamesodhunt) wrote : | # |
Hi Cameron,
Thanks for updating. There is still a problem with inet_pton(): the test should error if the value returned != 1 since currently all valid IPv6 addresses are rejected. Please could you fix that up and test.
I'm testing this with a job that specifies:
start on socket PROTO=inet6 PORT=80 ADDR=::1
script
echo ok
set
end script
Then running the event monitor:
$ upstart-monitor &
Then, running the bridge in debug mode in the foreground:
$ sudo ./upstart-
And finally connecting to port 80 via IPv6:
$ echo foo | socat 'tcp6-connect:
Thanks!
- 1599. By Cameron Norman
-
fixed ptons usage
Cameron Norman (cameronnemo) wrote : | # |
Thanks for the help and patience James. I am going to try to work on this tonight and tommorow, and will comment here if I reach success, or if I need any help (if that is alright).
James Hunt (jamesodhunt) wrote : | # |
Hi Cameron - no problem. Feel free to join #upstart on freenode if you have questions.
Cameron Norman (cameronnemo) wrote : | # |
So I tested it on a Saucy machine and it seems to be working. Would you like to re-review it?
James Hunt (jamesodhunt) wrote : | # |
Hi Cameron,
Thanks for updating. I've found another issue though: if there are 2 jobs that specify 'start on socket', one for IPv4 the other for IPv6, the bridge will be unable to call bind(2) for each protocol version currently:
$ cat <<EOT | sudo tee /etc/init/
start on socket PROTO=inet PORT=80 ADDR=127.0.0.1
exec echo ipv4
EOT
$ cat <<EOT | sudo tee /etc/init/
start on socket PROTO=inet6 PORT=80 ADDR=::1
exec echo ipv6
EOT
$ sudo extra/upstart-
job_add_socket: Found socket
upstart_job_added: Job got added /com/ubuntu/
job_add_socket: Found socket
upstart-
I think we may need some IPV6_V6ONLY setsockopt(2) magic to resolve this?
Cameron Norman (cameronnemo) wrote : | # |
I have a question about that. Should we not instead do IPv4 mapped IPv6?
Cameron Norman (cameronnemo) wrote : | # |
But you would still have to use IPV6_V6ONLY in that scenario. And even then with IPV4 mapped IPv6 you would need to give the job two fd's, which we cannot do with the current socket activation protocol (correct?).
- 1600. By Cameron Norman
-
use setsockopt to only bind to ipv6
- 1601. By Cameron Norman
-
whoops
- 1602. By Cameron Norman
-
put the setsockopt /after/ the sockfd is created
Cameron Norman (cameronnemo) wrote : | # |
Ok, so I got it working and tested it with the following job:
start on socket PROTO=inet6 PORT=80 ADDR=::1
exec socat fd:"$UPSTART_FDS" -
James Hunt (jamesodhunt) wrote : | # |
Hi Cameron,
Yes, this does seem to be working, but if you run 'sudo upstart-
$ sudo extra/upstart-
job_add_socket: Found socket
upstart_job_added: Job got added /com/ubuntu/
job_add_socket: Found socket
upstart-
Cameron Norman (cameronnemo) wrote : | # |
That is strange, because I am not seeing the failure. Are you sure you are using r1602?
Cameron Norman (cameronnemo) wrote : | # |
Wait, I think I may understand why. Is the system upstart-
Cameron Norman (cameronnemo) wrote : | # |
why the ipv6 one is not binding
I meant that it is binding, sorry.
James Hunt (jamesodhunt) wrote : | # |
Hi Cameron - yes, you were right :-)
Merged - thanks again for your and Kai's work on this!
Preview Diff
1 | === modified file 'ChangeLog' | |||
2 | --- ChangeLog 2014-02-04 11:32:56 +0000 | |||
3 | +++ ChangeLog 2014-02-11 02:35:11 +0000 | |||
4 | @@ -1,3 +1,10 @@ | |||
5 | 1 | 2014-02-04 Cameron Norman <camerontnorman@gmail.com> | ||
6 | 2 | |||
7 | 3 | * extra/upstart-socket-bridge.c: | ||
8 | 4 | - epoll_watcher(): Fix missing breaks in a switch, move buffer and | ||
9 | 5 | remove its nih_local usage. | ||
10 | 6 | - job_add_socket(): added case for name_len being a AF_INET6 address. | ||
11 | 7 | |||
12 | 1 | 2014-02-04 James Hunt <james.hunt@ubuntu.com> | 8 | 2014-02-04 James Hunt <james.hunt@ubuntu.com> |
13 | 2 | 9 | ||
14 | 3 | * extra/man/socket-event.7: Environment variable is | 10 | * extra/man/socket-event.7: Environment variable is |
15 | @@ -9,6 +16,15 @@ | |||
16 | 9 | stealing the console in an container: fopen(3) may not specify | 16 | stealing the console in an container: fopen(3) may not specify |
17 | 10 | O_NOCTTY and Upstart should not own the console (LP: #1263738). | 17 | O_NOCTTY and Upstart should not own the console (LP: #1263738). |
18 | 11 | 18 | ||
19 | 19 | 2014-01-20 Cameron Norman <camerontnorman@gmail.com> | ||
20 | 20 | |||
21 | 21 | * extra/upstart-socket-bridge.c: Fixed indentation, used nih_local. | ||
22 | 22 | * extra/man/socket-event.7: Added inet6 example. | ||
23 | 23 | |||
24 | 24 | 2014-01-19 Kai Mast <mail@kai-mast.de> | ||
25 | 25 | |||
26 | 26 | * extra/upstart-socket-bridge.c: Added IPv6 support (LP: #942955). | ||
27 | 27 | |||
28 | 12 | 2014-01-17 James Hunt <james.hunt@ubuntu.com> | 28 | 2014-01-17 James Hunt <james.hunt@ubuntu.com> |
29 | 13 | 29 | ||
30 | 14 | * init/conf.c: | 30 | * init/conf.c: |
31 | 15 | 31 | ||
32 | === modified file 'extra/man/socket-event.7' | |||
33 | --- extra/man/socket-event.7 2014-02-04 11:32:56 +0000 | |||
34 | +++ extra/man/socket-event.7 2014-02-11 02:35:11 +0000 | |||
35 | @@ -39,7 +39,7 @@ | |||
36 | 39 | .\" | 39 | .\" |
37 | 40 | .SH EXAMPLES | 40 | .SH EXAMPLES |
38 | 41 | .\" | 41 | .\" |
40 | 42 | .SS Internet socket | 42 | .SS Internet (IPv4) socket |
41 | 43 | Start web server when first client connects from localhost: | 43 | Start web server when first client connects from localhost: |
42 | 44 | .RS | 44 | .RS |
43 | 45 | .nf | 45 | .nf |
44 | @@ -48,6 +48,15 @@ | |||
45 | 48 | .fi | 48 | .fi |
46 | 49 | .RE | 49 | .RE |
47 | 50 | .\" | 50 | .\" |
48 | 51 | .SS Internet (IPv6) socket | ||
49 | 52 | Start job when a client connects from localhost: | ||
50 | 53 | .RS | ||
51 | 54 | .nf | ||
52 | 55 | |||
53 | 56 | start on socket PROTO=inet6 PORT=80 ADDR=::1 | ||
54 | 57 | .fi | ||
55 | 58 | .RE | ||
56 | 59 | .\" | ||
57 | 51 | .SS Local socket | 60 | .SS Local socket |
58 | 52 | .P | 61 | .P |
59 | 53 | .RS | 62 | .RS |
60 | 54 | 63 | ||
61 | === modified file 'extra/upstart-socket-bridge.c' | |||
62 | --- extra/upstart-socket-bridge.c 2013-10-23 16:33:09 +0000 | |||
63 | +++ extra/upstart-socket-bridge.c 2014-02-11 02:35:11 +0000 | |||
64 | @@ -67,9 +67,10 @@ | |||
65 | 67 | NihList entry; | 67 | NihList entry; |
66 | 68 | 68 | ||
67 | 69 | union { | 69 | union { |
71 | 70 | struct sockaddr addr; | 70 | struct sockaddr addr; |
72 | 71 | struct sockaddr_in sin_addr; | 71 | struct sockaddr_in sin_addr; |
73 | 72 | struct sockaddr_un sun_addr; | 72 | struct sockaddr_in6 sin6_addr; |
74 | 73 | struct sockaddr_un sun_addr; | ||
75 | 73 | }; | 74 | }; |
76 | 74 | socklen_t addrlen; | 75 | socklen_t addrlen; |
77 | 75 | 76 | ||
78 | @@ -295,6 +296,7 @@ | |||
79 | 295 | size_t env_len = 0; | 296 | size_t env_len = 0; |
80 | 296 | char *var; | 297 | char *var; |
81 | 297 | DBusPendingCall *pending_call; | 298 | DBusPendingCall *pending_call; |
82 | 299 | char buffer[INET6_ADDRSTRLEN]; | ||
83 | 298 | 300 | ||
84 | 299 | if (event[i].events & EPOLLIN) | 301 | if (event[i].events & EPOLLIN) |
85 | 300 | nih_debug ("%p EPOLLIN", sock); | 302 | nih_debug ("%p EPOLLIN", sock); |
86 | @@ -308,18 +310,35 @@ | |||
87 | 308 | switch (sock->addr.sa_family) { | 310 | switch (sock->addr.sa_family) { |
88 | 309 | case AF_INET: | 311 | case AF_INET: |
89 | 310 | NIH_MUST (nih_str_array_add (&env, NULL, &env_len, | 312 | NIH_MUST (nih_str_array_add (&env, NULL, &env_len, |
102 | 311 | "PROTO=inet")); | 313 | "PROTO=inet")); |
103 | 312 | 314 | ||
104 | 313 | var = NIH_MUST (nih_sprintf (NULL, "PORT=%d", | 315 | var = NIH_MUST (nih_sprintf (NULL, "PORT=%d", |
105 | 314 | ntohs (sock->sin_addr.sin_port))); | 316 | ntohs (sock->sin_addr.sin_port))); |
106 | 315 | NIH_MUST (nih_str_array_addp (&env, NULL, &env_len, | 317 | NIH_MUST (nih_str_array_addp (&env, NULL, &env_len, |
107 | 316 | var)); | 318 | var)); |
108 | 317 | nih_discard (var); | 319 | nih_discard (var); |
109 | 318 | 320 | ||
110 | 319 | var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s", | 321 | var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s", |
111 | 320 | inet_ntoa (sock->sin_addr.sin_addr))); | 322 | inet_ntoa (sock->sin_addr.sin_addr))); |
112 | 321 | NIH_MUST (nih_str_array_addp (&env, NULL, &env_len, | 323 | NIH_MUST (nih_str_array_addp (&env, NULL, &env_len, |
113 | 322 | var)); | 324 | var)); |
114 | 325 | nih_discard (var); | ||
115 | 326 | break; | ||
116 | 327 | case AF_INET6: | ||
117 | 328 | NIH_MUST (nih_str_array_add (&env, NULL, &env_len, | ||
118 | 329 | "PROTO=inet6")); | ||
119 | 330 | |||
120 | 331 | var = NIH_MUST (nih_sprintf (NULL, "PORT=%d", | ||
121 | 332 | ntohs (sock->sin6_addr.sin6_port))); | ||
122 | 333 | NIH_MUST (nih_str_array_addp (&env, NULL, &env_len, | ||
123 | 334 | var)); | ||
124 | 335 | nih_discard (var); | ||
125 | 336 | |||
126 | 337 | var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s", | ||
127 | 338 | inet_ntop(AF_INET6, &sock->sin6_addr.sin6_addr, buffer, INET6_ADDRSTRLEN))); | ||
128 | 339 | |||
129 | 340 | NIH_MUST (nih_str_array_addp (&env, NULL, &env_len, | ||
130 | 341 | var)); | ||
131 | 323 | nih_discard (var); | 342 | nih_discard (var); |
132 | 324 | break; | 343 | break; |
133 | 325 | case AF_UNIX: | 344 | case AF_UNIX: |
134 | @@ -504,6 +523,11 @@ | |||
135 | 504 | sock->sin_addr.sin_family = AF_INET; | 523 | sock->sin_addr.sin_family = AF_INET; |
136 | 505 | sock->sin_addr.sin_addr.s_addr = INADDR_ANY; | 524 | sock->sin_addr.sin_addr.s_addr = INADDR_ANY; |
137 | 506 | components = 1; | 525 | components = 1; |
138 | 526 | } else if (! strcmp (val, "inet6")) { | ||
139 | 527 | sock->addrlen = sizeof sock->sin6_addr; | ||
140 | 528 | sock->sin6_addr.sin6_family = AF_INET6; | ||
141 | 529 | sock->sin6_addr.sin6_addr = in6addr_any; | ||
142 | 530 | components = 1; | ||
143 | 507 | } else if (! strcmp (val, "unix")) { | 531 | } else if (! strcmp (val, "unix")) { |
144 | 508 | sock->addrlen = sizeof sock->sun_addr; | 532 | sock->addrlen = sizeof sock->sun_addr; |
145 | 509 | sock->sun_addr.sun_family = AF_UNIX; | 533 | sock->sun_addr.sun_family = AF_UNIX; |
146 | @@ -515,22 +539,31 @@ | |||
147 | 515 | } | 539 | } |
148 | 516 | 540 | ||
149 | 517 | } else if (! strncmp (*env, "PORT", name_len) | 541 | } else if (! strncmp (*env, "PORT", name_len) |
151 | 518 | && (sock->sin_addr.sin_family == AF_INET)) { | 542 | && (sock->sin_addr.sin_family == AF_INET)) { |
152 | 519 | sock->sin_addr.sin_port = htons (atoi (val)); | 543 | sock->sin_addr.sin_port = htons (atoi (val)); |
153 | 520 | components--; | 544 | components--; |
155 | 521 | 545 | } else if (! strncmp (*env, "PORT", name_len) | |
156 | 546 | && (sock->sin6_addr.sin6_family == AF_INET6)) { | ||
157 | 547 | sock->sin6_addr.sin6_port = htons (atoi (val)); | ||
158 | 548 | components--; | ||
159 | 522 | } else if (! strncmp (*env, "ADDR", name_len) | 549 | } else if (! strncmp (*env, "ADDR", name_len) |
161 | 523 | && (sock->sin_addr.sin_family == AF_INET)) { | 550 | && (sock->sin_addr.sin_family == AF_INET)) { |
162 | 524 | if (inet_aton (val, &(sock->sin_addr.sin_addr)) == 0) { | 551 | if (inet_aton (val, &(sock->sin_addr.sin_addr)) == 0) { |
163 | 525 | nih_warn ("Ignored socket event with invalid ADDR=%s in %s", | 552 | nih_warn ("Ignored socket event with invalid ADDR=%s in %s", |
168 | 526 | val, job->path); | 553 | val, job->path); |
169 | 527 | goto error; | 554 | goto error; |
170 | 528 | } | 555 | } |
171 | 529 | 556 | } else if (! strncmp (*env, "ADDR", name_len) | |
172 | 557 | && (sock->sin6_addr.sin6_family == AF_INET6)) { | ||
173 | 558 | if (inet_pton (AF_INET6, val, &(sock->sin6_addr.sin6_addr)) != 1) { | ||
174 | 559 | nih_warn ("Ignored socket event with invalid ADDR=%s in %s", | ||
175 | 560 | val, job->path); | ||
176 | 561 | goto error; | ||
177 | 562 | } | ||
178 | 530 | } else if (! strncmp (*env, "PATH", name_len) | 563 | } else if (! strncmp (*env, "PATH", name_len) |
180 | 531 | && (sock->sun_addr.sun_family == AF_UNIX)) { | 564 | && (sock->sun_addr.sun_family == AF_UNIX)) { |
181 | 532 | strncpy (sock->sun_addr.sun_path, val, | 565 | strncpy (sock->sun_addr.sun_path, val, |
183 | 533 | sizeof sock->sun_addr.sun_path); | 566 | sizeof sock->sun_addr.sun_path); |
184 | 534 | 567 | ||
185 | 535 | if (sock->sun_addr.sun_path[0] == '@') | 568 | if (sock->sun_addr.sun_path[0] == '@') |
186 | 536 | sock->sun_addr.sun_path[0] = '\0'; | 569 | sock->sun_addr.sun_path[0] = '\0'; |
187 | @@ -567,6 +600,15 @@ | |||
188 | 567 | goto error; | 600 | goto error; |
189 | 568 | } | 601 | } |
190 | 569 | 602 | ||
191 | 603 | /* If socket is ipv6, need to set IPV6_V6ONLY option */ | ||
192 | 604 | if (sock->sin6_addr.sin6_family == AF_INET6 && | ||
193 | 605 | setsockopt (sock->sock, SOL_IPV6, IPV6_V6ONLY, | ||
194 | 606 | &opt, sizeof opt) < 0) { | ||
195 | 607 | nih_warn ("Failed to set IPV6 only option in %s: %s", | ||
196 | 608 | job->path, strerror (errno)); | ||
197 | 609 | goto error; | ||
198 | 610 | } | ||
199 | 611 | |||
200 | 570 | if (bind (sock->sock, &sock->addr, sock->addrlen) < 0) { | 612 | if (bind (sock->sock, &sock->addr, sock->addrlen) < 0) { |
201 | 571 | nih_warn ("Failed to bind socket in %s: %s", | 613 | nih_warn ("Failed to bind socket in %s: %s", |
202 | 572 | job->path, strerror (errno)); | 614 | job->path, strerror (errno)); |
Hi Cameron,
Thanks to you and Kai for undertaking this new feature!
A few comments:
* extra/upstart- socket- bridge. c:
- epoll_watcher():
- 'break' has been lost from end of the AF_INET block.
- buffer should not use nih_local (as that is only for allocated storage) and should really be at the
top of the 'for' block.
- AF_INET6 block missing a 'break' at the end.
- job_add_socket(): '! strcmp (*env, "ADDR")' needs to be updated for IPv6 (using inet_pton() rather than
inet_aton() maybe?).
I tried testing this using the new IPv6 example from socket-event.7, but I think the last comment above is causing the job to be rejected.