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

Fix MODE parameter parsing #766

Merged
merged 3 commits into from Dec 11, 2017

Conversation

Projects
None yet
4 participants
@horgh
Member

horgh commented Oct 8, 2017

This is to fix #601.

I tested this with the added unit tests, as well as with an IRCD modified to always prefix the last parameter to MODE with a ":", whether needed or not.

@horgh

This comment has been minimized.

Member

horgh commented Oct 8, 2017

Travis doesn't like the addition of trying to run make check. I'm working on it!

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Oct 8, 2017

we should discuss a bit more detailed how to best add tests into irssi

@horgh

This comment has been minimized.

Member

horgh commented Oct 8, 2017

Sure! I'm happy to change this. Mainly I wanted tests to try to understand what was going on with the relevant functions and this was an easy way.

These tests use automake's test suite functionality. It is pretty flexible and there can be several test programs that will run. Since the one added here is a regular C program, we can also run it under valgrind and gdb and the like.

Another way I sometimes like to write tests like these is to have the test code in the actual source files guarded by ifdefs, and conditionally compile a main function in there. That way static functions can be tested.

What do you think? I'm not sure of the best way that would work here. We could consider test suite libraries/frameworks as well, if we wanted to think about making tests easier to write.

@ailin-nemui ailin-nemui referenced this pull request Oct 9, 2017

Open

testing irssi #767

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Oct 9, 2017

(I opened a separate discussion about this)

@dequis

Seems mostly ok. Some style nitpicks. I'm allergic to strcats. Probably best to leave the test code out of this PR and make another one for it later.

BTW that test code could really benefit from some arrays of struct of [input, expected_target, expected_modes] or equivalent. Way too copypastey.

}
s = event_get_param(&params_ptr);
if (s == NULL || strlen(s) == 0) {

This comment has been minimized.

@dequis

dequis Oct 11, 2017

Member

s == NULL || *s == '\0'

// "#channel +o :test" gets parsed to: target = "#channel" and modes = "+o test"
//
// The caller must free the target and modes strings.
void get_mode_params(char const * const params, char * * target, char * * modes)

This comment has been minimized.

@dequis

dequis Oct 11, 2017

Member

(char const *const params, char **target, char **modes)

char * params_copy = NULL;
char * params_ptr = NULL;
char * s = NULL;
char * mode_buf = NULL;

This comment has been minimized.

@dequis

dequis Oct 11, 2017

Member

char *mode_buf

// "#channel :+o test" gets parsed to: target = "#channel" and modes = "+o test"
// "#channel +o :test" gets parsed to: target = "#channel" and modes = "+o test"
//
// The caller must free the target and modes strings.

This comment has been minimized.

@dequis

dequis Oct 11, 2017

Member

A block level comment would make sense. The code style generally prefers /* */ comments everywhere, even if it's single line

if (strlen(mode_buf) > 0) {
strcat(mode_buf, " ");
}
strcat(mode_buf, s);

This comment has been minimized.

@dequis

dequis Oct 11, 2017

Member

Use GString instead of g_malloc0 / strcat / etc

https://developer.gnome.org/glib/stable/glib-Strings.html

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Oct 11, 2017

If I correctly understand what #601 is about all we have to do is preprocess the rest string to make sure an eventual : is weeded out: this leads to a much simpler fix IMO that only needs a memmove. Maybe not even that since, after a quick glance at the code, the whitespace is ignored when the mode argument is parsed, you could just replace the : with a and call it a day.

Just my 0.02€

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Oct 13, 2017

let's investigate if there is a simpler solution àla LemonBoy's suggestion, and move the test to a separate PR. As per #767 I would like to see something based on glib test for that in tests/irc

@horgh

This comment has been minimized.

Member

horgh commented Oct 22, 2017

Thanks for the reviews. I force pushed stripping the ":" as suggested by @LemonBoy. I opted to strip it rather than replace it with a space as this function is used for more than mode parsing, and I don't know that all uses would ignore it. I'll move the tests to a separate PR.

@dequis

This comment has been minimized.

Member

dequis commented Oct 25, 2017

This will just look for any : anywhere and remove it. Colons are only meaningful at the beginning of a parameter, not in the middle.

@horgh

This comment has been minimized.

Member

horgh commented Oct 25, 2017

Hmm yeah, good catch. For some reason I thought we needed to escape parameters containing them with a : prefix, but apparently not!

Strip : from <trailing> parameters
This is to fix #601. The function used to extract the mode string
assumed that ":" would only occur in a particular spot. This lead to the
possibility that ":" could be treated as part of things like nicknames
or mode arguments, where it should have been stripped as part of
protocol escaping.
@horgh

This comment has been minimized.

Member

horgh commented Oct 25, 2017

I force pushed an update.

@@ -224,7 +224,7 @@ static void event_nick(IRC_SERVER_REC *server, const char *data,
static void event_mode(IRC_SERVER_REC *server, const char *data,
const char *nick, const char *addr)
{
char *params, *channel, *mode;
char *params = NULL, *channel = NULL, *mode = NULL;

This comment has been minimized.

@ailin-nemui

ailin-nemui Nov 26, 2017

Contributor

for the moment we try to stay c89 compatible so we cannot initialise them here

This comment has been minimized.

@horgh

horgh Nov 28, 2017

Member

Ah okay. I've fixed that. Thanks for looking at this!

This comment has been minimized.

@dequis

dequis Nov 28, 2017

Member

FTR this is actually legal under c89 https://godbolt.org/g/9S3BWR

(but no need to change it back)

IRC_CHANNEL_REC *chanrec;
char *params, *channel, *mode;
IRC_CHANNEL_REC *chanrec = NULL;
char *params = NULL, *channel = NULL, *mode = NULL;

This comment has been minimized.

@ailin-nemui

ailin-nemui Nov 26, 2017

Contributor

see above ..

Revert initializing pointers to NULL
To maintain C89 compatibility
/* Given a string containing <params>, strip any colon prefixing <trailing>. */
static void strip_params_colon(char *const params)
{
if (!params) {

This comment has been minimized.

@ailin-nemui

ailin-nemui Nov 28, 2017

Contributor

now we just need to get your code into the same "style" as irssi. instead of !var we have to use var == NULL

This comment has been minimized.

@horgh

horgh Nov 28, 2017

Member

Ah yeah. I will update all these. Thanks! Having clang-format will be nice!

}
s = strchr(s, ' ');
if (!s) {

This comment has been minimized.

@ailin-nemui
return;
}
char *s = params;

This comment has been minimized.

@ailin-nemui

ailin-nemui Nov 28, 2017

Contributor

and declar char *s on top, assign here

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Dec 2, 2017

@ailin-nemui ailin-nemui merged commit 48e909d into irssi:master Dec 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment