>From [email protected] Sat May 3 17:29:56 2014
>Delivered-To: [email protected]
>DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
> d=gmail.com; s=20120113;
> h=mime-version:in-reply-to:references:date:message-id:subject:from:to
> :cc:content-type;
> bh=Ddl7wFdM4PSG/9UgmJmHZAcpeI1W6ZA+onqKqQepU4M=;
> b=dBPC5jVm8ZwnKLNSXaY3L0K8NDGx+7NXTJadZcP95fKFIG7gTMPSjuKyuA7Rncvjrh
> GwOda/98XMkdBzuS4Cuir5PAVHF8UnI1q1qemGNe9n57qb+KlCagn4TkRWAYg015rI2x
> o8X53pUbT4VaLqA5TWn9WdAKdauO/yMsi3rhGhIb07R+kMJOcALW+JnLuRhrYBXyImD8
> g+bGMqyJgBbtVIHHGNWKTXdaWgNLctzkq1+sB+5gPlQwpx9rj9ijZeMKaDAus8/czBSu
> /i09QeGZjk2MAwpE7xRe6mmEd8C4kr6PAtrhaVR0tZKG8CR9s6n5xse9Vu1crMUU7QUD
> fsBw==
>MIME-Version: 1.0
>X-Received: by 10.50.30.167 with SMTP id t7mr14268026igh.17.1399163350533;
> Sat, 03 May 2014 17:29:10 -0700 (PDT)
>References: <[email protected]>
>Subject: Re: [PATCH] 1 of 2 s3_lib.c KNF retry
>From: Philip Guenther <[email protected]>
>To: Chris Hettrick <[email protected]>
>Cc: tech-openbsd <[email protected]>
>Content-Type: text/plain; charset=UTF-8
>X-Content-Discarded: text/html
>List-Help: <mailto:[email protected]?body=help>
>List-ID: <tech.openbsd.org>
>List-Owner: <mailto:[email protected]>
>List-Post: <mailto:[email protected]>
>List-Subscribe: <mailto:[email protected]?body=sub%20tech>
>List-Unsubscribe: <mailto:[email protected]?body=unsub%20tech>
>X-Loop: [email protected]
>Precedence: list
>Sender: [email protected]
>
>On Sat, May 3, 2014 at 12:40 PM, Chris Hettrick <[email protected]>wrote:
>>
>> I was too ambitious with the previous diffs. Here they are again.
>> First diff is whitespace only that can be checked with tr and md5.
>> Second diff is for non-whitespace KNF cleanup.
>>
>
>Uh, thanks, but bulk KNF diffs are really best done by the person
>committing them. I mean, we love diffs, but if it's a bulk change, then
>it's better to provide someone with the script used to make the change
>instead of the raw diff, so that we don't have to verify that the diff is
>just what it claims to be and can instead verify the process/algorithm.
> Now, if we've done a KNF pass and missed something, please let us know, or
>perhaps ping the person who did the KNF pass to see if it was intentional.
>
>(This is actually true of the entire OpenBSD tree: style is important, but
>style < content.)
>
>For this particular diff, I see a mix of stuff I agree with (spaces around
>assignment operators) and stuff I don't agree with and think are "out of
>style", like the blank line removals and the unindentation of code in
>braces after a 'case'. No way am I throwing that all in at once.
>
>
>Philip Guenther
>
Philip,
Thanks for the feedback. I appreciate it.
No script was used for the changes, so I will just leave the finer KNF
details to one of the developer's bulk cleanup.
The `ugly' bracing was corrected (removed) with my second diff, so that the
first
could be verified to only have whitespace changes. It is fine to leave it as is.
Here is a smaller diff that is for spacing and parens around return values.
Regards,
Chris Hettrick
Index: s3_lib.c
===================================================================
RCS file: /cvs/src/lib/libssl/src/ssl/s3_lib.c,v
retrieving revision 1.35
diff -u -p -r1.35 s3_lib.c
--- s3_lib.c 24 Apr 2014 13:06:52 -0000 1.35
+++ s3_lib.c 5 May 2014 01:15:37 -0000
@@ -2769,10 +2769,10 @@ int
ssl3_pending(const SSL *s)
{
if (s->rstate == SSL_ST_READ_BODY)
- return 0;
+ return (0);
- return (s->s3->rrec.type == SSL3_RT_APPLICATION_DATA) ?
- s->s3->rrec.length : 0;
+ return ((s->s3->rrec.type == SSL3_RT_APPLICATION_DATA) ?
+ s->s3->rrec.length : 0);
}
int
@@ -3080,18 +3080,18 @@ ssl3_ctrl(SSL *s, int cmd, long larg, vo
if (strlen((char *)parg) > TLSEXT_MAXLEN_host_name) {
SSLerr(SSL_F_SSL3_CTRL,
SSL_R_SSL3_EXT_INVALID_SERVERNAME);
- return 0;
+ return (0);
}
if ((s->tlsext_hostname = BUF_strdup((char *)parg))
== NULL) {
SSLerr(SSL_F_SSL3_CTRL,
ERR_R_INTERNAL_ERROR);
- return 0;
+ return (0);
}
} else {
SSLerr(SSL_F_SSL3_CTRL,
SSL_R_SSL3_EXT_INVALID_SERVERNAME_TYPE);
- return 0;
+ return (0);
}
break;
case SSL_CTRL_SET_TLSEXT_DEBUG_ARG:
@@ -3154,7 +3154,7 @@ ssl3_ctrl(SSL *s, int cmd, long larg, vo
case SSL_CTRL_GET_TLSEXT_STATUS_REQ_OCSP_RESP:
*(unsigned char **)parg = s->tlsext_ocsp_resp;
- return s->tlsext_ocsp_resplen;
+ return (s->tlsext_ocsp_resplen);
case SSL_CTRL_SET_TLSEXT_STATUS_REQ_OCSP_RESP:
if (s->tlsext_ocsp_resp)
@@ -3275,20 +3275,20 @@ ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, lon
if ((new = DHparams_dup(dh)) == NULL) {
SSLerr(SSL_F_SSL3_CTX_CTRL,
ERR_R_DH_LIB);
- return 0;
+ return (0);
}
if (!(ctx->options & SSL_OP_SINGLE_DH_USE)) {
if (!DH_generate_key(new)) {
SSLerr(SSL_F_SSL3_CTX_CTRL,
ERR_R_DH_LIB);
DH_free(new);
- return 0;
+ return (0);
}
}
if (cert->dh_tmp != NULL)
DH_free(cert->dh_tmp);
cert->dh_tmp = new;
- return 1;
+ return (1);
}
/*break; */
case SSL_CTRL_SET_TMP_DH_CB:
@@ -3307,20 +3307,20 @@ ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, lon
if (parg == NULL) {
SSLerr(SSL_F_SSL3_CTX_CTRL,
ERR_R_ECDH_LIB);
- return 0;
+ return (0);
}
ecdh = EC_KEY_dup((EC_KEY *)parg);
if (ecdh == NULL) {
SSLerr(SSL_F_SSL3_CTX_CTRL,
ERR_R_EC_LIB);
- return 0;
+ return (0);
}
if (!(ctx->options & SSL_OP_SINGLE_ECDH_USE)) {
if (!EC_KEY_generate_key(ecdh)) {
EC_KEY_free(ecdh);
SSLerr(SSL_F_SSL3_CTX_CTRL,
ERR_R_ECDH_LIB);
- return 0;
+ return (0);
}
}
@@ -3328,7 +3328,7 @@ ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, lon
EC_KEY_free(cert->ecdh_tmp);
}
cert->ecdh_tmp = ecdh;
- return 1;
+ return (1);
}
/* break; */
case SSL_CTRL_SET_TMP_ECDH_CB:
@@ -3348,11 +3348,11 @@ ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, lon
{
unsigned char *keys = parg;
if (!keys)
- return 48;
+ return (48);
if (larg != 48) {
SSLerr(SSL_F_SSL3_CTX_CTRL,
SSL_R_INVALID_TICKET_KEYS_LENGTH);
- return 0;
+ return (0);
}
if (cmd == SSL_CTRL_SET_TLSEXT_TICKET_KEYS) {
memcpy(ctx->tlsext_tick_key_name, keys, 16);
@@ -3366,23 +3366,23 @@ ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, lon
memcpy(keys + 32,
ctx->tlsext_tick_aes_key, 16);
}
- return 1;
+ return (1);
}
#ifdef TLSEXT_TYPE_opaque_prf_input
case SSL_CTRL_SET_TLSEXT_OPAQUE_PRF_INPUT_CB_ARG:
ctx->tlsext_opaque_prf_input_callback_arg = parg;
- return 1;
+ return (1);
#endif
case SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB_ARG:
ctx->tlsext_status_arg = parg;
- return 1;
+ return (1);
break;
#ifndef OPENSSL_NO_SRP
case SSL_CTRL_SET_TLS_EXT_SRP_USERNAME:
- ctx->srp_ctx.srp_Mask|=SSL_kSRP;
+ ctx->srp_ctx.srp_Mask |= SSL_kSRP;
if (ctx->srp_ctx.login != NULL)
free(ctx->srp_ctx.login);
ctx->srp_ctx.login = NULL;
@@ -3392,12 +3392,12 @@ ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, lon
strlen((const char *)parg) < 1) {
SSLerr(SSL_F_SSL3_CTX_CTRL,
SSL_R_INVALID_SRP_USERNAME);
- return 0;
+ return (0);
}
if ((ctx->srp_ctx.login = BUF_strdup((char *)parg)) == NULL) {
SSLerr(SSL_F_SSL3_CTX_CTRL,
ERR_R_INTERNAL_ERROR);
- return 0;
+ return (0);
}
break;
case SSL_CTRL_SET_TLS_EXT_SRP_PASSWORD:
@@ -3406,7 +3406,7 @@ ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, lon
ctx->srp_ctx.info = parg;
break;
case SSL_CTRL_SET_SRP_ARG:
- ctx->srp_ctx.srp_Mask|=SSL_kSRP;
+ ctx->srp_ctx.srp_Mask |= SSL_kSRP;
ctx->srp_ctx.SRP_cb_arg = parg;
break;
@@ -3422,7 +3422,7 @@ ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, lon
if ((ctx->extra_certs = sk_X509_new_null()) == NULL)
return (0);
}
- sk_X509_push(ctx->extra_certs,(X509 *)parg);
+ sk_X509_push(ctx->extra_certs, (X509 *)parg);
break;
case SSL_CTRL_GET_EXTRA_CHAIN_CERTS:
@@ -3493,17 +3493,17 @@ ssl3_ctx_callback_ctrl(SSL_CTX *ctx, int
#ifndef OPENSSL_NO_SRP
case SSL_CTRL_SET_SRP_VERIFY_PARAM_CB:
- ctx->srp_ctx.srp_Mask|=SSL_kSRP;
+ ctx->srp_ctx.srp_Mask |= SSL_kSRP;
ctx->srp_ctx.SRP_verify_param_callback =
(int (*)(SSL *, void *))fp;
break;
case SSL_CTRL_SET_TLS_EXT_SRP_USERNAME_CB:
- ctx->srp_ctx.srp_Mask|=SSL_kSRP;
+ ctx->srp_ctx.srp_Mask |= SSL_kSRP;
ctx->srp_ctx.TLS_ext_srp_username_callback =
(int (*)(SSL *, int *, void *))fp;
break;
case SSL_CTRL_SET_SRP_GIVE_CLIENT_PWD_CB:
- ctx->srp_ctx.srp_Mask|=SSL_kSRP;
+ ctx->srp_ctx.srp_Mask |= SSL_kSRP;
ctx->srp_ctx.SRP_give_srp_client_pwd_callback =
(char *(*)(SSL *, void *))fp;
break;
@@ -3532,9 +3532,9 @@ ssl3_get_cipher_by_char(const unsigned c
fprintf(stderr, "Unknown cipher ID %x\n", (p[0] << 8) | p[1]);
#endif
if (cp == NULL || cp->valid == 0)
- return NULL;
+ return (NULL);
else
- return cp;
+ return (cp);
}
int
@@ -3552,7 +3552,8 @@ ssl3_put_cipher_by_char(const SSL_CIPHER
return (2);
}
-SSL_CIPHER *ssl3_choose_cipher(SSL *s, STACK_OF(SSL_CIPHER) *clnt,
+SSL_CIPHER *
+ssl3_choose_cipher(SSL *s, STACK_OF(SSL_CIPHER) *clnt,
STACK_OF(SSL_CIPHER) *srvr)
{
SSL_CIPHER *c, *ret = NULL;
@@ -3585,13 +3586,13 @@ SSL_CIPHER *ssl3_choose_cipher(SSL *s, S
(void *)srvr);
for (i = 0; i < sk_SSL_CIPHER_num(srvr); ++i) {
c = sk_SSL_CIPHER_value(srvr, i);
- printf("%p:%s\n",(void *)c, c->name);
+ printf("%p:%s\n", (void *)c, c->name);
}
printf("Client sent %d from %p:\n", sk_SSL_CIPHER_num(clnt),
(void *)clnt);
for (i = 0; i < sk_SSL_CIPHER_num(clnt); ++i) {
c = sk_SSL_CIPHER_value(clnt, i);
- printf("%p:%s\n",(void *)c, c->name);
+ printf("%p:%s\n", (void *)c, c->name);
}
#endif
@@ -3622,7 +3623,7 @@ SSL_CIPHER *ssl3_choose_cipher(SSL *s, S
#endif
#ifdef KSSL_DEBUG
-/* printf("ssl3_choose_cipher %d alg= %lx\n", i,c->algorithms);*/
+/* printf("ssl3_choose_cipher %d alg= %lx\n", i, c->algorithms); */
#endif /* KSSL_DEBUG */
alg_k = c->algorithm_mkey;
@@ -3630,7 +3631,7 @@ SSL_CIPHER *ssl3_choose_cipher(SSL *s, S
#ifndef OPENSSL_NO_KRB5
if (alg_k & SSL_kKRB5) {
- if (!kssl_keytab_is_available(s->kssl_ctx) )
+ if (!kssl_keytab_is_available(s->kssl_ctx))
continue;
}
#endif /* OPENSSL_NO_KRB5 */
@@ -3651,7 +3652,7 @@ SSL_CIPHER *ssl3_choose_cipher(SSL *s, S
ok = (alg_k & mask_k) && (alg_a & mask_a);
#ifdef CIPHER_DEBUG
printf("%d:[%08lX:%08lX:%08lX:%08lX]%p:%s\n",
- ok, alg_k, alg_a, mask_k, mask_a,(void *)c,
+ ok, alg_k, alg_a, mask_k, mask_a, (void *)c,
c->name);
#endif
}
@@ -3745,7 +3746,7 @@ SSL_CIPHER *ssl3_choose_cipher(SSL *s, S
}
if ((ec_search1 != 0) || (ec_search2 != 0)) {
for (j = 0; j <
s->session->tlsext_ellipticcurvelist_length / 2; j++) {
- if
((s->session->tlsext_ellipticcurvelist[2*j] == ec_search1) &&
(s->session->tlsext_ellipticcurvelist[2*j + 1] == ec_search2)) {
+ if
((s->session->tlsext_ellipticcurvelist[2 * j] == ec_search1) &&
(s->session->tlsext_ellipticcurvelist[2 * j + 1] == ec_search2)) {
ec_ok = 1;
break;
}
@@ -3784,7 +3785,7 @@ SSL_CIPHER *ssl3_choose_cipher(SSL *s, S
}
if ((ec_search1 != 0) || (ec_search2 != 0)) {
for (j = 0; j <
s->session->tlsext_ellipticcurvelist_length / 2; j++) {
- if
((s->session->tlsext_ellipticcurvelist[2*j] == ec_search1) &&
(s->session->tlsext_ellipticcurvelist[2*j + 1] == ec_search2)) {
+ if
((s->session->tlsext_ellipticcurvelist[2 * j] == ec_search1) &&
(s->session->tlsext_ellipticcurvelist[2 * j + 1] == ec_search2)) {
ec_ok = 1;
break;
}
@@ -3834,12 +3835,12 @@ ssl3_get_req_cert_type(SSL *s, unsigned
#endif
#ifndef OPENSSL_NO_DH
- if (alg_k & (SSL_kDHr|SSL_kEDH)) {
+ if (alg_k & (SSL_kDHr | SSL_kEDH)) {
p[ret++] = SSL3_CT_RSA_FIXED_DH;
p[ret++] = SSL3_CT_DSS_FIXED_DH;
}
if ((s->version == SSL3_VERSION) &&
- (alg_k & (SSL_kEDH|SSL_kDHd|SSL_kDHr))) {
+ (alg_k & (SSL_kEDH | SSL_kDHd | SSL_kDHr))) {
p[ret++] = SSL3_CT_RSA_EPHEMERAL_DH;
p[ret++] = SSL3_CT_DSS_EPHEMERAL_DH;
}
@@ -3847,7 +3848,7 @@ ssl3_get_req_cert_type(SSL *s, unsigned
p[ret++] = SSL3_CT_RSA_SIGN;
p[ret++] = SSL3_CT_DSS_SIGN;
#ifndef OPENSSL_NO_ECDH
- if ((alg_k & (SSL_kECDHr|SSL_kECDHe)) && (s->version >= TLS1_VERSION)) {
+ if ((alg_k & (SSL_kECDHr | SSL_kECDHe)) && (s->version >=
TLS1_VERSION)) {
p[ret++] = TLS_CT_RSA_FIXED_ECDH;
p[ret++] = TLS_CT_ECDSA_FIXED_ECDH;
}
@@ -3875,12 +3876,12 @@ ssl3_shutdown(SSL *s)
* we don't want to send messages :-)
*/
if ((s->quiet_shutdown) || (s->state == SSL_ST_BEFORE)) {
- s->shutdown = (SSL_SENT_SHUTDOWN|SSL_RECEIVED_SHUTDOWN);
+ s->shutdown = (SSL_SENT_SHUTDOWN | SSL_RECEIVED_SHUTDOWN);
return (1);
}
if (!(s->shutdown & SSL_SENT_SHUTDOWN)) {
- s->shutdown|=SSL_SENT_SHUTDOWN;
+ s->shutdown |= SSL_SENT_SHUTDOWN;
#if 1
ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_CLOSE_NOTIFY);
#endif
@@ -3889,7 +3890,7 @@ ssl3_shutdown(SSL *s)
* to be written, s->s3->alert_dispatch will be true
*/
if (s->s3->alert_dispatch)
- return(-1); /* return WANT_WRITE */
+ return (-1); /* return WANT_WRITE */
} else if (s->s3->alert_dispatch) {
/* resend it if not sent */
#if 1
@@ -3908,11 +3909,11 @@ ssl3_shutdown(SSL *s)
/* If we are waiting for a close from our peer, we are closed */
s->method->ssl_read_bytes(s, 0, NULL, 0, 0);
if (!(s->shutdown & SSL_RECEIVED_SHUTDOWN)) {
- return(-1); /* return WANT_READ */
+ return (-1); /* return WANT_READ */
}
}
- if ((s->shutdown == (SSL_SENT_SHUTDOWN|SSL_RECEIVED_SHUTDOWN)) &&
+ if ((s->shutdown == (SSL_SENT_SHUTDOWN | SSL_RECEIVED_SHUTDOWN)) &&
!s->s3->alert_dispatch)
return (1);
else
@@ -3960,7 +3961,7 @@ ssl3_write(SSL *s, const void *buf, int
/* We have flushed the buffer, so remove it */
ssl_free_wbio_buffer(s);
- s->s3->flags&= ~SSL3_FLAGS_POP_BUFFER;
+ s->s3->flags &= ~SSL3_FLAGS_POP_BUFFER;
ret = s->s3->delay_buf_pop_ret;
s->s3->delay_buf_pop_ret = 0;
@@ -4061,7 +4062,7 @@ ssl_get_algorithm2(SSL *s)
long alg2 = s->s3->tmp.new_cipher->algorithm2;
if (s->method->version == TLS1_2_VERSION &&
- alg2 == (SSL_HANDSHAKE_MAC_DEFAULT|TLS1_PRF))
- return SSL_HANDSHAKE_MAC_SHA256 | TLS1_PRF_SHA256;
- return alg2;
+ alg2 == (SSL_HANDSHAKE_MAC_DEFAULT | TLS1_PRF))
+ return (SSL_HANDSHAKE_MAC_SHA256 | TLS1_PRF_SHA256);
+ return (alg2);
}
