Re: [PATCH v2 bpf-next 4/5] selftests/bpf: Add connect_fd_to_fd, connect_wait net helpers

[Andrey Ignatov <rdna@fb.com>]

Yonghong Song <yhs@fb.com> [Thu, 2020-05-14 08:56 -0700]:
> On 5/13/20 2:38 PM, Andrey Ignatov wrote:

> > @@ -77,8 +81,6 @@ static const size_t timeo_optlen = sizeof(timeo_sec);
> >   int connect_to_fd(int family, int type, int server_fd)
> >   {
> > -	struct sockaddr_storage addr;
> > -	socklen_t len = sizeof(addr);
> >   	int fd;
> >   	fd = socket(family, type, 0);
> > @@ -87,24 +89,64 @@ int connect_to_fd(int family, int type, int server_fd)
> >   		return -1;
> >   	}
> > -	if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec, timeo_optlen)) {
> > +	if (connect_fd_to_fd(fd, server_fd) < 0 && errno != EINPROGRESS) {
> > +		close(fd);
> 
> Remote possibility. close(fd) may change error code?

It can in some cases that are rather theoritical in this case (e.g.
buggy multi-threaded program closes fd from another thread right before
this close()).

But I can save/restore it before/after close just in case.

> In my opinion, maybe convert the original syscall failure errno to return
> value and carrying on might be a simpler choice?

I wanted to preserve semantics of connect(2) here: return -1 on error,
or fd >= 0 on success.

I guess if I save/restore errno it should be fine.

> > +		return -1;
> > +	}
> > +
> > +	return fd;
> > +}
> > +
> > +int connect_fd_to_fd(int client_fd, int server_fd)
> > +{
> > +	struct sockaddr_storage addr;
> > +	socklen_t len = sizeof(addr);
> > +
> > +	if (setsockopt(client_fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec,
> > +		       timeo_optlen)) {
> >   		log_err("Failed to set SO_RCVTIMEO");
> > -		goto out;
> > +		return -1;
> >   	}
> >   	if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
> >   		log_err("Failed to get server addr");
> > -		goto out;
> > +		return -1;
> >   	}
> > -	if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
> > -		log_err("Fail to connect to server with family %d", family);
> > -		goto out;
> > +	if (connect(client_fd, (const struct sockaddr *)&addr, len) < 0) {
> > +		if (errno != EINPROGRESS)
> > +			log_err("Failed to connect to server");
> 
> Not saying it is possible, but any remote possibility log_err()
> may change error code to EINPROGRESS?

To my best knowledge, neither fprintf(3) nor strerror(3) use
EINPROGRESS, but since in this case having a reliable way to communicate
EINPROGRESS from connect is rather required, I'll save/restore errno for
caling log_err.

-- 
Andrey Ignatov