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

CAP 3.2 support #775

Merged
merged 19 commits into from Feb 5, 2018
Merged

CAP 3.2 support #775

merged 19 commits into from Feb 5, 2018

Conversation

@LemonBoy
Copy link
Member

LemonBoy commented Oct 21, 2017

This is a prerequisite for the IRC v3.2 compliance.
Fuck this shit, I've accidentally implemented the whole thing.

This has been tested this with Freenode and Unrealircd and found no problem with the old 3.1 flow and Valgrind looks happy with this.
I've tested the multiline thing using netcat and the strings found on the ircv3 page and the ADD/DEL things are untested as of now (I'd like to talk with a real ircd instead of netcat...)
I'm not sure about the XS part as it kept loading the Irssi.pm module off /usr so feel free to give it a shot.

@LemonBoy LemonBoy added the WIP label Oct 21, 2017
@LemonBoy LemonBoy force-pushed the LemonBoy:caps_kv branch 2 times, most recently from 8173d01 to 293503b Oct 21, 2017
@LemonBoy LemonBoy changed the title Parse the K/V form in CAP LS CAP 3.2 support Oct 21, 2017
@ailin-nemui
Copy link
Contributor

ailin-nemui commented Oct 23, 2017

is this finished already or still wip?

@LemonBoy LemonBoy added needs testing and removed WIP labels Oct 23, 2017
@LemonBoy
Copy link
Member Author

LemonBoy commented Oct 23, 2017

I guess it can be reviewed, yeah

Copy link
Member

dequis left a comment

Let's cover case insensitivity for commands, but not for caps.

