Hello,
On 2023/06/22 13:22:56 +0000, Robert Larsson <larssonrobert...@gmail.com> wrote:
> Hello,
>
> Syslog can be configured to send log to remote hosts using both
> IP-addresses and resolvable hostnames. DNS resolution of these hostnames
> is done in the "cfline()" function, which is invoked only during parsing
> of the configuration file.
>
> If DNS is not available when syslog starts, no attempts are made at a
> later stage to perform this resolution, so a temporary outage during
> startup causes the log stream to be permanently disabled.
>
> I took a stab at implementing a fix to this, but would very much
> appreciate if someone experienced could review it as I am not at all
> confident in my C coding abilities.
>
> The patch adds a new variable - "f_resolved" - which is set to zero on
> startup. If syslog is about to attempt a TCP connection and "f_resolved"
> is zero, a new function "resolve_host" is called to try a DNS
> resolution. With UDP, this resolution is attempted before every sendmsg
> if address resolution failed earlier - I'm worried this might cause
> overhead, but I also didn't want to overcomplicate things.
I don't have any opinion on this, I just stumbled on the thread and
noticed that the diff was mangled due to whitespaces. Please consider
fixing your MUA or attaching a diff. To test, you could send the diff
to yourself and see if it applies.
While here however, point out some obvious style(9) violation
> Index: syslogd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.277
> diff -u -p -u -r1.277 syslogd.c
> --- syslogd.c 16 Mar 2023 18:22:08 -0000 1.277
> +++ syslogd.c 22 Jun 2023 12:41:09 -0000
> @@ -159,6 +159,7 @@ struct filed {
> struct tls *f_ctx;
> char *f_host;
> int f_reconnectwait;
> + char f_resolved;
> } f_forw; /* forwarding address */
> char f_fname[PATH_MAX];
> struct {
> @@ -1446,6 +1447,75 @@ tcp_errorcb(struct bufferevent *bufev, s
> log_info(LOG_WARNING, "%s", ebuf);
> }
>
> +int
> +resolve_host(struct filed *f)
> +{
> + int retval = -1;
> + char *loghostcpy = NULL, *ipproto, *proto, *host, *port;
> +
> + if (f->f_un.f_forw.f_loghost[0] != '@') {
> + log_warnx("invalid loghost \"%s\"",
> + f->f_un.f_forw.f_loghost);
> + goto cleanup;
> + }
> +
> + // loghost_parse breaks the loghost line in a strtok:y way,
> + // so we work on a copy. Note that loghost_parse uses this
> + // buffer to store proto, host and port, so don't free this
> + // until we are completely done.
please don't use C++ style comments, but /* C style comments */
> + loghostcpy = strdup(f->f_un.f_forw.f_loghost+1);
> + if(loghostcpy == NULL) {
use spaces between if and the condition
+ if (loghostcpy == NULL) {
> + log_warnx("strdup error: %d", errno);
I fear the raw `errno' value is useless. consider using
log_warn("strdup");
instead, which appends the error description to the message. (note
the difference, it's log_warn not log_warnx.)
> + goto cleanup;
> + }
> +
> + if(loghost_parse(loghostcpy, &proto, &host, &port) == -1) {
same spacing as noted before.
> + log_warnx("bad loghost \"%s\"",
> + f->f_un.f_forw.f_loghost);
> + goto cleanup;
> + }
> +
> + // This code somewhat repeats tests in cfline(), but maybe
> + // not enought to warrant breaking it out into a separate
> + // function?
> + ipproto = proto;
> + if (strcmp(proto, "tls") == 0) {
> + ipproto = "tcp";
> + } else if (strcmp(proto, "tls4") == 0) {
> + ipproto = "tcp4";
> + } else if (strcmp(proto, "tls6") == 0) {
> + ipproto = "tcp6";
> + } else if (
> + strcmp(proto, "tcp") == 0 || \
> + strcmp(proto, "tcp4") == 0 || \
> + strcmp(proto, "tcp6") == 0 || \
> + strcmp(proto, "udp") == 0 || \
> + strcmp(proto, "udp4") == 0 || \
> + strcmp(proto, "udp6") == 0) {
this is not shell scripting, no need for \ to break lines.
also, similar constructs are often spelled as
else if (strcmp(...) == 0 ||
strcmp(...) == 0 ||
strcmp(...) == 0 ||
...) {
/* ... */
}
> + ;
instead of an empty body I'd reverse the previous logic and error
there.
> + } else {
> + log_warnx("bad protocol \"%s\"",
> + f->f_un.f_forw.f_loghost);
> + goto cleanup;
> + }
> +
> + if (priv_getaddrinfo(ipproto, host, port,
> + (struct sockaddr*)&f->f_un.f_forw.f_addr,
> + sizeof(f->f_un.f_forw.f_addr)) != 0) {
> + log_warnx("could not resolve \"%s\"",
> + host);
> + goto cleanup;
> + }
> +
> + f->f_un.f_forw.f_resolved = 1;
> + retval = 0;
> +cleanup:
> + if(loghostcpy != NULL)
> + free(loghostcpy);
> + log_debug("resolve host: %s for \"%s\"", retval ? "failed" :
> "successful", f->f_un.f_forw.f_loghost);
> + return retval;
> +}
> +
> void
> tcp_connectcb(int fd, short event, void *arg)
> {
> @@ -1453,6 +1523,12 @@ tcp_connectcb(int fd, short event, void
> struct bufferevent *bufev = f->f_un.f_forw.f_bufev;
> int s;
>
> + if(!f->f_un.f_forw.f_resolved) {
> + if(resolve_host(f) == -1) {
> + goto error;
> + }
> + }
> +
> if ((s = tcp_socket(f)) == -1) {
> tcp_connect_retry(bufev, f);
> return;
> @@ -1943,6 +2019,22 @@ fprintlog(struct filed *f, int flags, ch
> else
> iov[5].iov_len = 0;
> }
> +
> + if(!f->f_un.f_forw.f_resolved) {
> + if(resolve_host(f) == -1) {
> + break;
> + }
> + switch (f->f_un.f_forw.f_addr.ss_family) {
> + case AF_INET:
> + f->f_file = fd_udp;
> + break;
> + case AF_INET6:
> + f->f_file = fd_udp6;
> + break;
> + }
> + }
> +
> +
> memset(&msghdr, 0, sizeof(msghdr));
> msghdr.msg_name = &f->f_un.f_forw.f_addr;
> msghdr.msg_namelen = f->f_un.f_forw.f_addr.ss_len;
> @@ -2704,25 +2796,12 @@ cfline(char *line, char *progblock, char
> f->f_un.f_forw.f_loghost);
> break;
> }
> - if (priv_getaddrinfo(ipproto, host, port,
> - (struct sockaddr*)&f->f_un.f_forw.f_addr,
> - sizeof(f->f_un.f_forw.f_addr)) != 0) {
> - log_warnx("bad hostname \"%s\"",
> - f->f_un.f_forw.f_loghost);
> - break;
> - }
> f->f_file = -1;
> if (strncmp(proto, "udp", 3) == 0) {
> - switch (f->f_un.f_forw.f_addr.ss_family) {
> - case AF_INET:
> - f->f_file = fd_udp;
> - break;
> - case AF_INET6:
> - f->f_file = fd_udp6;
> - break;
> - }
> + f->f_un.f_forw.f_resolved = 0;
> f->f_type = F_FORWUDP;
> } else if (strncmp(ipproto, "tcp", 3) == 0) {
> + f->f_un.f_forw.f_resolved = 0;
> if ((f->f_un.f_forw.f_bufev = bufferevent_new(-1,
> tcp_dropcb, tcp_writecb, tcp_errorcb, f)) == NULL) {
> log_warn("bufferevent \"%s\"",
Thanks,
Omar Polo
