On 05/12/24 19:21, Stefan Sperling wrote:
> Tweak gotwebd log levels of debugging and error messages to make
> logging at -v more useful.
>
> ok?

for the most part i wholeheartedly agree, just a few comments inline for things I'm not so sure warrants the visibility bump.

> M  gotwebd/config.c   |   2+   2-
> M  gotwebd/fcgi.c     |   3+   4-
> M  gotwebd/sockets.c  |  10+  14-
>
> 3 files changed, 15 insertions(+), 20 deletions(-)
>
> commit - 8c5906074ad5756611aa1977542e314d8e3662bb
> commit + cc0513813a42f2bc2a34e98df48153ef4bba95ba
> blob - df0bdb7345fb1eed068c93d28ff3765b72dda2c7
> blob + c5992e15779413a10bed882be8e302a360de28e6
> --- gotwebd/config.c
> +++ gotwebd/config.c
> @@ -133,7 +133,7 @@ config_getsock(struct gotwebd *env, struct imsg *imsg)
>  	memcpy(&sock_conf, p, sizeof(sock_conf));
>  
>  	if (IMSG_DATA_SIZE(imsg) != sizeof(sock_conf)) {
> -		log_debug("%s: imsg size error", __func__);
> +		log_warnx("%s: imsg size error", __func__);
>  		return 1;
>  	}
>  
> @@ -164,7 +164,7 @@ config_setfd(struct gotwebd *env)
>  {
>  	int i, j, fd;
>  
> -	log_debug("%s: Allocating %d file descriptors",
> +	log_info("%s: Allocating %d file descriptors",
>  	    __func__, PRIV_FDS__MAX + GOTWEB_PACK_NUM_TEMPFILES);


not so sure about this


>  	for (i = 0; i < PRIV_FDS__MAX + GOTWEB_PACK_NUM_TEMPFILES; i++) {
> blob - ecad0c59902279155f09c3e1229c8ba25759a7d5
> blob + 4146e12a72f68e0d527dc89a9c4df46fd602ee21
> --- gotwebd/fcgi.c
> +++ gotwebd/fcgi.c
> @@ -76,7 +76,7 @@ fcgi_request(int fd, short events, void *arg)
>  		break;
>  
>  	case 0:
> -		log_debug("closed connection");
> +		log_info("closed connection");
>  		goto fail;
>  	default:
>  		break;
> @@ -326,14 +326,13 @@ send_response(struct request *c, int type, const uint8
>  				nanosleep(&ts, NULL);
>  				continue;
>  			}
> -			log_debug("%s: write failure: %s", __func__,
> -			    strerror(errno));
> +			log_warn("%s: write failure", __func__);
>  			c->sock->client_status = CLIENT_DISCONNECT;
>  			return -1;
>  		}
>  
>  		if (nw != tot)
> -			log_debug("%s: partial write: %zu vs %zu", __func__,
> +			log_warnx("%s: partial write: %zu vs %zu", __func__,
>  			    nw, tot);
>  
>  		tot -= nw;

> blob - 7663af4588aefb8de1d6370b1dbb0d302d3e5f76
> blob + bf08e907f6aa6059cac714973e9dd139009154dc
> --- gotwebd/sockets.c
> +++ gotwebd/sockets.c
> @@ -199,7 +199,7 @@ sockets_launch(void)
>  	const struct got_error *error;
>  
>  	TAILQ_FOREACH(sock, &gotwebd_env->sockets, entry) {
> -		log_debug("%s: configuring socket %d (%d)", __func__,
> +		log_info("%s: configuring socket %d (%d)", __func__,
>  		    sock->conf.id, sock->fd);

why it's useful to show this at info level and not debug?

>  
>  		event_set(&sock->ev, sock->fd, EV_READ | EV_PERSIST,
> @@ -210,7 +210,7 @@ sockets_launch(void)
>  
>  		evtimer_set(&sock->pause, sockets_accept_paused, sock);
>  
> -		log_debug("%s: running socket listener %d", __func__,
> +		log_info("%s: running socket listener %d", __func__,
>  		    sock->conf.id);
>  	}
>  
> @@ -361,24 +361,20 @@ int
>  sockets_privinit(struct gotwebd *env, struct socket *sock)
>  {
>  	if (sock->conf.af_type == AF_UNIX) {
> -		log_debug("%s: initializing unix socket %s", __func__,
> +		log_info("%s: initializing unix socket %s", __func__,
>  		    sock->conf.unix_socket_name);
>  		sock->fd = sockets_unix_socket_listen(env, sock);
> -		if (sock->fd == -1) {
> -			log_warnx("%s: create unix socket failed", __func__);
> +		if (sock->fd == -1)
>  			return -1;
> -		}
>  	}
>  
>  	if (sock->conf.af_type == AF_INET || sock->conf.af_type == AF_INET6) {
> -		log_debug("%s: initializing %s FCGI socket on port %d",
> +		log_info("%s: initializing %s FCGI socket on port %d",
>  		    __func__, sock->conf.af_type == AF_INET ? "inet" : "inet6",
>  		    sock->conf.fcgi_socket_port);
>  		sock->fd = sockets_create_socket(&sock->conf.addr);
> -		if (sock->fd == -1) {
> -			log_warnx("%s: create FCGI socket failed", __func__);
> +		if (sock->fd == -1)
>  			return -1;
> -		}
>  	}
>  
>  	return 0;
> @@ -464,14 +460,14 @@ sockets_create_socket(struct address *a)
>  	flags = fcntl(fd, F_GETFL);
>  	flags |= O_NONBLOCK;
>  	if (fcntl(fd, F_SETFL, flags) == -1) {
> -		log_info("%s: could not enable non-blocking I/O", __func__);
> +		log_warn("%s: could not enable non-blocking I/O", __func__);
>  		close(fd);
>  		return -1;
>  	}
>  
>  	if (bind(fd, (struct sockaddr *)&a->ss, a->slen) == -1) {
>  		close(fd);
> -		log_info("%s: can't bind to port %d", __func__, a->port);
> +		log_warn("%s: can't bind to port %d", __func__, a->port);
>  		return -1;
>  	}
>  
> @@ -492,7 +488,7 @@ sockets_accept_reserve(int sockfd, struct sockaddr *ad
>  
>  	if (getdtablecount() + reserve +
>  	    ((*counter + 1) * FD_NEEDED) >= getdtablesize()) {
> -		log_debug("inflight fds exceeded");
> +		log_warnx("inflight fds exceeded");
>  		errno = EMFILE;
>  		return -1;
>  	}
> @@ -603,7 +599,7 @@ sockets_rlimit(int maxfd)
>  
>  	if (getrlimit(RLIMIT_NOFILE, &rl) == -1)
>  		fatal("%s: failed to get resource limit", __func__);
> -	log_debug("%s: max open files %llu", __func__,
> +	log_info("%s: max open files %llu", __func__,
>  	    (unsigned long long)rl.rlim_max);

same here