Merge lp:~cameronnemo/upstart/ipv6 into lp:upstart

Proposed by Cameron Norman on 2014-02-04
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
Reviewer Review Type Date Requested Status
James Hunt 2014-02-04 Approve on 2014-02-19
Review via email: mp+204599@code.launchpad.net

Description of the change

Adds the inet6 option for the socket event parameter PROTO and the corresponding man page examples.

To post a comment you must log in.
lp:~cameronnemo/upstart/ipv6 updated on 2014-02-04
1595. By Cameron Norman on 2014-02-04

remerge from trunk

1596. By Cameron Norman on 2014-02-04

whoopsie in ChangeLog

James Hunt (jamesodhunt) wrote :

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.

review: Needs Fixing
lp:~cameronnemo/upstart/ipv6 updated on 2014-02-05
1597. By Cameron Norman on 2014-02-05

upstart-socket-bridge.c: epoll_watcher() and job_add_socket() fixes for ipv6

1598. By Cameron Norman on 2014-02-05

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-socket-bridge --debug

And finally connecting to port 80 via IPv6:

$ echo foo | socat 'tcp6-connect:[::1]:80' -

Thanks!

review: Needs Fixing
lp:~cameronnemo/upstart/ipv6 updated on 2014-02-06
1599. By Cameron Norman on 2014-02-06

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/test-socket-bridge-ipv4.confj
start on socket PROTO=inet PORT=80 ADDR=127.0.0.1
exec echo ipv4
EOT

$ cat <<EOT | sudo tee /etc/init/test-socket-bridge-ipv6.conf
start on socket PROTO=inet6 PORT=80 ADDR=::1
exec echo ipv6
EOT

$ sudo extra/upstart-socket-bridge --debug
job_add_socket: Found socket
upstart_job_added: Job got added /com/ubuntu/Upstart/jobs/test_2dsocket_2dbridge_2dipv6
job_add_socket: Found socket
upstart-socket-bridge: Failed to bind socket in /com/ubuntu/Upstart/jobs/test_2dsocket_2dbridge_2dipv4: Address already in use

I think we may need some IPV6_V6ONLY setsockopt(2) magic to resolve this?

review: Needs Fixing
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?).

lp:~cameronnemo/upstart/ipv6 updated on 2014-02-11
1600. By Cameron Norman on 2014-02-11

use setsockopt to only bind to ipv6

1601. By Cameron Norman on 2014-02-11

whoops

1602. By Cameron Norman on 2014-02-11

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-socket-bridge --debug' with the 2 test jobs above, an error is raised on the 2nd bind:

