280537 - Enhancement/Cleanup for nsNNTPProtocol class

Status: RESOLVED DUPLICATE of bug 400331

(SeaMonkey :: MailNews: Backend, enhancement)

Description

[Tim Ruehsen]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.3; Linux) KHTML/3.3.2 (like Gecko)
Build Identifier: 

Mainly, I replaced NS_MsgSACopy/NS_MsgSACat/SendData combinations with a new 
helper function called PrintfToStream. 
 
This new function also removes the limits on some parts of the code when using 
a static buffer. e.g.: 
  char buf[OUTPUT_BUFFER_SIZE]; 
  PR_snprintf(buf,sizeof(buf),"GROUP %.512s"CRLF, groupname); 
  status = SendData(url, buf); 
now looks like 
  status = PrintfToStream(url, "GROUP %s", groupname); 
 
PrintfToStream() uses an 512 bytes buffer on the stack, and falls back to 
memory allocation if needed. It also calls SendData() without recalculating the 
data size via PL_strlen(). 
 
After all, this makes the code cleaner and reduces memory allocations. 
Comments are welcome. 
 

Reproducible: Always

Comment 1

[Tim Ruehsen]

Attachment: cvs diff

Comment 2

[David :Bienvenu]

Comment on attachment 172958 [details] [diff] [review]
cvs diff

thx for looking at this code. a -uw diff would be helpful, so I don't have to
look at all the whitespace changes...

Comment 3

[Tim Ruehsen]

Sorry about that. 
But i am not able to reconstruct my changes with 'patch' using the diff file 
(says, it can't locate mailnews/news/src/nsNNTPProtocol.cpp, which is there). 
My changed files where overwritten, so using patch would be my only possibility 
to reconstruct them. So I can't provide you with a patch without whitespace 
changes. 
Please try to manage the whitespace changes. For the future I will always use 
-w. 
 

Comment 4

[Frank Wein [:mcsmurf]]

Attachment: Patch for trunk, -uw

This is a new version which will work on trunk (i used the old one and applied most of the changes manually) and also fixed a little bug in it. So far it seems to work fine (post, read, cancel postings,etc.).

Comment 5

[Frank Wein [:mcsmurf]]

Comment on attachment 201422 [details] [diff] [review]
Patch for trunk, -uw


This is a new version which will work on trunk (i used the old one and applied
most of the changes manually) and also fixed a little bug in it. So far it
seems to work fine (post, read, cancel postings,etc.).

Comment 6

[Frank Wein [:mcsmurf]]

Ah i forgot the // XXX - FIX ME? is because Bug 313147 added a "if (!m_socketIsOpen || !m_dataBuf)" check, but m_dataBuf does no longer exist with this patch. I did not look at this further, for now i have just deleted that check for m_dataBuf.

Comment 7

[David :Bienvenu]

we need the equivalent of the !m_dataBuf check, some check that we've called Cleanup on the protocol object, and gotten to where that check is w/o re-initing m_dataBuf (if you did a -uwp diff, it would be easier to give the name of the functions)

It looks like you've got tabs in your patch, e.g., +PRInt32 nsNNTPProtocol::VPrintfToStream

also, in +PRInt32 nsNNTPProtocol::VPrintfToStream, aren't you leaking datap in the case where you have to allocate it because it's too big?

Here,

+
+  if (bufSize)
+    len = bufSize;
+  else
+    len = PL_strlen(dataBuffer);
+

this can just be:

len = (bufSize) ? bufSize : PL_strlen(dataBuffer);

Why not add the logging param to +PRInt32 nsNNTPProtocol::PrintfToStream(nsIURI * aURL, const char * aFormat, ...)

instead of having two identical functions, one to pass false and one to pass true to VPrintfToStream? You can default the param to PR_TRUE so only the callers that don't want logging need to pass in the last param.


 		if (m_typeWanted == CANCEL_WANTED) {
-			NS_MsgSACopy(&command, "HEAD ");
+			PrintfToStream(mailnewsurl, "HEAD %s%s%s"CRLF,
+				*m_path != '<' ? "<" : "",
+				m_path,
+				PL_strchr(m_path, '>') == 0 ? ">" : "");
         }
 		else {
             NS_ASSERTION(m_typeWanted == ARTICLE_WANTED, "not cancel, and not article");
-			NS_MsgSACopy(&command, "ARTICLE ");
+			PrintfToStream(mailnewsurl, "ARTICLE %s%s%s"CRLF,
+				*m_path != '<' ? "<" : "",
+				m_path,
+				PL_strchr(m_path, '>') == 0 ? ">" : "");
         }

Here, you're just adding more code where there was less :-( you could just do:

const char *formatString;
formatString = (m_typeWanted == CANCEL_WANTED) ? "HEAD %s%s%s"CRLF : "ARTICLE %s%s%s"CRLF;
			PrintfToStream(mailnewsurl, formatString,
				*m_path != '<' ? "<" : "",
				m_path,
				PL_strchr(m_path, '>') == 0 ? ">" : "");

Or not even use the local var formatString at all.

Comment 8

[Frank Wein [:mcsmurf]]

Comment on attachment 201422 [details] [diff] [review]
Patch for trunk, -uw

ok will probably attach a new patch

Comment 9

[Frank Wein [:mcsmurf]]

Bug 400331 is also about cleanup and more active.