Hi Omar,
Thank you for the pointers - apologies for missing the style guide. I
also didn't know I could attach diffs rather than fight with GMail
about whitespace preservation - I've attached a diff that should look
a lot better.
1. I've fixed the styling issues you mentioned.
2. I implemented a time limit so that I don't attempt DNS resolution
on every UDP packet if resolution consistently fails - now it tries at
most every 10 seconds. To do this I've added f_resolvetime to f_forw -
I chose this approach rather than the TCP evtimer because I don't
fully understand the concurrency or blocking aspects of evtimer, and
because any future additions (e. g. resolving the hostname again after
a certain period of time) benefits from having an approximate time of
first resolve saved.
Regards,
R
Den ons 28 juni 2023 kl 16:16 skrev Omar Polo <[email protected]>:
>
> Hello,
>
> On 2023/06/22 13:22:56 +0000, Robert Larsson <[email protected]>
> 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
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 29 Jun 2023 13:23:02 -0000
@@ -159,6 +159,8 @@ struct filed {
struct tls *f_ctx;
char *f_host;
int f_reconnectwait;
+ char f_resolved; /* indicate if
address resolution successful */
+ long f_resolvetime; /* save time of
resolution for retry attempts */
} f_forw; /* forwarding address */
char f_fname[PATH_MAX];
struct {
@@ -1446,6 +1448,74 @@ 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. */
+ loghostcpy = strdup(f->f_un.f_forw.f_loghost+1);
+ if (loghostcpy == NULL) {
+ log_warn("strdup");
+ goto cleanup;
+ }
+
+ if (loghost_parse(loghostcpy, &proto, &host, &port) == -1) {
+ 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) {
+ ;
+ } 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;
@@ -1933,6 +2009,31 @@ fprintlog(struct filed *f, int flags, ch
case F_FORWUDP:
log_debug(" %s", f->f_un.f_forw.f_loghost);
+
+ if (!f->f_un.f_forw.f_resolved) {
+ (void)gettimeofday(&now, NULL);
+ if ((now.tv_sec - f->f_un.f_forw.f_resolvetime) > 10) {
+ /* attempt resolution only if more than 10
seconds have
+ passed since last failed attempt. */
+ f->f_un.f_forw.f_resolvetime = now.tv_sec;
+ 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;
+ }
+ } else {
+ /* the host is not resolved, but it's also too
soon for us to
+ try to resolve it again, so just abort. */
+ break;
+ }
+ }
+
l = iov[0].iov_len + iov[1].iov_len + iov[2].iov_len +
iov[3].iov_len + iov[4].iov_len + iov[5].iov_len +
iov[6].iov_len;
@@ -1943,6 +2044,7 @@ fprintlog(struct filed *f, int flags, ch
else
iov[5].iov_len = 0;
}
+
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 +2806,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\"",
