266941 - selfserv should print out throughput

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P3)

Description

[Julien Pierre]

Currently, when using the -L <period> parameter, the average ops/s are displayed
periodically.

We also need to display the throughput .

This should be shown in both directions, at the application level (above SSL)
and at the TCP socket level (NSPR layer). The later is important in order to
figure out if a network interface is physically maxed, and the throughput above
the SSL layer does not tell us that due to additional SSL record overhead.

The code to do this needs to use atomics. Unfortunately, PR_AtomicAdd only works
on a 32-bit signed int.

With a server using a gigabit ethernet network interface, it is possible to
overflow this int very quickly, theoritically in as little as 16 seconds. With a
server using 2 gigabit NICs, it will take only 8 seconds ...

64-bit atomic operations in NSPR would be very useful in this case in order to
make it possible to measure average throughput accurately over longer periods.

Alternately, we could of course use locks around the counters, but it would have
a negative performance impact and affect the test results.

Comment 1

[Julien Pierre]

Actually, on many platforms, the atomics operations aren't really implemented
efficiently and use locks underneath. It might be better to use a single lock to
protect all the stat variables rather than have individual atomic ops calls for
each one.

Another component of this enhancement work is to add a mode to keep the HTTP
connection open and have the server send data continuously to the client, in
order to be able to measure the overhead of only bulk encryption & hashing,
without any SSL handshakes taking place. This won't require any change on the
client side (strsclnt).

Testing large amounts in the other direction (client to server), or in both
directions at once, is more complicated, and will be the subject of another
enhancement.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

> add a mode to keep the HTTP connection open and have the server send data 
> continuously to the client, in order to be able to measure the overhead 
> of only bulk encryption & hashing, without any SSL handshakes taking place. 
> This won't require any change on the client side (strsclnt).

IIRC, strsclnt and selfserv already have a rarely-used option, -2, that
tests two-way simultaneous (full duplex) transmission over SSL with 
separate threads for each direction.  This option requires that each end
specify the name of a file to send to the other, IIRC, so naming a 
particularly large file should be one way to measure throughput.  

Our QA doesn't presently test that mode, AFAIK.  But it should because 
two way simultaneous operation is a feature of SSL/TLS that is implemented
in NSS.  

Comment 3

[Julien Pierre]

Nelson,

Thanks for mentioning -2, I should read the selfserv code more. For the
throughput test, we need something that isn't limited to a file even if large,
but can basically keep streaming forever to avoid any new TCP connections or
handshakes. The reading of the file is unnecessary and may actually be harmful
to what we want trying to measure. Feel free to continue this discussion in bug
266948 .

Comment 4

[Saul Edwards]

Attachment: Add throughput test, plaintext mode, and TCP logging

Sorry for the indent problems in the patch... I'm still working on this.  These
two files use a variety of tabs and spaces all over the place, so being
consistent is pretty difficult.

The patch adds three new options: -a, -g, and -j.  

-a cuts out the SSL protocol altogether and sends plaintext HTTP responses to
the client.  It is also implemented in strsclnt.

-g keeps the connection, whether plaintext or SSL, open after negotiation.  It
continues to send buffers to the client indefinitely.  The blocks are 1024
bytes by default, but the g option can be followed by an integer to specify any
size.  In addition, instead of outputting connections per second, selfserv now
outputs app data transferred per second in MB.

-j inserts a logging layer above the TCP level that records bytes being sent to
the client.  This only works with the -g mode, and only logs data moving after
the first PR_Writev call.

Possible enhancements:
-Create a bidirectional mode for the throughput test (-g) and add
implementation in strsclnt.  This would also require adding more functions at
the logging layer (read, transmitfile, etc)
-Add remaining cipher suites to both server and client
-Don't print TCP level logging with -g but no -j (currently it outputs 0.000)
-Add more logging (TCP, app data transferred) to strsclnt
-Alter strsclnt so that with -g mode it can connect to the server with more
than one thread

Comment 5

[Julien Pierre]

Saul,

1. in your selfserv logger thread, you need to change 

+        totalPeriodBytes += loggerBytes;
+        totalPeriodBytesTCP += loggerBytesTCP;
+        PR_AtomicSet(&loggerBytes, 0);
+        PR_AtomicSet(&loggerBytesTCP, 0);
+

to

+        totalPeriodBytes += PR_AtomicSet(&loggerBytes, 0);
+        totalPeriodBytesTCP += PR_AtomicSet(&loggerBytesTCP, 0);

Otherwise, the counters could be incremented by another thread in between the
addition and the reset to zero, and those bytes would never be counted by your
logger thread.

2. when incrementing the loggerBytes counter in selfserv, you should use the
return from PR_Write, as you correctly did in the layer functions :

ie.

+                rv = PR_Write(ssl_sock, testBulkBuf, testBulkSize);
+                if (rv < 0) {
+                    errWarn("PR_Write");
+                    break;
+                }
+                PR_AtomicAdd(&loggerBytes, testBulkSize);

replace the last line with :
+                PR_AtomicAdd(&loggerBytes, rv);

3. nit: in the selfserv parser, for consistency, you may want to move the code
that does the buffer allocation out of the case statement, and keep only the
parsing code there, as is done for the other options .

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 166534 [details] [diff] [review]
Add throughput test, plaintext mode, and TCP logging

Saul,  The indentation problems in the patch are serious enough that 
the patch gets a negative review on them alone.  When editing NSS code,
set your editor so that tab characters get expanded to 8 column intervals,
then use tabs and/or spaces to ensure that each new level of indentation
is 4 columns more than the previous one.  That's the NSS coding standard.
MOST (not all) files in NSS follow it.	The SSL test clients and server
sources do.  

Selfserv already has command line options to disable SSL2, SSL3, and 
SSL 3.1 (TLS).	To disable the use of SSL/TLS alltogther, it should
suffice to disable all 3.  It should not be necessary to add yet 
another command line option (-a) to mean "don't use SSL/TLS at all".
Specifying the options -23T should suffice.

I think there are several problems with the new logger code in selfserv.
I will email you about them separately.

Comment 7

[Saul Edwards]

Attachment: Clean up diff, implement suggested changes, remove -a

I discovered that -t forces the diff output to expand tabs similarly to emacs,
so all of the indentation problems go away.

I implemented all of your suggested changes, including avoiding
PR_TRUE/PR_FALSE in conditionals, usage of atomic operations, removing -a,
moving init code beyond the usage loop, etc.  I also cleaned up random white
space changes.	I think it's time for a review.

To eliminate the -a option in strsclnt, I had to add the -S option to disable
SSLv2, which was already present in selfserv.

Comment 8

[Julien Pierre]

Comment on attachment 167376 [details] [diff] [review]
Clean up diff, implement suggested changes, remove -a

Saul, this patch looks good, except for the indentation at the beginning of
server_main, which still seems to be off. r+ with the condition you fix this
part before checkin.

Comment 9

[Saul Edwards]

Julien, the indentation weirdness in server_main is a compromise that Nelson
came up with.  We wanted to add the big basic block (if !plaintextSock) but
didn't want to further indent the 60 lines of code within the if statement. 
Therefore, we decided to put the if statement starting at column 3 and leave the
conditional code starting at column 5.  Make sense?

Comment 10

[Wan-Teh Chang]

Comment on attachment 167376 [details] [diff] [review]
Clean up diff, implement suggested changes, remove -a

We should indent those 60 lines of code so
they look right.

To make it easier to see what the essential
changes are, you can generate a second diff
using "cvs diff -uw".  The -w flag ignores
white space changes.

Comment 11

[Julien Pierre]

I agree with Wan-Teh that we should indent the code, since we are logically
moving the code into the if block .

Comment 12

[Saul Edwards]

OK, assuming I indent the code in server_main, does anybody have a problem with
the content of the patch?  Nelson, could you finish your review?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 167376 [details] [diff] [review]
Clean up diff, implement suggested changes, remove -a

sr- 
I gave comments to Saul off-line.  Some lines too long.  optstate->value can't
be NULL for options declared with ":" (e.g. g:).  -g option should test
PR_WriteV in addition to (or instead of) PR_Write, and should test sizes larger
than sn SSL3 record.  -g option should be scriptable, meaning that the test
should be finite, even if long.

Comment 14

[Julien Pierre]

This patch no longer applies to the trunk . I would really like to see this go
in, so we can simulate the specweb benchmark .

We could lose the "chunk size" feature . Using a hardcoded chunk size is fine .
I would suggest using from datasize to 16 KB (max SSL record size).

It should be possible to limit the amount of data sent by the server. Zero would
indicate to keep the connection open, as this patch currently does .

Comment 15

[Slavomir Katuscak]

Attachment: Combined strsclnt and selfserv patch, v1

Improved version of Saul's patch. 

-integrated to latest selfserv/strsclnt
-fixed indentation issues
-enforced parameter to -g option + changed to number of fixed-sized chunks (size set to 16kb)
-using PR_WriteV function instead of PR_Write

Latest patch is more than 3 years old, since this time many things changes, so let me know if you want some more changes there.

Comment 16

[Julien Pierre]

Slavo,

I would prefer to wait for Nelson's review before reviewing this new patch. I had already given r+ to Saul for the previous patch, so I would approve a proper merge. I don't know what Nelson's other objections sent by email were.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Slavo's combined patch v1, made with diff -pu5w, to aid review

