On Friday 02 of April 2010 20:42:16 Daniel Stenberg wrote: > On Fri, 2 Apr 2010, Kamil Dudka wrote: > > Thanks for looking into this! I am about to write the patch and now got > > a bit confused. What's the proper place for function's documentation in > > the cURL coding style? It seems fairly inconsistent to me. > > Right, we've never been very picky on how or where the docs for the > internal functions is made. Inconsistency is far better than total absense! > > > Should the documentation of a function be in .h or in .c when the > > function has an external declaration? Or both places? If so, should the > > documentation be identical? > > I think I've tried to put them in both places, but I could also be happy > with only having them in the .h file. > > > Going through the SSL backends, I see the _send and _recv functions has > > also some fragments of documentation. What if we replace them by a link > > to the documentation in sslgen.h or sslgen.c to keep it at one place? > > > > /* for documentation see Curl_ssl_send() in sslgen.h */ > > Sure, that'd be great!
Here is my attack on interfaces of Curl_ssl_send() and Curl_ssl_recv(), based on your original patch. I've tried the test-suite with OpenSSL, GnuTLS and NSS. It looks like nothing is broken by the patch so far. Only tests 301 and 306 fail with GnuTLS on my box, but they fail also without the patch applied. Any feedbeck welcome! Kamil
From 91840562164fae664dd69dc9b853505782ac6064 Mon Sep 17 00:00:00 2001 From: Kamil Dudka <[email protected]> Date: Fri, 2 Apr 2010 22:34:23 +0200 Subject: [PATCH] refactorize interface of Curl_ssl_recv/Curl_ssl_send --- lib/gtls.c | 33 +++++++++++++++++---------------- lib/gtls.h | 16 ++++++++-------- lib/nss.c | 31 +++++++++++++++++-------------- lib/nssg.h | 10 +++++++--- lib/sendf.c | 34 +++++++++++++++++++++++++--------- lib/sslgen.c | 36 ++++++++++-------------------------- lib/sslgen.h | 27 +++++++++++++++++++++------ lib/ssluse.c | 25 +++++++++++++------------ lib/ssluse.h | 10 +++++++--- 9 files changed, 125 insertions(+), 97 deletions(-) diff --git a/lib/gtls.c b/lib/gtls.c index 079c6b1..b7fa3c9 100644 --- a/lib/gtls.c +++ b/lib/gtls.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2009, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2010, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -638,18 +638,21 @@ Curl_gtls_connect(struct connectdata *conn, } -/* return number of sent (non-SSL) bytes */ +/* for documentation see Curl_ssl_send() in sslgen.h */ ssize_t Curl_gtls_send(struct connectdata *conn, int sockindex, const void *mem, - size_t len) + size_t len, + int *curlcode) { ssize_t rc = gnutls_record_send(conn->ssl[sockindex].session, mem, len); if(rc < 0 ) { - if(rc == GNUTLS_E_AGAIN) - return 0; /* EWOULDBLOCK equivalent */ - rc = -1; /* generic error code for send failure */ + *curlcode = (rc == GNUTLS_E_AGAIN) + ? /* EWOULDBLOCK */ -1 + : CURLE_SEND_ERROR; + + rc = -1; } return rc; @@ -748,22 +751,18 @@ int Curl_gtls_shutdown(struct connectdata *conn, int sockindex) return retval; } -/* - * If the read would block we return -1 and set 'wouldblock' to TRUE. - * Otherwise we return the amount of data read. Other errors should return -1 - * and set 'wouldblock' to FALSE. - */ +/* for documentation see Curl_ssl_recv() in sslgen.h */ ssize_t Curl_gtls_recv(struct connectdata *conn, /* connection data */ int num, /* socketindex */ char *buf, /* store read data here */ size_t buffersize, /* max amount to read */ - bool *wouldblock) + int *curlcode) { ssize_t ret; ret = gnutls_record_recv(conn->ssl[num].session, buf, buffersize); if((ret == GNUTLS_E_AGAIN) || (ret == GNUTLS_E_INTERRUPTED)) { - *wouldblock = TRUE; + *curlcode = -1; return -1; } @@ -773,20 +772,22 @@ ssize_t Curl_gtls_recv(struct connectdata *conn, /* connection data */ CURLcode rc = handshake(conn, conn->ssl[num].session, num, FALSE); if(rc) /* handshake() writes error message on its own */ - return rc; - *wouldblock = TRUE; /* then return as if this was a wouldblock */ + *curlcode = rc; + else + *curlcode = -1; /* then return as if this was a wouldblock */ return -1; } - *wouldblock = FALSE; if(!ret) { failf(conn->data, "Peer closed the TLS connection"); + *curlcode = CURLE_RECV_ERROR; return -1; } if(ret < 0) { failf(conn->data, "GnuTLS recv error (%d): %s", (int)ret, gnutls_strerror((int)ret)); + *curlcode = CURLE_RECV_ERROR; return -1; } diff --git a/lib/gtls.h b/lib/gtls.h index 0d3f3fa..9fe618a 100644 --- a/lib/gtls.h +++ b/lib/gtls.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2008, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2010, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -35,14 +35,14 @@ void Curl_gtls_close_all(struct SessionHandle *data); /* close a SSL connection */ void Curl_gtls_close(struct connectdata *conn, int sockindex); -/* return number of sent (non-SSL) bytes */ +/* for documentation see Curl_ssl_send() in sslgen.h */ ssize_t Curl_gtls_send(struct connectdata *conn, int sockindex, - const void *mem, size_t len); -ssize_t Curl_gtls_recv(struct connectdata *conn, /* connection data */ - int num, /* socketindex */ - char *buf, /* store read data here */ - size_t buffersize, /* max amount to read */ - bool *wouldblock); + const void *mem, size_t len, int *curlcode); + +/* for documentation see Curl_ssl_recv() in sslgen.h */ +ssize_t Curl_gtls_recv(struct connectdata *conn, int num, char *buf, + size_t buffersize, int *curlcode); + void Curl_gtls_session_free(void *ptr); size_t Curl_gtls_version(char *buffer, size_t size); int Curl_gtls_shutdown(struct connectdata *conn, int sockindex); diff --git a/lib/nss.c b/lib/nss.c index 2babfdf..560154d 100644 --- a/lib/nss.c +++ b/lib/nss.c @@ -1340,47 +1340,50 @@ CURLcode Curl_nss_connect(struct connectdata *conn, int sockindex) return curlerr; } -/* return number of sent (non-SSL) bytes */ +/* for documentation see Curl_ssl_send() in sslgen.h */ int Curl_nss_send(struct connectdata *conn, /* connection data */ int sockindex, /* socketindex */ const void *mem, /* send this data */ - size_t len) /* amount to write */ + size_t len, /* amount to write */ + int *curlcode) { int rc; rc = PR_Send(conn->ssl[sockindex].handle, mem, (int)len, 0, -1); if(rc < 0) { - failf(conn->data, "SSL write: error %d", PR_GetError()); + PRInt32 err = PR_GetError(); + if(err == PR_WOULD_BLOCK_ERROR) + *curlcode = -1; /* EWOULDBLOCK */ + else { + failf(conn->data, "SSL write: error %d", err); + *curlcode = CURLE_SEND_ERROR; + } return -1; } return rc; /* number of bytes */ } -/* - * If the read would block we return -1 and set 'wouldblock' to TRUE. - * Otherwise we return the amount of data read. Other errors should return -1 - * and set 'wouldblock' to FALSE. - */ +/* for documentation see Curl_ssl_recv() in sslgen.h */ ssize_t Curl_nss_recv(struct connectdata * conn, /* connection data */ int num, /* socketindex */ char *buf, /* store read data here */ size_t buffersize, /* max amount to read */ - bool * wouldblock) + int *curlcode) { ssize_t nread; nread = PR_Recv(conn->ssl[num].handle, buf, (int)buffersize, 0, -1); - *wouldblock = FALSE; if(nread < 0) { /* failed SSL read */ PRInt32 err = PR_GetError(); - if(err == PR_WOULD_BLOCK_ERROR) { - *wouldblock = TRUE; - return -1; /* basically EWOULDBLOCK */ + if(err == PR_WOULD_BLOCK_ERROR) + *curlcode = -1; /* EWOULDBLOCK */ + else { + failf(conn->data, "SSL read: errno %d", err); + *curlcode = CURLE_RECV_ERROR; } - failf(conn->data, "SSL read: errno %d", err); return -1; } return nread; diff --git a/lib/nssg.h b/lib/nssg.h index 6486e62..309c3d6 100644 --- a/lib/nssg.h +++ b/lib/nssg.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2008, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2010, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -42,15 +42,19 @@ int Curl_nss_close_all(struct SessionHandle *data); int Curl_nss_init(void); void Curl_nss_cleanup(void); +/* for documentation see Curl_ssl_send() in sslgen.h */ int Curl_nss_send(struct connectdata *conn, int sockindex, const void *mem, - size_t len); + size_t len, + int *curlcode); + +/* for documentation see Curl_ssl_recv() in sslgen.h */ ssize_t Curl_nss_recv(struct connectdata *conn, /* connection data */ int num, /* socketindex */ char *buf, /* store read data here */ size_t buffersize, /* max amount to read */ - bool *wouldblock); + int *curlcode); size_t Curl_nss_version(char *buffer, size_t size); int Curl_nss_check_cxn(struct connectdata *cxn); diff --git a/lib/sendf.c b/lib/sendf.c index 7f7c2cb..90c5275 100644 --- a/lib/sendf.c +++ b/lib/sendf.c @@ -268,6 +268,9 @@ static ssize_t send_plain(struct connectdata *conn, /* * Curl_write() is an internal write function that sends data to the * server. Works with plain sockets, SCP, SSL or kerberos. + * + * If the write would block (EWOULDBLOCK), we return CURLE_OK and + * (*written == 0). Otherwise we return regular CURLcode value. */ CURLcode Curl_write(struct connectdata *conn, curl_socket_t sockfd, @@ -276,11 +279,11 @@ CURLcode Curl_write(struct connectdata *conn, ssize_t *written) { ssize_t bytes_written; - CURLcode retcode; + int curlcode = CURLE_OK; int num = (sockfd == conn->sock[SECONDARYSOCKET]); if(conn->ssl[num].state == ssl_connection_complete) - bytes_written = Curl_ssl_send(conn, num, mem, len); + bytes_written = Curl_ssl_send(conn, num, mem, len, &curlcode); else if(Curl_ssh_enabled(conn, PROT_SCP)) bytes_written = Curl_scp_send(conn, num, mem, len); else if(Curl_ssh_enabled(conn, PROT_SFTP)) @@ -291,9 +294,24 @@ CURLcode Curl_write(struct connectdata *conn, bytes_written = send_plain(conn, num, mem, len); *written = bytes_written; - retcode = (-1 != bytes_written)?CURLE_OK:CURLE_SEND_ERROR; + if(-1 != bytes_written) + /* we completely ignore the curlcode value when -1 is not returned */ + return CURLE_OK; - return retcode; + /* handle EWOULDBLOCK or a send failure */ + switch(curlcode) { + case /* EWOULDBLOCK */ -1: + *written = /* EWOULDBLOCK */ 0; + return CURLE_OK; + + case CURLE_OK: + /* general send failure */ + return CURLE_SEND_ERROR; + + default: + /* we got a specific curlcode, forward it */ + return (CURLcode)curlcode; + } } /* @@ -535,13 +553,11 @@ int Curl_read(struct connectdata *conn, /* connection data */ } if(conn->ssl[num].state == ssl_connection_complete) { - nread = Curl_ssl_recv(conn, num, buffertofill, bytesfromsocket); + int curlcode; + nread = Curl_ssl_recv(conn, num, buffertofill, bytesfromsocket, &curlcode); if(nread == -1) - return -1; /* -1 from Curl_ssl_recv() means EWOULDBLOCK */ - else if(nread == -2) - /* -2 from Curl_ssl_recv() means a true error, not EWOULDBLOCK */ - return CURLE_RECV_ERROR; + return curlcode; } else if(Curl_ssh_enabled(conn, (PROT_SCP|PROT_SFTP))) { if(conn->protocol & PROT_SCP) diff --git a/lib/sslgen.c b/lib/sslgen.c index a050f10..df2a407 100644 --- a/lib/sslgen.c +++ b/lib/sslgen.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2009, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2010, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -408,38 +408,22 @@ struct curl_slist *Curl_ssl_engines_list(struct SessionHandle *data) return curlssl_engines_list(data); } -/* return number of sent (non-SSL) bytes; -1 on error */ ssize_t Curl_ssl_send(struct connectdata *conn, int sockindex, const void *mem, - size_t len) + size_t len, + int *curlcode) { - return curlssl_send(conn, sockindex, mem, len); + return curlssl_send(conn, sockindex, mem, len, curlcode); } -/* return number of received (decrypted) bytes */ - -/* - * If the read would block (EWOULDBLOCK) we return -1. If an error occurs during - * the read, we return -2. Otherwise we return the count of bytes transfered. - */ -ssize_t Curl_ssl_recv(struct connectdata *conn, /* connection data */ - int sockindex, /* socketindex */ - char *mem, /* store read data here */ - size_t len) /* max amount to read */ +ssize_t Curl_ssl_recv(struct connectdata *conn, + int sockindex, + char *mem, + size_t len, + int *curlcode) { - ssize_t nread; - bool block = FALSE; - - nread = curlssl_recv(conn, sockindex, mem, len, &block); - if(nread == -1) { - if(!block) - return -2; /* this is a true error, not EWOULDBLOCK */ - else - return -1; /* EWOULDBLOCK */ - } - - return nread; + return curlssl_recv(conn, sockindex, mem, len, curlcode); } diff --git a/lib/sslgen.h b/lib/sslgen.h index 820f532..18858af 100644 --- a/lib/sslgen.h +++ b/lib/sslgen.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2008, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2008, 2010, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -44,14 +44,29 @@ CURLcode Curl_ssl_set_engine(struct SessionHandle *data, const char *engine); /* Sets engine as default for all SSL operations */ CURLcode Curl_ssl_set_engine_default(struct SessionHandle *data); struct curl_slist *Curl_ssl_engines_list(struct SessionHandle *data); -ssize_t Curl_ssl_send(struct connectdata *conn, - int sockindex, - const void *mem, - size_t len); + +/* If the write would block (EWOULDBLOCK) or fail, we we return -1. + * The error or -1 (for EWOULDBLOCK) is then stored in *curlcode. + * Otherwise we return the count of (non-SSL) bytes transfered. + */ +ssize_t Curl_ssl_send(struct connectdata *conn, /* connection data */ + int sockindex, /* socketindex */ + const void *mem, /* data to write */ + size_t len, /* max amount to write */ + int *curlcode); /* error to return, + -1 means EWOULDBLOCK */ + +/* If the read would block (EWOULDBLOCK) or fail, we we return -1. + * The error or -1 (for EWOULDBLOCK) is then stored in *curlcode. + * Otherwise we return the count of (non-SSL) bytes transfered. + */ ssize_t Curl_ssl_recv(struct connectdata *conn, /* connection data */ int sockindex, /* socketindex */ char *mem, /* store read data here */ - size_t len); /* max amount to read */ + size_t len, /* max amount to read */ + int *curlcode); /* error to return, + -1 means EWOULDBLOCK */ + /* init the SSL session ID cache */ CURLcode Curl_ssl_initsessions(struct SessionHandle *, long); size_t Curl_ssl_version(char *buffer, size_t size); diff --git a/lib/ssluse.c b/lib/ssluse.c index bc35432..d9cf382 100644 --- a/lib/ssluse.c +++ b/lib/ssluse.c @@ -2482,11 +2482,12 @@ bool Curl_ossl_data_pending(const struct connectdata *conn, return FALSE; } -/* return number of sent (non-SSL) bytes */ +/* for documentation see Curl_ssl_send() in sslgen.h */ ssize_t Curl_ossl_send(struct connectdata *conn, int sockindex, const void *mem, - size_t len) + size_t len, + int *curlcode) { /* SSL_write() is said to return 'int' while write() and send() returns 'size_t' */ @@ -2509,10 +2510,12 @@ ssize_t Curl_ossl_send(struct connectdata *conn, /* The operation did not complete; the same TLS/SSL I/O function should be called again later. This is basicly an EWOULDBLOCK equivalent. */ - return 0; + *curlcode = /* EWOULDBLOCK */ -1; + return -1; case SSL_ERROR_SYSCALL: failf(conn->data, "SSL_write() returned SYSCALL, errno = %d", SOCKERRNO); + *curlcode = CURLE_SEND_ERROR; return -1; case SSL_ERROR_SSL: /* A failure in the SSL library occurred, usually a protocol error. @@ -2520,25 +2523,23 @@ ssize_t Curl_ossl_send(struct connectdata *conn, sslerror = ERR_get_error(); failf(conn->data, "SSL_write() error: %s", ERR_error_string(sslerror, error_buffer)); + *curlcode = CURLE_SEND_ERROR; return -1; } /* a true error */ failf(conn->data, "SSL_write() return error %d", err); + *curlcode = CURLE_SEND_ERROR; return -1; } return (ssize_t)rc; /* number of bytes */ } -/* - * If the read would block we return -1 and set 'wouldblock' to TRUE. - * Otherwise we return the amount of data read. Other errors should return -1 - * and set 'wouldblock' to FALSE. - */ +/* for documentation see Curl_ssl_recv() in sslgen.h */ ssize_t Curl_ossl_recv(struct connectdata *conn, /* connection data */ int num, /* socketindex */ char *buf, /* store read data here */ size_t buffersize, /* max amount to read */ - bool *wouldblock) + int *curlcode) { char error_buffer[120]; /* OpenSSL documents that this must be at least 120 bytes long. */ @@ -2548,7 +2549,6 @@ ssize_t Curl_ossl_recv(struct connectdata *conn, /* connection data */ buffsize = (buffersize > (size_t)INT_MAX) ? INT_MAX : (int)buffersize; nread = (ssize_t)SSL_read(conn->ssl[num].handle, buf, buffsize); - *wouldblock = FALSE; if(nread < 0) { /* failed SSL_read */ int err = SSL_get_error(conn->ssl[num].handle, (int)nread); @@ -2560,14 +2560,15 @@ ssize_t Curl_ossl_recv(struct connectdata *conn, /* connection data */ case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: /* there's data pending, re-invoke SSL_read() */ - *wouldblock = TRUE; - return -1; /* basically EWOULDBLOCK */ + *curlcode = -1; /* EWOULDBLOCK */ + return -1; default: /* openssl/ssl.h says "look at error stack/return value/errno" */ sslerror = ERR_get_error(); failf(conn->data, "SSL read: %s, errno %d", ERR_error_string(sslerror, error_buffer), SOCKERRNO); + *curlcode = CURLE_RECV_ERROR; return -1; } } diff --git a/lib/ssluse.h b/lib/ssluse.h index 9e58d20..00357af 100644 --- a/lib/ssluse.h +++ b/lib/ssluse.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2008, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2010, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -56,15 +56,19 @@ struct curl_slist *Curl_ossl_engines_list(struct SessionHandle *data); int Curl_ossl_init(void); void Curl_ossl_cleanup(void); +/* for documentation see Curl_ssl_send() in sslgen.h */ ssize_t Curl_ossl_send(struct connectdata *conn, int sockindex, const void *mem, - size_t len); + size_t len, + int *curlcode); + +/* for documentation see Curl_ssl_recv() in sslgen.h */ ssize_t Curl_ossl_recv(struct connectdata *conn, /* connection data */ int num, /* socketindex */ char *buf, /* store read data here */ size_t buffersize, /* max amount to read */ - bool *wouldblock); + int *curlcode); size_t Curl_ossl_version(char *buffer, size_t size); int Curl_ossl_check_cxn(struct connectdata *cxn); -- 1.7.0.2
------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
