Re: [PATCH liburing v2 12/12] shutdown test: bind to ephemeral port

[Dylan Yudaken <dylany@fb.com>]

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