This patch is the same as Slavo's patch, except this patch ignores white
space changes.  This way, in the diffs, the logic changes in the files 
will not be overwhelmed by the indentation changes.  This is only to help 
me review Slavo's patch.  This is not a replacement for Slavo's patch.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 304463 [details] [diff] [review]
Combined strsclnt and selfserv patch, v1

This patch changes two files.  
These review comments are for the changes to strsclnt only.  
Review commands for selfserv will follow.

There are one or two problems with this patch, one very small, one 
maybe big.

1) This patch removes a closing brace from a section of code that is only 
compiled if/when USE_SOCK_PEER_ID is undefined.  

> #else
>             /* force a full handshake by setting the no cache option */
>             SSL_OptionSet(ssl_sock, SSL_NO_CACHE, 1);
>-        }
> #endif

Consequently, the code will no longer compile without error when that symbol 
is undefined.  There are at least two possible solutions to this:

a) put the brace back,

b) unifdef the code, so that only the code that presently is compiled when 
USE_SOCK_PEER_ID is defined remains in the source, and the code that is compiled only when USE_SOCK_PEER_ID is undefined is removed from the source.  

2) The purpose of strsclnt is to test ssl sockets. Therefore, we should not
skip the creation and "import" of the SSL socket layer, unless we absolutely
must skip it.  If all 3 forms of SSL (SSL2, SSL3, TLS) are disabled, rather 
than skipping the creation of the SSL socket, it would be better to disable
all use of the SSL protocol within the ssl socket.  That can be done with 
this simple change to one line of code.

-     rv = SSL_OptionSet(model_sock, SSL_SECURITY, 1);
+     rv = SSL_OptionSet(model_sock, SSL_SECURITY, 
+          !(disableSSL2 && disableSSL3 && disableTLS));

Now, it may be that that feature (disabling SSL_SECURITY) no longer works,
and that the ssl socket will misbehave when configured that way. 
Please try this and let me know.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 304463 [details] [diff] [review]
Combined strsclnt and selfserv patch, v1

The patch to selfserv.c tries to make two conceptually separate changes
in a single patch.  One is to deal with the case where all 3 versions
of SSL have been disabled.  The other is to do some sort of bulk testing.

Those should be done in two separate patches that can be reviewed 
separately.  IMO, there really should be a separate bug about that 
issue, and the patches to selfserv and strclnt that are intended to 
address that issue should be attached to that separate bug.

I have the same concern for selfserv that I expressed above for strsclnt
about the approach taken to disable SSL when all 3 versions of SSL have
been disabled.  

The rest of the review comments in this bug comment will be about the 
bulk testing.  

1. PR_SecondsToInterval takes an integer argument, but you are passing
a floating point value to it.  1.0 is a floating point value.

>-    PRIntervalTime logPeriodTicks = PR_SecondsToInterval(logPeriod);
>+    PRIntervalTime logPeriodTicks = PR_SecondsToInterval(1.0);
>     PRFloat64 secondsPerTick = 1.0 / (PRFloat64)PR_TicksPerSecond();

Also, PR_SecondsToInterval(1) is equivalent to PR_TicksPerSecond()
and PR_TicksPerSecond() is more efficient.  Please use it instead,
but only call it once, not twice on two successive lines.

2. issues with testBulkSize
You patch creates a variable named testBulkSize which is initialized 
to a constant value.  There is no way for this value to be changed.
Yet the code tests it to see if it is in an acceptable range. 
This test must always be true.

>+#define DEFAULT_BULK_TEST 16384
>+static PRUint32 testBulkSize       = DEFAULT_BULK_TEST;
>+            testBulkSize = DEFAULT_BULK_TEST;

The above assignment is unnecessary since the variable was already 
initialized with that value.

