better account tracking #1250

ailin-nemui · 2021-01-06T00:55:00Z

No description provided.

ailin-nemui · 2021-01-11T11:39:57Z

@irssi/developers

ailin-nemui · 2021-01-24T21:30:11Z

https://github.com/mquin/irssi-scripts/blob/master/account-notify.pl

ailin-nemui · 2021-01-27T22:55:11Z

if disable extended-join, the chase account code runs into verne.freenode.net 263 Nei WHO :This command could not be completed because it has been used recently, and is rate-limited. on freenode

at least make sure the chase is run only 1 time per user

ailin-nemui · 2021-01-29T09:24:45Z

we're now only querying one user once even if they join multiple channels, but still:

I get the 263 error for something like WHO losuler %tna

the common issue seems to be that the person (nick) has left already (QUIT the network or changed nick)
at the time my queued who command makes it through

ailin-nemui · 2021-01-29T15:31:00Z

now we purge the WHOs from the queue. hope that's good enough

ailin-nemui · 2021-01-30T20:31:16Z

@irssi/developers

ailin-nemui · 2021-01-31T16:02:49Z

here is a script that makes use of the account field http://anti.teamidiot.de/static/nei/*/Code/Irssi/account_expando.pl

ailin-nemui · 2021-02-26T13:42:56Z

@irssi/developers

dequis · 2021-02-27T17:45:49Z

"update clang-format to clang-format-11, fixes enum bug" what enum bug?

ailin-nemui · 2021-02-27T17:48:32Z

clang-format < 11 will want to force-format enums like this, iirc:

enum {	CHANNEL_QUERY_MODE,
		CHANNEL_QUERY_WHO,
		CHANNEL_QUERY_BMODE,

		CHANNEL_QUERIES
};

dequis · 2021-02-27T17:58:38Z

src/irc/core/channels-query.c

+	g_hash_table_foreach(rec->accountqueries, (GHFunc) g_free, NULL);
+	g_hash_table_destroy(rec->accountqueries);


void (*GHFunc) (gpointer key, gpointer value, gpointer user_data);

So that foreach does g_free(key)? And doesn't touch the value

g_hash_table_new_full() was called with g_free as the destructor for keys, and nothing for value.

and g_hash_table_destroy() calls the destructor, so it looks like it will double-free the keys.

good catch! double free here, I initially copied the code without a DestroyNotify!

but the values don't need to be freed here AFAIK (maybe you can double check for me) as we're using this table as a set only

removed that foreach now

dequis · 2021-02-27T18:02:40Z

src/irc/core/channels-query.c

-
-	GSList *queries[CHANNEL_QUERIES]; /* All queries that need to be asked from server */
-} SERVER_QUERY_REC;
+#define WHO_Ps_PPtna_745 "WHO %s %%tna,745"


what even is this name

the name is a direct copy of the string content... can you suggest a good name? The reason I have this as a constant is that the similar string is used in more than 1 place and this way when editing the code later on I will be sure to notice that the string must be changed in more than 1 place

oooh lol, P means percentage, now i get it. But okay, just add a comment next to it explaining what it means? WHOX is cryptic

now resolved by your suggestion below

dequis · 2021-02-27T18:15:28Z

src/irc/core/massjoin.c

 	if (g_ascii_strcasecmp(nick, server->nick) == 0) {
-		/* You joined, no need to do anything here */
-		return;
+		/* You joined, do not massjoin */
+		send_massjoin = FALSE;
+	} else {
+		send_massjoin = TRUE;
 	}


Hmm, can you explain this part? The return not being here means we call irc_nicklist_insert, which we previously didn't at this point. What is this for?

I think I understand the massjoin part but don't care much about it.

The goal is that we fill in our OWN account here as well, if we receive it. Otherwise we would need more special code for that

should I expand the comment?

I think the comment is fine for the code, I'm just wondering about the diff. So it's fine to just do the irc_nicklist_insert here? I guess something else might do that later with fewer details and end up as a no-op?

normally our own nick gets inserted only as part of event_names_list in irc-nicklist.c when the NAMES are received after join (with a special case further down to insert our own nick in any case if it was NOT contained in NAMES). The code in massjoin.c handles all the JOINs that happen after we are joined to a channel.

When I relax that check, the (special?) self-JOIN that we get when joining a channel will also be fed through the JOIN code in massjoin.c, with the benefit that we can leech on the extended-account there.

As a consequence, I had to change irc-nicklist.c's NAMES handler to update our CHANNEL USER MODES from the NAMES.

I don't know if all of this is a very bad(tm) idea and if it would be better to do it in some other way.

What do you think? Can you follow my explanation?

dequis · 2021-02-27T18:17:00Z

src/irc/core/servers-redirect.c

