400331 - Clean up (and document) nsNNTPProtocol

Status: RESOLVED WONTFIX

(MailNews Core :: Networking: NNTP, defect)

Description

[Joshua Cranmer [:jcranmer]]

This bug is a holding bug for effort to clean up the NNTP protocol state machine, as well as document it. 

The URL references my current stub for documentation, including most notably a mostly-machine generated visual graph of the state machine.

My current looking indicates that these states have no cases in the large switch statement, and a few are not externally referenced:
NNTP_BLOCK_UNTIL_CONNECTIONS_ARE_AVAILABLE
NNTP_CONNECTIONS_ARE_AVAILABLE
NNTP_CONNECT
NNTP_CONNECT_WAIT
NNTP_PROFILE_ADD_RESPONSE
NNTP_PROFILE_ADD
NNTP_PROFILE_DELETE
NNTP_PROFILE_DELETE_RESPONSE
NNTP_PRINT_ARTICLE_HEADERS

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: A python script that can generate a state diagram

This is the python script I am currently using to generate the state diagrams. Be forewarned: they get large quickly.

This script is capable of deducing for *every* function the settings of the member variables, with the caveat that member variables must be prefixed with a `m_' and they must be assigned in the form `<variable> = <value>;' This appears to not hold true for very few variables, so it's not high on my list to fix this.

It also has special scraping capabilities for the stateLabels[] and switch(m_nextState) loop; modifying this to work with other state machines in the mozilla database should hopefully be obvious enough.

Feel free to modify to your heart's content.

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Unsanitized preliminary patch

Raw, unsanitized hg mq patch. I am nowhere near done with cleaning up, but I felt that I might put up what I have right now. I've gone through the URI parsing and fixed up the list of accepted URIs, and moved that list to where the URIs are actually parsed (LoadURI).

I've also done some heavy cleanup, enforcing in most places a strict 80-column rule and changing all

if (...) {

to

if (...)
{

as the latter seems to have predominating form. In many cases, where the if statement consists of only one line, I have removed the braces altogether.

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: Updated version of cleanup

An updated version of my preliminary patch, also unsanitized.

As part of my discoveries when cleaning up code:
1. Most of the #ifdef UNREADY_CODE hasn't been touched since 1999.
2. diff REALLY sucks at handling whitespace changes en masse. I might just end up doing a manual diff with a -w just to kill its annoyances...
3. The code with the extensions was based off of a draft RFC written in 1997. The actual RFC 3977 is, in many places, far away from the aforementioned draft:

  RFC 3977          draft RFC
CAPABILITIES     LIST EXTENSIONS
LIST [ACTIVE]    XACTIVE
( none )         LIST SEARCHES
LIST HEADERS     LIST SRCHFIELDS
( none ?)        GET
LIST NEWSGROUPS  LIST PRETTYNAMES

Capabilities:
( none )         SETGET
NEWNEWS          ?
HDR              ?
LIST [*]         LISTPNAMES, XACTIVE, ?

Note that the number of servers that support the CAPABILITIES command is probably very small at this time: RFC 3977 is barely a year old, and INN, which is AFAICT the most prevalent news server implementation, doesn't support it yet, although several other portions appear to be supported and it is aiming to get it supported as soon as possible.

(Just discovered): Maybe a link to http://www.eyrie.org/~eagle/nntp/drafts/ is helpful here: it is the listing of the earlier drafts before RFC 3977. Time to go hunting down differences. <sarcasm>Yay!</sarcasm>

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Larger update (-w, for your sanity)

This is a -w version of my latest diffs, but it's still pretty big (Never mind the fact that a fair amount of the purpose of this bug involves -w).

Some notable features in this latest round is the start of PRInt32->nsresult conversion, removal of half the warnings, the start of dismantling SendFirstNNTPCommand, trackers for potential large blocks of code removal, and some places where function blocks are shifted around for logical coherency.

You can easily see some of the places where I'm noting my WIPs...

Comment 5

[Joshua Cranmer [:jcranmer]]

Attachment: Current WIP, not -w

As much as a -w would be helpful, that probably changes no more than 100 lines due to the wide range of lines affected.

I'm continuing on SendFirstNNTPCommand dismantling and have put more effort into reducing the size of the object itself. (Distraction on IRC at this time...)

Anyways, the code is becoming easier to understand, I hope.

Comment 7

[Wayne Mery (:wsmwk)]

Obsolete?