>+        if (testBulkSize > 0 && testBulkSize <= MAX_BULK_TEST) {
>+            testBulkBuf = PORT_Malloc(testBulkSize);

If the above test fails (which it cannot), the patch attempts to print 
a warning, (see below)

>+        } else {
>+            errWarn("Invalid buffer size for bulk encryption testing. "
>+                "Using 1024 bytes.");
>+            testBulkBuf = PORT_Malloc(DEFAULT_BULK_TEST);
>+            testBulkSize = DEFAULT_BULK_TEST;

but there are several problems with that warning.  First, the function
errWarn prints out the string for the current NSPR (PORT) error code,
but no error code has been set.  Second, this string incorrectly reports
the buffer size that will be used.  DEFAULT_BULK_TEST is not 1024.

Since the test cannot fail, the code in the else block is dead code. 
Please don't add dead code to the source.

>+        for (i = 0; i < testBulkSize; i++)
>+            testBulkBuf[i] = i; /* No CRLFs */

The comment "No CRLFs" makes me think that you intended that this loop 
should never insert a CR or LF character into the buffer.  
Is that what you intended?  

This loop puts both the CR and LF characters into the buffer, repeatedly.
If the comment was meant to convey some other meaning, please explain.

If plaintextSock is true, you don't push your IO layer, which means you
won't gather any statistics about bytes written.  You need to push your
statistic gathering layer whether SSL is in use, or not. 

You probably always want to push you layer just above the bottom layer.
You can do that without knowing whether the SSL is present, or not.
Just walk down the layer chain until you find the bottom layer, then
call PR_GetLayersIdentity on the bottom layer to get its identity.
That's the identity that you will use in the PR_PushIOLayer call.

Finally, I have some comments about the way you compute elapsed time 
in function logger().  I will write those in a separate bug comment, 
to follow.



>Index: cmd/selfserv/selfserv.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v
>retrieving revision 1.81
>diff -p -u -9 -r1.81 selfserv.c
>--- cmd/selfserv/selfserv.c	11 Sep 2007 22:40:37 -0000	1.81
>+++ cmd/selfserv/selfserv.c	20 Feb 2008 14:39:44 -0000
>@@ -87,21 +87,36 @@
> #endif
> 
> int NumSidCacheEntries = 1024;
> 
> static int handle_connection( PRFileDesc *, PRFileDesc *, int );
> 
> static const char envVarName[] = { SSL_ENV_VAR_NAME };
> static const char inheritableSockName[] = { "SELFSERV_LISTEN_SOCKET" };
> 
>+#define DEFAULT_BULK_TEST 16384
>+#define MAX_BULK_TEST     1048576 /* 1 MB */
>+static PRBool testBulk             = PR_FALSE;
>+static PRBool plaintextSock        = PR_FALSE;
>+static PRUint32 testBulkSize       = DEFAULT_BULK_TEST;
>+static PRUint32 testBulkTotal      = 0;
>+static char* testBulkBuf           = NULL;
>+static PRDescIdentity log_layer_id = PR_INVALID_IO_LAYER;
>+static PRFileDesc *loggingFD       = NULL;
>+static PRIOMethods loggingMethods;
>+
> static PRBool logStats = PR_FALSE;
>+static PRBool loggingLayer = PR_FALSE;
> static int logPeriod = 30;
> static PRUint32 loggerOps = 0;
>+static PRUint32 loggerBytes = 0;
>+static PRUint32 loggerBytesTCP = 0;
>+static PRUint32 bulkSentChunks= 0;
> 
> const int ssl2CipherSuites[] = {
>     SSL_EN_RC4_128_WITH_MD5,			/* A */
>     SSL_EN_RC4_128_EXPORT40_WITH_MD5,		/* B */
>     SSL_EN_RC2_128_CBC_WITH_MD5,		/* C */
>     SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5,	/* D */
>     SSL_EN_DES_64_CBC_WITH_MD5,			/* E */
>     SSL_EN_DES_192_EDE3_CBC_WITH_MD5,		/* F */
>     0
>@@ -179,41 +194,45 @@ Usage(const char *progName)
> #ifdef NSS_ENABLE_ECC
> "         [-i pid_file] [-c ciphers] [-d dbdir] [-e ec_nickname] \n"
> "         [-f fortezza_nickname] [-L [seconds]] [-M maxProcs] [-P dbprefix]\n"
> #else
> "         [-i pid_file] [-c ciphers] [-d dbdir] [-f fortezza_nickname] \n"
> "         [-L [seconds]] [-M maxProcs] [-P dbprefix] [-C SSLCacheEntries]\n"
> #endif /* NSS_ENABLE_ECC */
> "-S means disable SSL v2\n"
> "-3 means disable SSL v3\n"
>+"-T means disable TLS\n"
> "-B bypasses the PKCS11 layer for SSL encryption and MACing\n"
> "-q checks for bypassability\n"
> "-D means disable Nagle delays in TCP\n"
> "-E means disable export ciphersuites and SSL step down key gen\n"
>-"-T means disable TLS\n"
> "-R means disable detection of rollback from TLS to SSL3\n"
> "-b means try binding to the port and exit\n"
> "-m means test the model-socket feature of SSL_ImportFD.\n"
> "-r flag is interepreted as follows:\n"
> "    1 -r  means request, not require, cert on initial handshake.\n"
> "    2 -r's mean request  and require, cert on initial handshake.\n"
> "    3 -r's mean request, not require, cert on second handshake.\n"
> "    4 -r's mean request  and require, cert on second handshake.\n"
> "-s means disable SSL socket locking for performance\n"
> "-v means verbose output\n"
> "-x means use export policy.\n"
> "-L seconds means log statistics every 'seconds' seconds (default=30).\n"
> "-M maxProcs tells how many processes to run in a multi-process server\n"
> "-N means do NOT use the server session cache.  Incompatible with -M.\n"
> "-t threads -- specify the number of threads to use for connections.\n"
> "-i pid_file file to write the process id of selfserve\n"
> "-l means use local threads instead of global threads\n"
>-"-C SSLCacheEntries sets the maximum number of entries in the SSL session cache\n"
>+"-g means test throughput by sending total n chunks of size 16kb\n" 
>+"     to the client, 0 means unlimited (default=0)\n"
>+"-j means measure TCP throughput (for use with -g)\n"
>+"-C SSLCacheEntries sets the maximum number of entries in the SSL\n" 
>+ "    session cache\n"
> "-c ciphers   Letter(s) chosen from the following list\n"
> "A    SSL2 RC4 128 WITH MD5\n"
> "B    SSL2 RC4 128 EXPORT40 WITH MD5\n"
> "C    SSL2 RC2 128 CBC WITH MD5\n"
> "D    SSL2 RC2 128 CBC EXPORT40 WITH MD5\n"
> "E    SSL2 DES 64 CBC WITH MD5\n"
> "F    SSL2 DES 192 EDE3 CBC WITH MD5\n"
> "\n"
> "c    SSL3 RSA WITH RC4 128 MD5\n"
>@@ -615,32 +634,71 @@ static void 
> logger(void *arg)
> {
>     PRFloat64 seconds;
>     PRFloat64 opsPerSec;
>     PRIntervalTime period;
>     PRIntervalTime previousTime;
>     PRIntervalTime latestTime;
>     PRUint32 previousOps;
>     PRUint32 ops;
>-    PRIntervalTime logPeriodTicks = PR_SecondsToInterval(logPeriod);
>+    PRIntervalTime logPeriodTicks = PR_SecondsToInterval(1.0);
>     PRFloat64 secondsPerTick = 1.0 / (PRFloat64)PR_TicksPerSecond();
>+    int iterations = 0;
>+    int secondsElapsed = 0;
>+    static PRUint64 totalPeriodBytes = 0;
>+    static PRUint64 totalPeriodBytesTCP = 0;
> 
>     previousOps = loggerOps;
>     previousTime = PR_IntervalNow();
>  
>     for (;;) {
>-    	PR_Sleep(logPeriodTicks);
>+        /* OK, implementing a new sleep algorithm here... always sleep 
>+           for 1 second but print out info at the user-specified interval.
>+           This way, we don't overflow all of our PR_Atomic* functions and 
>+           we don't have to use locks. */
>+        PR_Sleep(logPeriodTicks);
>+        secondsElapsed++;
>+        totalPeriodBytes +=  PR_AtomicSet(&loggerBytes, 0);
>+        totalPeriodBytesTCP += PR_AtomicSet(&loggerBytesTCP, 0);
>+        if (secondsElapsed != logPeriod) {
>+            continue;
>+        }
>+        /* when we reach the user-specified logging interval, print out all
>+           data */
>+        secondsElapsed = 0;
>         latestTime = PR_IntervalNow();
>         ops = loggerOps;
>         period = latestTime - previousTime;
>         seconds = (PRFloat64) period*secondsPerTick;
>         opsPerSec = (ops - previousOps) / seconds;
>-        printf("%.2f ops/second, %d threads\n", opsPerSec, threadCount);
>+        if (testBulk) {
>+            if (iterations == 0) {
>+                /* with a plaintext socket, TCP data == app data */
>+                if (!plaintextSock)
>+                    printf("Conn.--------App Data--------TCP Data\n");
>+                else
>+                    printf("Conn.--------App Data\n");
>+            }
>+            if (!plaintextSock) {
>+                printf("%4.d       %5.3f MB/s      %5.3f MB/s\n", ops, 
>+                       totalPeriodBytes / (seconds * 1000000.0), 
>+                       totalPeriodBytesTCP / (seconds * 1000000.0));
>+            } else {
>+                printf("%4.d       %5.3f MB/s\n", ops, 
>+                       totalPeriodBytes / (seconds * 1000000.0));
>+            }
>+            totalPeriodBytes = 0;
>+            totalPeriodBytesTCP = 0;
>+            /* Print the "legend" every 20 iterations */
>+            iterations = (iterations + 1) % 20; 
>+        } else {
>+            printf("%.2f ops/second, %d threads\n", opsPerSec, threadCount);
>+        }
>         fflush(stdout);
>         previousOps = ops;
>         previousTime = latestTime;
>     }
> }
> 
> 
> /**************************************************************************
> ** End   thread management routines.
>@@ -918,18 +976,19 @@ handle_connection( 
>     int                reqLen;
>     int                rv;
>     int                numIOVs;
>     PRSocketOptionData opt;
>     PRIOVec            iovs[16];
>     char               msgBuf[160];
>     char               buf[10240];
>     char               fileName[513];
>     char               proto[128];
>+    PRDescIdentity     aboveLayer = PR_INVALID_IO_LAYER;
> 
>     pBuf   = buf;
>     bufRem = sizeof buf;
> 
>     VLOG(("selfserv: handle_connection: starting"));
>     opt.option             = PR_SockOpt_Nonblocking;
>     opt.value.non_blocking = PR_FALSE;
>     PR_SetSocketOption(tcp_sock, &opt);
> 
>@@ -944,18 +1003,35 @@ handle_connection( 
> 	rv = SSL_ResetHandshake(ssl_sock, /* asServer */ 1);
> 	if (rv != SECSuccess) {
> 	    errWarn("SSL_ResetHandshake");
> 	    goto cleanup;
> 	}
>     } else {
> 	ssl_sock = tcp_sock;
>     }
> 
>+    if (loggingLayer && !plaintextSock) {
>+        /* find the layer where our new layer is to be pushed */
>+        aboveLayer = PR_GetLayersIdentity(ssl_sock->lower);
>+        if (aboveLayer == PR_INVALID_IO_LAYER) {
>+            errExit("PRGetUniqueIdentity");
>+        }
>+        /* create the new layer - this is a very cheap operation */
>+        loggingFD = PR_CreateIOLayerStub(log_layer_id, &loggingMethods);
>+        if (!loggingFD)
>+            errExit("PR_CreateIOLayerStub");
>+        /* push the layer below ssl but above TCP */
>+        rv = PR_PushIOLayer(ssl_sock, aboveLayer, loggingFD);
>+        if (rv != PR_SUCCESS) {
>+            errExit("PR_PushIOLayer");
>+        }
>+    }
>+
>     if (noDelay) {
> 	opt.option         = PR_SockOpt_NoDelay;
> 	opt.value.no_delay = PR_TRUE;
> 	status = PR_SetSocketOption(ssl_sock, &opt);
> 	if (status != PR_SUCCESS) {
> 	    errWarn("PR_SetSocketOption(PR_SockOpt_NoDelay, PR_TRUE)");
>             if (ssl_sock) {
> 	        PR_Close(ssl_sock);
>             }
>@@ -975,19 +1051,21 @@ handle_connection( 
> 	}
> 	if (rv < 0) {
> 	    errWarn("HDX PR_Read");
> 	    goto cleanup;
> 	}
> 	/* NULL termination */
> 	pBuf[rv] = 0;
> 	if (firstTime) {
> 	    firstTime = 0;
>-	    printSecurityInfo(ssl_sock);
>+	    if (!plaintextSock) {
>+	        printSecurityInfo(ssl_sock);
>+	    }
> 	}
> 
> 	pBuf   += rv;
> 	bufRem -= rv;
> 	bufDat = pBuf - buf;
> 	/* Parse the input, starting at the beginning of the buffer.
> 	 * Stop when we detect two consecutive \n's (or \r\n's) 
> 	 * as this signifies the end of the GET or POST portion.
> 	 * The posted data follows.
>@@ -1161,31 +1239,49 @@ handle_connection( 
> 	}
> 
> 	if (reqLen > 0) {
> 	    if (verbose > 1) 
> 	    	fwrite(buf, 1, reqLen, stdout);	/* display it */
> 
> 	    iovs[numIOVs].iov_base = buf;
> 	    iovs[numIOVs].iov_len  = reqLen;
> 	    numIOVs++;
>-
>-/*	    printSecurityInfo(ssl_sock); */
> 	}
> 
>-	iovs[numIOVs].iov_base = (char *)EOFmsg;
>-	iovs[numIOVs].iov_len  = sizeof EOFmsg - 1;
>-	numIOVs++;
>+	/* Don't add the EOF if we want to test bulk encryption */
>+	if (!testBulk) {
>+	    iovs[numIOVs].iov_base = (char *)EOFmsg;
>+	    iovs[numIOVs].iov_len  = sizeof EOFmsg - 1;
>+	    numIOVs++;
>+	}
> 
> 	rv = PR_Writev(ssl_sock, iovs, numIOVs, PR_INTERVAL_NO_TIMEOUT);
> 	if (rv < 0) {
> 	    errWarn("PR_Writev");
> 	    break;
> 	}
>+       
>+        /* Send testBulkTotal chunks to the client. Unlimited if 0. */
>+        if (testBulk) {
>+            while (0 < (rv = PR_Writev(ssl_sock, iovs, numIOVs, 
>+                PR_INTERVAL_NO_TIMEOUT))) { 
>+                PR_AtomicAdd(&loggerBytes, rv);
>+                PR_AtomicIncrement(&bulkSentChunks);
>+                if ((bulkSentChunks > testBulkTotal) && (testBulkTotal != 0))
>+                    break;
>+            }
>+            /* There was a write error, so close this connection. */
>+            if (bulkSentChunks <= testBulkTotal) {
>+                errWarn("PR_Write");
>+            }
>+            PR_AtomicDecrement(&loggerOps);
>+            break;
>+        }
>     } while (0);
> 
> cleanup:
>     if (ssl_sock) {
>         PR_Close(ssl_sock);
>     } else if (tcp_sock) {
>         PR_Close(tcp_sock);
>     }
>     if (local_file_fd)
>@@ -1256,19 +1352,19 @@ do_accepts(
> 		continue;
> 	    }
> 	    stopping = 1;
> 	    break;
> 	}
> 
>         VLOG(("selfserv: do_accept: Got connection\n"));
> 
>         if (logStats) {
>-            loggerOps++;
>+            PR_AtomicIncrement(&loggerOps);
>         }
> 
> 	PZ_Lock(qLock);
> 	while (PR_CLIST_IS_EMPTY(&freeJobs) && !stopping) {
>             PZ_WaitCondVar(freeListNotEmptyCv, PR_INTERVAL_NO_TIMEOUT);
> 	}
> 	if (stopping) {
> 	    PZ_Unlock(qLock);
>             if (tcp_sock) {
>@@ -1354,152 +1450,212 @@ getBoundListenSocket(unsigned short port
>     }
> 
>     prStatus = PR_Listen(listen_sock, listenQueueDepth);
>     if (prStatus < 0) {
> 	errExit("PR_Listen");
>     }
>     return listen_sock;
> }
> 
>+PRInt32 PR_CALLBACK 
>+logWritev (
>+    PRFileDesc     *fd,
>+    const PRIOVec  *iov,
>+    PRInt32         size, 
>+    PRIntervalTime  timeout )
>+{
>+    PRInt32 rv = (fd->lower->methods->writev)(fd->lower, iov, size, 
>+        timeout);
>+    /* Add the amount written, but not if there's an error */
>+    if (rv > 0) 
>+        PR_AtomicAdd(&loggerBytesTCP, rv);
>+    return rv;
>+}
>+    
>+PRInt32 PR_CALLBACK 
>+logWrite (
>+    PRFileDesc  *fd, 
>+    const void  *buf, 
>+    PRInt32      amount)
>+{   
>+    PRInt32 rv = (fd->lower->methods->write)(fd->lower, buf, amount);
>+    /* Add the amount written, but not if there's an error */
>+    if (rv > 0) 
>+        PR_AtomicAdd(&loggerBytesTCP, rv);
>+    
>+    return rv;
>+}
>+
>+PRInt32 PR_CALLBACK 
>+logSend (
>+    PRFileDesc     *fd, 
>+    const void     *buf, 
>+    PRInt32         amount, 
>+    PRIntn          flags, 
>+    PRIntervalTime  timeout)
>+{
>+    PRInt32 rv = (fd->lower->methods->send)(fd->lower, buf, amount, 
>+        flags, timeout);
>+    /* Add the amount written, but not if there's an error */
>+    if (rv > 0) 
>+        PR_AtomicAdd(&loggerBytesTCP, rv);
>+    return rv;
>+}
>+ 
>+void initLoggingLayer(void)
>+{   
>+    /* get a new layer ID */
>+    log_layer_id = PR_GetUniqueIdentity("Selfserv Logging");
>+    if (log_layer_id == PR_INVALID_IO_LAYER)
>+        errExit("PR_GetUniqueIdentity");
>+    
>+    /* setup the default IO methods with my custom write methods */
>+    memcpy(&loggingMethods, PR_GetDefaultIOMethods(), sizeof(PRIOMethods));
>+    loggingMethods.writev = logWritev;
>+    loggingMethods.write  = logWrite;
>+    loggingMethods.send   = logSend;
>+}
>+
> void
> server_main(
>     PRFileDesc *        listen_sock,
>     int                 requestCert, 
>     SECKEYPrivateKey ** privKey,
>     CERTCertificate **  cert)
> {
>     PRFileDesc *model_sock	= NULL;
>     int         rv;
>     SSLKEAType  kea;
>     SECStatus	secStatus;
> 
>-    if (useModelSocket) {
>-    	model_sock = PR_NewTCPSocket();
>-	if (model_sock == NULL) {
>-	    errExit("PR_NewTCPSocket on model socket");
>-	}
>-	model_sock = SSL_ImportFD(NULL, model_sock);
>-	if (model_sock == NULL) {
>-	    errExit("SSL_ImportFD");
>-	}
>-    } else {
>-	model_sock = listen_sock = SSL_ImportFD(NULL, listen_sock);
>-	if (listen_sock == NULL) {
>-	    errExit("SSL_ImportFD");
>-	}
>-    }
>+    if (!plaintextSock) {
>+        if (useModelSocket) {
>+            model_sock = PR_NewTCPSocket();
>+            if (model_sock == NULL) {
>+                errExit("PR_NewTCPSocket on model socket");
>+            }
>+            model_sock = SSL_ImportFD(NULL, model_sock);
>+            if (model_sock == NULL) {
>+                errExit("SSL_ImportFD");
>+            }
>+        } else {
>+            model_sock = listen_sock = SSL_ImportFD(NULL, listen_sock);
>+            if (listen_sock == NULL) {
>+                errExit("SSL_ImportFD");
>+            }
>+        }
> 
>-    /* do SSL configuration. */
>-    /* all suites except RSA_NULL_MD5 are enabled by default */
>+        /* do SSL configuration. */
>+        /* all suites except RSA_NULL_MD5 are enabled by default */
> 
> #if 0
>-    /* This is supposed to be true by default.
>-    ** Setting it explicitly should not be necessary.
>-    ** Let's test and make sure that's true.
>-    */
>-    rv = SSL_OptionSet(model_sock, SSL_SECURITY, 1);
>-    if (rv < 0) {
>-	errExit("SSL_OptionSet SSL_SECURITY");
>-    }
>+        /* This is supposed to be true by default.
>+        ** Setting it explicitly should not be necessary.
>+        ** Let's test and make sure that's true.
>+        */
>+        rv = SSL_OptionSet(model_sock, SSL_SECURITY, 1);
>+        if (rv < 0) {
>+            errExit("SSL_OptionSet SSL_SECURITY");
>+        }
> #endif
> 
>-    rv = SSL_OptionSet(model_sock, SSL_ENABLE_SSL3, !disableSSL3);
>-    if (rv != SECSuccess) {
>-	errExit("error enabling SSLv3 ");
>-    }
>+        rv = SSL_OptionSet(model_sock, SSL_ENABLE_SSL3, !disableSSL3);
>+        if (rv != SECSuccess) {
>+            errExit("error enabling SSLv3 ");
>+        }
> 
>-    rv = SSL_OptionSet(model_sock, SSL_ENABLE_TLS, !disableTLS);
>-    if (rv != SECSuccess) {
>-	errExit("error enabling TLS ");
>-    }
>+        rv = SSL_OptionSet(model_sock, SSL_ENABLE_TLS, !disableTLS);
>+        if (rv != SECSuccess) {
>+            errExit("error enabling TLS ");
>+        }
> 
>-    rv = SSL_OptionSet(model_sock, SSL_ENABLE_SSL2, !disableSSL2);
>-    if (rv != SECSuccess) {
>-	errExit("error enabling SSLv2 ");
>-    }
>+        rv = SSL_OptionSet(model_sock, SSL_ENABLE_SSL2, !disableSSL2);
>+        if (rv != SECSuccess) {
>+            errExit("error enabling SSLv2 ");
>+        }
>     
>-    rv = SSL_OptionSet(model_sock, SSL_ROLLBACK_DETECTION, !disableRollBack);
>-    if (rv != SECSuccess) {
>-	errExit("error enabling RollBack detection ");
>-    }
>-    if (disableStepDown) {
>-	rv = SSL_OptionSet(model_sock, SSL_NO_STEP_DOWN, PR_TRUE);
>-	if (rv != SECSuccess) {
>-	    errExit("error disabling SSL StepDown ");
>-	}
>-    }
>-    if (bypassPKCS11) {
>-       rv = SSL_OptionSet(model_sock, SSL_BYPASS_PKCS11, PR_TRUE);
>-	if (rv != SECSuccess) {
>-	    errExit("error enabling PKCS11 bypass ");
>-	}
>-    }
>-    if (disableLocking) {
>-       rv = SSL_OptionSet(model_sock, SSL_NO_LOCKS, PR_TRUE);
>-	if (rv != SECSuccess) {
>-	    errExit("error disabling SSL socket locking ");
>-	}
>-    } 
>-
>-    for (kea = kt_rsa; kea < kt_kea_size; kea++) {
>-	if (cert[kea] != NULL) {
>-	    secStatus = SSL_ConfigSecureServer(model_sock, 
>-	    		cert[kea], privKey[kea], kea);
>-	    if (secStatus != SECSuccess)
>-		errExit("SSL_ConfigSecureServer");
>-	}
>-    }
>-
>-    if (bigBuf.data) { /* doing FDX */
>-	rv = SSL_OptionSet(model_sock, SSL_ENABLE_FDX, 1);
>-	if (rv < 0) {
>-	    errExit("SSL_OptionSet SSL_ENABLE_FDX");
>-	}
>-    }
>-
>-    if (NoReuse) {
>-        rv = SSL_OptionSet(model_sock, SSL_NO_CACHE, 1);
>-        if (rv < 0) {
>-            errExit("SSL_OptionSet SSL_NO_CACHE");
>+        rv = SSL_OptionSet(model_sock, SSL_ROLLBACK_DETECTION, 
>+            !disableRollBack);
>+        if (rv != SECSuccess) {
>+            errExit("error enabling RollBack detection ");
>         }
>-    }
>+        if (disableStepDown) {
>+            rv = SSL_OptionSet(model_sock, SSL_NO_STEP_DOWN, PR_TRUE);
>+            if (rv != SECSuccess) {
>+                errExit("error disabling SSL StepDown ");
>+            }
>+        }
>+        if (bypassPKCS11) {
>+            rv = SSL_OptionSet(model_sock, SSL_BYPASS_PKCS11, PR_TRUE);
>+            if (rv != SECSuccess) {
>+                errExit("error enabling PKCS11 bypass ");
>+            }
>+        }
>+        if (disableLocking) {
>+            rv = SSL_OptionSet(model_sock, SSL_NO_LOCKS, PR_TRUE);
>+            if (rv != SECSuccess) {
>+                errExit("error disabling SSL socket locking ");
>+            }
>+        } 
> 
>-    /* This cipher is not on by default. The Acceptance test
>-     * would like it to be. Turn this cipher on.
>-     */
>+        for (kea = kt_rsa; kea < kt_kea_size; kea++) {
>+            if (cert[kea] != NULL) {
>+                secStatus = SSL_ConfigSecureServer(model_sock, 
>+                    cert[kea], privKey[kea], kea);
>+               if (secStatus != SECSuccess)
>+                   errExit("SSL_ConfigSecureServer");
>+            }
>+        }
> 
>-    secStatus = SSL_CipherPrefSetDefault( SSL_RSA_WITH_NULL_MD5, PR_TRUE);
>-    if ( secStatus != SECSuccess ) {
>-	errExit("SSL_CipherPrefSetDefault:SSL_RSA_WITH_NULL_MD5");
>-    }
>+        if (bigBuf.data) { /* doing FDX */
>+            rv = SSL_OptionSet(model_sock, SSL_ENABLE_FDX, 1);
>+            if (rv < 0) {
>+                errExit("SSL_OptionSet SSL_ENABLE_FDX");
>+            }
>+        }
> 
>+        if (NoReuse) {
>+            rv = SSL_OptionSet(model_sock, SSL_NO_CACHE, 1);
>+            if (rv < 0) {
>+                errExit("SSL_OptionSet SSL_NO_CACHE");
>+            }
>+        }
> 
>-    if (requestCert) {
>-	SSL_AuthCertificateHook(model_sock, mySSLAuthCertificate, 
>-	                        (void *)CERT_GetDefaultCertDB());
>-	if (requestCert <= 2) { 
>-	    rv = SSL_OptionSet(model_sock, SSL_REQUEST_CERTIFICATE, 1);
>-	    if (rv < 0) {
>-		errExit("first SSL_OptionSet SSL_REQUEST_CERTIFICATE");
>-	    }
>-	    rv = SSL_OptionSet(model_sock, SSL_REQUIRE_CERTIFICATE, 
>-	                    (requestCert == 2));
>-	    if (rv < 0) {
>-		errExit("first SSL_OptionSet SSL_REQUIRE_CERTIFICATE");
>-	    }
>-	}
>-    }
>+        /* This cipher is not on by default. The Acceptance test
>+         * would like it to be. Turn this cipher on.
>+         */
>+
>+        secStatus = SSL_CipherPrefSetDefault( SSL_RSA_WITH_NULL_MD5, PR_TRUE);
>+        if ( secStatus != SECSuccess ) {
>+            errExit("SSL_CipherPrefSetDefault:SSL_RSA_WITH_NULL_MD5");
>+        }
> 
>-    if (MakeCertOK)
>-	SSL_BadCertHook(model_sock, myBadCertHandler, NULL);
>+        if (requestCert) {
>+            SSL_AuthCertificateHook(model_sock, mySSLAuthCertificate, 
>+                (void *)CERT_GetDefaultCertDB());
>+            if (requestCert <= 2) { 
>+                rv = SSL_OptionSet(model_sock, SSL_REQUEST_CERTIFICATE, 1);
>+                if (rv < 0) {
>+                    errExit("first SSL_OptionSet SSL_REQUEST_CERTIFICATE");
>+                }
>+                rv = SSL_OptionSet(model_sock, SSL_REQUIRE_CERTIFICATE, 
>+                    (requestCert == 2));
>+                if (rv < 0) {
>+                    errExit("first SSL_OptionSet SSL_REQUIRE_CERTIFICATE");
>+                }
>+            }
>+        }
> 
>-    /* end of ssl configuration. */
>+        if (MakeCertOK)
>+            SSL_BadCertHook(model_sock, myBadCertHandler, NULL);
> 
>+        /* end of ssl configuration. */
>+    } /* end plaintextSock == false */
> 
>     /* Now, do the accepting, here in the main thread. */
>     rv = do_accepts(listen_sock, model_sock, requestCert);
> 
>     terminateWorkerThreads();
> 
>     if (useModelSocket && model_sock) {
>         if (model_sock) {
>             PR_Close(model_sock);
>@@ -1679,31 +1835,32 @@ main(int argc, char **argv)
>     PRBool               useExportPolicy = PR_FALSE;
>     PRBool               useLocalThreads = PR_FALSE;
>     PLOptState		*optstate;
>     PLOptStatus          status;
>     PRThread             *loggerThread;
>     PRBool               debugCache = PR_FALSE; /* bug 90518 */
>     char                 emptyString[] = { "" };
>     char*                certPrefix = emptyString;
>     PRUint32		 protos = 0;
>+    int                  i;
>  
>     tmp = strrchr(argv[0], '/');
>     tmp = tmp ? tmp + 1 : argv[0];
>     progName = strrchr(tmp, '\\');
>     progName = progName ? progName + 1 : tmp;
> 
>     PR_Init( PR_SYSTEM_THREAD, PR_PRIORITY_NORMAL, 1);
> 
>     /* please keep this list of options in ASCII collating sequence.
>     ** numbers, then capital letters, then lower case, alphabetical. 
>     */
>     optstate = PL_CreateOptState(argc, argv, 
>-        "2:3BC:DEL:M:NP:RSTbc:d:e:f:hi:lmn:op:qrst:vw:xy");
>+        "2:3BC:DEL:M:NP:RSTbc:d:e:f:g:hi:jlmn:op:qrst:vw:xy");
>     while ((status = PL_GetNextOpt(optstate)) == PL_OPT_OK) {
> 	++optionsFound;
> 	switch(optstate->option) {
> 	case '2': fileName = optstate->value; break;
> 
> 	case '3': disableSSL3 = PR_TRUE; break;
> 
> 	case 'B': bypassPKCS11 = PR_TRUE; break;
> 
>@@ -1742,22 +1899,33 @@ main(int argc, char **argv)
> 
> 	case 'd': dir = optstate->value; break;
> 
> #ifdef NSS_ENABLE_ECC
> 	case 'e': ecNickName = PORT_Strdup(optstate->value); break;
> #endif /* NSS_ENABLE_ECC */
> 
> 	case 'f': fNickName = PORT_Strdup(optstate->value); break;
> 
>+        case 'g': 
>+            testBulk = PR_TRUE;
>+            testBulkSize = DEFAULT_BULK_TEST;
>+            testBulkTotal = PORT_Atoi(optstate->value); break;
>+            break;
>+
> 	case 'h': Usage(progName); exit(0); break;
> 
> 	case 'i': pidFile = optstate->value; break;
> 
>+        case 'j': 
>+            initLoggingLayer(); 
>+            loggingLayer = PR_TRUE;
>+            break;
>+
>         case 'l': useLocalThreads = PR_TRUE; break;
> 
> 	case 'm': useModelSocket = PR_TRUE; break;
> 
> 	case 'n': nickName = PORT_Strdup(optstate->value); break;
> 
> 	case 'P': certPrefix = PORT_Strdup(optstate->value); break;
> 
> 	case 'o': MakeCertOK = 1; break;
>@@ -1842,18 +2010,38 @@ main(int argc, char **argv)
>     if (pidFile) {
> 	FILE *tmpfile=fopen(pidFile,"w+");
> 
> 	if (tmpfile) {
> 	    fprintf(tmpfile,"%d",getpid());
> 	    fclose(tmpfile);
> 	}
>     }
> 
>+    /* allocate and initialize app data for bulk encryption testing */
>+    if (testBulk) {
>+        if (testBulkSize > 0 && testBulkSize <= MAX_BULK_TEST) {
>+            testBulkBuf = PORT_Malloc(testBulkSize);
>+        } else {
>+            errWarn("Invalid buffer size for bulk encryption testing. "
>+                "Using 1024 bytes.");
>+            testBulkBuf = PORT_Malloc(DEFAULT_BULK_TEST);
>+            testBulkSize = DEFAULT_BULK_TEST;
>+        }
>+        if (testBulkBuf == NULL)
>+            errExit("Out of memory: testBulkBuf");
>+        for (i = 0; i < testBulkSize; i++)
>+            testBulkBuf[i] = i; /* No CRLFs */
>+    }
>+
>+    /* if all SSL is disabled, use plaintext sockets */
>+    if (disableSSL2 && disableSSL3 && disableTLS)
>+        plaintextSock = PR_TRUE;
>+
>     envString = getenv(envVarName);
>     tmp = getenv("TMP");
>     if (!tmp)
> 	tmp = getenv("TMPDIR");
>     if (!tmp)
> 	tmp = getenv("TEMP");
>     if (envString) {
> 	/* we're one of the children in a multi-process server. */
> 	listen_sock = PR_GetInheritedFD(inheritableSockName);
>Index: cmd/strsclnt/strsclnt.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v
>retrieving revision 1.59
>diff -p -u -9 -r1.59 strsclnt.c
>--- cmd/strsclnt/strsclnt.c	5 Nov 2007 17:13:27 -0000	1.59
>+++ cmd/strsclnt/strsclnt.c	20 Feb 2008 14:39:45 -0000
>@@ -154,18 +154,19 @@ static PRInt32 numUsed;
> static SSL3Statistics * ssl3stats;
> 
> static int failed_already = 0;
> static PRBool disableSSL2     = PR_FALSE;
> static PRBool disableSSL3     = PR_FALSE;
> static PRBool disableTLS      = PR_FALSE;
> static PRBool bypassPKCS11    = PR_FALSE;
> static PRBool disableLocking  = PR_FALSE;
> static PRBool ignoreErrors    = PR_FALSE;
>+static PRBool plaintextSock   = PR_FALSE;
> 
> PRIntervalTime maxInterval    = PR_INTERVAL_NO_TIMEOUT;
> 
> char * progName;
> 
> char * ownPasswd( PK11SlotInfo *slot, PRBool retry, void *arg)
> {
>         char *passwd = NULL;
> 
>@@ -688,19 +689,21 @@ handle_connection( PRFileDesc *ssl_sock,
> 
>     rv = PR_Send(ssl_sock, request, strlen(request), 0, maxInterval);
>     if (rv <= 0) {
> 	errWarn("PR_Send");
> 	PR_Free(buf);
> 	buf = 0;
>         failed_already = 1;
> 	return SECFailure;
>     }
>-    printSecurityInfo(ssl_sock);
>+
>+    if (!plaintextSock) 
>+        printSecurityInfo(ssl_sock);
> 
>     /* read until EOF */
>     while (1) {
> 	rv = PR_Recv(ssl_sock, buf, RD_BUF_SIZE, 0, maxInterval);
> 	if (rv == 0) {
> 	    break;	/* EOF */
> 	}
> 	if (rv < 0) {
> 	    errWarn("PR_Recv");
>@@ -822,59 +825,68 @@ retry:
>     } else {
>         if (ThrottleUp) {
>             PRTime now = PR_Now();
>             PR_Lock(threadLock);
>             lastConnectSuccess = PR_MAX(now, lastConnectSuccess);
>             PR_Unlock(threadLock);
>         }
>     }
> 
>-    ssl_sock = SSL_ImportFD(model_sock, tcp_sock);
>-    /* XXX if this import fails, close tcp_sock and return. */
>-    if (!ssl_sock) {
>-    	PR_Close(tcp_sock);
>-	return SECSuccess;
>-    }
>-    if (fullhs != NO_FULLHS_PERCENTAGE) {
>+    if (plaintextSock) {
>+        ssl_sock = tcp_sock;
>+        /* NULL tcp_sock so the "done" code doesn't try to close
>+           close the same socket twice. */
>+        tcp_sock = NULL;
>+    } else {
>+        ssl_sock = SSL_ImportFD(model_sock, tcp_sock);
>+        /* XXX if this import fails, close tcp_sock and return. */
>+        if (!ssl_sock) {
>+            PR_Close(tcp_sock);
>+            return SECSuccess;
>+        }
>+
>+        if (fullhs != NO_FULLHS_PERCENTAGE) {
> #ifdef USE_SOCK_PEER_ID
>-        char sockPeerIDString[512];
>-        static PRInt32 sockPeerID = 0; /* atomically incremented */
>-        PRInt32 thisPeerID;
>+            char sockPeerIDString[512];
>+            static PRInt32 sockPeerID = 0; /* atomically incremented */
>+            PRInt32 thisPeerID;
> #endif
>-        PRInt32 savid = PR_AtomicIncrement(&globalconid);
>-        PRInt32 conid = 1 + (savid - 1) % 100;
>-        /* don't change peer ID on the very first handshake, which is always
>-           a full, so the session gets stored into the client cache */
>-        if ( (savid != 1) &&
>-            ( ( (savid <= total_connections_rounded_down_to_hundreds) &&
>-                (conid <= fullhs) ) ||
>-              (conid*100 <= total_connections_modulo_100*fullhs ) ) ) {
>+            PRInt32 savid = PR_AtomicIncrement(&globalconid);
>+            PRInt32 conid = 1 + (savid - 1) % 100;
>+            /* don't change peer ID on the very first handshake, 
>+               which is always a full, so the session gets stored 
>+               into the client cache */
>+            if ( (savid != 1) &&
>+                ( ( (savid <= total_connections_rounded_down_to_hundreds) &&
>+                    (conid <= fullhs) ) ||
>+                  (conid*100 <= total_connections_modulo_100*fullhs ) ) ) {
> #ifdef USE_SOCK_PEER_ID
>-            /* force a full handshake by changing the socket peer ID */
>-            thisPeerID = PR_AtomicIncrement(&sockPeerID);
>-        } else {
>-            /* reuse previous sockPeerID for restart handhsake */
>-            thisPeerID = lastFullHandshakePeerID;
>-        }
>-        PR_snprintf(sockPeerIDString, sizeof(sockPeerIDString), "ID%d",
>-                    thisPeerID);
>-        SSL_SetSockPeerID(ssl_sock, sockPeerIDString);
>-        SSL_HandshakeCallback(ssl_sock, myHandshakeCallback, (void*)thisPeerID);
>+                /* force a full handshake by changing the socket peer ID */
>+                thisPeerID = PR_AtomicIncrement(&sockPeerID);
>+            } else {
>+                /* reuse previous sockPeerID for restart handhsake */
>+                thisPeerID = lastFullHandshakePeerID;
>+            }
>+            PR_snprintf(sockPeerIDString, sizeof(sockPeerIDString), "ID%d",
>+                        thisPeerID);
>+            SSL_SetSockPeerID(ssl_sock, sockPeerIDString);
>+            SSL_HandshakeCallback(ssl_sock, myHandshakeCallback, 
>+                (void*)thisPeerID);
> #else
>             /* force a full handshake by setting the no cache option */
>             SSL_OptionSet(ssl_sock, SSL_NO_CACHE, 1);
>-        }
> #endif
>-    }
>-    rv = SSL_ResetHandshake(ssl_sock, /* asServer */ 0);
>-    if (rv != SECSuccess) {
>-	errWarn("SSL_ResetHandshake");
>-	goto done;
>+        }
>+        rv = SSL_ResetHandshake(ssl_sock, /* asServer */ 0);
>+        if (rv != SECSuccess) {
>+            errWarn("SSL_ResetHandshake");
>+            goto done;
>+        }
>     }
> 
>     PR_AtomicIncrement(&numConnected);
> 
>     if (bigBuf.data != NULL) {
> 	result = handle_fdx_connection( ssl_sock, tid);
>     } else {
> 	result = handle_connection( ssl_sock, tid);
>     }
>@@ -1416,18 +1428,22 @@ main(int argc, char **argv)
>     	readBigFile(fileName);
> 
>     /* set our password function */
>     if ( passwd ) {
> 	PK11_SetPasswordFunc(ownPasswd);
>     } else {
> 	PK11_SetPasswordFunc(SECU_GetModulePassword);
>     }
> 
>+    /* if all SSL is disabled, use plaintext sockets */
>+    if (disableSSL2 && disableSSL3 && disableTLS)
>+        plaintextSock = PR_TRUE;
>+
>     tmp = PR_GetEnv("NSS_DEBUG_TIMEOUT");
>     if (tmp && tmp[0]) {
>         int sec = PORT_Atoi(tmp);
> 	if (sec > 0) {
> 	    maxInterval = PR_SecondsToInterval(sec);
>     	}
>     }
> 
>     /* Call the libsec initialization routines */

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 304463 [details] [diff] [review]
Combined strsclnt and selfserv patch, v1

I'm sorry that the entire patch got included in that last comment.
I wish there was some way I could edit it out. :-(
I have no further review comments for this patch.

Comment 21

[Slavomir Katuscak]

(In reply to comment #18)
> 1) This patch removes a closing brace from a section of code that is only 
> compiled if/when USE_SOCK_PEER_ID is undefined.  
> 
> > #else
> >             /* force a full handshake by setting the no cache option */
> >             SSL_OptionSet(ssl_sock, SSL_NO_CACHE, 1);
> >-        }
> > #endif

This bug was there also before, in new patch will be fixed.

> 2) The purpose of strsclnt is to test ssl sockets. Therefore, we should not
> skip the creation and "import" of the SSL socket layer, unless we absolutely
> must skip it.  If all 3 forms of SSL (SSL2, SSL3, TLS) are disabled, rather 
> than skipping the creation of the SSL socket, it would be better to disable
> all use of the SSL protocol within the ssl socket.  That can be done with 
> this simple change to one line of code.
> 
> -     rv = SSL_OptionSet(model_sock, SSL_SECURITY, 1);
> +     rv = SSL_OptionSet(model_sock, SSL_SECURITY, 
> +          !(disableSSL2 && disableSSL3 && disableTLS));
> 
> Now, it may be that that feature (disabling SSL_SECURITY) no longer works,
> and that the ssl socket will misbehave when configured that way. 
> Please try this and let me know.  

Seems that works, i removed all plaintextSock occurences and used this method in new patch.

Comment 22

[Slavomir Katuscak]

Attachment: Strsclnt patch, v2

See comment #21.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 307033 [details] [diff] [review]
Strsclnt patch, v2


> #else
>-            /* force a full handshake by setting the no cache option */
>-            SSL_OptionSet(ssl_sock, SSL_NO_CACHE, 1);
>-        }
>+        /* force a full handshake by setting the no cache option */
>+        SSL_OptionSet(ssl_sock, SSL_NO_CACHE, 1);

Those last two lines should remain indented, and should not be moved out.
They are conditional on the preceding if statement.  Their previous level
of indentation was correct.  

So, re-indent those two lines.  Then, r=nelson

Comment 24

[Slavomir Katuscak]

Checking in strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.60; previous revision: 1.59
done

Comment 25

[Slavomir Katuscak]

(In reply to comment #19)
> (From update of attachment 304463 [details] [diff] [review])

> Those should be done in two separate patches that can be reviewed 
> separately.  IMO, there really should be a separate bug about that 
> issue, and the patches to selfserv and strclnt that are intended to 
> address that issue should be attached to that separate bug.

For now I would rather keep it in one patch, my patch is based on Saul's patch and it's only additional work to split it to two patches and then merge together.


> I have the same concern for selfserv that I expressed above for strsclnt
> about the approach taken to disable SSL when all 3 versions of SSL have
> been disabled.  

Done.

> 1. PR_SecondsToInterval takes an integer argument, but you are passing
> a floating point value to it.  1.0 is a floating point value.
> 
> >-    PRIntervalTime logPeriodTicks = PR_SecondsToInterval(logPeriod);
> >+    PRIntervalTime logPeriodTicks = PR_SecondsToInterval(1.0);
> >     PRFloat64 secondsPerTick = 1.0 / (PRFloat64)PR_TicksPerSecond();
> 
> Also, PR_SecondsToInterval(1) is equivalent to PR_TicksPerSecond()
> and PR_TicksPerSecond() is more efficient.  Please use it instead,
> but only call it once, not twice on two successive lines.

Done.
 
> 2. issues with testBulkSize
> Since the test cannot fail, the code in the else block is dead code. 
> Please don't add dead code to the source.

Removed.

> >+        for (i = 0; i < testBulkSize; i++)
> >+            testBulkBuf[i] = i; /* No CRLFs */
> 
> The comment "No CRLFs" makes me think that you intended that this loop 
> should never insert a CR or LF character into the buffer.  
> Is that what you intended?  

This comment is there from Saul's patch. I don't see any reason or explanation there, removing comment. 

> If plaintextSock is true, you don't push your IO layer, which means you
> won't gather any statistics about bytes written.  You need to push your
> statistic gathering layer whether SSL is in use, or not. 

PlaintextSock option completely removed, logging layer is always created (if SSL not defined, then ssl_sock is set to tcp_sock value).

> Finally, I have some comments about the way you compute elapsed time 
> in function logger().  I will write those in a separate bug comment, 
> to follow.

Feel free to come with new suggestions.

Comment 26

[Slavomir Katuscak]

Attachment: Selfserv patch, v2

See comment #25.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Slavo's combined patch v2, (incomparable)

This patch combines Slavo's last two patches, so that it can be compared
against the previous patch.  Slavo, please ensure that your patches can 
be interdiff'ed against your previous patches.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Another incomparable diff :(

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Yet another incomparable diff

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Attachment: combined patch v2, comparable to patch v1, but includes Julien's recent checkins :(

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Attachment: combined patch v2, comparable to patch v1, without Julien's recent checkins

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 307201 [details] [diff] [review]
Selfserv patch, v2

Slavo, here are some review comments & questions for you to answer.

>+        /* Send testBulkTotal chunks to the client. Unlimited if 0. */
>+        if (testBulk) {

The following block of code:

>+            iovs[0].iov_base = (char *) testBulkBuf;
>+            iovs[0].iov_len  = testBulkSize;
>+            numIOVs = 1;
>+
>+            while (0 < (rv = PR_Writev(ssl_sock, iovs, numIOVs, 
>+                PR_INTERVAL_NO_TIMEOUT))) { 

is equivalent to the much simpler:

              while (0 < (rv = PR_Write(ssl_sock, testBulkBuf, testBulkSize))) {

Please use it instead.

>+                PR_AtomicAdd(&loggerBytes, rv);
>+                PR_AtomicIncrement(&bulkSentChunks);
>+                if ((bulkSentChunks > testBulkTotal) && (testBulkTotal != 0))
>+                    break;
>+            }
>+
>+            /* There was a write error, so close this connection. */
>+            if (bulkSentChunks <= testBulkTotal) {
>+                errWarn("PR_Write");
>+            }
>+            PR_AtomicDecrement(&loggerOps);

Why is that Atomic Decrement there?  
Is it necessary? and if so, Why?
If it is correct, can you explain why it is correct?

>+            break;
>+        }
>     } while (0);

That break is unnecessary.  Please remove it.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 307201 [details] [diff] [review]
Selfserv patch, v2

Just a few issues remain. The first is a nit.
1. Initializing static and extern variables to zero/false.

It is undesirable to grow the size of the binary files unnecessarily.
The c language guarantees that all static and extern variables will
be initialized to zero, unless you initialize them to some other
value.  Initializing them to any value causes the size of the 
executable or shared library to grow by the size of the initialization
data.  So, initializing them to zero is a waste of file size. 
This applies only to static and externs, not to automatic variables
nor to variables allocated from the heap.  

The following lines are candidates to have the redundant initialization 
removed.

>+static PRUint32 testBulkTotal      = 0;
>+static char* testBulkBuf           = NULL;
>+static PRFileDesc *loggingFD       = NULL;
> static PRBool logStats = PR_FALSE;
>+static PRBool loggingLayer = PR_FALSE;
> static PRUint32 loggerOps = 0;
>+static PRUint32 loggerBytes = 0;
>+static PRUint32 loggerBytesTCP = 0;
>+static PRUint32 bulkSentChunks= 0;

2.  Not a nit.  You're adding two command line options to this program,
but you didn't add them to the command summary in the usage message.
Also, the comment about -g doesn't mention that it takes an argument.

> "Usage: %s -n rsa_nickname -p port [-3BDENRSTblmrsvx] [-w password] [-t threads]\n"
> #ifdef NSS_ENABLE_ECC
> "         [-i pid_file] [-c ciphers] [-d dbdir] [-e ec_nickname] \n"
> "         [-f fortezza_nickname] [-L [seconds]] [-M maxProcs] [-P dbprefix]\n"
> #else
> "         [-i pid_file] [-c ciphers] [-d dbdir] [-f fortezza_nickname] \n"
> "         [-L [seconds]] [-M maxProcs] [-P dbprefix] [-C SSLCacheEntries]\n"
> #endif /* NSS_ENABLE_ECC */

The above command line summary needs to also mention [-j] and [-g numblocks]

>+"-g means test throughput by sending total n chunks of size 16kb\n" 
>+"    to the client, 0 means unlimited (default=0)\n"

That needs to say -g numblocks means ... sending numblocks chunks ...
(don't use 'n', because it's ambiguous)

>+"-j means measure TCP throughput (for use with -g)\n"

That comment suggests that -j and -g must be used together.
Can -j be used without -g numblocks?
Can -g numblocks be used without -j? 
If so, the above comment should be changed to not suggest that they
must be used together.
If not, if -j and -g must be used together, then they should not be
separate options. They should be combined into one argument.

3. The block comments below do not follow the file's existing style for 
multi-line block comments.  

>+        /* OK, implementing a new sleep algorithm here... always sleep 
>+           for 1 second but print out info at the user-specified interval.
>+           This way, we don't overflow all of our PR_Atomic* functions and 
>+           we don't have to use locks. */

>+        /* when we reach the user-specified logging interval, print out all
>+           data */

The file's existing style for multi-line block comments in code is:

         /*  first line
          *  second line
          */

Comment 34

[Slavomir Katuscak]

(In reply to comment #32)
> Slavo, here are some review comments & questions for you to answer.
> 
> >+        /* Send testBulkTotal chunks to the client. Unlimited if 0. */
> >+        if (testBulk) {
> 
> The following block of code:
> 
> >+            iovs[0].iov_base = (char *) testBulkBuf;
> >+            iovs[0].iov_len  = testBulkSize;
> >+            numIOVs = 1;
> >+
> >+            while (0 < (rv = PR_Writev(ssl_sock, iovs, numIOVs, 
> >+                PR_INTERVAL_NO_TIMEOUT))) { 
> 
> is equivalent to the much simpler:
> 
>               while (0 < (rv = PR_Write(ssl_sock, testBulkBuf, testBulkSize)))
> {
> 
> Please use it instead.

Done.

> 
> >+                PR_AtomicAdd(&loggerBytes, rv);
> >+                PR_AtomicIncrement(&bulkSentChunks);
> >+                if ((bulkSentChunks > testBulkTotal) && (testBulkTotal != 0))
> >+                    break;
> >+            }
> >+
> >+            /* There was a write error, so close this connection. */
> >+            if (bulkSentChunks <= testBulkTotal) {
> >+                errWarn("PR_Write");
> >+            }
> >+            PR_AtomicDecrement(&loggerOps);
> 
> Why is that Atomic Decrement there?  
> Is it necessary? and if so, Why?
> If it is correct, can you explain why it is correct?

Variable can be changed from any thread, so yes, it's necessary.
 
> >+            break;
> >+        }
> >     } while (0);
> 
> That break is unnecessary.  Please remove it.  

Removed.

(In reply to comment #33)
> Just a few issues remain. The first is a nit.
> 1. Initializing static and extern variables to zero/false.
> 
> It is undesirable to grow the size of the binary files unnecessarily.
> The c language guarantees that all static and extern variables will
> be initialized to zero, unless you initialize them to some other
> value.  Initializing them to any value causes the size of the 
> executable or shared library to grow by the size of the initialization
> data.  So, initializing them to zero is a waste of file size. 

I think this is something what compiler should do. If there is default value set, it should just skip it as one of most simple optimalizations. Using initialization values is more clear for me then skipping them, also when we keep them default.

> 2.  Not a nit.  You're adding two command line options to this program,
> but you didn't add them to the command summary in the usage message.
> Also, the comment about -g doesn't mention that it takes an argument.
> 
> > "Usage: %s -n rsa_nickname -p port [-3BDENRSTblmrsvx] [-w password] [-t threads]\n"
> > #ifdef NSS_ENABLE_ECC
> > "         [-i pid_file] [-c ciphers] [-d dbdir] [-e ec_nickname] \n"
> > "         [-f fortezza_nickname] [-L [seconds]] [-M maxProcs] [-P dbprefix]\n"
> > #else
> > "         [-i pid_file] [-c ciphers] [-d dbdir] [-f fortezza_nickname] \n"
> > "         [-L [seconds]] [-M maxProcs] [-P dbprefix] [-C SSLCacheEntries]\n"
> > #endif /* NSS_ENABLE_ECC */
> 
> The above command line summary needs to also mention [-j] and [-g numblocks]

Done.

> >+"-g means test throughput by sending total n chunks of size 16kb\n" 
> >+"    to the client, 0 means unlimited (default=0)\n"
> 
> That needs to say -g numblocks means ... sending numblocks chunks ...
> (don't use 'n', because it's ambiguous)

Done.

> >+"-j means measure TCP throughput (for use with -g)\n"
> 
> That comment suggests that -j and -g must be used together.
> Can -j be used without -g numblocks?

Can be, but it doesn't have any effect.

> Can -g numblocks be used without -j? 

Yes, it can be (I fixed there reporting part for case that -j is not set and all values about TCP data were always 0).

> If so, the above comment should be changed to not suggest that they
> must be used together.

It's suggested for -j to use also -g. 

> 3. The block comments below do not follow the file's existing style for 
> multi-line block comments.  

Done.

Comment 35

[Slavomir Katuscak]

Attachment: Selfserv patch v3.

Changes from comment 34 + merged with latest version.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 308410 [details] [diff] [review]
Selfserv patch v3.

Slavo,
1. I require the code to NOT initialize static and extern variables to 
zero/false.  

2. This latest patch is not comparable to the previous patch.  If you click
on the diff link for the latest patch, and then in the diff page, choose
the previous patch "Selfserv patch, v2", and click the diff button, you will
see that the two patches are not comparable.  There are two ways that you 
can fix this.
a) produce a new patch which is your old patch v2 carried forward to the same
version of selfserv as your new patch, or 
b) produce a new patch v3 which is a patch to the same version of selfserv as 
patch v2.  

I would prefer the first method.  Please do that, and then produce a patch v4
which includes the initialization changes. Please make sure that your patch 
v4 is comparable to some version of your patch v2 before requesting review 
for it.

Comment 37

[Slavomir Katuscak]

Attachment: Patch v2 applied on latest version of selfserv.c.

Comment 38

[Slavomir Katuscak]

Attachment: Patch v4.

Static variables not initialized, should be comparable with previous patch.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #34)

> > >+            /* There was a write error, so close this connection. */
> > >+            if (bulkSentChunks <= testBulkTotal) {
> > >+                errWarn("PR_Write");
> > >+            }
> > >+            PR_AtomicDecrement(&loggerOps);
> > 
> > Why is that Atomic Decrement there?  
> > Is it necessary? and if so, Why?
> > If it is correct, can you explain why it is correct?
> 
> Variable can be changed from any thread, so yes, it's necessary.

I think you tried to answer the question "Why are you using an atomic
operator to do this decrement?".  The question I tried to ask (but not
clearly enough) was "Why is there a decrement operation being performed
here?  Why is it desirable to subtract 1 from the value of loggerOps at
this point in the code?"  That's not obvious to me.


Thank you for producing the patch entitled "Patch v2 applied on latest 
version of selfserv.c".  I was able to do a much more complete review
using it than I had been able to do before.   I will submit some review
comments on patch 4 shortly.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 308845 [details] [diff] [review]
Patch v4.

Thanks for addressing my previous review comments. 
There are a few minor issues left which will be easy to fix.

1. Your patch reveals that there was a bug in the Usage message code
before you patched it.  Your patch makes that old bug more obvious.
Since you're changing the buggy lines of code, I'm going to ask you
to fix the bug while you're working on it.  The bug is that the old
usage code only showed the "[-C SSLCacheEntries]" option if ECC was 
NOT enabled.  That was mistaken.  That option exists whether ECC is 
enabled or not.  Please fix it so that that option is shown whether
ECC is enabled or not.

2. Your patch reveals a second bug also in the usage code.  You added 
the letter 'u' to the string passed to PL_CreateOptState, in addition
to the letters 'g' and 'j' that your patch intended to add.  This 
apparently corrects an omission from a previous checkin for the 
session ticket extension work.  However, it reveals that that -u
option is not found in the Usage message's syntax synopsis.  
Please add it to that synopsis.  I'd suggest putting into this part:
"[-3BDENRSTbjlmrsvx]".

Please also remember to answer the question I asked about the decrement 
operation in the preceding comment.

Comment 41

[Julien Pierre]

Nelson,

Regarding the decrement operation, I think the idea behind it was not to count the operation if it resulted in an error. Otherwise, some very large, bogus number of operation/s can sometimes be reported. There may be other cases where we want to do that for completeness. Another approach would be to keep a separate counter of failed ops.

As to the reason the operation has to be atomic : prior to the patch, loggerOps was only shared by 2 threads - the acceptor thread, and the logger thread. The only writer thread was acceptor, and the only reader was logger, and this was a 32-bit value, so no atomic operations were needed.

With this patch, there are now 2 cases of writing to loggerOps - not only in do_accepts, but also somewhere in the worker thread in the failure case, which tries to decrement it. S,o the changes to this variable have to be atomic, otherwise some of these increments/increments may be lost due to a race.

Comment 42

[Slavomir Katuscak]

Attachment: Patch v5.

Fixed issues from comment 40. 
Question about atomic operations is already answerd by Julien in comment 41.

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 309412 [details] [diff] [review]
Patch v5.

Slavo, there were a bunch of indentation problems with your patch v3 
that were fixed in patch v4, but now have reappeared in patch v5.  
Can you explain that? 
Are some of your patches being produced with cvs diff -w and others 
being produced without -w? 
Please fix the white space (indentation) problems that appear in patch v5.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 309412 [details] [diff] [review]
Patch v5.

This patch addresses all the review comments I had about the previous
patch v3 and patch v4, but it appears to reintroduce a bunch of 
indentation problems.  Perhaps (I'm guess) that is because it was
generated with diff -w.  If so, then perhaps generating a patch without
the -w will solve that.

When the whitespace issues are resolved, this patch will be ready to 
commit.

Comment 45

[Slavomir Katuscak]

Attachment: Patch v5 generated without -w.

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 310193 [details] [diff] [review]
Patch v5 generated without -w.

Thanks, Slavo.  
When submitting a patch made with -w, it's a good idea to mention that the patch was made with -w in the name of the attachment, e.g. "Patch v4, made with -w".  That avoids confusion.

Comment 47

[Slavomir Katuscak]

Checking in selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.85; previous revision: 1.84
done

Comment 48

[Slavomir Katuscak]

Attachment: Patch to fix data type errors.

There were some errors on LEIA/Windows build reported:
E:/security/securitytip/builds/20080319.1/leia_WIN/mozilla/security/nss/cmd/selfserv/selfserv.c(2045) : warning C4018: '<' : signed/unsigned mismatch
E:/security/securitytip/builds/20080319.1/leia_WIN/mozilla/security/nss/cmd/selfserv/selfserv.c(697) : error C2520: conversion from unsigned __int64 to double not implemented, use signed __int64
E:/security/securitytip/builds/20080319.1/leia_WIN/mozilla/security/nss/cmd/selfserv/selfserv.c(697) : error C2520: conversion from unsigned __int64 to double not implemented, use signed __int64
E:/security/securitytip/builds/20080319.1/leia_WIN/mozilla/security/nss/cmd/selfserv/selfserv.c(700) : error C2520: conversion from unsigned __int64 to double not implemented, use signed __int64

I changed types how is suggested in error messages, but I was not able to test it on LEIA (complicated rsh access). Christophe, if you have some script which you can use to run simple build on LEIA, please check it, thanks. 

Tested on my laptop (MacOS) and one Solaris Tinderbox machine for now.

Comment 49

[Christophe Ravel]

Comment on attachment 310495 [details] [diff] [review]
Patch to fix data type errors.

Tested on Windows too. The build passes now.
r=christophe.

Comment 50

[Slavomir Katuscak]

Checking in selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.86; previous revision: 1.85
done