Cyrus reviews

[Ken Murchison]

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