| Message ID | 20220901093303.1974274-13-dylany@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Defer taskrun changes | expand |
On 9/1/22 4:33 PM, Dylan Yudaken wrote: > This test would occasionally fail if the chosen port was in use. Rather > bind to an ephemeral port which will not be in use. > > Signed-off-by: Dylan Yudaken<dylany@fb.com> > --- > test/shutdown.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/test/shutdown.c b/test/shutdown.c > index 14c7407b5492..c584893bdd28 100644 > --- a/test/shutdown.c > +++ b/test/shutdown.c > @@ -30,6 +30,7 @@ int main(int argc, char *argv[]) > int32_t recv_s0; > int32_t val = 1; > struct sockaddr_in addr; > + socklen_t addrlen; > > if (argc > 1) > return 0; > @@ -44,7 +45,7 @@ int main(int argc, char *argv[]) > assert(ret != -1); > > addr.sin_family = AF_INET; > - addr.sin_port = htons((rand() % 61440) + 4096); > + addr.sin_port = 0; > addr.sin_addr.s_addr = inet_addr("127.0.0.1"); > > ret = bind(recv_s0, (struct sockaddr*)&addr, sizeof(addr)); > @@ -52,6 +53,10 @@ int main(int argc, char *argv[]) > ret = listen(recv_s0, 128); > assert(ret != -1); > > + addrlen = (socklen_t)sizeof(addr); > + assert(!getsockname(recv_s0, (struct sockaddr *)&addr, > + &addrlen)); > + Hi Jens, Hi Dylan, I like the idea of using an ephemeral port. This is the most reliable way to get a port number that's not in use. However, we have many places that do this rand() mechanism to generate a port number. This patch only fixes shutdown.c. What do you think of creating a new function helper like this? int t_bind_ephemeral(int fd, struct sockaddr_in *addr) { socklen_t addrlen; int ret; addr->sin_port = 0; if (bind(fd, (struct sockaddr*)addr, sizeof(*addr))) return -errno; addrlen = sizeof(*addr); assert(!getsockname(fd, (struct sockaddr *)&addr, &addrlen)); return 0; } We can avoid code duplication by doing that. I can do that. If everybody agrees, let's drop this patch and I will wire up t_bind_ephemeral() function. Yes? No?
On Thu, 2022-09-01 at 18:44 +0700, Ammar Faizi wrote: > On 9/1/22 4:33 PM, Dylan Yudaken wrote: > > This test would occasionally fail if the chosen port was in use. > > Rather > > bind to an ephemeral port which will not be in use. > > > > Signed-off-by: Dylan Yudaken<dylany@fb.com> > > --- > > test/shutdown.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/test/shutdown.c b/test/shutdown.c > > index 14c7407b5492..c584893bdd28 100644 > > --- a/test/shutdown.c > > +++ b/test/shutdown.c > > @@ -30,6 +30,7 @@ int main(int argc, char *argv[]) > > int32_t recv_s0; > > int32_t val = 1; > > struct sockaddr_in addr; > > + socklen_t addrlen; > > > > if (argc > 1) > > return 0; > > @@ -44,7 +45,7 @@ int main(int argc, char *argv[]) > > assert(ret != -1); > > > > addr.sin_family = AF_INET; > > - addr.sin_port = htons((rand() % 61440) + 4096); > > + addr.sin_port = 0; > > addr.sin_addr.s_addr = inet_addr("127.0.0.1"); > > > > ret = bind(recv_s0, (struct sockaddr*)&addr, sizeof(addr)); > > @@ -52,6 +53,10 @@ int main(int argc, char *argv[]) > > ret = listen(recv_s0, 128); > > assert(ret != -1); > > > > + addrlen = (socklen_t)sizeof(addr); > > + assert(!getsockname(recv_s0, (struct sockaddr *)&addr, > > + &addrlen)); > > + > > Hi Jens, > Hi Dylan, > > I like the idea of using an ephemeral port. This is the most > reliable way to get a port number that's not in use. However, > we have many places that do this rand() mechanism to generate > a port number. This patch only fixes shutdown.c. Good point. I only fixed that test as it seemed to be breaking often. Maybe something unlucky with the port that was selected. > > What do you think of creating a new function helper like this? > > int t_bind_ephemeral(int fd, struct sockaddr_in *addr) > { > socklen_t addrlen; > int ret; > > addr->sin_port = 0; > if (bind(fd, (struct sockaddr*)addr, sizeof(*addr))) > return -errno; > > addrlen = sizeof(*addr); > assert(!getsockname(fd, (struct sockaddr *)&addr, > &addrlen)); > return 0; > } > > We can avoid code duplication by doing that. I can do that. > If everybody agrees, let's drop this patch and I will wire up > t_bind_ephemeral() function. > > Yes? No? > I think something like that sounds sensible. There is also some duplication with t_create_socket_pair, as I suspect most tests could just be rewritten to use that instead - depending on how much effort you are looking to put into this. For now I think dropping the patch and doing it properly in some form makes a lot of sense. Dylan
On 9/1/22 7:54 PM, Dylan Yudaken wrote: > I think something like that sounds sensible. > > There is also some duplication with t_create_socket_pair, as I suspect > most tests could just be rewritten to use that instead - depending on > how much effort you are looking to put into this. > > For now I think dropping the patch and doing it properly in some form > makes a lot of sense. OK, I will do the t_bind_ephemeral() part first for now.
diff --git a/test/shutdown.c b/test/shutdown.c index 14c7407b5492..c584893bdd28 100644 --- a/test/shutdown.c +++ b/test/shutdown.c @@ -30,6 +30,7 @@ int main(int argc, char *argv[]) int32_t recv_s0; int32_t val = 1; struct sockaddr_in addr; + socklen_t addrlen; if (argc > 1) return 0; @@ -44,7 +45,7 @@ int main(int argc, char *argv[]) assert(ret != -1); addr.sin_family = AF_INET; - addr.sin_port = htons((rand() % 61440) + 4096); + addr.sin_port = 0; addr.sin_addr.s_addr = inet_addr("127.0.0.1"); ret = bind(recv_s0, (struct sockaddr*)&addr, sizeof(addr)); @@ -52,6 +53,10 @@ int main(int argc, char *argv[]) ret = listen(recv_s0, 128); assert(ret != -1); + addrlen = (socklen_t)sizeof(addr); + assert(!getsockname(recv_s0, (struct sockaddr *)&addr, + &addrlen)); + p_fd[1] = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP); val = 1;
This test would occasionally fail if the chosen port was in use. Rather bind to an ephemeral port which will not be in use. Signed-off-by: Dylan Yudaken <dylany@fb.com> --- test/shutdown.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)