Re: [PATCH v5 01/10] daemon: libify socket setup and option functions
- From
Victoria Dye <vdye@github.com>
- Date
- Jan 12, 2023, 19:35 UTC
- Message-ID
- <e1b1a53b-69d7-6fd6-513d-0f6cb3751b51@github.com>
- In-Reply-To
- <74b0de14185120c9d53d7470e59f57fa20a1927f.1673475190.git.gitgitgadget@gmail.com>
Matthew John Cheetham via GitGitGadget wrote:
Show 6 quoted lines
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Extract functions for setting up listening sockets and keep-alive options
> from `daemon.c` to new `daemon-utils.{c,h}` files. Remove direct
> dependencies on global state by inlining the behaviour at the callsites
> for all libified functions.Thanks for making this change, the reduced code duplication should make the common daemon-related code more maintainable.
For reference, I used
'git blame -s -b -C -C -C master..<this patch> -- daemon-utils.c'
to help identify which lines in 'daemon-utils.c' were changed from their original implementation in 'daemon.c'. I'll try to rearrange the diff to show those differences more directly.
The first main change I see is that 'logerror' and 'reuseaddr' are changed from global references to arguments in 'set_keep_alive()', 'setup_named_sock()' (same for 'NO_IPV6' defined and undefined), and 'socksetup()':
> -static void set_keep_alive(int sockfd) > +void set_keep_alive(int sockfd, log_fn logerror)
> -static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist) > +static int setup_named_sock(char *listen_addr, int listen_port, > + struct socketlist *socklist, int reuseaddr, > + log_fn logerror)
> -static void socksetup(struct string_list *listen_addr, int listen_port, struct socketlist *socklist) > +void socksetup(struct string_list *listen_addr, int listen_port, > + struct socketlist *socklist, int reuseaddr, > + log_fn logerror)
The external calls in 'daemon.c' to 'set_keep_alive()' and 'socksetup()' are updated to pass the 'logerror()' function and global 'reuseaddr' as arguments, so there isn't any change in behavior.
Show 19 quoted lines
> @@ -759,7 +748,7 @@ static int execute(void)
> if (addr)
> loginfo("Connection from %s:%s", addr, port);
>
> - set_keep_alive(0);
> + set_keep_alive(0, logerror);
> alarm(init_timeout ? init_timeout : timeout);
> pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
> alarm(0);
> @@ -1246,7 +1039,8 @@ static int serve(struct string_list *listen_addr, int listen_port,
> {
> struct socketlist socklist = { NULL, 0, 0 };
>
> - socksetup(listen_addr, listen_port, &socklist);
> + socksetup(listen_addr, listen_port, &socklist, reuseaddr,
> + logerror);
> if (socklist.nr == 0)
> die("unable to allocate any listen sockets on port %u",
> listen_port);The other notable change is moving the 'if (!reusaddr) return 0' block in 'set_reuse_addr()' to its callers in both 'setup_named_sock()'s:
Show 6 quoted lines
> +#ifndef NO_IPV6
> +
> +static int setup_named_sock(char *listen_addr, int listen_port,
> + struct socketlist *socklist, int reuseaddr,
> + log_fn logerror)
> +{...
Show 5 quoted lines
> + if (reuseaddr && set_reuse_addr(sockfd)) {
> + logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
> + close(sockfd);
> + continue;
> + }...
Show 8 quoted lines
> +}
> +
> +#else /* NO_IPV6 */
> +
> +static int setup_named_sock(char *listen_addr, int listen_port,
> + struct socketlist *socklist, int reuseaddr,
> + log_fn logerror)
> +{...
Show 5 quoted lines
> + if (reuseaddr && set_reuse_addr(sockfd)) {
> + logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
> + close(sockfd);
> + return 0;
> + }...
> +} > + > +#endif
Where, previously, that region looked like:
> -#ifndef NO_IPV6
> -
> -static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
> -{...
Show 5 quoted lines
> - if (set_reuse_addr(sockfd)) {
> - logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
> - close(sockfd);
> - continue;
> - }...
Show 6 quoted lines
> -}
> -
> -#else /* NO_IPV6 */
> -
> -static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
> -{...
Show 5 quoted lines
> - if (set_reuse_addr(sockfd)) {
> - logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
> - close(sockfd);
> - return 0;
> - }...
> -} > - > -#endif
'reuseaddr' is passed into 'setup_named_sock()' from 'socksetup()' calls in 'daemon.c', so this also won't result in changed behavior.
Otherwise, you only expose functions & types that are called in 'daemon.c' (the rest are still static), and everything else is a verbatim copy. Looks good to me!