master process handling patch

[Henrique de Moraes Holschuh]

On Tue, 31 Aug 2010, Patrick Goetz wrote:
> On 07/21/2010 05:58 PM, Henrique de Moraes Holschuh wrote:
> 1. Doesn't handle the case of hdr = NULL very well, since \r\n are
> always appended to the end of ibuf even when hdr is NULL.  Of course
> maybe this is what the code writer intended; there's no way of
> knowing because...

You need to track down all callers and check them.

> 2. No comments explaining how the function is supposed to be used.
> 3. K&R-style function parameters (Seriously?  How long has ANSI C
> been available??)

The cyrus code is full of that kind of crap :p

> 4. Looking at the code, what would this indentation style be called?

Whatever mess someone wrote it in.  Again, it is useless to bother, the
cyrus code is a mess of styles.

> 5. (minor) Misuse of the term header -- the function should probably
> be called message_parse_field, since it's only parsing one field in
> the header and not the whole header.  Seriously , the function name
> alone confused me for a while until I spent the time to look up RFC
> 822/bis for header specifications.  By RFC 822, each line must be
> terminated by \r\n.  The code "fixes" the line endings at the end of
> the field, but ignores any possible bad line endings in the middle.

Heh.  Although ignoring bad line endings in the middle might be either a bug
or by design.

> >Patches 12 and 13 have been cleaned up (13 was broken up into separate
> >patches as well), and documented.
> 
> Great!  Are you making these changes in your git repository or in
> Sven's SVN?  I've been going through the patches making notes on

I send it to SVN when it's good.  Most of it is there already.

> > Now I am going to try fixing the !@#$
> >master state machine for good, it is subtly broken, and it has been subtly
> >broken for a *LONG* time.  Needs a rather pathological load to cause any
> >issues, though.
> 
> I assume you're working on this in conjunction with upstream and not
> just as another Debian patch?  It's my strong opinion that it is not

I am the one who did the design of the Cyrus master state machine 10 years
ago, and also wrote the first implementation (which runs on Debian Cyrus
2.1), which upstream forward-ported to what became Cyrus 2.2.

Looking at the code 10 years later, I found one design bug, and one nasty
documentation bug that helped hide a few implementation bugs.  I squashed
them in SVN, but this is the sort of stuff that needs to go to bugzilla and
get some peer review.  I will forward them to bugzilla next time I have a
Cyrus hacking window with an internet connection close by.

Do look at the stuff currently in SVN, you will find it is quite different
from what was there before.

It is *still* not perfect.  Even with the fixes, Cyrus still cannot handle
duplicated PIDs showing up because of PID warp-around.  Fixing that is not
trivial, but I have something half-done already in my private git tree.

What is *really* annoying is that we were NOT disciplined in our use of the
branches/ namespace, so I can't just use git-svn.  Damn.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh