Hi,
On Wed, 2015-08-05 at 19:16 +0300, Alexey Kodanev wrote:
> Hi,
> On 08/05/2015 05:00 PM, Cyril Hrubis wrote:
> > Hi!
> >> +int safe_bind(const char *file, const int lineno, void (cleanup_fn)(void),
> >> + int socket, const struct sockaddr *address,
> >> + socklen_t address_len)
> >> +{
> >> + int err, ret, i;
> >> +
> >> + for (i = 0; i < 30; i++) {
> 30 sec still may not be enough, I would set it to 2 minutes.
To be honest, I am not sure how long we should wait.
2 minutes is OK for me.
>
> >> + ret = bind(socket, address, address_len);
> >> + err = errno;
> >> +
> >> + if (!ret)
> >> + return 0;
> >> +
> >> + if (err != EADDRINUSE) {
> >> + tst_brkm(TBROK | TERRNO, cleanup_fn,
>
> Also, we don't need ret and err vars, we could do:
>
> if (!bind(socket, address, address_len))
> return 0;
>
> if (errno != EADDRINUSE)
> tst_brkm(TBROK | TERRNO, ....);
>
Hmm, yes. :)
> >> + "%s:%d: bind(%d, %p, %d) failed", file, lineno,
> >> + socket, address, address_len);
> > I think that it would be a bit nicer if we created a helper to translate
> > the socket structure to it's string representation (and use it whenever
> > we deal with struct sockaddr passed from user). We would have to switch
> > on sa_family and then print either ip address or path (for unix
> > sockets), etc. But that would get us bind(8, '127.0.0.1:1024', ...)
> > failed instead of the raw pointer to the structure which is kind of
> > useless.
> >
> >> + }
> >> +
> >> + tst_resm(TINFO, "bind('%p') failed with %s, try %2i...",
> >> + address, tst_strerrno(err), i+1);
> > Hmm, why do we do tst_strerno(err) here when
> > the only value for errno can be EADDRINUSE
> > (since we exit with tst_brkm() otherwise)
> > Why not just hardcode EADDRINUSE to the
> > message?
>
> Also, we shouldn't print the message every second while waiting, could be:
>
> if ((i + 1) % 10 == 0)
> tst_resm(TINFO, "address is in use, waited %3i sec", i + 1);
I am tend to use "(i+1) % 10 == 1" or "i % 10 == 0".
Thank & best regards,
Zeng
>
> Best regards,
> Alexey
>
------------------------------------------------------------------------------
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
