Elia Pinto <gitter.spiros@xxxxxxxxx> writes:
> diff --git a/http.c b/http.c
> index e9c6fdd..1504005 100644
> --- a/http.c
> +++ b/http.c
> @@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
> static int curl_ssl_try;
> static const char *ssl_cert;
> static const char *ssl_cipherlist;
> +static const char *ssl_version;
> +static long sslversion = CURL_SSLVERSION_DEFAULT;
I think you shouldn't have to initialize this variable.
See below.
> init_curl_http_auth(result);
>
> +
An unnecessary double blank?
> + if (getenv("GIT_SSL_VERSION"))
> + ssl_version = getenv("GIT_SSL_VERSION");
> + if (ssl_version != NULL && *ssl_version) {
> + if (!strcmp(ssl_version,"tlsv1")) {
> + sslversion = CURL_SSLVERSION_TLSv1;
> + } else if (!strcmp(ssl_version,"sslv2")) {
> + sslversion = CURL_SSLVERSION_SSLv2;
> + } else if (!strcmp(ssl_version,"sslv3")) {
> + sslversion = CURL_SSLVERSION_SSLv3;
> +#if LIBCURL_VERSION_NUM >= 0x072200
> + } else if (!strcmp(ssl_version,"tlsv1.0")) {
> + sslversion = CURL_SSLVERSION_TLSv1_0;
> + } else if (!strcmp(ssl_version,"tlsv1.1")) {
> + sslversion = CURL_SSLVERSION_TLSv1_1;
> + } else if (!strcmp(ssl_version,"tlsv1.2")) {
> + sslversion = CURL_SSLVERSION_TLSv1_2;
> +#endif
> + } else {
> + warning("unsupported ssl version %s : using default",
> + ssl_version);
> + }
> + }
> + curl_easy_setopt(result, CURLOPT_SSLVERSION,
> + sslversion);
A few problems:
- Why do we even have to call this when sslversion is not given by
the end user, either from the environment or from the config?
Wouldn't we use the default version if we do not make this call?
- It is true that 0x072200 is described as introducing these three
in [*1*] but the structure is maintenance burden waiting to
happen. If you #if/#endif based on defined(CURL_SSLVERSION_$v),
it will be obvious to other people how to add their future
favourite versions in their patches. Also it may be read better
if you separated the logic and the code by using a table like
this:
static struct {
const char *name;
long ssl_version;
} sslversions[] = {
{ "tlsv1", CURL_SSLVERSION_TLSv1 },
{ "sslv2", CURL_SSLVERSION_SSLv2 },
...
#ifdef CURL_SSLVERSION_TLSv1_0
{ "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
#endif
...,
{ NULL }
};
> if (getenv("GIT_SSL_CIPHER_LIST"))
> ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> -
This blank removal is understandable (i.e. group related things
together).
> if (ssl_cipherlist != NULL && *ssl_cipherlist)
> curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
> ssl_cipherlist);
> -
This is not.
> if (ssl_cert != NULL)
> curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> if (has_cert_password())
[References]
*1* https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html