$ sudo extra/upstart-socket-bridge --debug
job_add_socket: Found socket
upstart_job_added: Job got added /com/ubuntu/Upstart/jobs/test_2dsocket_2dbridge_2dipv6
job_add_socket: Found socket
upstart-socket-bridge: Failed to bind socket in /com/ubuntu/Upstart/jobs/test_2dsocket_2dbridge_2dipv4: Address already in use

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-socket-bridge creating the socket for test-socket-bridge-ipv4.conf at boot? That would explain why the ipv6 one is not binding: the socket bridge you run at boot can not bind to the ipv6 socket. Other than that, I am at a loss as to why it works here, but not there.

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!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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+2014-02-04 Cameron Norman <camerontnorman@gmail.com>
6+
7+ * extra/upstart-socket-bridge.c:
8+ - epoll_watcher(): Fix missing breaks in a switch, move buffer and
9+ remove its nih_local usage.
10+ - job_add_socket(): added case for name_len being a AF_INET6 address.
11+
12 2014-02-04 James Hunt <james.hunt@ubuntu.com>
13
14 * extra/man/socket-event.7: Environment variable is
15@@ -9,6 +16,15 @@
16 stealing the console in an container: fopen(3) may not specify
17 O_NOCTTY and Upstart should not own the console (LP: #1263738).
18
19+2014-01-20 Cameron Norman <camerontnorman@gmail.com>
20+
21+ * extra/upstart-socket-bridge.c: Fixed indentation, used nih_local.
22+ * extra/man/socket-event.7: Added inet6 example.
23+
24+2014-01-19 Kai Mast <mail@kai-mast.de>
25+
26+ * extra/upstart-socket-bridge.c: Added IPv6 support (LP: #942955).
27+
28 2014-01-17 James Hunt <james.hunt@ubuntu.com>
29
30 * init/conf.c:
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 .\"
37 .SH EXAMPLES
38 .\"
39-.SS Internet socket
40+.SS Internet (IPv4) socket
41 Start web server when first client connects from localhost:
42 .RS
43 .nf
44@@ -48,6 +48,15 @@
45 .fi
46 .RE
47 .\"
48+.SS Internet (IPv6) socket
49+Start job when a client connects from localhost:
50+.RS
51+.nf
52+
53+start on socket PROTO=inet6 PORT=80 ADDR=::1
54+.fi
55+.RE
56+.\"
57 .SS Local socket
58 .P
59 .RS
60
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 NihList entry;
66
67 union {
68- struct sockaddr addr;
69- struct sockaddr_in sin_addr;
70- struct sockaddr_un sun_addr;
71+ struct sockaddr addr;
72+ struct sockaddr_in sin_addr;
73+ struct sockaddr_in6 sin6_addr;
74+ struct sockaddr_un sun_addr;
75 };
76 socklen_t addrlen;
77
78@@ -295,6 +296,7 @@
79 size_t env_len = 0;
80 char *var;
81 DBusPendingCall *pending_call;
82+ char buffer[INET6_ADDRSTRLEN];
83
84 if (event[i].events & EPOLLIN)
85 nih_debug ("%p EPOLLIN", sock);
86@@ -308,18 +310,35 @@
87 switch (sock->addr.sa_family) {
88 case AF_INET:
89 NIH_MUST (nih_str_array_add (&env, NULL, &env_len,
90- "PROTO=inet"));
91-
92- var = NIH_MUST (nih_sprintf (NULL, "PORT=%d",
93- ntohs (sock->sin_addr.sin_port)));
94- NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
95- var));
96- nih_discard (var);
97-
98- var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s",
99- inet_ntoa (sock->sin_addr.sin_addr)));
100- NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
101- var));
102+ "PROTO=inet"));
103+
104+ var = NIH_MUST (nih_sprintf (NULL, "PORT=%d",
105+ ntohs (sock->sin_addr.sin_port)));
106+ NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
107+ var));
108+ nih_discard (var);
109+
110+ var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s",
111+ inet_ntoa (sock->sin_addr.sin_addr)));
112+ NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
113+ var));
114+ nih_discard (var);
115+ break;
116+ case AF_INET6:
117+ NIH_MUST (nih_str_array_add (&env, NULL, &env_len,
118+ "PROTO=inet6"));
119+
120+ var = NIH_MUST (nih_sprintf (NULL, "PORT=%d",
121+ ntohs (sock->sin6_addr.sin6_port)));
122+ NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
123+ var));
124+ nih_discard (var);
125+
126+ var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s",
127+ inet_ntop(AF_INET6, &sock->sin6_addr.sin6_addr, buffer, INET6_ADDRSTRLEN)));
128+
129+ NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
130+ var));
131 nih_discard (var);
132 break;
133 case AF_UNIX:
134@@ -504,6 +523,11 @@
135 sock->sin_addr.sin_family = AF_INET;
136 sock->sin_addr.sin_addr.s_addr = INADDR_ANY;
137 components = 1;
138+ } else if (! strcmp (val, "inet6")) {
139+ sock->addrlen = sizeof sock->sin6_addr;
140+ sock->sin6_addr.sin6_family = AF_INET6;
141+ sock->sin6_addr.sin6_addr = in6addr_any;
142+ components = 1;
143 } else if (! strcmp (val, "unix")) {
144 sock->addrlen = sizeof sock->sun_addr;
145 sock->sun_addr.sun_family = AF_UNIX;
146@@ -515,22 +539,31 @@
147 }
148
149 } else if (! strncmp (*env, "PORT", name_len)
150- && (sock->sin_addr.sin_family == AF_INET)) {
151+ && (sock->sin_addr.sin_family == AF_INET)) {
152 sock->sin_addr.sin_port = htons (atoi (val));
153 components--;
154-
155+ } else if (! strncmp (*env, "PORT", name_len)
156+ && (sock->sin6_addr.sin6_family == AF_INET6)) {
157+ sock->sin6_addr.sin6_port = htons (atoi (val));
158+ components--;
159 } else if (! strncmp (*env, "ADDR", name_len)
160- && (sock->sin_addr.sin_family == AF_INET)) {
161+ && (sock->sin_addr.sin_family == AF_INET)) {
162 if (inet_aton (val, &(sock->sin_addr.sin_addr)) == 0) {
163 nih_warn ("Ignored socket event with invalid ADDR=%s in %s",
164- val, job->path);
165- goto error;
166- }
167-
168+ val, job->path);
169+ goto error;
170+ }
171+ } else if (! strncmp (*env, "ADDR", name_len)
172+ && (sock->sin6_addr.sin6_family == AF_INET6)) {
173+ if (inet_pton (AF_INET6, val, &(sock->sin6_addr.sin6_addr)) != 1) {
174+ nih_warn ("Ignored socket event with invalid ADDR=%s in %s",
175+ val, job->path);
176+ goto error;
177+ }
178 } else if (! strncmp (*env, "PATH", name_len)
179- && (sock->sun_addr.sun_family == AF_UNIX)) {
180+ && (sock->sun_addr.sun_family == AF_UNIX)) {
181 strncpy (sock->sun_addr.sun_path, val,
182- sizeof sock->sun_addr.sun_path);
183+ sizeof sock->sun_addr.sun_path);
184
185 if (sock->sun_addr.sun_path[0] == '@')
186 sock->sun_addr.sun_path[0] = '\0';
187@@ -567,6 +600,15 @@
188 goto error;
189 }
190
191+ /* If socket is ipv6, need to set IPV6_V6ONLY option */
192+ if (sock->sin6_addr.sin6_family == AF_INET6 &&
193+ setsockopt (sock->sock, SOL_IPV6, IPV6_V6ONLY,
194+ &opt, sizeof opt) < 0) {
195+ nih_warn ("Failed to set IPV6 only option in %s: %s",
196+ job->path, strerror (errno));
197+ goto error;
198+ }
199+
200 if (bind (sock->sock, &sock->addr, sock->addrlen) < 0) {
201 nih_warn ("Failed to bind socket in %s: %s",
202 job->path, strerror (errno));

Subscribers

People subscribed via source and target branches