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@xxxxxx>
> > ---
> > 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
