Re: [PATCH 6/8] treewide: Address cert-err33-c clang-tidy warnings for clock and timer functions

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5084 bytes --]

On Fri, Oct 25, 2024 at 01:04:36AM +0200, Stefano Brivio wrote:
> For clock_gettime(), we shouldn't ignore errors if they happen at
> initialisation phase, because something is seriously wrong and it's
> not helpful if we proceed as if nothing happened.
> 
> As we're up and running, though, it's probably better to use a stale
> value than to terminate altogether. Make sure we use a zero value if
> we don't have a stale one somewhere.
> 
> For timerfd_gettime() and timerfd_settime() failures, report an error,
> there isn't much else we can do.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  passt.c |  8 +++++---
>  pcap.c  | 12 ++++++------
>  tcp.c   | 12 +++++++++---
>  3 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/passt.c b/passt.c
> index ad6f0bc..e987f0d 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -207,7 +207,8 @@ int main(int argc, char **argv)
>  	struct timespec now;
>  	struct sigaction sa;
>  
> -	clock_gettime(CLOCK_MONOTONIC, &log_start);
> +	if (clock_gettime(CLOCK_MONOTONIC, &log_start))
> +		die_perror("Failed to get CLOCK_MONOTONIC time");
>  
>  	arch_avx2_exec(argv);
>  
> @@ -265,7 +266,8 @@ int main(int argc, char **argv)
>  
>  	secret_init(&c);
>  
> -	clock_gettime(CLOCK_MONOTONIC, &now);
> +	if (clock_gettime(CLOCK_MONOTONIC, &now))
> +		die_perror("Failed to get CLOCK_MONOTONIC time");
>  
>  	flow_init();
>  
> @@ -313,7 +315,7 @@ loop:
>  	if (nfds == -1 && errno != EINTR)
>  		die_perror("epoll_wait() failed in main loop");
>  
> -	clock_gettime(CLOCK_MONOTONIC, &now);
> +	(void)clock_gettime(CLOCK_MONOTONIC, &now);

I think we should err() here.

>  
>  	for (i = 0; i < nfds; i++) {
>  		union epoll_ref ref = *((union epoll_ref *)&events[i].data.u64);
> diff --git a/pcap.c b/pcap.c
> index 6753cfb..156c953 100644
> --- a/pcap.c
> +++ b/pcap.c
> @@ -100,12 +100,12 @@ static void pcap_frame(const struct iovec *iov, size_t iovcnt,
>  void pcap(const char *pkt, size_t l2len)
>  {
>  	struct iovec iov = { (char *)pkt, l2len };
> -	struct timespec now;
> +	struct timespec now = { 0 };
>  
>  	if (pcap_fd == -1)
>  		return;
>  
> -	clock_gettime(CLOCK_REALTIME, &now);
> +	(void)clock_gettime(CLOCK_REALTIME, &now);

..and here..

>  	pcap_frame(&iov, 1, 0, &now);
>  }
>  
> @@ -119,13 +119,13 @@ void pcap(const char *pkt, size_t l2len)
>  void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
>  		   size_t offset)
>  {
> -	struct timespec now;
> +	struct timespec now = { 0 };
>  	unsigned int i;
>  
>  	if (pcap_fd == -1)
>  		return;
>  
> -	clock_gettime(CLOCK_REALTIME, &now);
> +	(void)clock_gettime(CLOCK_REALTIME, &now);

..and here..

>  	for (i = 0; i < n; i++)
>  		pcap_frame(iov + i * frame_parts, frame_parts, offset, &now);
> @@ -143,12 +143,12 @@ void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
>  /* cppcheck-suppress unusedFunction */
>  void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset)
>  {
> -	struct timespec now;
> +	struct timespec now = { 0 };
>  
>  	if (pcap_fd == -1)
>  		return;
>  
> -	clock_gettime(CLOCK_REALTIME, &now);
> +	(void)clock_gettime(CLOCK_REALTIME, &now);

..and here..

>  	pcap_frame(iov, iovcnt, offset, &now);
>  }
>  
> diff --git a/tcp.c b/tcp.c
> index 0d22e07..d55caee 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -548,7 +548,8 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
>  		 (unsigned long long)it.it_value.tv_sec,
>  		 (unsigned long long)it.it_value.tv_nsec / 1000 / 1000);
>  
> -	timerfd_settime(conn->timer, 0, &it, NULL);
> +	if (timerfd_settime(conn->timer, 0, &it, NULL))
> +		flow_err(conn, "failed to set timer: %s", strerror(errno));
>  }
>  
>  /**
> @@ -2240,7 +2241,9 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
>  	 * timer is currently armed, this event came from a previous setting,
>  	 * and we just set the timer to a new point in the future: discard it.
>  	 */
> -	timerfd_gettime(conn->timer, &check_armed);
> +	if (timerfd_gettime(conn->timer, &check_armed))
> +		flow_err(conn, "failed to read timer: %s", strerror(errno));
> +
>  	if (check_armed.it_value.tv_sec || check_armed.it_value.tv_nsec)
>  		return;
>  
> @@ -2278,7 +2281,10 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
>  		 * case. This avoids having to preemptively reset the timer on
>  		 * ~ACK_TO_TAP_DUE or ~ACK_FROM_TAP_DUE.
>  		 */
> -		timerfd_settime(conn->timer, 0, &new, &old);
> +		if (timerfd_settime(conn->timer, 0, &new, &old))
> +			flow_err(conn, "failed to set timer: %s",
> +				 strerror(errno));
> +
>  		if (old.it_value.tv_sec == ACT_TIMEOUT) {
>  			flow_dbg(conn, "activity timeout");
>  			tcp_rst(c, conn);

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]