From: Stuart Henderson <stu@spacehopper.org>
Subject: Re: dhcpd(8) sync verbosity
To: Michał Markowski <markowski1@gmail.com>
Cc: tech-openbsd <tech@openbsd.org>
Date: Tue, 4 Jun 2024 13:41:22 +0100

On 2024/06/04 14:20, Michał Markowski wrote:
> Diff below also rectifies some "if (sync_debug) log_info()" while at it.

I think that might have been intentional. There's no way to enable
sync_debug without recompiling and if you've done that you presumably
would like to have the information without jumping through extra hoops
to enable daemon.debug in syslog.

> diff --git a/usr.sbin/dhcpd/sync.c b/usr.sbin/dhcpd/sync.c
> index 1f701f60d46..43ec5bc1582 100644
> --- a/usr.sbin/dhcpd/sync.c
> +++ b/usr.sbin/dhcpd/sync.c
> @@ -105,7 +105,7 @@ sync_addhost(const char *name, u_short port)
>  	LIST_INSERT_HEAD(&sync_hosts, shost, h_entry);
>  
>  	if (sync_debug)
> -		log_info("added dhcp sync host %s (address %s, port %d)\n",
> +		log_debug("added dhcp sync host %s (address %s, port %d)\n",
>  		    shost->h_name, inet_ntoa(shost->sh_addr.sin_addr), port);
>  
>  	return (0);
> @@ -276,7 +276,7 @@ sync_recv(void)
>  		goto trunc;
>  
>  	if (sync_debug)
> -		log_info("%s(sync): received packet of %d bytes\n",
> +		log_debug("%s(sync): received packet of %d bytes\n",
>  		    inet_ntoa(addr.sin_addr), (int)len);
>  
>  	p = (u_int8_t *)(hdr + 1);
> @@ -307,13 +307,14 @@ sync_recv(void)
>  			    sizeof(lp->ip_addr));
>  			memcpy(&lp->hardware_addr, &lv->lv_hardware_addr,
>  			    sizeof(lp->hardware_addr));
> -			log_info("DHCP_SYNC_LEASE from %s for hw %s -> ip %s, "
> -			    "start %lld, end %lld",
> -			    inet_ntoa(addr.sin_addr),
> -			    print_hw_addr(lp->hardware_addr.htype,
> -			    lp->hardware_addr.hlen, lp->hardware_addr.haddr),
> -			    piaddr(lp->ip_addr),
> -			    (long long)lp->starts, (long long)lp->ends);
> +			if (sync_debug)
> +				log_debug("DHCP_SYNC_LEASE from %s for hw %s -> ip %s, "

This takes some lines above 80 columns (against KNF). But also means
that these log messages are unusable without recompile. When getting
things setup for sync in the first place I think they are useful so,
if changing anything, I think I'd just change this from log_info to
log_debug without the extra conditional.

> +					"start %lld, end %lld",
> +					inet_ntoa(addr.sin_addr),
> +					print_hw_addr(lp->hardware_addr.htype,
> +					lp->hardware_addr.hlen, lp->hardware_addr.haddr),
> +					piaddr(lp->ip_addr),
> +					(long long)lp->starts, (long long)lp->ends);
>  			/* now whack the lease in there */
>  			if (lease == NULL) {
>  				enter_lease(lp);
> @@ -345,7 +346,7 @@ sync_recv(void)
>  
>   trunc:
>  	if (sync_debug)
> -		log_info("%s(sync): truncated or invalid packet\n",
> +		log_debug("%s(sync): truncated or invalid packet\n",
>  		    inet_ntoa(addr.sin_addr));
>  }
>  
> @@ -365,7 +366,7 @@ sync_send(struct iovec *iov, int iovlen)
>  
>  	if (sendmcast) {
>  		if (sync_debug)
> -			log_info("sending multicast sync message\n");
> +			log_debug("sending multicast sync message\n");
>  		msg.msg_name = &sync_out;
>  		msg.msg_namelen = sizeof(sync_out);
>  		if (sendmsg(syncfd, &msg, 0) == -1)
> @@ -374,7 +375,7 @@ sync_send(struct iovec *iov, int iovlen)
>  
>  	LIST_FOREACH(shost, &sync_hosts, h_entry) {
>  		if (sync_debug)
> -			log_info("sending sync message to %s (%s)\n",
> +			log_debug("sending sync message to %s (%s)\n",
>  			    shost->h_name, inet_ntoa(shost->sh_addr.sin_addr));
>  		msg.msg_name = &shost->sh_addr;
>  		msg.msg_namelen = sizeof(shost->sh_addr);
> @@ -431,10 +432,11 @@ sync_lease(struct lease *lease)
>  	memcpy(&lv.lv_ip_addr, &lease->ip_addr, sizeof(lv.lv_ip_addr));
>  	memcpy(&lv.lv_hardware_addr, &lease->hardware_addr,
>  	    sizeof(lv.lv_hardware_addr));
> -	log_info("sending DHCP_SYNC_LEASE for hw %s -> ip %s, start %d, "
> -	    "end %d", print_hw_addr(lv.lv_hardware_addr.htype,
> -	    lv.lv_hardware_addr.hlen, lv.lv_hardware_addr.haddr),
> -	    piaddr(lease->ip_addr), ntohl(lv.lv_starts), ntohl(lv.lv_ends));
> +	if (sync_debug)
> +		log_debug("sending DHCP_SYNC_LEASE for hw %s -> ip %s, start %d, "

same as previous.

> +			"end %d", print_hw_addr(lv.lv_hardware_addr.htype,
> +			lv.lv_hardware_addr.hlen, lv.lv_hardware_addr.haddr),
> +			piaddr(lease->ip_addr), ntohl(lv.lv_starts), ntohl(lv.lv_ends));
>  	iov[i].iov_base = &lv;
>  	iov[i].iov_len = sizeof(lv);
>  	if (!HMAC_Update(ctx, iov[i].iov_base, iov[i].iov_len))
> 
> 
> 
> Best regards
> Michał Markowski
>