Re: [PATCH] suppress "return value is not checked" warnings

[Willy Tarreau]

On Fri, Jul 31, 2020 at 02:32:02PM +0500,  ??? wrote:
> > Probably we should proceed differently and have something like these
> > for the cases where no return is desired:
> >
> > fcntl_noret()  => unchecked fcntl()
> > setsockopt_noret() => unchecked setsockopt()
> >
> > Both would be just static inline functions wrapping the other ones in a
> > DISGUISE() if needed.
> >
> > Then we could change the calls above to be more explicit about the intent
> > of not having a return, and less ugly to read.
> >
> > Do you want to try to do something like this ?
> >
> 
> those issues are not urgent.
> I will try.

Thank you. Do not hesitate to ask if you need help or if some parts
are not clear yet.

Cheers,
Willy

Re: [PATCH] suppress "return value is not checked" warnings

[Илья Шипицин]

пт, 31 июл. 2020 г. в 14:26, Willy Tarreau :

> Hi Ilya,
>
> On Sun, Jul 26, 2020 at 03:06:06PM +0500,  ??? wrote:
> > From e5a49969d374e3e8e9da695dca48cb6fa82ca13d Mon Sep 17 00:00:00 2001
> > From: Ilya Shipitsin 
> > Date: Sun, 26 Jul 2020 15:01:10 +0500
> > Subject: [PATCH] CLEANUP: suppress coverity warnings
> >
> > Coverity is not happy when return value is not examined. Those
> > warnings are already marked as "Intentional", let us explicitely
> > mark them with DISGUISE(..)
> (...)
> > - setsockopt(conn->handle.fd, IPPROTO_IP, IP_TOS, ,
> sizeof(tos));
> > + DISGUISE(setsockopt(conn->handle.fd, IPPROTO_IP, IP_TOS,
> , sizeof(tos)));
> (...)
>
> This is getting quite ugly over time. We already had to deal with some
> of these when some libcs added a must_check attribute to some calls
> like write() which we used only for debugging.
>
> Your change doesn't just address Coverity's caprices but could as well
> serve if some libcs add this attribute later. But I don't like what the
> code looks like (mainly because it becomes ubiquitous and not exceptional
> anymore).
>
> Probably we should proceed differently and have something like these
> for the cases where no return is desired:
>
> fcntl_noret()  => unchecked fcntl()
> setsockopt_noret() => unchecked setsockopt()
>
> Both would be just static inline functions wrapping the other ones in a
> DISGUISE() if needed.
>
> Then we could change the calls above to be more explicit about the intent
> of not having a return, and less ugly to read.
>
> Do you want to try to do something like this ?
>

those issues are not urgent.
I will try.



>
> Thanks,
> Willy
>

Re: [PATCH] suppress "return value is not checked" warnings

[Willy Tarreau]

Hi Ilya,

On Sun, Jul 26, 2020 at 03:06:06PM +0500,  ??? wrote:
> From e5a49969d374e3e8e9da695dca48cb6fa82ca13d Mon Sep 17 00:00:00 2001
> From: Ilya Shipitsin 
> Date: Sun, 26 Jul 2020 15:01:10 +0500
> Subject: [PATCH] CLEANUP: suppress coverity warnings
> 
> Coverity is not happy when return value is not examined. Those
> warnings are already marked as "Intentional", let us explicitely
> mark them with DISGUISE(..)
(...)
> - setsockopt(conn->handle.fd, IPPROTO_IP, IP_TOS, , 
> sizeof(tos));
> + DISGUISE(setsockopt(conn->handle.fd, IPPROTO_IP, IP_TOS, , 
> sizeof(tos)));
(...)

This is getting quite ugly over time. We already had to deal with some
of these when some libcs added a must_check attribute to some calls
like write() which we used only for debugging.

Your change doesn't just address Coverity's caprices but could as well
serve if some libcs add this attribute later. But I don't like what the
code looks like (mainly because it becomes ubiquitous and not exceptional
anymore).

Probably we should proceed differently and have something like these
for the cases where no return is desired:

fcntl_noret()  => unchecked fcntl()
setsockopt_noret() => unchecked setsockopt()

Both would be just static inline functions wrapping the other ones in a
DISGUISE() if needed.

Then we could change the calls above to be more explicit about the intent
of not having a return, and less ugly to read.

Do you want to try to do something like this ?

Thanks,
Willy

Re: [PATCH] suppress "return value is not checked" warnings

[Илья Шипицин]

ping.

вс, 26 июл. 2020 г. в 15:06, Илья Шипицин :

> Hello,
>
> cleanup patch attached.
>
>
> Ilya Shipitcin
>

[PATCH] suppress "return value is not checked" warnings

[Илья Шипицин]

Hello,

cleanup patch attached.


Ilya Shipitcin
From e5a49969d374e3e8e9da695dca48cb6fa82ca13d Mon Sep 17 00:00:00 2001
From: Ilya Shipitsin 
Date: Sun, 26 Jul 2020 15:01:10 +0500
Subject: [PATCH] CLEANUP: suppress coverity warnings

Coverity is not happy when return value is not examined. Those
warnings are already marked as "Intentional", let us explicitely
mark them with DISGUISE(..)
---
 include/haproxy/connection.h | 10 +-
 src/cli.c|  2 +-
 src/dns.c|  2 +-
 src/fd.c |  2 +-
 src/haproxy.c|  2 +-
 src/listener.c   |  2 +-
 src/log.c|  4 ++--
 src/mworker.c|  2 +-
 src/pipe.c   |  2 +-
 src/proto_sockpair.c |  4 ++--
 src/proto_tcp.c  | 14 +++---
 src/proto_udp.c  |  2 +-
 src/proto_uxst.c |  4 ++--
 src/session.c| 16 
 14 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index 05e85e1cb..c7eb7ab19 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -582,15 +582,15 @@ static inline void conn_set_tos(const struct connection *conn, int tos)
 
 #ifdef IP_TOS
 	if (conn->src->ss_family == AF_INET)