if (!g_strcmp0(evt, "LS")) {
if (!strcmp(evt, "LS")) {
/* Throw away everything and start from scratch */
g_hash_table_remove_all(server->cap_supported);

This comment has been minimized.

@dequis

dequis Oct 29, 2017 Member

Wait, why? Doesn't this end up with reading only the last line of a multiline response? Am I reading this wrong?

This comment has been minimized.

@LemonBoy

LemonBoy Oct 29, 2017 Author Member

Good catch, I moved some code and forgot to re-test with multiline responses.
The aim was to make the parsing stateless but I think the best way to handle this case is to keep track of the completeness of the response in a(nother) flag in the IRC_SERVER_CONNECT_REC

if (params == NULL)
return;

/* Multiline responses have an additional parameter and we have to do
* this stupid dance to parse them */
if (evt[0] == 'L' && !strcmp(star, "*")) {

This comment has been minimized.

@dequis

dequis Oct 29, 2017 Member

L for LS and LIST? IIIII guess that works unless more commands starting with L are added in future cap versions... Except that LIST isn't even handled below so, might as well just compare against LS?

Also, lowercase is valid too.

Also, the old code had null checking for this parameter, this one dereferences evt right away. Not sure if it ever gets to be null.

}
/* This branch covers the '*' parameter isn't present, adjust the
* parameter pointer to compensate for this */
else if (list[0] == '\0') {

This comment has been minimized.

@dequis

dequis Oct 29, 2017 Member

Same as above, not sure if this one can be null instead of an empty string.

/* Strip the trailing whitespaces before splitting the string, some servers send responses with
* superfluous whitespaces that g_strsplit the interprets as tokens */
caps = g_strsplit(g_strchomp(list), " ", -1);
caps_length = g_strv_length(caps);

if (!g_strcmp0(evt, "LS")) {
if (!strcmp(evt, "LS")) {

This comment has been minimized.

@dequis

dequis Oct 29, 2017 Member

Keeping g_strcmp0 everywhere doesn't hurt imo. Also, the old version of the code was wrong and this should have been case insensitive.

if (!g_hash_table_insert(server->cap_supported, key, val)) {
/* The specification doesn't say anything about
* duplicated values, let's just warn the user */
g_warning("Duplicate value %s", key);

This comment has been minimized.

@dequis

dequis Oct 29, 2017 Member

Sounds like an okay way to handle it, but a more specific message would be neater.

}
}
else if (!g_strcmp0(evt, "ACK")) {
else if (!strcmp(evt, "ACK")) {

This comment has been minimized.

@dequis

dequis Oct 29, 2017 Member

Same as above

char *val = (char *)val_;
hv_store(hv_, key, strlen(key), new_pv(val), 0);
}
(void) hv_store(hv, "cap_supported", 13, newRV_noinc((SV*)hv_), 0);

This comment has been minimized.

@dequis

dequis Oct 29, 2017 Member

So this is a perl api change, right? Does perl handle this decently enough, falling back to keys only if it is accessed like an array instead of a hash? @ailin-nemui

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 30, 2017 Contributor

no that would not work..

This comment has been minimized.

@ailin-nemui

ailin-nemui Jan 7, 2018 Contributor

(but I guess we can break the API)

This comment has been minimized.

@LemonBoy

LemonBoy Jan 7, 2018 Author Member

My (wild) guess is that we have no users of this API heh
I'd avoid such a breaking change in a non-point release though, got a better idea that's backward compatible?

This comment has been minimized.

@ailin-nemui

ailin-nemui Jan 7, 2018 Contributor

well, I can't think of any way other than adding another key, say cap_supported_hash or another method, since we initially put it it all in an array. but that seems silly imo

@ailin-nemui
Copy link
Contributor

ailin-nemui commented Jan 5, 2018

what's the status here @LemonBoy @dequis

@LemonBoy
Copy link
Member Author

LemonBoy commented Jan 5, 2018

It's missing another round of review, an idea about what/how to expose those new fields to the perl side and a use case :)

@@ -218,6 +218,19 @@ GSList *gslist_remove_string (GSList *list, const char *str)
return list;
}

GSList *gslist_delete_string (GSList *list, const char *str, GDestroyNotify free_func)

This comment has been minimized.

@ailin-nemui

ailin-nemui Jan 6, 2018 Contributor

you can remove the remove_string method, which is apparently flawed

This comment has been minimized.

@dequis

dequis Jan 24, 2018 Member

lol this comment came back

*key = g_strdup(name);
*val = NULL;
return TRUE;
}

This comment has been minimized.

@ailin-nemui

ailin-nemui Jan 6, 2018 Contributor

I also like this code style but until we get the fully automated code formatter working please adjust to } else if { on a single line

}
/* If the string ends after the '=' consider the value
* as invalid */
else {

This comment has been minimized.

@ailin-nemui

ailin-nemui Jan 7, 2018 Contributor

does the spec not allow empty values?

This comment has been minimized.

@LemonBoy

LemonBoy Jan 7, 2018 Author Member

That's a good question, the spec states the format is <name>[=<value>] with no further indications.

@@ -440,8 +440,10 @@ static void sig_disconnected(IRC_SERVER_REC *server)
gslist_free_full(server->cap_active, (GDestroyNotify) g_free);
server->cap_active = NULL;

gslist_free_full(server->cap_supported, (GDestroyNotify) g_free);
server->cap_supported = NULL;
if (server->cap_supported) {

This comment has been minimized.

@ailin-nemui

ailin-nemui Jan 7, 2018 Contributor

indentation is off here. you can also try our new git-clang-format

@ailin-nemui
Copy link
Contributor

ailin-nemui commented Jan 7, 2018

would be nice if you can reply to all open comments @LemonBoy then we can merge this.

LemonBoy added 13 commits Oct 21, 2017
This is a prerequisite for the IRC v3.2 compliance.
This is also needed for CAP NEW and CAP DEL.
Glib doesn't like that and shows a harmless warning.
The parsing logic isn't too elegant because of the optional parameter
used for signaling if a response has a continuation one.
This is the last piece of the puzzle.
We forgot to free the link and the data, oops.
There's no need to use the latter.
Always create the cap_supported table when a CAP event is received.
Stylistic stuff, please ignore.
When a CAP DEL is received the key/val pair is not stored in the
hashtable at all so just free them when we're done.
If an invalid CAP is found we keep going by parsing the next one.
Early exit, simpler code.
Do not take the string case into account when comparing the command
name.
@LemonBoy LemonBoy force-pushed the LemonBoy:caps_kv branch from 9b12512 to 7b36a21 Jan 7, 2018
@ailin-nemui
Copy link
Contributor

ailin-nemui commented Jan 9, 2018

(and fix the errors caused by -Werror=declaration-after-statement, see travis)

@LemonBoy LemonBoy force-pushed the LemonBoy:caps_kv branch from 7b36a21 to a4e2fe9 Jan 10, 2018
@ailin-nemui
Copy link
Contributor

ailin-nemui commented Jan 19, 2018

@irssi/developers tagging this for merge

@dequis
Copy link
Member

dequis commented Jan 19, 2018

Hm, haven't looked at this one in a while (silly rebases making it hard to tell which was the last commit i reviewed), I'll look again over the weekend.

@@ -207,13 +207,15 @@ void gslist_free_full (GSList *list, GDestroyNotify free_func)
g_slist_free(list);
}

GSList *gslist_remove_string (GSList *list, const char *str)
GSList *gslist_delete_string (GSList *list, const char *str, GDestroyNotify free_func)

This comment has been minimized.

@dequis

dequis Jan 22, 2018 Member

gslist_remove_string was API and is still in misc.h

This comment has been minimized.

@LemonBoy

LemonBoy Jan 22, 2018 Author Member

Indeed, but the old one was wrong and potentially leaky.
We could re-add it in a correct form using g_slist_delete_link instead or re-write it in terms of gslist_delete_string (with a NULL free_func, with minor modifications to gslist_delete_string).

This comment has been minimized.

@dequis

dequis Jan 24, 2018 Member

Hmm, i see, but its leakiness could be part of the api too (fixing it may introduce double free? not sure)

I'd say re-add it in the exact same form as before, mark it as deprecated, include it in the irssi 1.2 branch and nuke it around irssi 1.3

This comment has been minimized.

@ailin-nemui

ailin-nemui Feb 3, 2018 Contributor

I see, but only LemonBoy was using this api

*val = g_strdup(eq + 1);
return TRUE;
/* If the string ends after the '=' consider the value
* as invalid */

This comment has been minimized.

@dequis

dequis Jan 22, 2018 Member

I dunno about this, a blank value could be meaningful. The spec doesn't consider this case but it doesn't say it should be rejected either.

This comment has been minimized.

@LemonBoy

LemonBoy Jan 22, 2018 Author Member

My reasoning was that you either provide a value or you don't, the specification isn't very clear with it's name[=value] description and I'd suggest to raise this issue with the irc3x folks.

This comment has been minimized.

@dequis

dequis Jan 24, 2018 Member

There's this: ircv3/ircv3-specifications#334 - it's actually about message-tags, but people seem to agree that the solution should be the same for cap values too.

I like the idea of parsing a= as key="a" value="" (and a as key="a" value=NULL), it's fairly natural, so, just remove the special case imo

FWIW, dan- is working on a rewrite of the cap specs to make the whole thing less confusing, he's aware of this now.

else
server->cap_active = g_slist_prepend(server->cap_active, g_strdup(caps[i]));

if (!g_strcmp0(caps[i], "sasl"))
if (!strcmp(caps[i], "sasl"))

This comment has been minimized.

@dequis

dequis Jan 22, 2018 Member

This change is just increasing the crashiness lol.

This comment has been minimized.

@LemonBoy

LemonBoy Jan 22, 2018 Author Member

Not quite as the right hand side is a constant string and the left hand side comes from a null-terminated vector of strings, I used the plain strcmp because of that.

This comment has been minimized.

@dequis

dequis Jan 24, 2018 Member

It just seemed pointless to bother changing it, but sure, it's harmless.

@LemonBoy LemonBoy force-pushed the LemonBoy:caps_kv branch from a4e2fe9 to 2607334 Jan 24, 2018
@LemonBoy
Copy link
Member Author

LemonBoy commented Jan 24, 2018

Done, I've addressed all the review comments. I think the whole PR should be squashed before merging it.

@ailin-nemui
Copy link
Contributor

ailin-nemui commented Feb 2, 2018

@LemonBoy you still need to remove the remove_string method

@LemonBoy
Copy link
Member Author

LemonBoy commented Feb 2, 2018

remove_string has been deprecated since @dequis noted that's part of the public API.

@dequis
Copy link
Member

dequis commented Feb 2, 2018

Yeah in #775 (comment) I basically suggested the opposite of what nei suggested in #775 (review) (which i hadn't seen before) so adding that function back meant showing that comment again

@ailin-nemui
Copy link
Contributor

ailin-nemui commented Feb 2, 2018

we need to get rid of broken functions

@ailin-nemui ailin-nemui merged commit f8fbc1e into irssi:master Feb 5, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dequis
Copy link
Member

dequis commented Apr 8, 2018

lol we forgot to send CAP LS 302

@LemonBoy
Copy link
Member Author

LemonBoy commented Apr 8, 2018

The plan was to bump the required version once we had some feature requiring to do so.

@ailin-nemui ailin-nemui added this to the 1.2.0 milestone Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.