On Sat, 2021-09-11 at 04:05 +0300, Vitaliy Makkoveev wrote:
> On Fri, Sep 10, 2021 at 07:40:58PM +0200, Alexander Bluhm wrote:
> > Hi,
> >
> > After converting syslogd messages to iov, UDP can use sendmsg(2)
> > instead of snprintf().
> >
> > ok?
> >
>
> I have one question below, but the diff is ok by mvs@ if it is not
> significant.
>
> It is expected we could drop iov[6].iov_len bytes more from `msg' but
> keep newline stuff when message size exceeds MAX_UDPMSG? The original
> code has the different behaviour.
Maybe I'm misreading, but the iov[6] is set in the final switch
statement, which lacks the F_FORWUDP case, so it drops to default,
which has an empty string, not a newline.
As I already mailed bluhm@ in private; I'm more curious about the
else case for if (iov[5].iov_len > l). In this case iov[0..4]
still overflow MAX_UDPMSG and we also drop the entire "msg"
itself, which leaves basically no useful information.
Since we only prepend pribuf (13 bytes), lasttime (33 bytes + 1 space),
and prevhost/localhostname (HOST_NAME_MAX=255 + 1 bytes + 1 space) we
can at most reach 304 bytes, which is a lot shorter than the current
MAX_UDPMSG of 1180 bytes.
If we trust that noone will be stupid enough to blindly change
MAX_UDPMSG to <304 bytes, or start bloating the preemble >876 bytes I
don't see a reason to add this case.
If it's a genuine concern then I think it would make more sense
to just drop the message altogether, since everything that could be
useful is already lost.
>
> > bluhm
> >
> > Index: usr.sbin/syslogd/syslogd.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
> > retrieving revision 1.269
> > diff -u -p -r1.269 syslogd.c
> > --- usr.sbin/syslogd/syslogd.c 10 Sep 2021 15:18:36 -0000 1.269
> > +++ usr.sbin/syslogd/syslogd.c 10 Sep 2021 17:36:45 -0000
> > @@ -1897,6 +1897,7 @@ void
> > fprintlog(struct filed *f, int flags, char *msg)
> > {
> > struct iovec iov[IOVCNT], *v;
> > + struct msghdr msghdr;
> > int l, retryonce;
> > char line[LOG_MAXLINE + 1], pribuf[13], greetings[500], repbuf[80];
> > char ebuf[ERRBUFSIZE];
> > @@ -2037,20 +2038,22 @@ fprintlog(struct filed *f, int flags, ch
> >
> > case F_FORWUDP:
> > log_debug(" %s", f->f_un.f_forw.f_loghost);
> > - l = snprintf(line, MINIMUM(MAX_UDPMSG + 1, sizeof(line)),
> > - "%s%s%s%s%s%s%s", (char *)iov[0].iov_base,
> > - (char *)iov[1].iov_base, (char *)iov[2].iov_base,
> > - (char *)iov[3].iov_base, (char *)iov[4].iov_base,
> > - (char *)iov[5].iov_base, (char *)iov[6].iov_base);
> > - if (l < 0)
> > - l = strlcpy(line, iov[5].iov_base, sizeof(line));
> > - if (l >= sizeof(line))
> > - l = sizeof(line) - 1;
> > - if (l >= MAX_UDPMSG + 1)
> > - l = MAX_UDPMSG;
> > - if (sendto(f->f_file, line, l, 0,
> > - (struct sockaddr *)&f->f_un.f_forw.f_addr,
> > - f->f_un.f_forw.f_addr.ss_len) != l) {
> > + 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;
> > + if (l > MAX_UDPMSG) {
> > + l -= MAX_UDPMSG;
> > + if (iov[5].iov_len > l)
> > + iov[5].iov_len -= l;
> > + 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;
> > + msghdr.msg_iov = iov;
> > + msghdr.msg_iovlen = IOVCNT;
> > + if (sendmsg(f->f_file, &msghdr, 0) == -1) {
> > switch (errno) {
> > case EADDRNOTAVAIL:
> > case EHOSTDOWN:
> > @@ -2063,7 +2066,7 @@ fprintlog(struct filed *f, int flags, ch
> > break;
> > default:
> > f->f_type = F_UNUSED;
> > - log_warn("sendto \"%s\"",
> > + log_warn("sendmsg to \"%s\"",
> > f->f_un.f_forw.f_loghost);
> > break;
> > }
> >
>
