Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for -notls and -notls_verify #702

Merged
merged 4 commits into from May 14, 2017

Conversation

Projects
None yet
3 participants
@vague666
Copy link
Member

commented May 11, 2017

No description provided.

@@ -156,6 +156,10 @@ static void cmd_server_add_modify(const char *data, gboolean add)

if (g_hash_table_lookup(optlist, "tls") || g_hash_table_lookup(optlist, "ssl"))
rec->use_tls = TRUE;
else if (g_hash_table_lookup(optlist, "notls") || g_hash_table_lookup(optlist, "nossl")) {

This comment has been minimized.

Copy link
@ailin-nemui

ailin-nemui May 11, 2017

Contributor

please add { } around the if as well

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

lgtm

Jari Matilainen
@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

}
else if (g_hash_table_lookup(optlist, "notls") || g_hash_table_lookup(optlist, "nossl")) {
rec->use_tls = FALSE;
rec->tls_verify = FALSE;

This comment has been minimized.

Copy link
@LemonBoy

LemonBoy May 11, 2017

Member

Why does the tls option affect tls_verify? Afaics the latter is only used when the former is true

This comment has been minimized.

Copy link
@vague666

vague666 May 11, 2017

Author Member

If the latter exists in rec it will set use_tls = TRUE later on in the code

This comment has been minimized.

Copy link
@LemonBoy

LemonBoy May 11, 2017

Member

I see, better document this with a comment

Jari Matilainen
@@ -159,6 +159,8 @@ static void cmd_server_add_modify(const char *data, gboolean add)
}
else if (g_hash_table_lookup(optlist, "notls") || g_hash_table_lookup(optlist, "nossl")) {
rec->use_tls = FALSE;
/* if rec has tls_verify = TRUE then use_tls will be set to true on lines 224-225

This comment has been minimized.

Copy link
@LemonBoy

LemonBoy May 11, 2017

Member

Ehh, I guess something like /* tls_verify implies use_tls, disable it explicitly */ would cut it better

This comment has been minimized.

Copy link
@vague666

vague666 May 11, 2017

Author Member

It's there now! Next time say what you want me to write :P

This comment has been minimized.

Copy link
@LemonBoy

LemonBoy May 12, 2017

Member

(You probably forgot to push)
You've committed the capital sin of embedding the LoCs, the comment is gonna get stale as soon as a single line is added :)
LGTM

@ailin-nemui ailin-nemui merged commit 10cea61 into irssi:master May 14, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request Dec 7, 2017

Merge pull request irssi#702 from vague666/server_modify_notls
Added support for -notls and -notls_verify
(cherry picked from commit 10cea61)

@ailin-nemui ailin-nemui added this to the 1.0.3 milestone Jan 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.