-			signal_emit(rec->failure_signal, 1, server);
+			signal_emit(rec->failure_signal, 3, server, rec->cmd->name, rec->arg);


Is this safe?

It's an API change, we add 2 more arguments to the failure signal. As far as I know existing code is also sloppy on arguments so this should not be more unsafe than what we have in other places.

(In general the whole signal emission system is a bit unsafe)

dequis · 2021-02-27T18:19:53Z

src/irc/core/irc-nicklist.c

+	signal_add_first("event 354", (SIGNAL_FUNC) event_whox_743);
 	signal_add("silent event who", (SIGNAL_FUNC) event_who);
+	signal_add("silent event whox", (SIGNAL_FUNC) event_whox_743);
+	signal_add("silent event whox useraccount", (SIGNAL_FUNC) event_whox_745);


What do 743 and 745 mean here?

Good point! These are arbitrary 3-digit numerics that the client should choose for their WHOX queries. I threw dice to get them. Make them a constant? Choose different numbers?

I thought about encoding the selected fields in the number but 1000 numbers is not enough for all combinations

Hahah dice, amazing.

How about this: Put both WHOX commands in some header, something like this

/* identifiers chosen by fair dice roll, guaranteed to be random */ #define WHOX_FULL_ID "743" #define WHOX_USERACCOUNT_ID "745" /* some explanation here about WHOX because it's a bullshit spec */ #define WHOX_FULL_CMD "WHO %s %%tcuhnfdar," WHOX_FULL_ID #define WHOX_USERACCOUNT_CMD "WHO %s %%tna," WHOX_USERACCOUNT_ID

And rename the callbacks from event_whox_745 to event_whox_useraccount, etc.

thanks, I implemented your suggestion

This reverts commit 79be775.

ailin-nemui · 2021-03-12T10:28:58Z

@dequis friendly ping :-)

ailin-nemui force-pushed the whox branch from decbbba to 018f911 Compare January 6, 2021 00:58

ailin-nemui force-pushed the whox branch 3 times, most recently from e1d82c3 to abccc4b Compare January 28, 2021 22:26

ailin-nemui force-pushed the whox branch from eea2b8c to 2894674 Compare January 30, 2021 22:17

Ailin Nemui added 10 commits February 26, 2021 14:19

raise glib version requirement to 2.32 for g_hash_table_contains

7adf81f

update clang-format to clang-format-11, fixes enum bug

bf1376e

better account tracking

dc87199

Track the account queries which are in progress

04035ab

purge whos from queue

04089d5

fix missing output

a0a9a82

maybe this matches the command now

09c17c3

fix off by one

b11f973

queue the who command on join

79be775

up abi

3a92989

ailin-nemui force-pushed the whox branch from f4e6e24 to 3a92989 Compare February 26, 2021 13:29

ailin-nemui added the auto-merge This PR is scheduled for merge if no further comments are opened label Feb 26, 2021

ailin-nemui added the needs testing label Feb 26, 2021

dequis reviewed Feb 27, 2021

View reviewed changes

ailin-nemui removed the auto-merge This PR is scheduled for merge if no further comments are opened label Feb 27, 2021

Ailin Nemui added 4 commits February 27, 2021 21:02

Revert "queue the who command on join"

04b44a8

This reverts commit 79be775.

fix double free

6eeec6a

queue the who command on join

18ee6bb

add constants for WHOX queries

b957d3a

ailin-nemui merged commit 437accd into irssi:master Apr 1, 2021

ailin-nemui deleted the whox branch April 1, 2021 19:27

ailin-nemui added this to the 1.3.0 milestone Oct 11, 2021

		g_hash_table_foreach(rec->accountqueries, (GHFunc) g_free, NULL);
		g_hash_table_destroy(rec->accountqueries);

		signal_emit(rec->failure_signal, 1, server);
		signal_emit(rec->failure_signal, 3, server, rec->cmd->name, rec->arg);

better account tracking #1250

better account tracking #1250

Uh oh!

Conversation

ailin-nemui commented Jan 6, 2021

Uh oh!

ailin-nemui commented Jan 11, 2021

Uh oh!

ailin-nemui commented Jan 24, 2021

Uh oh!

ailin-nemui commented Jan 27, 2021

Uh oh!

ailin-nemui commented Jan 29, 2021

Uh oh!

ailin-nemui commented Jan 29, 2021

Uh oh!

ailin-nemui commented Jan 30, 2021

Uh oh!

ailin-nemui commented Jan 31, 2021

Uh oh!

ailin-nemui commented Feb 26, 2021

Uh oh!

dequis commented Feb 27, 2021

Uh oh!

ailin-nemui commented Feb 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ailin-nemui commented Mar 12, 2021

Uh oh!

Uh oh!