Greg Banks wrote:
G'day,
I've been told I should do reviews more openly. Ok, here goes.
commit "rename: ensure user owns both source and dest for Bug #3586 workaround"
Ok, but why?
commit "nntpd: use defaultdomain in conjunction with newspostuser to create Reply-To
header addresses"
Looks good
commit "nntpd: when adding newsgroup "post" addresses to Reply-To, check for
"poster""
commit "nntpd: fix handling of Followup-To:poster when using From: header"
I find the add_header() function really confusing, and hard to work
out what it's supposed to do. Is there any chance of a comment
and/or some tests?
Most of this code was already in savemsg() and pulled out into a
separate function (with some forward ported fixes from 2.4). I can
certainly add comments.
In add_header(), the variable sep could be a const char *.
This code
+ newdest = buf_release(&buf);
will leak the string, as newdest is never free()d (and indeed in some other
branches
of the logic, cannot be). A better solution would be
const char *newdest = NULL;
...
newdest = buf_cstring(&buf);
...
buf_free(&buf);
}
I thought the same thing when I looked as what was already in master
when I began my forward port:
replyto = buf_release(&buf);
if (body) {
/* replace the existing header */
spool_replace_header(xstrdup("Reply-To"), replyto,
m->hdrcache);
} else {
/* add the new header to the cache */
spool_cache_header(xstrdup("Reply-To"), replyto,
m->hdrcache);
}
} else {
/* no newspostuser, use original replyto */
replyto = (char *) body[0];
}
Was this also incorrect?
Otherwise, looks good. I'm a little surprised we didn't explicitly handle
"Followup-To: poster", it's been around since RFC1036.
I'm not sure anyone really uses that code. I wrote it when at my old
employer. It only came up again when we did the RSS stuff at CMU.
commit "nntpd: added 'newsaddheaders' option..."
The documentation doesn't describe what happens if the incoming message
already contains the To: or Reply-To: header fields. From reading the code, I
think they're passed through untouched, perhaps it would be nice to document
the intended semantics.
Otherwise, looks good.
--
Kenneth Murchison
Principal Systems Software Engineer
Carnegie Mellon University
