Cyrus reviews

[Greg Banks]

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?

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);
}

Otherwise, looks good.  I'm a little surprised we didn't explicitly handle
"Followup-To: poster", it's been around since RFC1036.

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.    

-- 
Greg.