You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by "Noel J. Bergman" <no...@devtech.com> on 2003/01/10 00:37:16 UTC
[PATCH] NNTP patches
These patches are hopefully non-controversial ones. I have hand-checked
each patch against the RFC 977 and RFC 2980, and they appear to be correct.
By doing this I checked each one, rather than apply what I was sent. In one
case, NEWNEWS, I found a missing inner loop.
There may certainly be MORE things to fix, but I do not see anything here
that should get in the way of other fixes.
I am building with these changes now, and will be testing with Outlook
Express. If people would PLEASE review these and comment, it would be
appreciated.
--- Noel
Specific changes fixed:
NNTPHandler.java:
1. Corrected message from Remote Manager to NNTP.
2. Corrected parsing of NEWNEWS to skip the
newsgroups (wildmat) before getting date.
3. Corrected NEWNEWS to iterate through the
matching newsgroups AND the new messages
within each.
NNTPArticleImpl.java:
1. Added missing ArticleNumber to writeOverview protocol response.
This is per RFC 2980, section 2.8 (emphasis mine):
"Each line of output will be formatted with the ARTICLE NUMBER,
FOLLOWED BY each of the headers in the overview database or the
article itself (when the data is not available in the overview
database) for that article separated by a tab character. The
sequence of fields must be in this order: SUBJECT, AUTHOR, DATE,
MESSAGE-ID, REFERENCES, BYTE COUNT, and LINE COUNT."
2. RFC 2980 states "Note that any tab and end-of-line characters
in any header data that is returned will be converted to a
space character." Based upon RFC 977 #2.3, "Each command line
must be terminated by a CR-LF (Carriage Return - Line Feed)
pair.", I added \r to cleanHeader() on the grounds that it is
safer to be conservative.
NNTPGroupImpl.java:
This code is a mess! I would have no concerns about seeing the entire thing
rewritten. In the meantime, all I did was address boundary conditions.
1. Boundary condition to handle negative numbers.
2. Boundary condition to handle initial posting.
This code is still broken because the last article number is not persistent,
and is based upon the highest article currently present in the current
group, regardless of what might have been present and deleted.
NNTPSpooler.java:
1. Made the debug message clearer.
2. Nulled out the list array, as agreed months ago during code freeze.
3. Make sure that input/ouput streams are closed.
4. Log error if we can't delete spoolFile.
That's it. Nothing controversial as far as I can see, nor anything that
should get in the way of further cleanup, which this code desperately needs.
--- Noel
RE: [PATCH] NNTP patches
Posted by "Noel J. Bergman" <no...@devtech.com>.
> Noel. Looks good.
> I would recommend only bug fixes in 2.1 branch and fixes like logging
> comment changes etc. in 3.0 branch.
IMO, James v3 is at least 6 months off from being stable, and probably close
to a year to a Release Build unless we get some additional developer
resources. My expectation is that entire repository code will be replaced,
amongst other things.
James v2 is stable. Features and fixes that can be made within the stable
code base without negatively impacting it ought to be made, especially if
they are forward compatible with v3.
I don't care if we call it 2.1.x, 2.2, or 2.foo, but I don't think that we
should stop work on James v2, especially if it is forward portable. On the
other hand, I think that other things certainly should be defered to James
v3 in order to take advantage of its new features.
> NNTPGroupImpl.java:
> This seems bad and buggy.
> For reasons unknown to me and comments don't give a clue
> - This has evolved from a read only to read write structure
> - Response format has been added. Orig. intent was to separate the
response
> format from datastructures sort of like Model/View.
All I did was fix the initial boundary conditions for article numbers.
There is a lot of work to be done fixing this code.
> May I say, you should have taken this from me when I committed myself or
> waited for me to finish it
> As I said earlier I have too much on my hands atm and it is relief
> to me to have nntp working well.
Well, I guess the latter is a good reason for my not doing the former. You
don't need the stress, and these patches really did need to be made.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] NNTP patches
Posted by Harmeet Bedi <ha...@kodemuse.com>.
Noel. Looks good.
> NNTPHandler.java:
I would recommend only bug fixes in 2.1 branch and fixes like logging
comment changes etc. in 3.0 branch.
NEWNEWS looks good. I don't remember which client sends or does not send
newnews but would be good to validate it if possible with a client.
> NNTPArticleImpl.java:
Overview change addresses regression that rendered OE useless.
Changes look good. '\r' is the correct thing todo.
> NNTPGroupImpl.java:
This seems bad and buggy.
For reasons unknown to me and comments don't give a clue
- This has evolved from a read only to read write structure
- Response format has been added. Orig. intent was to separate the response
format from datastructures sort of like Model/View.
> NNTPSpooler.java:
These are minor changes, don't really address bugs. Would be good to only do
bug fixes in 2.1 branch.
May I say, you should have taken this from me when I committed myself or
waited for me to finish it at least for the the time frame I had committed
to(3 days away). But either way, good improvements and thanks for taking
this up. As I said earlier I have too much on my hands atm and it is relief
to me to have nntp working well.
Harmeet
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> These patches are hopefully non-controversial ones. I have hand-checked
> each patch against the RFC 977 and RFC 2980, and they appear to be
correct.
> By doing this I checked each one, rather than apply what I was sent. In
one
> case, NEWNEWS, I found a missing inner loop.
>
> There may certainly be MORE things to fix, but I do not see anything here
> that should get in the way of other fixes.
>
> I am building with these changes now, and will be testing with Outlook
> Express. If people would PLEASE review these and comment, it would be
> appreciated.
>
> --- Noel
>
> Specific changes fixed:
>
> NNTPHandler.java:
>
> 1. Corrected message from Remote Manager to NNTP.
> 2. Corrected parsing of NEWNEWS to skip the
> newsgroups (wildmat) before getting date.
> 3. Corrected NEWNEWS to iterate through the
> matching newsgroups AND the new messages
> within each.
>
> NNTPArticleImpl.java:
>
> 1. Added missing ArticleNumber to writeOverview protocol response.
> This is per RFC 2980, section 2.8 (emphasis mine):
>
> "Each line of output will be formatted with the ARTICLE NUMBER,
> FOLLOWED BY each of the headers in the overview database or the
> article itself (when the data is not available in the overview
> database) for that article separated by a tab character. The
> sequence of fields must be in this order: SUBJECT, AUTHOR, DATE,
> MESSAGE-ID, REFERENCES, BYTE COUNT, and LINE COUNT."
>
> 2. RFC 2980 states "Note that any tab and end-of-line characters
> in any header data that is returned will be converted to a
> space character." Based upon RFC 977 #2.3, "Each command line
> must be terminated by a CR-LF (Carriage Return - Line Feed)
> pair.", I added \r to cleanHeader() on the grounds that it is
> safer to be conservative.
>
> NNTPGroupImpl.java:
>
> This code is a mess! I would have no concerns about seeing the entire
thing
> rewritten. In the meantime, all I did was address boundary conditions.
>
> 1. Boundary condition to handle negative numbers.
> 2. Boundary condition to handle initial posting.
>
> This code is still broken because the last article number is not
persistent,
> and is based upon the highest article currently present in the current
> group, regardless of what might have been present and deleted.
>
> NNTPSpooler.java:
>
> 1. Made the debug message clearer.
> 2. Nulled out the list array, as agreed months ago during code freeze.
> 3. Make sure that input/ouput streams are closed.
> 4. Log error if we can't delete spoolFile.
>
> That's it. Nothing controversial as far as I can see, nor anything that
> should get in the way of further cleanup, which this code desperately
needs.
>
> --- Noel
>
----------------------------------------------------------------------------
----
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP patches
Posted by "Noel J. Bergman" <no...@devtech.com>.
These are working fine so far for me. More bugs to fix, I am sure, but
these fixes should be OK for what they do, and shouldn't block any other
changes.
New bugs:
1. linecount is always -1 for XOVER
2. If you delete newsgroups from config.xml, they are still
present unless you delete the repositories as well.
NNTPRepositoryImpl.getGroups() doesn't know to check the configuration for
valid groups. We have the information to check against, but looking at the
code for configure(), the variable known as "addGroups" ... someone *knew*
and didn't document this "feature." Is this behavior intentional, or should
I fix it by filtering out newsgroups that are not listed in config.xml?
If this behavior is intended, we must document it.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>