-		setsockopt(conn->handle.fd, IPPROTO_IP, IP_TOS, , sizeof(tos));
+		DISGUISE(setsockopt(conn->handle.fd, IPPROTO_IP, IP_TOS, , sizeof(tos)));
 #endif
 #ifdef IPV6_TCLASS
 	if (conn->src->ss_family == AF_INET6) {
 		if (IN6_IS_ADDR_V4MAPPED(&((struct sockaddr_in6 *)conn->src)->sin6_addr))
 			/* v4-mapped addresses need IP_TOS */
-			setsockopt(conn->handle.fd, IPPROTO_IP, IP_TOS, , sizeof(tos));
+			DISGUISE(setsockopt(conn->handle.fd, IPPROTO_IP, IP_TOS, , sizeof(tos)));
 		else
-			setsockopt(conn->handle.fd, IPPROTO_IPV6, IPV6_TCLASS, , sizeof(tos));
+			DISGUISE(setsockopt(conn->handle.fd, IPPROTO_IPV6, IPV6_TCLASS, , sizeof(tos)));
 	}
 #endif
 }
@@ -604,7 +604,7 @@ static inline void conn_set_mark(const struct connection *conn, int mark)
 		return;
 
 #ifdef SO_MARK
-	setsockopt(conn->handle.fd, SOL_SOCKET, SO_MARK, , sizeof(mark));
+	DISGUISE(setsockopt(conn->handle.fd, SOL_SOCKET, SO_MARK, , sizeof(mark)));
 #endif
 }
 
@@ -617,7 +617,7 @@ static inline void conn_set_quickack(const struct connection *conn, int value)
 		return;
 
 #ifdef TCP_QUICKACK
-	setsockopt(conn->handle.fd, IPPROTO_TCP, TCP_QUICKACK, , sizeof(value));
+	DISGUISE(setsockopt(conn->handle.fd, IPPROTO_TCP, TCP_QUICKACK, , sizeof(value)));
 #endif
 }
 
diff --git a/src/cli.c b/src/cli.c
index c1964df72..87d585259 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -1717,7 +1717,7 @@ static int _getsocks(char **args, char *payload, struct appctx *appctx, void *pr
 		ha_warning("Cannot make the unix socket blocking\n");
 		goto out;
 	}
-	setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, (void *), sizeof(tv));
+	DISGUISE(setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, (void *), sizeof(tv)));
 	iov.iov_base = _fd_nb;
 	iov.iov_len = sizeof(tot_fd_nb);
 	if (!(strm_li(s)->bind_conf->level & ACCESS_FD_LISTENERS))
diff --git a/src/dns.c b/src/dns.c
index 6a8ab831c..81144d303 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -207,7 +207,7 @@ static int dns_connect_namesaver(struct dns_nameserver *ns)
 	}
 
 	/* Make the socket non blocking */
-	fcntl(fd, F_SETFL, O_NONBLOCK);
+	DISGUISE(fcntl(fd, F_SETFL, O_NONBLOCK));
 
 	/* Add the fd in the fd list and update its parameters */
 	dgram->t.sock.fd = fd;
diff --git a/src/fd.c b/src/fd.c
index bf2383e4d..d1a7dfbf0 100644
--- a/src/fd.c
+++ b/src/fd.c
@@ -636,7 +636,7 @@ static int init_pollers_per_thread()
 
 	poller_rd_pipe = mypipe[0];
 	poller_wr_pipe[tid] = mypipe[1];
-	fcntl(poller_rd_pipe, F_SETFL, O_NONBLOCK);
+	DISGUISE(fcntl(poller_rd_pipe, F_SETFL, O_NONBLOCK));
 	fd_insert(poller_rd_pipe, poller_pipe_io_handler, poller_pipe_io_handler,
 	tid_bit);
 	fd_want_recv(poller_rd_pipe);
diff --git a/src/haproxy.c b/src/haproxy.c
index c3bc1f0e5..906f69903 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -1160,7 +1160,7 @@ static int get_old_sockets(const char *unixsocket)
 			   unixsocket);
 		goto out;
 	}
-	setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *), sizeof(tv));
+	DISGUISE(setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *), sizeof(tv)));
 	iov.iov_base = _nb;
 	iov.iov_len = sizeof(fd_nb);
 	msghdr.msg_iov = 
diff --git a/src/listener.c b/src/listener.c
index cad0e0cc8..cadb99c59 100644
--- a/src/listener.c
+++ b/src/listener.c
@@ -802,7 +802,7 @@ void listener_accept(int fd)
 			(accept4_broken = 1
 #endif
 			if ((cfd = accept(fd, (struct sockaddr *), )) != -1)
-fcntl(cfd, F_SETFL, O_NONBLOCK);
+DISGUISE(fcntl(cfd, F_SETFL, O_NONBLOCK));
 
 		if (unlikely(cfd == -1)) {
 			switch (errno) {
diff --git a/src/log.c b/src/log.c
index 495a672d2..50d2ae4ec 100644
--- a/src/log.c
+++ b/src/log.c
@@ -1775,10 +1775,10 @@ static inline void __do_send_log(struct logsrv *logsrv,