make it so.. ok beck@
On Thu, Sep 10, 2015 at 1:06 AM, Brent Cook <bust...@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 9:43 PM, Lawrence Teo <l...@openbsd.org> wrote:
>> On Wed, Sep 09, 2015 at 11:17:55AM -0500, Brent Cook wrote:
>>> On Wed, Sep 9, 2015 at 10:15 AM, Todd C. Miller
>>> <todd.mil...@courtesan.com> wrote:
>>> > On Wed, 09 Sep 2015 10:02:17 -0400, Lawrence Teo wrote:
>>> >> In s_time.c, NO_SHUTDOWN is always defined, so there is no need for a
>>> >> bunch of NO_SHUTDOWN #ifdef blocks.
>>> >
>>> > I'm less sure about this as without calling SSL_shutdown() the
>>> > client is not notified. I suppose that's intentional as s_time is
>>> > just for timing connections?
>>> >
>>> > - todd
>>> >
>>>
>>> OK, who has a camera looking over my shoulder? I was just looking at this :)
>>
>> Oops! Please pay no attention to the drone buzzing behind you. :)
>>
>>> TBH, I'd rather this were a flag rather than a define. Yes, a knob,
>>> but this is a benchmark that really should be able to benchmark a full
>>> TLS connection and shutdown to be accurate. The default behavior of
>>> faking out the shutdown state machine does make the this run about 25%
>>> faster, but I would have never known that if not for playing with the
>>> define.
>>
>> Thank you all for the feedback. I agree that a flag would be preferred
>> over recompiling openssl(1).
>>
>> Here's a diff that adds a flag called -no_shutdown (the underscore is
>> there to match the -no_* flags used by other openssl(1) commands).
>>
>> The diff also changes the behavior of s_time so that it will perform
>> a proper full shutdown by default (i.e. if -no_shutdown is not
>> specified).
>>
>> Thoughts?
>
> Nice, I like this better, and the behavior change makes it a little more
> honest
> (but still allows apples-to-apples comparison with OpenSSL's version).
>
> ok bcook@
>
>>
>> Index: openssl.1
>> ===================================================================
>> RCS file: /cvs/src/usr.bin/openssl/openssl.1,v
>> retrieving revision 1.19
>> diff -u -p -u -p -r1.19 openssl.1
>> --- openssl.1 11 Aug 2015 05:01:03 -0000 1.19
>> +++ openssl.1 10 Sep 2015 01:51:18 -0000
>> @@ -7074,6 +7074,7 @@ unknown cipher suites a client says it s
>> .Op Fl key Ar keyfile
>> .Op Fl nbio
>> .Op Fl new
>> +.Op Fl no_shutdown
>> .Op Fl reuse
>> .Op Fl time Ar seconds
>> .Op Fl verify Ar depth
>> @@ -7135,6 +7136,10 @@ nor
>> .Fl reuse
>> are specified,
>> they are both on by default and executed in sequence.
>> +.It Fl no_shutdown
>> +Shutdown the connection without sending a
>> +.Dq close notify
>> +shutdown alert to the server.
>> .It Fl reuse
>> Performs the timing test using the same session ID;
>> this can be used as a test that session caching is working.
>> Index: s_time.c
>> ===================================================================
>> RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
>> retrieving revision 1.9
>> diff -u -p -u -p -r1.9 s_time.c
>> --- s_time.c 22 Aug 2015 16:36:05 -0000 1.9
>> +++ s_time.c 10 Sep 2015 01:58:17 -0000
>> @@ -56,8 +56,6 @@
>> * [including the GNU Public Licence.]
>> */
>>
>> -#define NO_SHUTDOWN
>> -
>> /*-----------------------------------------
>> s_time - SSL client connection timer program
>> Written and donated by Larry Streepy <stre...@healthcare.com>
>> @@ -114,6 +112,7 @@ struct {
>> char *keyfile;
>> int maxtime;
>> int nbio;
>> + int no_shutdown;
>> int perform;
>> int verify;
>> int verify_depth;
>> @@ -184,6 +183,12 @@ struct option s_time_options[] = {
>> .value = 1,
>> },
>> {
>> + .name = "no_shutdown",
>> + .desc = "Shutdown the connection without notifying the
>> server",
>> + .type = OPTION_FLAG,
>> + .opt.flag = &s_time_config.no_shutdown,
>> + },
>> + {
>> .name = "reuse",
>> .desc = "Reuse the same session ID for each connection",
>> .type = OPTION_VALUE,
>> @@ -221,7 +226,7 @@ s_time_usage(void)
>> "usage: s_time "
>> "[-bugs] [-CAfile file] [-CApath directory] [-cert file]\n"
>> " [-cipher cipherlist] [-connect host:port] [-key keyfile]\n"
>> - " [-nbio] [-new] [-reuse] [-time seconds]\n"
>> + " [-nbio] [-new] [-no_shutdown] [-reuse] [-time seconds]\n"
>> " [-verify depth] [-www page]\n\n");
>> options_usage(s_time_options);
>> }
>> @@ -341,11 +346,11 @@ s_time_main(int argc, char **argv)
>> while ((i = SSL_read(scon, buf, sizeof(buf))) > 0)
>> bytes_read += i;
>> }
>> -#ifdef NO_SHUTDOWN
>> - SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
>> SSL_RECEIVED_SHUTDOWN);
>> -#else
>> - SSL_shutdown(scon);
>> -#endif
>> + if (s_time_config.no_shutdown)
>> + SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
>> + SSL_RECEIVED_SHUTDOWN);
>> + else
>> + SSL_shutdown(scon);
>> shutdown(SSL_get_fd(scon), SHUT_RDWR);
>> close(SSL_get_fd(scon));
>>
>> @@ -400,11 +405,11 @@ next:
>> SSL_write(scon, buf, strlen(buf));
>> while (SSL_read(scon, buf, sizeof(buf)) > 0);
>> }
>> -#ifdef NO_SHUTDOWN
>> - SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN | SSL_RECEIVED_SHUTDOWN);
>> -#else
>> - SSL_shutdown(scon);
>> -#endif
>> + if (s_time_config.no_shutdown)
>> + SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
>> + SSL_RECEIVED_SHUTDOWN);
>> + else
>> + SSL_shutdown(scon);
>> shutdown(SSL_get_fd(scon), SHUT_RDWR);
>> close(SSL_get_fd(scon));
>>
>> @@ -434,11 +439,11 @@ next:
>> while ((i = SSL_read(scon, buf, sizeof(buf))) > 0)
>> bytes_read += i;
>> }
>> -#ifdef NO_SHUTDOWN
>> - SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
>> SSL_RECEIVED_SHUTDOWN);
>> -#else
>> - SSL_shutdown(scon);
>> -#endif
>> + if (s_time_config.no_shutdown)
>> + SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
>> + SSL_RECEIVED_SHUTDOWN);
>> + else
>> + SSL_shutdown(scon);
>> shutdown(SSL_get_fd(scon), SHUT_RDWR);
>> close(SSL_get_fd(scon));
>>
>>
>
